[Qemu-devel] [FOR 0.12 PATCH 00/18] QError conversions and more

[Qemu-devel] [FOR 0.12 PATCH 00/18] QError conversions and more

[Markus Armbruster]

This patch series converts monitor commands eject, change, closefd,
getfd to QError.

There's a simple bonus fix in PATCH 3.

Finally, PATCH 18 puts a human-readable error description into the QMP
error response.  For background, see the lengthy thread ending at
http://lists.gnu.org/archive/html/qemu-devel/2009-11/msg01221.html

Series should be applied after Luiz's work.

Luiz Capitulino (2):
  QError: new class for device encrypted errors
  monitor: do_cont(): Don't ask for passwords

Markus Armbruster (16):
  monitor: Fix double-prompt after "change vnc passwd BLA"
  QError: Put error definitions in alphabetical order
  QError: New QERR_DEVICE_LOCKED
  QError: New QERR_DEVICE_NOT_REMOVABLE
  monitor: convert do_eject() to QError
  QError: New QERR_INVALID_BLOCK_FORMAT
  QError: New QERR_SET_PASSWD_FAILED
  QError: New QERR_VNC_SERVER_FAILED
  monitor: convert do_change() to QObject, QError
  QError: New QERR_FD_NOT_FOUND
  monitor: convert do_closefd() to QError
  QError: New QERR_FD_NOT_SUPPLIED
  New QERR_INVALID_PARAMETER
  QError: New QERR_TOO_MANY_FILES
  monitor: convert do_getfd() to QError
  QMP: add human-readable description to error response

 QMP/qmp-spec.txt |    5 ++-
 monitor.c        |   55 +++++++++++++++++++++++-----------
 qemu-monitor.hx  |    3 +-
 qerror.c         |   85 +++++++++++++++++++++++++++++++++++++++++++-----------
 qerror.h         |   56 ++++++++++++++++++++++++++++-------
 5 files changed, 155 insertions(+), 49 deletions(-)

[Qemu-devel] [FOR 0.12 PATCH 01/18] QError: new class for device encrypted errors

[Markus Armbruster]

From: Luiz Capitulino <lcapitulino@redhat.com>

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qerror.c |    4 ++++
 qerror.h |    3 +++
 2 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/qerror.c b/qerror.c
index eb4ce33..d90529c 100644
--- a/qerror.c
+++ b/qerror.c
@@ -45,6 +45,10 @@ static const QErrorStringTable qerror_table[] = {
         .desc        = "The command %(name) has not been found",
     },
     {
+        .error_fmt = QERR_DEVICE_ENCRYPTED,
+        .desc      = "The %(device) is encrypted",
+    },
+    {
         .error_fmt = QERR_DEVICE_NOT_FOUND,
         .desc      = "The %(device) device has not been found",
     },
diff --git a/qerror.h b/qerror.h
index 5198adf..c9fcf97 100644
--- a/qerror.h
+++ b/qerror.h
@@ -41,6 +41,9 @@ QError *qobject_to_qerror(const QObject *obj);
 #define QERR_COMMAND_NOT_FOUND \
         "{ 'class': 'CommandNotFound', 'data': { 'name': %s } }"
 
+#define QERR_DEVICE_ENCRYPTED \
+        "{ 'class': 'DeviceEncrypted', 'data': { 'device': %s } }"
+
 #define QERR_DEVICE_NOT_FOUND \
         "{ 'class': 'DeviceNotFound', 'data': { 'device': %s } }"
 
-- 
1.6.2.5

[Qemu-devel] [FOR 0.12 PATCH 02/18] monitor: do_cont(): Don't ask for passwords

[Markus Armbruster]

From: Luiz Capitulino <lcapitulino@redhat.com>

The do_cont() function will ask the user to enter a password if a
device is encrypted.

