[Qemu-devel] [PATCH 0/3] qerror: proper errors for qmp_block_resize()

[Qemu-devel] [PATCH 0/3] qerror: proper errors for qmp_block_resize()

[Stefan Hajnoczi]

The block resize command returns undefined errors if things go wrong.  This is
bad since users will have no chance to understand what failed.

Patch 3 makes qmp_block_resize() use meaningful errors.  We introduce new
qerrors for ENOMEDIUM and EACCES since nothing exists yet.

When doing this I noticed that qerror definitions and table entries are not in
alphabetical order as required by the comments in qerror.h and qerror.c.
That's not a surprise since there is no automated way to enforce this.  I'm not
sure whether this requirement is useful or not in the first place, but felt
guilty creating more mess.  So I've restored alphabetical order and added a
script to verify that the requirement is met in Patches 1 and 2.

Stefan Hajnoczi (3):
  qerror: add check-qerror.sh to verify alphabetical order
  qerror: restore alphabetical order over qerrors
  block: use proper qerrors in qmp_block_resize

 blockdev.c              |   26 +++++++++----
 qerror.c                |   91 +++++++++++++++++++++++++---------------------
 qerror.h                |   78 +++++++++++++++++++++------------------
 scripts/check-qerror.sh |   22 +++++++++++
 4 files changed, 131 insertions(+), 86 deletions(-)
 create mode 100755 scripts/check-qerror.sh

-- 
1.7.7.3

[Qemu-devel] [PATCH 1/3] qerror: add check-qerror.sh to verify alphabetical order

[Stefan Hajnoczi]

We're supposed to keep qerror definitions and table entries in
alphabetical order.  In practice this is not checked.

I haven't found a nice way to integrate this into the makefile yet but
we can at least have this script which verifies that qerrors are in
alphabetical order.

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 qerror.c                |    3 +--
 qerror.h                |    2 +-
 scripts/check-qerror.sh |   22 ++++++++++++++++++++++
 3 files changed, 24 insertions(+), 3 deletions(-)
 create mode 100755 scripts/check-qerror.sh

