Re: [Qemu-devel] [PATCH v2 1/6] block: use Error mechanism instead of -errno for block_job_create()

[Luiz Capitulino]

On Tue, 24 Apr 2012 14:53:55 +0100
Stefan Hajnoczi <address@hidden> wrote:

> The block job API uses -errno return values internally and we convert
> these to Error in the QMP functions.  This is ugly because the Error
> should be created at the point where we still have all the relevant
> information.  More importantly, it is hard to add new error cases to
> this case since we quickly run out of -errno values without losing
> information.
> 
> Go ahead an use Error directly and don't convert later.
> 
> Signed-off-by: Stefan Hajnoczi <address@hidden>
> ---
>  block.c        |    4 +++-
>  block/stream.c |   11 +++++------
>  block_int.h    |   11 +++++++----
>  blockdev.c     |   16 +++++-----------
>  4 files changed, 20 insertions(+), 22 deletions(-)
> 
> diff --git a/block.c b/block.c
> index fe74ddd..2b72a0f 100644
> --- a/block.c
> +++ b/block.c
> @@ -4083,11 +4083,13 @@ out:
>  }
>  
>  void *block_job_create(const BlockJobType *job_type, BlockDriverState *bs,
> -                       BlockDriverCompletionFunc *cb, void *opaque)
> +                       BlockDriverCompletionFunc *cb, void *opaque,
> +                       Error **errp)
>  {
>      BlockJob *job;
>  
>      if (bs->job || bdrv_in_use(bs)) {
> +        error_set(errp, QERR_DEVICE_IN_USE, bdrv_get_device_name(bs));
>          return NULL;
>      }
>      bdrv_set_in_use(bs, 1);
> diff --git a/block/stream.c b/block/stream.c
> index 0efe1ad..7002dc8 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -280,16 +280,16 @@ static BlockJobType stream_job_type = {
>      .set_speed     = stream_set_speed,
>  };
>  
> -int stream_start(BlockDriverState *bs, BlockDriverState *base,
> -                 const char *base_id, BlockDriverCompletionFunc *cb,
> -                 void *opaque)
> +void stream_start(BlockDriverState *bs, BlockDriverState *base,
> +                  const char *base_id, BlockDriverCompletionFunc *cb,
> +                  void *opaque, Error **errp)
>  {
>      StreamBlockJob *s;
>      Coroutine *co;
>  
> -    s = block_job_create(&stream_job_type, bs, cb, opaque);
> +    s = block_job_create(&stream_job_type, bs, cb, opaque, errp);
>      if (!s) {
> -        return -EBUSY; /* bs must already be in use */
> +        return;
>      }
>  
>      s->base = base;
> @@ -300,5 +300,4 @@ int stream_start(BlockDriverState *bs, BlockDriverState 
> *base,
>      co = qemu_coroutine_create(stream_run);
>      trace_stream_start(bs, base, s, co, opaque);
>      qemu_coroutine_enter(co, s);
> -    return 0;
>  }
> diff --git a/block_int.h b/block_int.h
> index 0acb49f..8cf6ce9 100644
> --- a/block_int.h
> +++ b/block_int.h
> @@ -346,6 +346,7 @@ int is_windows_drive(const char *filename);
>   * @bs: The block
>   * @cb: Completion function for the job.
>   * @opaque: Opaque pointer value passed to @cb.
> + * @errp: A location to return DeviceInUse.

Quite minor, but this is not a good description. I'd say just "Error object".

>   *
>   * Create a new long-running block device job and return it.  The job
>   * will call @cb asynchronously when the job completes.  Note that
> @@ -357,7 +358,8 @@ int is_windows_drive(const char *filename);
>   * called from a wrapper that is specific to the job type.
>   */
>  void *block_job_create(const BlockJobType *job_type, BlockDriverState *bs,
> -                       BlockDriverCompletionFunc *cb, void *opaque);
> +                       BlockDriverCompletionFunc *cb, void *opaque,
> +                       Error **errp);
>  
>  /**
>   * block_job_complete:
> @@ -417,6 +419,7 @@ void block_job_cancel_sync(BlockJob *job);
>   * backing file if the job completes.  Ignored if @base is %NULL.
>   * @cb: Completion function for the job.
>   * @opaque: Opaque pointer value passed to @cb.
> + * @errp: A location to return DeviceInUse.
>   *
>   * Start a streaming operation on @bs.  Clusters that are unallocated
>   * in @bs, but allocated in any image between @base and @bs (both
> @@ -424,8 +427,8 @@ void block_job_cancel_sync(BlockJob *job);
>   * streaming job, the backing file of @bs will be changed to
>   * @base_id in the written image and to @base in the live BlockDriverState.
>   */
> -int stream_start(BlockDriverState *bs, BlockDriverState *base,
> -                 const char *base_id, BlockDriverCompletionFunc *cb,
> -                 void *opaque);
> +void stream_start(BlockDriverState *bs, BlockDriverState *base,
> +                  const char *base_id, BlockDriverCompletionFunc *cb,
> +                  void *opaque, Error **errp);
>  
>  #endif /* BLOCK_INT_H */
> diff --git a/blockdev.c b/blockdev.c
> index 0c2440e..a411477 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1095,7 +1095,7 @@ void qmp_block_stream(const char *device, bool has_base,
>  {
>      BlockDriverState *bs;
>      BlockDriverState *base_bs = NULL;
> -    int ret;
> +    Error *local_err = NULL;
>  
>      bs = bdrv_find(device);
>      if (!bs) {
> @@ -1111,16 +1111,10 @@ void qmp_block_stream(const char *device, bool 
> has_base,
>          }
>      }
>  
> -    ret = stream_start(bs, base_bs, base, block_stream_cb, bs);
> -    if (ret < 0) {
> -        switch (ret) {
> -        case -EBUSY:
> -            error_set(errp, QERR_DEVICE_IN_USE, device);
> -            return;
> -        default:
> -            error_set(errp, QERR_NOT_SUPPORTED);
> -            return;
> -        }

Just to be sureI got this right, the default clause is not actually possible
before this patch?

> +    stream_start(bs, base_bs, base, block_stream_cb, bs, &local_err);
> +    if (error_is_set(&local_err)) {
> +        error_propagate(errp, local_err);
> +        return;
>      }
>  
>      /* Grab a reference so hotplug does not delete the BlockDriverState from