This is invalid under QMP, so we raise a QERR_DEVICE_ENCRYPTED
error.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 monitor.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/monitor.c b/monitor.c
index 0ff208e..50c616d 100644
--- a/monitor.c
+++ b/monitor.c
@@ -148,7 +148,10 @@ static void monitor_read_command(Monitor *mon, int show_prompt)
 static int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func,
                                  void *opaque)
 {
-    if (mon->rs) {
+    if (monitor_ctrl_mode(mon)) {
+        qemu_error_new(QERR_MISSING_PARAMETER, "password");
+        return -EINVAL;
+    } else if (mon->rs) {
         readline_start(mon->rs, "Password: ", 1, readline_func, opaque);
         /* prompt is printed on return from the command handler */
         return 0;
@@ -4188,6 +4191,11 @@ void monitor_read_bdrv_key_start(Monitor *mon, BlockDriverState *bs,
         return;
     }
 
+    if (monitor_ctrl_mode(mon)) {
+        qemu_error_new(QERR_DEVICE_ENCRYPTED, bdrv_get_device_name(bs));
+        return;
+    }
+
     monitor_printf(mon, "%s (%s) is encrypted.\n", bdrv_get_device_name(bs),
                    bdrv_get_encrypted_filename(bs));
 
-- 
1.6.2.5

[Qemu-devel] [FOR 0.12 PATCH 03/18] monitor: Fix double-prompt after "change vnc passwd BLA"

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 monitor.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/monitor.c b/monitor.c
index 50c616d..b9d3d92 100644
--- a/monitor.c
+++ b/monitor.c
@@ -845,12 +845,17 @@ static void do_change_block(Monitor *mon, const char *device,
     monitor_read_bdrv_key_start(mon, bs, NULL, NULL);
 }
 
-static void change_vnc_password_cb(Monitor *mon, const char *password,
-                                   void *opaque)
+static void change_vnc_password(Monitor *mon, const char *password)
 {
     if (vnc_display_password(NULL, password) < 0)
         monitor_printf(mon, "could not set VNC server password\n");
 
+}
+
+static void change_vnc_password_cb(Monitor *mon, const char *password,
+                                   void *opaque)
+{
+    change_vnc_password(mon, password);
     monitor_read_command(mon, 1);
 }
 
@@ -862,7 +867,7 @@ static void do_change_vnc(Monitor *mon, const char *target, const char *arg)
             char password[9];
             strncpy(password, arg, sizeof(password));
             password[sizeof(password) - 1] = '\0';
-            change_vnc_password_cb(mon, password, NULL);
+            change_vnc_password(mon, password);
         } else {
             monitor_read_password(mon, change_vnc_password_cb, NULL);
         }
-- 
1.6.2.5

[Qemu-devel] [FOR 0.12 PATCH 04/18] QError: Put error definitions in alphabetical order

[Markus Armbruster]

Also fix the odd typoe and clean up whitespace.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qerror.c |   28 ++++++++++++++--------------
 qerror.h |   30 +++++++++++++++---------------
 2 files changed, 29 insertions(+), 29 deletions(-)

diff --git a/qerror.c b/qerror.c
index d90529c..03e3b34 100644
--- a/qerror.c
+++ b/qerror.c
@@ -41,28 +41,32 @@ static const QType qerror_type = {
  */
 static const QErrorStringTable qerror_table[] = {
     {
-        .error_fmt   = QERR_COMMAND_NOT_FOUND,
-        .desc        = "The command %(name) has not been found",
+        .error_fmt = QERR_COMMAND_NOT_FOUND,
+        .desc      = "The command %(name) has not been found",
     },
     {
         .error_fmt = QERR_DEVICE_ENCRYPTED,
         .desc      = "The %(device) is encrypted",
     },
     {
+        .error_fmt = QERR_DEVICE_NOT_ACTIVE,
+        .desc      = "The %(device) device has not been activated by the guest",
+    },
+    {
         .error_fmt = QERR_DEVICE_NOT_FOUND,
         .desc      = "The %(device) device has not been found",
     },
     {
-        .error_fmt = QERR_DEVICE_NOT_ACTIVE,
-        .desc      = "The %(device) device has not been activated by the guest",
+        .error_fmt = QERR_INVALID_PARAMETER_TYPE,
+        .desc      = "Invalid parameter type, expected: %(expected)",
     },
     {
-        .error_fmt   = QERR_INVALID_PARAMETER_TYPE,
-        .desc        = "Invalid parameter type, expected: %(expected)",
+        .error_fmt = QERR_INVALID_PASSWORD,
+        .desc      = "The entered password is invalid",
     },
     {
-        .error_fmt   = QERR_INVALID_PASSWORD,
-        .desc        = "The entered password is invalid",
+        .error_fmt = QERR_JSON_PARSING,
+        .desc      = "Invalid JSON syntax",
     },
     {
         .error_fmt = QERR_KVM_MISSING_CAP,
@@ -77,12 +81,8 @@ static const QErrorStringTable qerror_table[] = {
         .desc      = "Bad QMP input object",
     },
     {
-        .error_fmt = QERR_JSON_PARSING,
-        .desc      = "Invalid JSON synaxt",
-    },
-    {
-        .error_fmt   = QERR_UNDEFINED_ERROR,
-        .desc        = "An undefined error has ocurred",
+        .error_fmt = QERR_UNDEFINED_ERROR,
+        .desc      = "An undefined error has ocurred",
     },
     {}
 };
diff --git a/qerror.h b/qerror.h
index c9fcf97..062c0c4 100644
--- a/qerror.h
+++ b/qerror.h
@@ -39,36 +39,36 @@ QError *qobject_to_qerror(const QObject *obj);
  * QError class list
  */
 #define QERR_COMMAND_NOT_FOUND \
-        "{ 'class': 'CommandNotFound', 'data': { 'name': %s } }"
+    "{ 'class': 'CommandNotFound', 'data': { 'name': %s } }"
 
 #define QERR_DEVICE_ENCRYPTED \
-        "{ 'class': 'DeviceEncrypted', 'data': { 'device': %s } }"
-
-#define QERR_DEVICE_NOT_FOUND \
-        "{ 'class': 'DeviceNotFound', 'data': { 'device': %s } }"
+    "{ 'class': 'DeviceEncrypted', 'data': { 'device': %s } }"
 
 #define QERR_DEVICE_NOT_ACTIVE \
-        "{ 'class': 'DeviceNotActive', 'data': { 'device': %s } }"
+    "{ 'class': 'DeviceNotActive', 'data': { 'device': %s } }"
+
+#define QERR_DEVICE_NOT_FOUND \
+    "{ 'class': 'DeviceNotFound', 'data': { 'device': %s } }"
 
 #define QERR_INVALID_PARAMETER_TYPE \
-        "{ 'class': 'InvalidParameterType', 'data': { 'name': %s,'expected': %s } }"
+    "{ 'class': 'InvalidParameterType', 'data': { 'name': %s,'expected': %s } }"
 
 #define QERR_INVALID_PASSWORD \
-        "{ 'class': 'InvalidPassword', 'data': {} }"
+    "{ 'class': 'InvalidPassword', 'data': {} }"
+
+#define QERR_JSON_PARSING \
+    "{ 'class': 'JSONParsing', 'data': {} }"
 
 #define QERR_KVM_MISSING_CAP \
-        "{ 'class': 'KVMMissingCap', 'data': { 'capability': %s, 'feature': %s } }"
+    "{ 'class': 'KVMMissingCap', 'data': { 'capability': %s, 'feature': %s } }"
 
 #define QERR_MISSING_PARAMETER \
-        "{ 'class': 'MissingParameter', 'data': { 'name': %s } }"
+    "{ 'class': 'MissingParameter', 'data': { 'name': %s } }"
 
 #define QERR_QMP_BAD_INPUT_OBJECT \
-        "{ 'class': 'QMPBadInputObject', 'data': { 'expected': %s } }"
-
-#define QERR_JSON_PARSING \
-        "{ 'class': 'JSONParsing', 'data': {} }"
+    "{ 'class': 'QMPBadInputObject', 'data': { 'expected': %s } }"
 
 #define QERR_UNDEFINED_ERROR \
-        "{ 'class': 'UndefinedError', 'data': {} }"
+    "{ 'class': 'UndefinedError', 'data': {} }"
 
 #endif /* QERROR_H */
-- 
1.6.2.5

[Qemu-devel] [FOR 0.12 PATCH 05/18] QError: New QERR_DEVICE_LOCKED

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qerror.c |    4 ++++
 qerror.h |    3 +++
 2 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/qerror.c b/qerror.c
index 03e3b34..37e8072 100644
--- a/qerror.c
+++ b/qerror.c
@@ -49,6 +49,10 @@ static const QErrorStringTable qerror_table[] = {
         .desc      = "The %(device) is encrypted",
     },
     {
+        .error_fmt = QERR_DEVICE_LOCKED,
+        .desc      = "Device %(device) is locked",
+    },
+    {
         .error_fmt = QERR_DEVICE_NOT_ACTIVE,
         .desc      = "The %(device) device has not been activated by the guest",
     },
diff --git a/qerror.h b/qerror.h
index 062c0c4..be6cd68 100644
--- a/qerror.h
+++ b/qerror.h
@@ -44,6 +44,9 @@ QError *qobject_to_qerror(const QObject *obj);
 #define QERR_DEVICE_ENCRYPTED \
     "{ 'class': 'DeviceEncrypted', 'data': { 'device': %s } }"
 
+#define QERR_DEVICE_LOCKED                                      \
+    "{ 'class': 'DeviceLocked', 'data': { 'device': %s } }"
+
 #define QERR_DEVICE_NOT_ACTIVE \
     "{ 'class': 'DeviceNotActive', 'data': { 'device': %s } }"
 
-- 
1.6.2.5

[Qemu-devel] [FOR 0.12 PATCH 06/18] QError: New QERR_DEVICE_NOT_REMOVABLE

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qerror.c |    4 ++++
 qerror.h |    3 +++
 2 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/qerror.c b/qerror.c
index 37e8072..17532b0 100644
--- a/qerror.c
+++ b/qerror.c
@@ -61,6 +61,10 @@ static const QErrorStringTable qerror_table[] = {
         .desc      = "The %(device) device has not been found",
     },
     {
+        .error_fmt = QERR_DEVICE_NOT_REMOVABLE,
+        .desc      = "Device %(device) is not removable",
+    },
+    {
         .error_fmt = QERR_INVALID_PARAMETER_TYPE,
         .desc      = "Invalid parameter type, expected: %(expected)",
     },
diff --git a/qerror.h b/qerror.h
index be6cd68..2abff1e 100644
--- a/qerror.h
+++ b/qerror.h
@@ -53,6 +53,9 @@ QError *qobject_to_qerror(const QObject *obj);
 #define QERR_DEVICE_NOT_FOUND \
     "{ 'class': 'DeviceNotFound', 'data': { 'device': %s } }"
 
+#define QERR_DEVICE_NOT_REMOVABLE \
+    "{ 'class': 'DeviceNotRemovable', 'data': { 'device': %s } }"
+
 #define QERR_INVALID_PARAMETER_TYPE \
     "{ 'class': 'InvalidParameterType', 'data': { 'name': %s,'expected': %s } }"
 
-- 
1.6.2.5

[Qemu-devel] [FOR 0.12 PATCH 07/18] monitor: convert do_eject() to QError

[Markus Armbruster]

Also affects do_change(), because the two share eject_device().

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 monitor.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/monitor.c b/monitor.c
index b9d3d92..62d2ef5 100644
--- a/monitor.c
+++ b/monitor.c
@@ -778,11 +778,12 @@ static int eject_device(Monitor *mon, BlockDriverState *bs, int force)
     if (bdrv_is_inserted(bs)) {
         if (!force) {
             if (!bdrv_is_removable(bs)) {
-                monitor_printf(mon, "device is not removable\n");
+                qemu_error_new(QERR_DEVICE_NOT_REMOVABLE,
+                               bdrv_get_device_name(bs));
                 return -1;
             }
             if (bdrv_is_locked(bs)) {
-                monitor_printf(mon, "device is locked\n");
+                qemu_error_new(QERR_DEVICE_LOCKED, bdrv_get_device_name(bs));
                 return -1;
             }
         }
@@ -799,7 +800,7 @@ static void do_eject(Monitor *mon, const QDict *qdict, QObject **ret_data)
 
     bs = bdrv_find(filename);
     if (!bs) {
-        monitor_printf(mon, "device not found\n");
+        qemu_error_new(QERR_DEVICE_NOT_FOUND, filename);
         return;
     }
     eject_device(mon, bs, force);
-- 
1.6.2.5

[Qemu-devel] [FOR 0.12 PATCH 08/18] QError: New QERR_INVALID_BLOCK_FORMAT

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qerror.c |    4 ++++
 qerror.h |    3 +++
 2 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/qerror.c b/qerror.c
index 17532b0..56083c4 100644
--- a/qerror.c
+++ b/qerror.c
@@ -65,6 +65,10 @@ static const QErrorStringTable qerror_table[] = {
         .desc      = "Device %(device) is not removable",
     },
     {
+        .error_fmt = QERR_INVALID_BLOCK_FORMAT,
+        .desc      = "Invalid block format %(name)",
+    },
+    {
         .error_fmt = QERR_INVALID_PARAMETER_TYPE,
         .desc      = "Invalid parameter type, expected: %(expected)",
     },
diff --git a/qerror.h b/qerror.h
index 2abff1e..681390c 100644
--- a/qerror.h
+++ b/qerror.h
@@ -56,6 +56,9 @@ QError *qobject_to_qerror(const QObject *obj);
 #define QERR_DEVICE_NOT_REMOVABLE \
     "{ 'class': 'DeviceNotRemovable', 'data': { 'device': %s } }"
 
+#define QERR_INVALID_BLOCK_FORMAT \
+    "{ 'class': 'InvalidBlockFormat', 'data': { 'name': %s } }"
+
 #define QERR_INVALID_PARAMETER_TYPE \
     "{ 'class': 'InvalidParameterType', 'data': { 'name': %s,'expected': %s } }"
 
-- 
1.6.2.5

[Qemu-devel] [FOR 0.12 PATCH 09/18] QError: New QERR_SET_PASSWD_FAILED

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qerror.c |    4 ++++
 qerror.h |    3 +++
 2 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/qerror.c b/qerror.c
index 56083c4..1bb68c4 100644
--- a/qerror.c
+++ b/qerror.c
@@ -93,6 +93,10 @@ static const QErrorStringTable qerror_table[] = {
         .desc      = "Bad QMP input object",
     },
     {
+        .error_fmt = QERR_SET_PASSWD_FAILED,
+        .desc      = "Could not set password",
+    },
+    {
         .error_fmt = QERR_UNDEFINED_ERROR,
         .desc      = "An undefined error has ocurred",
     },
diff --git a/qerror.h b/qerror.h
index 681390c..b856899 100644
--- a/qerror.h
+++ b/qerror.h
@@ -77,6 +77,9 @@ QError *qobject_to_qerror(const QObject *obj);
 #define QERR_QMP_BAD_INPUT_OBJECT \
     "{ 'class': 'QMPBadInputObject', 'data': { 'expected': %s } }"
 
+#define QERR_SET_PASSWD_FAILED \
+    "{ 'class': 'SetPasswdFailed', 'data': {} }"
+
 #define QERR_UNDEFINED_ERROR \
     "{ 'class': 'UndefinedError', 'data': {} }"
 
-- 
1.6.2.5

[Qemu-devel] [FOR 0.12 PATCH 10/18] QError: New QERR_VNC_SERVER_FAILED

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qerror.c |    4 ++++
 qerror.h |    3 +++
 2 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/qerror.c b/qerror.c
index 1bb68c4..128a91e 100644
--- a/qerror.c
+++ b/qerror.c
@@ -100,6 +100,10 @@ static const QErrorStringTable qerror_table[] = {
         .error_fmt = QERR_UNDEFINED_ERROR,
         .desc      = "An undefined error has ocurred",
     },
+    {
+        .error_fmt = QERR_VNC_SERVER_FAILED,
+        .desc      = "Could not start VNC server on %(target)",
+    },
     {}
 };
 
diff --git a/qerror.h b/qerror.h
index b856899..1a05d9a 100644
--- a/qerror.h
+++ b/qerror.h
@@ -83,4 +83,7 @@ QError *qobject_to_qerror(const QObject *obj);
 #define QERR_UNDEFINED_ERROR \
     "{ 'class': 'UndefinedError', 'data': {} }"
 
+#define QERR_VNC_SERVER_FAILED \
+    "{ 'class': 'VNCServerFailed', 'data': { 'target': %s } }"
+
 #endif /* QERROR_H */
-- 
1.6.2.5

[Qemu-devel] [FOR 0.12 PATCH 11/18] monitor: convert do_change() to QObject, QError

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 monitor.c       |   19 +++++++++++--------
 qemu-monitor.hx |    3 ++-
 2 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/monitor.c b/monitor.c
index 62d2ef5..05233cc 100644
--- a/monitor.c
+++ b/monitor.c
@@ -830,13 +830,13 @@ static void do_change_block(Monitor *mon, const char *device,
 
     bs = bdrv_find(device);
     if (!bs) {
-        monitor_printf(mon, "device not found\n");
+        qemu_error_new(QERR_DEVICE_NOT_FOUND, device);
         return;
     }
     if (fmt) {
         drv = bdrv_find_whitelisted_format(fmt);
         if (!drv) {
-            monitor_printf(mon, "invalid format %s\n", fmt);
+            qemu_error_new(QERR_INVALID_BLOCK_FORMAT, fmt);
             return;
         }
     }
@@ -846,17 +846,17 @@ static void do_change_block(Monitor *mon, const char *device,
     monitor_read_bdrv_key_start(mon, bs, NULL, NULL);
 }
 
-static void change_vnc_password(Monitor *mon, const char *password)
+static void change_vnc_password(const char *password)
 {
     if (vnc_display_password(NULL, password) < 0)
-        monitor_printf(mon, "could not set VNC server password\n");
+        qemu_error_new(QERR_SET_PASSWD_FAILED);
 
 }
 
 static void change_vnc_password_cb(Monitor *mon, const char *password,
                                    void *opaque)
 {
-    change_vnc_password(mon, password);
+    change_vnc_password(password);
     monitor_read_command(mon, 1);
 }
 
@@ -868,17 +868,20 @@ static void do_change_vnc(Monitor *mon, const char *target, const char *arg)
             char password[9];
             strncpy(password, arg, sizeof(password));
             password[sizeof(password) - 1] = '\0';
-            change_vnc_password(mon, password);
+            change_vnc_password(password);
         } else {
             monitor_read_password(mon, change_vnc_password_cb, NULL);
         }
     } else {
         if (vnc_display_open(NULL, target) < 0)
-            monitor_printf(mon, "could not start VNC server on %s\n", target);
+            qemu_error_new(QERR_VNC_SERVER_FAILED, target);
     }
 }
 
-static void do_change(Monitor *mon, const QDict *qdict)
+/**
+ * do_change(): Change a removable medium, or VNC configuration
+ */
+static void do_change(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
     const char *device = qdict_get_str(qdict, "device");
     const char *target = qdict_get_str(qdict, "target");
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index cf2ef9b..c788c73 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -147,7 +147,8 @@ ETEXI
         .args_type  = "device:B,target:F,arg:s?",
         .params     = "device filename [format]",
         .help       = "change a removable medium, optional format",
-        .mhandler.cmd = do_change,
+        .user_print = monitor_user_noop,
+        .mhandler.cmd_new = do_change,
     },
 
 STEXI
-- 
1.6.2.5

[Qemu-devel] [FOR 0.12 PATCH 12/18] QError: New QERR_FD_NOT_FOUND

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qerror.c |    4 ++++
 qerror.h |    3 +++
 2 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/qerror.c b/qerror.c
index 128a91e..d75c69a 100644
--- a/qerror.c
+++ b/qerror.c
@@ -65,6 +65,10 @@ static const QErrorStringTable qerror_table[] = {
         .desc      = "Device %(device) is not removable",
     },
     {
+        .error_fmt = QERR_FD_NOT_FOUND,
+        .desc      = "Failed to find file descriptor named %(name)",
+    },
+    {
         .error_fmt = QERR_INVALID_BLOCK_FORMAT,
         .desc      = "Invalid block format %(name)",
     },
diff --git a/qerror.h b/qerror.h
index 1a05d9a..bda764f 100644
--- a/qerror.h
+++ b/qerror.h
@@ -56,6 +56,9 @@ QError *qobject_to_qerror(const QObject *obj);
 #define QERR_DEVICE_NOT_REMOVABLE \
     "{ 'class': 'DeviceNotRemovable', 'data': { 'device': %s } }"
 
+#define QERR_FD_NOT_FOUND \
+    "{ 'class': 'fd_not_found', 'data': { 'name': %s } }"
+
 #define QERR_INVALID_BLOCK_FORMAT \
     "{ 'class': 'InvalidBlockFormat', 'data': { 'name': %s } }"
 
-- 
1.6.2.5

[Qemu-devel] Re: [FOR 0.12 PATCH 12/18] QError: New QERR_FD_NOT_FOUND

[Luiz Capitulino]

On Mon,  7 Dec 2009 21:37:10 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> +#define QERR_FD_NOT_FOUND \
> +    "{ 'class': 'fd_not_found', 'data': { 'name': %s } }"
> +

 I think this came from your previous series of not using CamelCase?

 Also happens in next patches..

[Qemu-devel] Re: [FOR 0.12 PATCH 12/18] QError: New QERR_FD_NOT_FOUND

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Mon,  7 Dec 2009 21:37:10 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> +#define QERR_FD_NOT_FOUND \
>> +    "{ 'class': 'fd_not_found', 'data': { 'name': %s } }"
>> +
>
>  I think this came from your previous series of not using CamelCase?
>
>  Also happens in next patches..

Rats.  Fixing...

[Qemu-devel] [FOR 0.12 PATCH 13/18] monitor: convert do_closefd() to QError

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 monitor.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/monitor.c b/monitor.c
index 05233cc..c35a31e 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2195,8 +2195,7 @@ static void do_closefd(Monitor *mon, const QDict *qdict, QObject **ret_data)
         return;
     }
 
-    monitor_printf(mon, "Failed to find file descriptor named %s\n",
-                   fdname);
+    qemu_error_new(QERR_FD_NOT_FOUND, fdname);
 }
 
 static void do_loadvm(Monitor *mon, const QDict *qdict)
