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

[Stefan Hajnoczi]

On Tue, Apr 24, 2012 at 7:35 PM, Luiz Capitulino <address@hidden> wrote:
> 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".

I followed the style of the glib documentation for GError** arguments.
 I'm happy to change to 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?

I checked, you are right.  It was not used.  By removing the -errno ->
Error conversion layer we ensure this kind of inconsistency cannot
happen.

Stefan