Patchwork [v2,1/6] block: use Error mechanism instead of -errno for block_job_create()

login
register
mail settings
Submitter Stefan Hajnoczi
Date April 24, 2012, 1:53 p.m.
Message ID <1335275640-3349-2-git-send-email-stefanha@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/154680/
State New
Headers show

Comments

Stefan Hajnoczi - April 24, 2012, 1:53 p.m.
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 <stefanha@linux.vnet.ibm.com>
---
 block.c        |    4 +++-
 block/stream.c |   11 +++++------
 block_int.h    |   11 +++++++----
 blockdev.c     |   16 +++++-----------
 4 files changed, 20 insertions(+), 22 deletions(-)
Eric Blake - April 24, 2012, 2:44 p.m.
On 04/24/2012 07:53 AM, Stefan Hajnoczi 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.

s/an /and /
Luiz Capitulino - April 24, 2012, 6:35 p.m.
On Tue, 24 Apr 2012 14:53:55 +0100
Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> 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 <stefanha@linux.vnet.ibm.com>
> ---
>  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
Stefan Hajnoczi - April 25, 2012, 10:53 a.m.
On Tue, Apr 24, 2012 at 7:35 PM, Luiz Capitulino <lcapitulino@redhat.com> wrote:
> On Tue, 24 Apr 2012 14:53:55 +0100
> Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> 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 <stefanha@linux.vnet.ibm.com>
>> ---
>>  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

Patch

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.
  *
  * 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;
-        }
+    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