-- 
1.6.2.5

[Qemu-devel] [FOR 0.12 PATCH 14/18] QError: New QERR_FD_NOT_SUPPLIED

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qerror.c |    4 ++++
 qerror.h |    3 +++
 2 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/qerror.c b/qerror.c
index d75c69a..e6b7f62 100644
--- a/qerror.c
+++ b/qerror.c
@@ -69,6 +69,10 @@ static const QErrorStringTable qerror_table[] = {
         .desc      = "Failed to find file descriptor named %(name)",
     },
     {
+        .error_fmt = QERR_FD_NOT_SUPPLIED,
+        .desc      = "No file descriptor supplied via SCM_RIGHTS",
+    },
+    {
         .error_fmt = QERR_INVALID_BLOCK_FORMAT,
         .desc      = "Invalid block format %(name)",
     },
diff --git a/qerror.h b/qerror.h
index bda764f..2262619 100644
--- a/qerror.h
+++ b/qerror.h
@@ -59,6 +59,9 @@ QError *qobject_to_qerror(const QObject *obj);
 #define QERR_FD_NOT_FOUND \
     "{ 'class': 'fd_not_found', 'data': { 'name': %s } }"
 
+#define QERR_FD_NOT_SUPPLIED \
+    "{ 'class': 'fd_not_supplied', 'data': {} }"
+
 #define QERR_INVALID_BLOCK_FORMAT \
     "{ 'class': 'InvalidBlockFormat', 'data': { 'name': %s } }"
 
