Re: [Qemu-block] [PATCH v2 2/9] jobs: canonize Error object

[Max Reitz]

On 2018-08-24 00:08, John Snow wrote:
> Jobs presently use both an Error object in the case of the create job,
> and char strings in the case of generic errors elsewhere.
> 
> Unify the two paths as just j->err, and remove the extra argument from
> job_completed. The integer error code for job_completed is kept for now,
> to be removed shortly in a separate patch.
> 
> Signed-off-by: John Snow <address@hidden>
> ---
>  block/backup.c            |  2 +-
>  block/commit.c            |  2 +-
>  block/create.c            |  5 ++---
>  block/mirror.c            |  2 +-
>  block/stream.c            |  2 +-
>  include/qemu/job.h        | 10 ++++------
>  job-qmp.c                 |  5 +++--
>  job.c                     | 18 ++++++------------
>  tests/test-bdrv-drain.c   |  2 +-
>  tests/test-blockjob-txn.c |  2 +-
>  tests/test-blockjob.c     |  2 +-
>  11 files changed, 22 insertions(+), 30 deletions(-)

[...]

> diff --git a/include/qemu/job.h b/include/qemu/job.h
> index 9cf463d228..5c92c53ef0 100644
> --- a/include/qemu/job.h
> +++ b/include/qemu/job.h
> @@ -124,12 +124,12 @@ typedef struct Job {
>      /** Estimated progress_current value at the completion of the job */
>      int64_t progress_total;
>  
> -    /** Error string for a failed job (NULL if, and only if, job->ret == 0) 
> */
> -    char *error;
> -
>      /** ret code passed to job_completed. */
>      int ret;
>  
> +    /** Error object for a failed job **/
> +    Error *err;
> +

My question remains why you don't keep the iff condition here...

>      /** The completion function that will be called when the job completes.  
> */
>      BlockCompletionFunc *cb;
>  

[...]

> diff --git a/job.c b/job.c
> index 76988f6678..bc1d970df4 100644
> --- a/job.c
> +++ b/job.c

[...]

> @@ -546,7 +546,7 @@ static void coroutine_fn job_co_entry(void *opaque)
>  
>      assert(job && job->driver && job->driver->run);
>      job_pause_point(job);
> -    job->ret = job->driver->run(job, NULL);
> +    job->ret = job->driver->run(job, &job->err);

...by e.g. calling job_update_rc() here.

(Which seems reasonable since this did update the return code.)

Rest looks good, although I'm missing a "jobs: remove @ret from
job_completed" patch in one of the two series...

Max

signature.asc
Description: OpenPGP digital signature