diff --git a/qerror.c b/qerror.c
index 9a75d06..62c0c707 100644
--- a/qerror.c
+++ b/qerror.c
@@ -40,8 +40,7 @@ static const QType qerror_type = {
  * "running out of foo: %(foo)%%"
  *
  * Please keep the entries in alphabetical order.
- * Use "sed -n '/^static.*qerror_table\[\]/,/^};/s/QERR_/&/gp' qerror.c | sort -c"
- * to check.
+ * Use scripts/check-qerror.sh to check.
  */
 static const QErrorStringTable qerror_table[] = {
     {
diff --git a/qerror.h b/qerror.h
index efda232..36e0343 100644
--- a/qerror.h
+++ b/qerror.h
@@ -49,7 +49,7 @@ QError *qobject_to_qerror(const QObject *obj);
 /*
  * QError class list
  * Please keep the definitions in alphabetical order.
- * Use "grep '^#define QERR_' qerror.h | sort -c" to check.
+ * Use scripts/check-qerror.sh to check.
  */
 #define QERR_BAD_BUS_FOR_DEVICE \
     "{ 'class': 'BadBusForDevice', 'data': { 'device': %s, 'bad_bus_type': %s } }"
diff --git a/scripts/check-qerror.sh b/scripts/check-qerror.sh
new file mode 100755
index 0000000..af7fbd5
--- /dev/null
+++ b/scripts/check-qerror.sh
@@ -0,0 +1,22 @@
+#!/bin/sh
+# This script verifies that qerror definitions and table entries are
+# alphabetically ordered.
+
+check_order() {
+  errmsg=$1
+  shift
+
+  # sort -C verifies order but does not print a message.  sort -c does print a
+  # message.  These options are both in POSIX.
+  if ! "$@" | sort -C; then
+    echo "$errmsg"
+    "$@" | sort -c
+    exit 1
+  fi
+  return 0
+}
+
+check_order 'Definitions in qerror.h must be in alphabetical order:' \
+            grep '^#define QERR_' qerror.h
+check_order 'Entries in qerror.c:qerror_table must be in alphabetical order:' \
+            sed -n '/^static.*qerror_table\[\]/,/^};/s/QERR_/&/gp' qerror.c
-- 
1.7.7.3

[Qemu-devel] [PATCH 2/3] qerror: restore alphabetical order over qerrors

[Stefan Hajnoczi]

Over time these must have gotten out of order.  Put everything back in
alphabetical order.

This is purely a clean up.  In practice nothing depends on the order.

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 qerror.c |   84 +++++++++++++++++++++++++++++++-------------------------------
 qerror.h |   72 ++++++++++++++++++++++++++--------------------------
 2 files changed, 78 insertions(+), 78 deletions(-)

diff --git a/qerror.c b/qerror.c
index 62c0c707..2979b3e 100644
--- a/qerror.c
+++ b/qerror.c
@@ -44,6 +44,10 @@ static const QType qerror_type = {
  */
 static const QErrorStringTable qerror_table[] = {
     {
+        .error_fmt = QERR_ADD_CLIENT_FAILED,
+        .desc      = "Could not add client",
+    },
+    {
         .error_fmt = QERR_BAD_BUS_FOR_DEVICE,
         .desc      = "Device '%(device)' can't go on a %(bad_bus_type) bus",
     },
@@ -52,26 +56,30 @@ static const QErrorStringTable qerror_table[] = {
         .desc      = "Block format '%(format)' used by device '%(name)' does not support feature '%(feature)'",
     },
     {
-        .error_fmt = QERR_BUS_NOT_FOUND,
-        .desc      = "Bus '%(bus)' not found",
-    },
-    {
         .error_fmt = QERR_BUS_NO_HOTPLUG,
         .desc      = "Bus '%(bus)' does not support hotplugging",
     },
     {
-        .error_fmt = QERR_COMMAND_NOT_FOUND,
-        .desc      = "The command %(name) has not been found",
+        .error_fmt = QERR_BUS_NOT_FOUND,
+        .desc      = "Bus '%(bus)' not found",
     },
     {
         .error_fmt = QERR_COMMAND_DISABLED,
         .desc      = "The command %(name) has been disabled for this instance",
     },
     {
+        .error_fmt = QERR_COMMAND_NOT_FOUND,
+        .desc      = "The command %(name) has not been found",
+    },
+    {
         .error_fmt = QERR_DEVICE_ENCRYPTED,
         .desc      = "Device '%(device)' is encrypted",
     },
     {
+        .error_fmt = QERR_DEVICE_FEATURE_BLOCKS_MIGRATION,
+        .desc      = "Migration is disabled when using feature '%(feature)' in device '%(device)'",
+    },
+    {
         .error_fmt = QERR_DEVICE_INIT_FAILED,
         .desc      = "Device '%(device)' could not be initialized",
     },
@@ -80,10 +88,6 @@ static const QErrorStringTable qerror_table[] = {
         .desc      = "Device '%(device)' is in use",
     },
     {
-        .error_fmt = QERR_DEVICE_FEATURE_BLOCKS_MIGRATION,
-        .desc      = "Migration is disabled when using feature '%(feature)' in device '%(device)'",
-    },
-    {
         .error_fmt = QERR_DEVICE_LOCKED,
         .desc      = "Device '%(device)' is locked",
     },
@@ -92,6 +96,14 @@ static const QErrorStringTable qerror_table[] = {
         .desc      = "Device '%(device)' has multiple child busses",
     },
     {
+        .error_fmt = QERR_DEVICE_NO_BUS,
+        .desc      = "Device '%(device)' has no child bus",
+    },
+    {
+        .error_fmt = QERR_DEVICE_NO_HOTPLUG,
+        .desc      = "Device '%(device)' does not support hotplugging",
+    },
+    {
         .error_fmt = QERR_DEVICE_NOT_ACTIVE,
         .desc      = "Device '%(device)' has not been activated",
     },
@@ -108,14 +120,6 @@ static const QErrorStringTable qerror_table[] = {
         .desc      = "Device '%(device)' is not removable",
     },
     {
-        .error_fmt = QERR_DEVICE_NO_BUS,
-        .desc      = "Device '%(device)' has no child bus",
-    },
-    {
-        .error_fmt = QERR_DEVICE_NO_HOTPLUG,
-        .desc      = "Device '%(device)' does not support hotplugging",
-    },
-    {
         .error_fmt = QERR_DUPLICATE_ID,
         .desc      = "Duplicate ID '%(id)' for %(object)",
     },
@@ -140,6 +144,10 @@ static const QErrorStringTable qerror_table[] = {
         .desc      = "Invalid parameter '%(name)'",
     },
     {
+        .error_fmt = QERR_INVALID_PARAMETER_COMBINATION,
+        .desc      = "Invalid parameter combination",
+    },
+    {
         .error_fmt = QERR_INVALID_PARAMETER_TYPE,
         .desc      = "Invalid parameter type, expected: %(expected)",
     },
@@ -156,15 +164,15 @@ static const QErrorStringTable qerror_table[] = {
         .desc      = "An IO error has occurred",
     },
     {
-        .error_fmt = QERR_JSON_PARSING,
-        .desc      = "Invalid JSON syntax",
-    },
-    {
         .error_fmt = QERR_JSON_PARSE_ERROR,
         .desc      = "JSON parse error, %(message)",
 
     },
     {
+        .error_fmt = QERR_JSON_PARSING,
+        .desc      = "Invalid JSON syntax",
+    },
+    {
         .error_fmt = QERR_KVM_MISSING_CAP,
         .desc      = "Using KVM without %(capability), %(feature) unavailable",
     },
@@ -210,6 +218,14 @@ static const QErrorStringTable qerror_table[] = {
                      "value %(value) (minimum: %(min), maximum: %(max)'",
     },
     {
+        .error_fmt = QERR_QGA_COMMAND_FAILED,
+        .desc      = "Guest agent command failed, error was '%(message)'",
+    },
+    {
+        .error_fmt = QERR_QGA_LOGGING_FAILED,
+        .desc      = "Guest agent failed to log non-optional log statement",
+    },
+    {
         .error_fmt = QERR_QMP_BAD_INPUT_OBJECT,
         .desc      = "Expected '%(expected)' in QMP input",
     },
@@ -230,10 +246,6 @@ static const QErrorStringTable qerror_table[] = {
         .desc      = "Could not set password",
     },
     {
-        .error_fmt = QERR_ADD_CLIENT_FAILED,
-        .desc      = "Could not add client",
-    },
-    {
         .error_fmt = QERR_TOO_MANY_FILES,
         .desc      = "Too many open files",
     },
@@ -242,15 +254,15 @@ static const QErrorStringTable qerror_table[] = {
         .desc      = "An undefined error has occurred",
     },
     {
-        .error_fmt = QERR_UNSUPPORTED,
-        .desc      = "this feature or command is not currently supported",
-    },
-    {
         .error_fmt = QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
         .desc      = "'%(device)' uses a %(format) feature which is not "
                      "supported by this qemu version: %(feature)",
     },
     {
+        .error_fmt = QERR_UNSUPPORTED,
+        .desc      = "this feature or command is not currently supported",
+    },
+    {
         .error_fmt = QERR_VIRTFS_FEATURE_BLOCKS_MIGRATION,
         .desc      = "Migration is disabled when VirtFS export path '%(path)' "
                      "is mounted in the guest using mount_tag '%(tag)'",
@@ -259,18 +271,6 @@ static const QErrorStringTable qerror_table[] = {
         .error_fmt = QERR_VNC_SERVER_FAILED,
         .desc      = "Could not start VNC server on %(target)",
     },
-    {
-        .error_fmt = QERR_QGA_LOGGING_FAILED,
-        .desc      = "Guest agent failed to log non-optional log statement",
-    },
-    {
-        .error_fmt = QERR_QGA_COMMAND_FAILED,
-        .desc      = "Guest agent command failed, error was '%(message)'",
-    },
-    {
-        .error_fmt = QERR_INVALID_PARAMETER_COMBINATION,
-        .desc      = "Invalid parameter combination",
-    },
     {}
 };
 
diff --git a/qerror.h b/qerror.h
index 36e0343..c34674e 100644
--- a/qerror.h
+++ b/qerror.h
@@ -51,42 +51,54 @@ QError *qobject_to_qerror(const QObject *obj);
  * Please keep the definitions in alphabetical order.
  * Use scripts/check-qerror.sh to check.
  */
+#define QERR_ADD_CLIENT_FAILED \
+    "{ 'class': 'AddClientFailed', 'data': {} }"
+
 #define QERR_BAD_BUS_FOR_DEVICE \
     "{ 'class': 'BadBusForDevice', 'data': { 'device': %s, 'bad_bus_type': %s } }"
 
 #define QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED \
     "{ 'class': 'BlockFormatFeatureNotSupported', 'data': { 'format': %s, 'name': %s, 'feature': %s } }"
 
-#define QERR_BUS_NOT_FOUND \
-    "{ 'class': 'BusNotFound', 'data': { 'bus': %s } }"
+#define QERR_BUFFER_OVERRUN \
+    "{ 'class': 'BufferOverrun', 'data': {} }"
 
 #define QERR_BUS_NO_HOTPLUG \
     "{ 'class': 'BusNoHotplug', 'data': { 'bus': %s } }"
 
-#define QERR_COMMAND_NOT_FOUND \
-    "{ 'class': 'CommandNotFound', 'data': { 'name': %s } }"
+#define QERR_BUS_NOT_FOUND \
+    "{ 'class': 'BusNotFound', 'data': { 'bus': %s } }"
 
 #define QERR_COMMAND_DISABLED \
     "{ 'class': 'CommandDisabled', 'data': { 'name': %s } }"
 
+#define QERR_COMMAND_NOT_FOUND \
+    "{ 'class': 'CommandNotFound', 'data': { 'name': %s } }"
+
 #define QERR_DEVICE_ENCRYPTED \
     "{ 'class': 'DeviceEncrypted', 'data': { 'device': %s } }"
 
+#define QERR_DEVICE_FEATURE_BLOCKS_MIGRATION \
+    "{ 'class': 'DeviceFeatureBlocksMigration', 'data': { 'device': %s, 'feature': %s } }"
+
 #define QERR_DEVICE_INIT_FAILED \
     "{ 'class': 'DeviceInitFailed', 'data': { 'device': %s } }"
 
 #define QERR_DEVICE_IN_USE \
     "{ 'class': 'DeviceInUse', 'data': { 'device': %s } }"
 
-#define QERR_DEVICE_FEATURE_BLOCKS_MIGRATION \
-    "{ 'class': 'DeviceFeatureBlocksMigration', 'data': { 'device': %s, 'feature': %s } }"
-
 #define QERR_DEVICE_LOCKED \
     "{ 'class': 'DeviceLocked', 'data': { 'device': %s } }"
 
 #define QERR_DEVICE_MULTIPLE_BUSSES \
     "{ 'class': 'DeviceMultipleBusses', 'data': { 'device': %s } }"
 
+#define QERR_DEVICE_NO_BUS \
+    "{ 'class': 'DeviceNoBus', 'data': { 'device': %s } }"
+
+#define QERR_DEVICE_NO_HOTPLUG \
+    "{ 'class': 'DeviceNoHotplug', 'data': { 'device': %s } }"
+
 #define QERR_DEVICE_NOT_ACTIVE \
     "{ 'class': 'DeviceNotActive', 'data': { 'device': %s } }"
 
@@ -99,12 +111,6 @@ QError *qobject_to_qerror(const QObject *obj);
 #define QERR_DEVICE_NOT_REMOVABLE \
     "{ 'class': 'DeviceNotRemovable', 'data': { 'device': %s } }"
 
-#define QERR_DEVICE_NO_BUS \
-    "{ 'class': 'DeviceNoBus', 'data': { 'device': %s } }"
-
-#define QERR_DEVICE_NO_HOTPLUG \
-    "{ 'class': 'DeviceNoHotplug', 'data': { 'device': %s } }"
-
 #define QERR_DUPLICATE_ID \
     "{ 'class': 'DuplicateId', 'data': { 'id': %s, 'object': %s } }"
 
@@ -114,12 +120,18 @@ QError *qobject_to_qerror(const QObject *obj);
 #define QERR_FD_NOT_SUPPLIED \
     "{ 'class': 'FdNotSupplied', 'data': {} }"
 
+#define QERR_FEATURE_DISABLED \
+    "{ 'class': 'FeatureDisabled', 'data': { 'name': %s } }"
+
 #define QERR_INVALID_BLOCK_FORMAT \
     "{ 'class': 'InvalidBlockFormat', 'data': { 'name': %s } }"
 
 #define QERR_INVALID_PARAMETER \
     "{ 'class': 'InvalidParameter', 'data': { 'name': %s } }"
 
+#define QERR_INVALID_PARAMETER_COMBINATION \
+    "{ 'class': 'InvalidParameterCombination', 'data': {} }"
+
 #define QERR_INVALID_PARAMETER_TYPE \
     "{ 'class': 'InvalidParameterType', 'data': { 'name': %s,'expected': %s } }"
 
@@ -132,14 +144,11 @@ QError *qobject_to_qerror(const QObject *obj);
 #define QERR_IO_ERROR \
     "{ 'class': 'IOError', 'data': {} }"
 
-#define QERR_JSON_PARSING \
-    "{ 'class': 'JSONParsing', 'data': {} }"
-
 #define QERR_JSON_PARSE_ERROR \
     "{ 'class': 'JSONParseError', 'data': { 'message': %s } }"
 
-#define QERR_BUFFER_OVERRUN \
-    "{ 'class': 'BufferOverrun', 'data': {} }"
+#define QERR_JSON_PARSING \
+    "{ 'class': 'JSONParsing', 'data': {} }"
 
 #define QERR_KVM_MISSING_CAP \
     "{ 'class': 'KVMMissingCap', 'data': { 'capability': %s, 'feature': %s } }"
@@ -174,6 +183,12 @@ QError *qobject_to_qerror(const QObject *obj);
 #define QERR_PROPERTY_VALUE_OUT_OF_RANGE \
     "{ 'class': 'PropertyValueOutOfRange', 'data': { 'device': %s, 'property': %s, 'value': %"PRId64", 'min': %"PRId64", 'max': %"PRId64" } }"
 
+#define QERR_QGA_COMMAND_FAILED \
+    "{ 'class': 'QgaCommandFailed', 'data': { 'message': %s } }"
+
+#define QERR_QGA_LOGGING_FAILED \
+    "{ 'class': 'QgaLoggingFailed', 'data': {} }"
+
 #define QERR_QMP_BAD_INPUT_OBJECT \
     "{ 'class': 'QMPBadInputObject', 'data': { 'expected': %s } }"
 
@@ -189,37 +204,22 @@ QError *qobject_to_qerror(const QObject *obj);
 #define QERR_SET_PASSWD_FAILED \
     "{ 'class': 'SetPasswdFailed', 'data': {} }"
 
-#define QERR_ADD_CLIENT_FAILED \
-    "{ 'class': 'AddClientFailed', 'data': {} }"
-
 #define QERR_TOO_MANY_FILES \
     "{ 'class': 'TooManyFiles', 'data': {} }"
 
 #define QERR_UNDEFINED_ERROR \
     "{ 'class': 'UndefinedError', 'data': {} }"
 
-#define QERR_UNSUPPORTED \
-    "{ 'class': 'Unsupported', 'data': {} }"
-
 #define QERR_UNKNOWN_BLOCK_FORMAT_FEATURE \
     "{ 'class': 'UnknownBlockFormatFeature', 'data': { 'device': %s, 'format': %s, 'feature': %s } }"
 
+#define QERR_UNSUPPORTED \
+    "{ 'class': 'Unsupported', 'data': {} }"
+
 #define QERR_VIRTFS_FEATURE_BLOCKS_MIGRATION \
     "{ 'class': 'VirtFSFeatureBlocksMigration', 'data': { 'path': %s, 'tag': %s } }"
 
 #define QERR_VNC_SERVER_FAILED \
     "{ 'class': 'VNCServerFailed', 'data': { 'target': %s } }"
 
-#define QERR_FEATURE_DISABLED \
-    "{ 'class': 'FeatureDisabled', 'data': { 'name': %s } }"
-
-#define QERR_QGA_LOGGING_FAILED \
-    "{ 'class': 'QgaLoggingFailed', 'data': {} }"
-
-#define QERR_QGA_COMMAND_FAILED \
-    "{ 'class': 'QgaCommandFailed', 'data': { 'message': %s } }"
-
-#define QERR_INVALID_PARAMETER_COMBINATION \
-    "{ 'class': 'InvalidParameterCombination', 'data': {} }"
-
 #endif /* QERROR_H */
-- 
1.7.7.3

[Qemu-devel] [PATCH 3/3] block: use proper qerrors in qmp_block_resize

[Stefan Hajnoczi]

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(-)

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 } }"
 
-- 
1.7.7.3

Re: [Qemu-devel] [PATCH 3/3] block: use proper qerrors in qmp_block_resize

[Luiz Capitulino]

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 } }"
>  

Re: [Qemu-devel] [PATCH 0/3] qerror: proper errors for qmp_block_resize()

[Luiz Capitulino]

On Wed,  4 Jan 2012 17:38:20 +0000
Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> wrote:

> The block resize command returns undefined errors if things go wrong.  This is
> bad since users will have no chance to understand what failed.

Oops, this introduces a segfault try "block_resize ide1-cd0 100" in the monitor
and you'll see it.

> 
> Patch 3 makes qmp_block_resize() use meaningful errors.  We introduce new
> qerrors for ENOMEDIUM and EACCES since nothing exists yet.
> 
> When doing this I noticed that qerror definitions and table entries are not in
> alphabetical order as required by the comments in qerror.h and qerror.c.
> That's not a surprise since there is no automated way to enforce this.  I'm not
> sure whether this requirement is useful or not in the first place, but felt
> guilty creating more mess.  So I've restored alphabetical order and added a
> script to verify that the requirement is met in Patches 1 and 2.
> 
> Stefan Hajnoczi (3):
>   qerror: add check-qerror.sh to verify alphabetical order
>   qerror: restore alphabetical order over qerrors
>   block: use proper qerrors in qmp_block_resize
> 
>  blockdev.c              |   26 +++++++++----
>  qerror.c                |   91 +++++++++++++++++++++++++---------------------
>  qerror.h                |   78 +++++++++++++++++++++------------------
>  scripts/check-qerror.sh |   22 +++++++++++
>  4 files changed, 131 insertions(+), 86 deletions(-)
>  create mode 100755 scripts/check-qerror.sh
> 

Re: [Qemu-devel] [PATCH 0/3] qerror: proper errors for qmp_block_resize()

[Stefan Hajnoczi]

On Wed, Jan 4, 2012 at 8:01 PM, Luiz Capitulino <lcapitulino@redhat.com> wrote:
> On Wed,  4 Jan 2012 17:38:20 +0000
> Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> wrote:
>
>> The block resize command returns undefined errors if things go wrong.  This is
>> bad since users will have no chance to understand what failed.
>
> Oops, this introduces a segfault try "block_resize ide1-cd0 100" in the monitor
> and you'll see it.

Typo in the QERR_DEVICE_HAS_NO_MEDIUM JSON string format broke this.
Will fix in v2.

Thanks for discovering it!

Stefan

Re: [Qemu-devel] [PATCH 3/3] block: use proper qerrors in qmp_block_resize

[Stefan Hajnoczi]

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.