-- 
1.6.2.5

[Qemu-devel] [FOR 0.12 PATCH 15/18] New QERR_INVALID_PARAMETER

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qerror.c |    4 ++++
 qerror.h |    3 +++
 2 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/qerror.c b/qerror.c
index e6b7f62..aa89a3d 100644
--- a/qerror.c
+++ b/qerror.c
@@ -77,6 +77,10 @@ static const QErrorStringTable qerror_table[] = {
         .desc      = "Invalid block format %(name)",
     },
     {
+        .error_fmt = QERR_INVALID_PARAMETER,
+        .desc      = "Invalid parameter %(name)",
+    },
+    {
         .error_fmt = QERR_INVALID_PARAMETER_TYPE,
         .desc      = "Invalid parameter type, expected: %(expected)",
     },
diff --git a/qerror.h b/qerror.h
index 2262619..48f1ab6 100644
--- a/qerror.h
+++ b/qerror.h
@@ -65,6 +65,9 @@ QError *qobject_to_qerror(const QObject *obj);
 #define QERR_INVALID_BLOCK_FORMAT \
     "{ 'class': 'InvalidBlockFormat', 'data': { 'name': %s } }"
 
+#define QERR_INVALID_PARAMETER \
+    "{ 'class': 'invalid_parameter', 'data': { 'name': %s } }"
+
 #define QERR_INVALID_PARAMETER_TYPE \
     "{ 'class': 'InvalidParameterType', 'data': { 'name': %s,'expected': %s } }"
 
