Patchwork [3/3] block: use proper qerrors in qmp_block_resize

login
register
mail settings
Submitter Stefan Hajnoczi
Date Jan. 4, 2012, 5:38 p.m.
Message ID <1325698703-15370-4-git-send-email-stefanha@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/134308/
State New
Headers show

Comments

Stefan Hajnoczi - Jan. 4, 2012, 5:38 p.m.
Let's report specific errors so that management tools and users can
identify the problem.

Two new qerrors are needed:
 * QERR_DEVICE_HAS_NO_MEDIUM for ENOMEDIUM
 * QERR_DEVICE_IS_READ_ONLY for EACCES

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 blockdev.c |   26 ++++++++++++++++++--------
 qerror.c   |    8 ++++++++
 qerror.h   |    6 ++++++
 3 files changed, 32 insertions(+), 8 deletions(-)
Luiz Capitulino - Jan. 4, 2012, 7:59 p.m.
On Wed,  4 Jan 2012 17:38:23 +0000
Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> wrote:

> Let's report specific errors so that management tools and users can
> identify the problem.
> 
> Two new qerrors are needed:
>  * QERR_DEVICE_HAS_NO_MEDIUM for ENOMEDIUM
>  * QERR_DEVICE_IS_READ_ONLY for EACCES

Great series, the number of complaints about generic errors have increased
lately. It's to fix this.

There's a missing bit though, would you mind to update the block_resize's
command documentation in the schema?

Thanks!

> 
> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> ---
>  blockdev.c |   26 ++++++++++++++++++--------
>  qerror.c   |    8 ++++++++
>  qerror.h   |    6 ++++++
>  3 files changed, 32 insertions(+), 8 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index c832782..8c2c8cc 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -841,11 +841,6 @@ int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
>      return 0;
>  }
>  
> -/*
> - * XXX: replace the QERR_UNDEFINED_ERROR errors with real values once the
> - * existing QERR_ macro mess is cleaned up.  A good example for better
> - * error reports can be found in the qemu-img resize code.
> - */
>  void qmp_block_resize(const char *device, int64_t size, Error **errp)
>  {
>      BlockDriverState *bs;
> @@ -857,12 +852,27 @@ void qmp_block_resize(const char *device, int64_t size, Error **errp)
>      }
>  
>      if (size < 0) {
> -        error_set(errp, QERR_UNDEFINED_ERROR);
> +        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "size", "a >0 size");
>          return;
>      }
>  
> -    if (bdrv_truncate(bs, size)) {
> +    switch (bdrv_truncate(bs, size)) {
> +    case 0:
> +        break;
> +    case -ENOMEDIUM:
> +        error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
> +        break;
> +    case -ENOTSUP:
> +        error_set(errp, QERR_UNSUPPORTED);
> +        break;
> +    case -EACCES:
> +        error_set(errp, QERR_DEVICE_IS_READ_ONLY, device);
> +        break;
> +    case -EBUSY:
> +        error_set(errp, QERR_DEVICE_IN_USE, device);
> +        break;
> +    default:
>          error_set(errp, QERR_UNDEFINED_ERROR);
> -        return;
> +        break;
>      }
>  }
> diff --git a/qerror.c b/qerror.c
> index 2979b3e..3d95383 100644
> --- a/qerror.c
> +++ b/qerror.c
> @@ -80,6 +80,10 @@ static const QErrorStringTable qerror_table[] = {
>          .desc      = "Migration is disabled when using feature '%(feature)' in device '%(device)'",
>      },
>      {
> +        .error_fmt = QERR_DEVICE_HAS_NO_MEDIUM,
> +        .desc      = "Device '%(device)' has no medium",
> +    },
> +    {
>          .error_fmt = QERR_DEVICE_INIT_FAILED,
>          .desc      = "Device '%(device)' could not be initialized",
>      },
> @@ -88,6 +92,10 @@ static const QErrorStringTable qerror_table[] = {
>          .desc      = "Device '%(device)' is in use",
>      },
>      {
> +        .error_fmt = QERR_DEVICE_IS_READ_ONLY,
> +        .desc      = "Device '%(device)' is read only",
> +    },
> +    {
>          .error_fmt = QERR_DEVICE_LOCKED,
>          .desc      = "Device '%(device)' is locked",
>      },
> diff --git a/qerror.h b/qerror.h
> index c34674e..a693d49 100644
> --- a/qerror.h
> +++ b/qerror.h
> @@ -81,12 +81,18 @@ QError *qobject_to_qerror(const QObject *obj);
>  #define QERR_DEVICE_FEATURE_BLOCKS_MIGRATION \
>      "{ 'class': 'DeviceFeatureBlocksMigration', 'data': { 'device': %s, 'feature': %s } }"
>  
> +#define QERR_DEVICE_HAS_NO_MEDIUM \
> +    "{ 'class': 'DeviceHasNoMedium', 'data', { 'name': %s } }"
> +
>  #define QERR_DEVICE_INIT_FAILED \
>      "{ 'class': 'DeviceInitFailed', 'data': { 'device': %s } }"
>  
>  #define QERR_DEVICE_IN_USE \
>      "{ 'class': 'DeviceInUse', 'data': { 'device': %s } }"
>  
> +#define QERR_DEVICE_IS_READ_ONLY \
> +    "{ 'class': 'DeviceIsReadOnly', 'data': { 'device': %s } }"
> +
>  #define QERR_DEVICE_LOCKED \
>      "{ 'class': 'DeviceLocked', 'data': { 'device': %s } }"
>
Stefan Hajnoczi - Jan. 4, 2012, 10:21 p.m.
On Wed, Jan 4, 2012 at 7:59 PM, Luiz Capitulino <lcapitulino@redhat.com> wrote:
> On Wed,  4 Jan 2012 17:38:23 +0000
> Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> wrote:
>
>> Let's report specific errors so that management tools and users can
>> identify the problem.
>>
>> Two new qerrors are needed:
>>  * QERR_DEVICE_HAS_NO_MEDIUM for ENOMEDIUM
>>  * QERR_DEVICE_IS_READ_ONLY for EACCES
>
> Great series, the number of complaints about generic errors have increased
> lately. It's to fix this.
>
> There's a missing bit though, would you mind to update the block_resize's
> command documentation in the schema?