-- 
1.6.2.5

[Qemu-devel] [FOR 0.12 PATCH 16/18] QError: New QERR_TOO_MANY_FILES

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qerror.c |    4 ++++
 qerror.h |    3 +++
 2 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/qerror.c b/qerror.c
index aa89a3d..8ffe4f6 100644
--- a/qerror.c
+++ b/qerror.c
@@ -109,6 +109,10 @@ static const QErrorStringTable qerror_table[] = {
         .desc      = "Could not set password",
     },
     {
+        .error_fmt = QERR_TOO_MANY_FILES,
+        .desc      = "Too many open files",
+    },
+    {
         .error_fmt = QERR_UNDEFINED_ERROR,
         .desc      = "An undefined error has ocurred",
     },
diff --git a/qerror.h b/qerror.h
index 48f1ab6..9462d5c 100644
--- a/qerror.h
+++ b/qerror.h
@@ -92,6 +92,9 @@ QError *qobject_to_qerror(const QObject *obj);
 #define QERR_UNDEFINED_ERROR \
     "{ 'class': 'UndefinedError', 'data': {} }"
 
+#define QERR_TOO_MANY_FILES \
+    "{ 'class': 'fd_too_many_files', 'data': {} }"
+
 #define QERR_VNC_SERVER_FAILED \
     "{ 'class': 'VNCServerFailed', 'data': { 'target': %s } }"
 
-- 
1.6.2.5

[Qemu-devel] [FOR 0.12 PATCH 17/18] monitor: convert do_getfd() to QError

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 monitor.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/monitor.c b/monitor.c
index c35a31e..0bcffbe 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2145,19 +2145,21 @@ static void do_getfd(Monitor *mon, const QDict *qdict, QObject **ret_data)
 
     fd = qemu_chr_get_msgfd(mon->chr);
     if (fd == -1) {
-        monitor_printf(mon, "getfd: no file descriptor supplied via SCM_RIGHTS\n");
+        qemu_error_new(QERR_FD_NOT_SUPPLIED);
         return;
     }
 
     if (qemu_isdigit(fdname[0])) {
-        monitor_printf(mon, "getfd: monitor names may not begin with a number\n");
+        qemu_error_new(QERR_INVALID_PARAMETER, "fdname");
         return;
     }
 
     fd = dup(fd);
     if (fd == -1) {
-        monitor_printf(mon, "Failed to dup() file descriptor: %s\n",
-                       strerror(errno));
+        if (errno == EMFILE)
+            qemu_error_new(QERR_TOO_MANY_FILES);
+        else
+            qemu_error_new(QERR_UNDEFINED_ERROR);
         return;
     }
 
-- 
1.6.2.5

[Qemu-devel] [FOR 0.12 PATCH 18/18] QMP: add human-readable description to error response

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 QMP/qmp-spec.txt |    5 ++++-
 monitor.c        |    1 +
 qerror.c         |   21 ++++++++++++++++-----
 qerror.h         |    2 ++
 4 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/QMP/qmp-spec.txt b/QMP/qmp-spec.txt
index 8429789..1cbd21c 100644
--- a/QMP/qmp-spec.txt
+++ b/QMP/qmp-spec.txt
@@ -102,13 +102,16 @@ completed because of an error condition.
 
 The format is:
 
-{ "error": { "class": json-string, "data": json-value }, "id": json-value }
+{ "error": { "class": json-string, "data": json-value, "desc": json-string },
+  "id": json-value }
 
  Where,
 
 - The "class" member contains the error class name (eg. "ServiceUnavailable")
 - The "data" member contains specific error data and is defined in a
   per-command basis, it will be an empty json-object if the error has no data
+- The "desc" member is a human-readable error message.  Clients should
+  not attempt to parse this message.
 - The "id" member contains the transaction identification associated with
   the command execution (if issued by the Client)
 
diff --git a/monitor.c b/monitor.c
index 0bcffbe..4ad1b5e 100644
--- a/monitor.c
+++ b/monitor.c
@@ -305,6 +305,7 @@ static void monitor_protocol_emitter(Monitor *mon, QObject *data)
         }
     } else {
         /* error response */
+        qdict_put(mon->error->error, "desc", qerror_human(mon->error));
         qdict_put(qmp, "error", mon->error->error);
         QINCREF(mon->error->error);
         QDECREF(mon->error);
diff --git a/qerror.c b/qerror.c
index 8ffe4f6..5f8fc5d 100644
--- a/qerror.c
+++ b/qerror.c
@@ -283,13 +283,11 @@ static const char *append_field(QString *outstr, const QError *qerror,
 }
 
 /**
- * qerror_print(): Print QError data
+ * qerror_human(): Format QError data into human-readable string.
  *
- * This function will print the member 'desc' of the specified QError object,
- * it uses qemu_error() for this, so that the output is routed to the right
- * place (ie. stderr or Monitor's device).
+ * Formats according to member 'desc' of the specified QError object.
  */
-void qerror_print(const QError *qerror)
+QString *qerror_human(const QError *qerror)
 {
     const char *p;
     QString *qstring;
@@ -309,6 +307,19 @@ void qerror_print(const QError *qerror)
         }
     }
 
+    return qstring;
+}
+
+/**
+ * qerror_print(): Print QError data
+ *
+ * This function will print the member 'desc' of the specified QError object,
+ * it uses qemu_error() for this, so that the output is routed to the right
+ * place (ie. stderr or Monitor's device).
+ */
+void qerror_print(const QError *qerror)
+{
+    QString *qstring = qerror_human(qerror);
     qemu_error("%s\n", qstring_get_str(qstring));
     QDECREF(qstring);
 }
diff --git a/qerror.h b/qerror.h
index 9462d5c..09e32b9 100644
--- a/qerror.h
+++ b/qerror.h
@@ -13,6 +13,7 @@
 #define QERROR_H
 
 #include "qdict.h"