Sure, fixed in v2.

Patch

diff --git a/blockdev.c b/blockdev.c
index c832782..8c2c8cc 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -841,11 +841,6 @@  int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
     return 0;
 }
 
-/*
- * XXX: replace the QERR_UNDEFINED_ERROR errors with real values once the
- * existing QERR_ macro mess is cleaned up.  A good example for better
- * error reports can be found in the qemu-img resize code.
- */
 void qmp_block_resize(const char *device, int64_t size, Error **errp)
 {
     BlockDriverState *bs;
@@ -857,12 +852,27 @@  void qmp_block_resize(const char *device, int64_t size, Error **errp)
     }
 
     if (size < 0) {
-        error_set(errp, QERR_UNDEFINED_ERROR);
+        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "size", "a >0 size");
         return;
     }
 
-    if (bdrv_truncate(bs, size)) {
+    switch (bdrv_truncate(bs, size)) {
+    case 0:
+        break;
+    case -ENOMEDIUM:
+        error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
+        break;
+    case -ENOTSUP:
+        error_set(errp, QERR_UNSUPPORTED);
+        break;
+    case -EACCES:
+        error_set(errp, QERR_DEVICE_IS_READ_ONLY, device);
+        break;
+    case -EBUSY:
+        error_set(errp, QERR_DEVICE_IN_USE, device);
+        break;
+    default:
         error_set(errp, QERR_UNDEFINED_ERROR);
-        return;
+        break;
     }
 }
diff --git a/qerror.c b/qerror.c
index 2979b3e..3d95383 100644
--- a/qerror.c
+++ b/qerror.c
@@ -80,6 +80,10 @@  static const QErrorStringTable qerror_table[] = {
         .desc      = "Migration is disabled when using feature '%(feature)' in device '%(device)'",
     },
     {
+        .error_fmt = QERR_DEVICE_HAS_NO_MEDIUM,
+        .desc      = "Device '%(device)' has no medium",
+    },
+    {
         .error_fmt = QERR_DEVICE_INIT_FAILED,
         .desc      = "Device '%(device)' could not be initialized",
     },
@@ -88,6 +92,10 @@  static const QErrorStringTable qerror_table[] = {
         .desc      = "Device '%(device)' is in use",
     },
     {
+        .error_fmt = QERR_DEVICE_IS_READ_ONLY,
+        .desc      = "Device '%(device)' is read only",
+    },
+    {
         .error_fmt = QERR_DEVICE_LOCKED,
         .desc      = "Device '%(device)' is locked",
     },
diff --git a/qerror.h b/qerror.h
index c34674e..a693d49 100644
--- a/qerror.h
+++ b/qerror.h
@@ -81,12 +81,18 @@  QError *qobject_to_qerror(const QObject *obj);
 #define QERR_DEVICE_FEATURE_BLOCKS_MIGRATION \
     "{ 'class': 'DeviceFeatureBlocksMigration', 'data': { 'device': %s, 'feature': %s } }"
 
+#define QERR_DEVICE_HAS_NO_MEDIUM \
+    "{ 'class': 'DeviceHasNoMedium', 'data', { 'name': %s } }"
+
 #define QERR_DEVICE_INIT_FAILED \
     "{ 'class': 'DeviceInitFailed', 'data': { 'device': %s } }"
 
 #define QERR_DEVICE_IN_USE \
     "{ 'class': 'DeviceInUse', 'data': { 'device': %s } }"
 
+#define QERR_DEVICE_IS_READ_ONLY \
+    "{ 'class': 'DeviceIsReadOnly', 'data': { 'device': %s } }"
+
 #define QERR_DEVICE_LOCKED \
     "{ 'class': 'DeviceLocked', 'data': { 'device': %s } }"