+#include "qstring.h"
 #include <stdarg.h>
 
 typedef struct QErrorStringTable {
@@ -32,6 +33,7 @@ typedef struct QError {
 QError *qerror_new(void);
 QError *qerror_from_info(const char *file, int linenr, const char *func,
                          const char *fmt, va_list *va);
+QString *qerror_human(const QError *qerror);
 void qerror_print(const QError *qerror);
 QError *qobject_to_qerror(const QObject *obj);
 
-- 
1.6.2.5

[Qemu-devel] Re: [FOR 0.12 PATCH 18/18] QMP: add human-readable description to error response

[Luiz Capitulino]

On Mon,  7 Dec 2009 21:37:16 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> -{ "error": { "class": json-string, "data": json-value }, "id": json-value }
> +{ "error": { "class": json-string, "data": json-value, "desc": json-string },
> +  "id": json-value }
>  
>   Where,
>  
>  - The "class" member contains the error class name (eg. "ServiceUnavailable")
>  - The "data" member contains specific error data and is defined in a
>    per-command basis, it will be an empty json-object if the error has no data
> +- The "desc" member is a human-readable error message.  Clients should
> +  not attempt to parse this message.
>  - The "id" member contains the transaction identification associated with
>    the command execution (if issued by the Client)

 As we've talked on irc, I don't agree with this change.

 Basically, adding 'desc' to the standard error message introduces all
the problems we've discussed about free-form English strings.

 I feel that QError is becoming the worst of all proposals.

 I agree with you that it's not as easy as it should be to report errors,
but as we're targeting on Clients I was convinced that we could not have the
best API internally but offer a good interface for Clients.

 Now, having 'desc' as part of the standard protocol is like not having
the best API internally and offering a bad interface for Clients.

 Not to mention that those strings can't be modified when the protocol
becomes stable and we're probably talking about dozens if not a hundred
of strings. Ok, there isn't a reason to change them often, but it's
still one more thing to maintain.

 Having said that, I would agree to have 'desc' as part of debug
information. I have patches in my tree which adds CONFIG_DEBUG_QMP,
if one enables it information about the error location will also
be part of the error message. I would agree having 'desc' there
too.

Re: [Qemu-devel] Re: [FOR 0.12 PATCH 18/18] QMP: add human-readable description to error response

[Daniel P. Berrange]

On Tue, Dec 08, 2009 at 10:11:48AM -0200, Luiz Capitulino wrote:
> On Mon,  7 Dec 2009 21:37:16 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
> 
> > -{ "error": { "class": json-string, "data": json-value }, "id": json-value }
> > +{ "error": { "class": json-string, "data": json-value, "desc": json-string },
> > +  "id": json-value }
> >  
> >   Where,
> >  
> >  - The "class" member contains the error class name (eg. "ServiceUnavailable")
> >  - The "data" member contains specific error data and is defined in a
> >    per-command basis, it will be an empty json-object if the error has no data
> > +- The "desc" member is a human-readable error message.  Clients should
> > +  not attempt to parse this message.
> >  - The "id" member contains the transaction identification associated with
> >    the command execution (if issued by the Client)
> 
>  As we've talked on irc, I don't agree with this change.
> 
>  Basically, adding 'desc' to the standard error message introduces all
> the problems we've discussed about free-form English strings.
> 
>  I feel that QError is becoming the worst of all proposals.
> 
>  I agree with you that it's not as easy as it should be to report errors,
> but as we're targeting on Clients I was convinced that we could not have the
> best API internally but offer a good interface for Clients.
> 
>  Now, having 'desc' as part of the standard protocol is like not having
> the best API internally and offering a bad interface for Clients.
> 
>  Not to mention that those strings can't be modified when the protocol
> becomes stable and we're probably talking about dozens if not a hundred
> of strings. Ok, there isn't a reason to change them often, but it's
> still one more thing to maintain.

I think it is fine to declare that 'desc' strings are subject to 
arbitrary change. Even if QEMU includes this 'desc' I think in libvirt
will end up doing its own error code -> human string conversion,
simply because we need to translate the strings. So we'd only use
'desc' in any logging calls if it were present.

>  Having said that, I would agree to have 'desc' as part of debug
> information. I have patches in my tree which adds CONFIG_DEBUG_QMP,
> if one enables it information about the error location will also
> be part of the error message. I would agree having 'desc' there
> too.


Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

Re: [Qemu-devel] Re: [FOR 0.12 PATCH 18/18] QMP: add human-readable description to error response

[Luiz Capitulino]

On Tue, 8 Dec 2009 12:25:13 +0000
"Daniel P. Berrange" <berrange@redhat.com> wrote:

> On Tue, Dec 08, 2009 at 10:11:48AM -0200, Luiz Capitulino wrote:
> > On Mon,  7 Dec 2009 21:37:16 +0100
> > Markus Armbruster <armbru@redhat.com> wrote:
> > 
> > > -{ "error": { "class": json-string, "data": json-value }, "id": json-value }
> > > +{ "error": { "class": json-string, "data": json-value, "desc": json-string },
> > > +  "id": json-value }
> > >  
> > >   Where,
> > >  
> > >  - The "class" member contains the error class name (eg. "ServiceUnavailable")
> > >  - The "data" member contains specific error data and is defined in a
> > >    per-command basis, it will be an empty json-object if the error has no data
> > > +- The "desc" member is a human-readable error message.  Clients should
> > > +  not attempt to parse this message.
> > >  - The "id" member contains the transaction identification associated with
> > >    the command execution (if issued by the Client)
> > 
> >  As we've talked on irc, I don't agree with this change.
> > 
> >  Basically, adding 'desc' to the standard error message introduces all
> > the problems we've discussed about free-form English strings.
> > 
> >  I feel that QError is becoming the worst of all proposals.
> > 
> >  I agree with you that it's not as easy as it should be to report errors,
> > but as we're targeting on Clients I was convinced that we could not have the
> > best API internally but offer a good interface for Clients.
> > 
> >  Now, having 'desc' as part of the standard protocol is like not having
> > the best API internally and offering a bad interface for Clients.
> > 
> >  Not to mention that those strings can't be modified when the protocol
> > becomes stable and we're probably talking about dozens if not a hundred
> > of strings. Ok, there isn't a reason to change them often, but it's
> > still one more thing to maintain.
> 
> I think it is fine to declare that 'desc' strings are subject to 
> arbitrary change. Even if QEMU includes this 'desc' I think in libvirt
> will end up doing its own error code -> human string conversion,
> simply because we need to translate the strings. So we'd only use
> 'desc' in any logging calls if it were present.

 That's what we would expect from clients, but I'm not convinced that
they'll took the easy way and start using it the way they shouldn't.

Re: [Qemu-devel] Re: [FOR 0.12 PATCH 18/18] QMP: add human-readable description to error response

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Tue, 8 Dec 2009 12:25:13 +0000
> "Daniel P. Berrange" <berrange@redhat.com> wrote:
>
>> On Tue, Dec 08, 2009 at 10:11:48AM -0200, Luiz Capitulino wrote:
>> > On Mon,  7 Dec 2009 21:37:16 +0100
>> > Markus Armbruster <armbru@redhat.com> wrote:
>> > 
>> > > -{ "error": { "class": json-string, "data": json-value }, "id": json-value }
>> > > +{ "error": { "class": json-string, "data": json-value, "desc": json-string },
>> > > +  "id": json-value }
>> > >  
>> > >   Where,
>> > >  
>> > >  - The "class" member contains the error class name (eg. "ServiceUnavailable")
>> > >  - The "data" member contains specific error data and is defined in a
>> > >    per-command basis, it will be an empty json-object if the error has no data
>> > > +- The "desc" member is a human-readable error message.  Clients should
>> > > +  not attempt to parse this message.
>> > >  - The "id" member contains the transaction identification associated with
>> > >    the command execution (if issued by the Client)
>> > 
>> >  As we've talked on irc, I don't agree with this change.
>> > 
>> >  Basically, adding 'desc' to the standard error message introduces all
>> > the problems we've discussed about free-form English strings.
>> > 
>> >  I feel that QError is becoming the worst of all proposals.
>> > 
>> >  I agree with you that it's not as easy as it should be to report errors,
>> > but as we're targeting on Clients I was convinced that we could not have the
>> > best API internally but offer a good interface for Clients.
>> > 
>> >  Now, having 'desc' as part of the standard protocol is like not having
>> > the best API internally and offering a bad interface for Clients.
>> > 
>> >  Not to mention that those strings can't be modified when the protocol
>> > becomes stable and we're probably talking about dozens if not a hundred
>> > of strings. Ok, there isn't a reason to change them often, but it's
>> > still one more thing to maintain.
>> 
>> I think it is fine to declare that 'desc' strings are subject to 
>> arbitrary change. Even if QEMU includes this 'desc' I think in libvirt
>> will end up doing its own error code -> human string conversion,
>> simply because we need to translate the strings. So we'd only use
>> 'desc' in any logging calls if it were present.
>
>  That's what we would expect from clients, but I'm not convinced that
> they'll took the easy way and start using it the way they shouldn't.

Change a couple of common messages in every release, and they'll stop
doing that real fast ;)

[Qemu-devel] Re: [FOR 0.12 PATCH 18/18] QMP: add human-readable description to error response

[Paolo Bonzini]

On 12/08/2009 01:11 PM, Luiz Capitulino wrote:
>   Not to mention that those strings can't be modified when the protocol
> becomes stable and we're probably talking about dozens if not a hundred
> of strings.

I would say that there is _explicitly no promise_ of keeping these 
stable.  You can serve

HTTP/1.1 302 You have bad taste in music
Location: http://www.britneyspears.com/

and clients will not complain.  It's the same for QMP and desc.

(Disclaimer: I haven't read the entire series from Markus).

Paolo

[Qemu-devel] Re: [FOR 0.12 PATCH 18/18] QMP: add human-readable description to error response

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 12/08/2009 01:11 PM, Luiz Capitulino wrote:
>>   Not to mention that those strings can't be modified when the protocol
>> becomes stable and we're probably talking about dozens if not a hundred
>> of strings.
>
> I would say that there is _explicitly no promise_ of keeping these
> stable.  You can serve
>
> HTTP/1.1 302 You have bad taste in music
> Location: http://www.britneyspears.com/
>
> and clients will not complain.

And even serve later in the same session

HTTP/1.1 302 Bad taste in music you have
Location: http://www.britneyspears.com/

>                                 It's the same for QMP and desc.

Precisely.

> (Disclaimer: I haven't read the entire series from Markus).

[Qemu-devel] Re: [FOR 0.12 PATCH 18/18] QMP: add human-readable description to error response

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Mon,  7 Dec 2009 21:37:16 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> -{ "error": { "class": json-string, "data": json-value }, "id": json-value }
>> +{ "error": { "class": json-string, "data": json-value, "desc": json-string },
>> +  "id": json-value }
>>  
>>   Where,
>>  
>>  - The "class" member contains the error class name (eg. "ServiceUnavailable")
>>  - The "data" member contains specific error data and is defined in a
>>    per-command basis, it will be an empty json-object if the error has no data
>> +- The "desc" member is a human-readable error message.  Clients should
>> +  not attempt to parse this message.
>>  - The "id" member contains the transaction identification associated with
>>    the command execution (if issued by the Client)
>
>  As we've talked on irc, I don't agree with this change.
>
>  Basically, adding 'desc' to the standard error message introduces all
> the problems we've discussed about free-form English strings.

I disagree.  See below.

>  I feel that QError is becoming the worst of all proposals.

I'm not exactly thrilled about it either, but it could clearly be worse
in many ways, so it can't be the worst of all :)

>  I agree with you that it's not as easy as it should be to report errors,
> but as we're targeting on Clients I was convinced that we could not have the
> best API internally but offer a good interface for Clients.
>
>  Now, having 'desc' as part of the standard protocol is like not having
> the best API internally and offering a bad interface for Clients.
>
>  Not to mention that those strings can't be modified when the protocol
> becomes stable

This is incorrect.  We explicitly threaten client writers that these
strings are not to be interpreted, in qmp-spec.txt: "Clients should not
attempt to parse this message."  For me, that implies that they can
change at any time.  Maybe we could use even stronger language there.

>                and we're probably talking about dozens if not a hundred
> of strings. Ok, there isn't a reason to change them often, but it's
> still one more thing to maintain.
>
>  Having said that, I would agree to have 'desc' as part of debug
> information. I have patches in my tree which adds CONFIG_DEBUG_QMP,
> if one enables it information about the error location will also
> be part of the error message. I would agree having 'desc' there
> too.

Debugging is one use for human-readable text in the error reply.  But it
is *not* the only use.  We discussed this at length in October[*], and
again in November[**].  Any new arguments?


[*] Sub-thread starting at
http://lists.gnu.org/archive/html/qemu-devel/2009-10/msg01245.html

[**] Sub-thread starting at
http://lists.gnu.org/archive/html/qemu-devel/2009-11/msg01071.html

Re: [Qemu-devel] Re: [FOR 0.12 PATCH 18/18] QMP: add human-readable description to error response

[Anthony Liguori]

Luiz Capitulino wrote:
> On Mon,  7 Dec 2009 21:37:16 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
>
>   
>> -{ "error": { "class": json-string, "data": json-value }, "id": json-value }
>> +{ "error": { "class": json-string, "data": json-value, "desc": json-string },
>> +  "id": json-value }
>>  
>>   Where,
>>  
>>  - The "class" member contains the error class name (eg. "ServiceUnavailable")
>>  - The "data" member contains specific error data and is defined in a
>>    per-command basis, it will be an empty json-object if the error has no data
>> +- The "desc" member is a human-readable error message.  Clients should
>> +  not attempt to parse this message.
>>  - The "id" member contains the transaction identification associated with
>>    the command execution (if issued by the Client)
>>     
>
>  As we've talked on irc, I don't agree with this change.
>
>  Basically, adding 'desc' to the standard error message introduces all
> the problems we've discussed about free-form English strings.
>   

It's not free form English.  The 'desc' string is always autogenerated 
based on the error object.

It's completely redundant information because you can already generate 
that string, but it simplifies client creation because a lazy client 
does not have to include the conversion table if they only care about 
English error output.

Regards,

Anthony Liguori

Re: [Qemu-devel] Re: [FOR 0.12 PATCH 18/18] QMP: add human-readable description to error response

[Luiz Capitulino]

On Tue, 08 Dec 2009 07:18:11 -0600
Anthony Liguori <anthony@codemonkey.ws> wrote:

> Luiz Capitulino wrote:
> > On Mon,  7 Dec 2009 21:37:16 +0100
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >   
> >> -{ "error": { "class": json-string, "data": json-value }, "id": json-value }
> >> +{ "error": { "class": json-string, "data": json-value, "desc": json-string },
> >> +  "id": json-value }
> >>  
> >>   Where,
> >>  
> >>  - The "class" member contains the error class name (eg. "ServiceUnavailable")
> >>  - The "data" member contains specific error data and is defined in a
> >>    per-command basis, it will be an empty json-object if the error has no data
> >> +- The "desc" member is a human-readable error message.  Clients should
> >> +  not attempt to parse this message.
> >>  - The "id" member contains the transaction identification associated with
> >>    the command execution (if issued by the Client)
> >>     
> >
> >  As we've talked on irc, I don't agree with this change.
> >
> >  Basically, adding 'desc' to the standard error message introduces all
> > the problems we've discussed about free-form English strings.
> >   
> 
> It's not free form English.  The 'desc' string is always autogenerated 
> based on the error object.
> 
> It's completely redundant information because you can already generate 
> that string, but it simplifies client creation because a lazy client 
> does not have to include the conversion table if they only care about 
> English error output.

 I wonder if we could have a simpler design for the internal API if
we knew in advance that 'desc' would be part of the standard
error message, eg. error code based.

 But, as Markus said it's not the 'worst' as I've put it, let's
move forward then.

[Qemu-devel] Re: [FOR 0.12 PATCH 00/18] QError conversions and more

[Luiz Capitulino]

On Mon,  7 Dec 2009 21:36:58 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> This patch series converts monitor commands eject, change, closefd,
> getfd to QError.

 Thanks a lot for this work.

 Although the series is already merged, I have some comments and will
reply the patches.