[Qemu-devel] [PATCH v2 0/9] qmp hmp balloon: Cleanups around error reporting

[Qemu-devel] [PATCH v2 0/9] qmp hmp balloon: Cleanups around error reporting

[Markus Armbruster]

I'm including balloon patches in the hope that they too can go through
Luiz's tree.

v2:
* PATCH 1+8: Use bool [Eric]
* PATCH 9: Clarify commit message [Eric]
* R-bys retrained since changes are trivial

Markus Armbruster (9):
  qmp hmp: Factor out common "using spice" test
  qmp hmp: Improve error messages when SPICE is not in use
  hmp: Compile hmp_info_spice() only with CONFIG_SPICE
  qmp: Clean up qmp_query_spice() #ifndef !CONFIG_SPICE dummy
  qmp: Simplify recognition of capability negotiation command
  qmp: Eliminate silly QERR_COMMAND_NOT_FOUND macro
  balloon: Inline qemu_balloon(), qemu_balloon_status()
  balloon: Factor out common "is balloon active" test
  balloon: Eliminate silly QERR_ macros

 balloon.c                 | 59 ++++++++++++++++++-----------------------------
 hmp.c                     |  2 ++
 include/qapi/qmp/qerror.h |  9 --------
 include/ui/qemu-spice.h   | 10 ++++++++
 monitor.c                 | 19 +++++++--------
 qapi/qmp-dispatch.c       |  3 ++-
 qmp.c                     | 27 +++++++++++-----------
 7 files changed, 58 insertions(+), 71 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH v2 1/9] qmp hmp: Factor out common "using spice" test

[Markus Armbruster]

Into qemu_using_spice().  For want of a better place, put it next the
existing monitor command handler dummies in qemu-spice.h.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>
---
 include/ui/qemu-spice.h | 10 ++++++++++
 monitor.c               |  5 +++--
 qmp.c                   | 11 +++--------
 3 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/include/ui/qemu-spice.h b/include/ui/qemu-spice.h
index a93b4b2..db7926d 100644
--- a/include/ui/qemu-spice.h
+++ b/include/ui/qemu-spice.h
@@ -88,4 +88,14 @@ static inline int qemu_spice_display_add_client(int csock, int skipauth,
 
 #endif /* CONFIG_SPICE */
 
+static inline bool qemu_using_spice(Error **errp)
+{
+    if (!using_spice) {
+        /* correct one? spice isn't a device ,,, */
+        error_set(errp, QERR_DEVICE_NOT_ACTIVE, "spice");
+        return false;
+    }
+    return true;
+}
+
 #endif /* QEMU_SPICE_H */
diff --git a/monitor.c b/monitor.c
index 7e4f605..8323de3 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1095,11 +1095,12 @@ static int client_migrate_info(Monitor *mon, const QDict *qdict,
     const char *subject  = qdict_get_try_str(qdict, "cert-subject");
     int port             = qdict_get_try_int(qdict, "port", -1);
     int tls_port         = qdict_get_try_int(qdict, "tls-port", -1);
+    Error *err;
     int ret;
 
     if (strcmp(protocol, "spice") == 0) {
-        if (!using_spice) {
-            qerror_report(QERR_DEVICE_NOT_ACTIVE, "spice");
+        if (!qemu_using_spice(&err)) {
+            qerror_report_err(err);
             return -1;
         }
 
diff --git a/qmp.c b/qmp.c
index 963305c..ef155ff 100644
--- a/qmp.c
+++ b/qmp.c
@@ -287,9 +287,7 @@ void qmp_set_password(const char *protocol, const char *password,
     }
 
     if (strcmp(protocol, "spice") == 0) {
-        if (!using_spice) {
-            /* correct one? spice isn't a device ,,, */
-            error_set(errp, QERR_DEVICE_NOT_ACTIVE, "spice");
+        if (!qemu_using_spice(errp)) {
             return;
         }
         rc = qemu_spice_set_passwd(password, fail_if_connected,
@@ -335,9 +333,7 @@ void qmp_expire_password(const char *protocol, const char *whenstr,
     }
 
     if (strcmp(protocol, "spice") == 0) {
-        if (!using_spice) {
-            /* correct one? spice isn't a device ,,, */
-            error_set(errp, QERR_DEVICE_NOT_ACTIVE, "spice");
+        if (!qemu_using_spice(errp)) {
             return;
         }
         rc = qemu_spice_set_pw_expire(when);
@@ -575,8 +571,7 @@ void qmp_add_client(const char *protocol, const char *fdname,
     }
 
     if (strcmp(protocol, "spice") == 0) {
-        if (!using_spice) {
-            error_set(errp, QERR_DEVICE_NOT_ACTIVE, "spice");
+        if (!qemu_using_spice(errp)) {
             close(fd);
             return;
         }
-- 
1.9.3

[Qemu-devel] [PATCH v2 2/9] qmp hmp: Improve error messages when SPICE is not in use

[Markus Armbruster]

Commit 7572150 adopted QERR_DEVICE_NOT_ACTIVE for the purpose,
probably because adding another error seemed cumbersome overkill.
Produces "No spice device has been activated", which is awkward.

We've since abandoned our quest for "rich" error objects.  Time to
undo the damage to this error message.  Replace it by "SPICE is not in
use".

Keep the stupid DeviceNotActive ErrorClass for compatibility, even
though Libvirt doesn't use it.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>
---
 include/ui/qemu-spice.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/ui/qemu-spice.h b/include/ui/qemu-spice.h
index db7926d..762e063 100644
--- a/include/ui/qemu-spice.h
+++ b/include/ui/qemu-spice.h
@@ -91,8 +91,8 @@ static inline int qemu_spice_display_add_client(int csock, int skipauth,
 static inline bool qemu_using_spice(Error **errp)
 {
     if (!using_spice) {
-        /* correct one? spice isn't a device ,,, */
-        error_set(errp, QERR_DEVICE_NOT_ACTIVE, "spice");
+        error_set(errp, ERROR_CLASS_DEVICE_NOT_ACTIVE,
+                  "SPICE is not in use");
         return false;
     }
     return true;
-- 
1.9.3

[Qemu-devel] [PATCH v2 3/9] hmp: Compile hmp_info_spice() only with CONFIG_SPICE

[Markus Armbruster]

It's dead code when CONFIG_SPICE is off.  If it wasn't, it would crash
dereferencing the null pointer returned by the qmp_query_spice()
dummy in qmp.c.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hmp.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hmp.c b/hmp.c
index 481be80..a42c5c0 100644
--- a/hmp.c
+++ b/hmp.c
@@ -535,6 +535,7 @@ out:
     qapi_free_VncInfo(info);
 }
 
+#ifdef CONFIG_SPICE
 void hmp_info_spice(Monitor *mon, const QDict *qdict)
 {
     SpiceChannelList *chan;
@@ -581,6 +582,7 @@ void hmp_info_spice(Monitor *mon, const QDict *qdict)
 out:
     qapi_free_SpiceInfo(info);
 }
+#endif
 
 void hmp_info_balloon(Monitor *mon, const QDict *qdict)
 {
-- 
1.9.3

[Qemu-devel] [PATCH v2 4/9] qmp: Clean up qmp_query_spice() #ifndef !CONFIG_SPICE dummy

[Markus Armbruster]

QMP command query-spice exists only #ifdef CONFIG_SPICE.  Due to QAPI
limitations, we need a dummy function anyway, but it's unreachable.

Our current dummy function goes out of its way to produce the exact
same error as the QMP core does for unknown commands.  Cute, but both
unclean and unnecessary.  Replace by straight abort().

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>
---
 qmp.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/qmp.c b/qmp.c
index ef155ff..7f2d25a 100644
--- a/qmp.c
+++ b/qmp.c
@@ -137,14 +137,18 @@ VncInfo *qmp_query_vnc(Error **errp)
 #endif
 
 #ifndef CONFIG_SPICE
-/* If SPICE support is enabled, the "true" query-spice command is
-   defined in the SPICE subsystem. Also note that we use a small
-   trick to maintain query-spice's original behavior, which is not
-   to be available in the namespace if SPICE is not compiled in */
+/*
+ * qmp-commands.hx ensures that QMP command query-spice exists only
+ * #ifdef CONFIG_SPICE.  Necessary for an accurate query-commands
+ * result.  However, the QAPI schema is blissfully unaware of that,
+ * and the QAPI code generator happily generates a dead
+ * qmp_marshal_input_query_spice() that calls qmp_query_spice().
+ * Provide it one, or else linking fails.
+ * FIXME Educate the QAPI schema on CONFIG_SPICE.
+ */
 SpiceInfo *qmp_query_spice(Error **errp)
 {
-    error_set(errp, QERR_COMMAND_NOT_FOUND, "query-spice");
-    return NULL;
+    abort();
 };
 #endif
 
-- 
1.9.3

[Qemu-devel] [PATCH v2 5/9] qmp: Simplify recognition of capability negotiation command

[Markus Armbruster]

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

diff --git a/monitor.c b/monitor.c
index 8323de3..2ac40c9 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4783,9 +4783,9 @@ static int monitor_can_read(void *opaque)
     return (mon->suspend_cnt == 0) ? 1 : 0;
 }
 
-static int invalid_qmp_mode(const Monitor *mon, const char *cmd_name)
+static int invalid_qmp_mode(const Monitor *mon, const mon_cmd_t *cmd)
 {
-    int is_cap = compare_cmd(cmd_name, "qmp_capabilities");
+    int is_cap = cmd->mhandler.cmd_new == do_qmp_capabilities;
     return (qmp_cmd_mode(mon) ? is_cap : !is_cap);
 }
 
@@ -5079,13 +5079,8 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
 
     cmd_name = qdict_get_str(input, "execute");
     trace_handle_qmp_command(mon, cmd_name);
-    if (invalid_qmp_mode(mon, cmd_name)) {
-        qerror_report(QERR_COMMAND_NOT_FOUND, cmd_name);
-        goto err_out;
-    }
-
     cmd = qmp_find_cmd(cmd_name);
-    if (!cmd) {
+    if (!cmd || invalid_qmp_mode(mon, cmd)) {
         qerror_report(QERR_COMMAND_NOT_FOUND, cmd_name);
         goto err_out;
     }
-- 
1.9.3

[Qemu-devel] [PATCH v2 6/9] qmp: Eliminate silly QERR_COMMAND_NOT_FOUND macro

[Markus Armbruster]

The QERR_ macros are leftovers from the days of "rich" error objects.
They're used with error_set() and qerror_report(), and expand into the
first *two* arguments.  This trickiness has become pointless.  Clean
this one up.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 include/qapi/qmp/qerror.h | 3 ---
 monitor.c                 | 3 ++-
 qapi/qmp-dispatch.c       | 3 ++-
 3 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
index 0ca6cbd..28f980e 100644
--- a/include/qapi/qmp/qerror.h
+++ b/include/qapi/qmp/qerror.h
@@ -52,9 +52,6 @@ void qerror_report_err(Error *err);
 #define QERR_BUS_NOT_FOUND \
     ERROR_CLASS_GENERIC_ERROR, "Bus '%s' not found"
 
-#define QERR_COMMAND_NOT_FOUND \
-    ERROR_CLASS_COMMAND_NOT_FOUND, "The command %s has not been found"
-
 #define QERR_DEVICE_ENCRYPTED \
     ERROR_CLASS_DEVICE_ENCRYPTED, "'%s' (%s) is encrypted"
 
diff --git a/monitor.c b/monitor.c
index 2ac40c9..2e2b0e5 100644
--- a/monitor.c
+++ b/monitor.c
@@ -5081,7 +5081,8 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
     trace_handle_qmp_command(mon, cmd_name);
     cmd = qmp_find_cmd(cmd_name);
     if (!cmd || invalid_qmp_mode(mon, cmd)) {
-        qerror_report(QERR_COMMAND_NOT_FOUND, cmd_name);
+        qerror_report(ERROR_CLASS_COMMAND_NOT_FOUND,
+                      "The command %s has not been found", cmd_name);
         goto err_out;
     }
 
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index 168b083..2227420 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -76,7 +76,8 @@ static QObject *do_qmp_dispatch(QObject *request, Error **errp)
     command = qdict_get_str(dict, "execute");
     cmd = qmp_find_command(command);
     if (cmd == NULL) {
-        error_set(errp, QERR_COMMAND_NOT_FOUND, command);
+        error_set(errp, ERROR_CLASS_COMMAND_NOT_FOUND,
+                  "The command %s has not been found", command);
         return NULL;
     }
     if (!cmd->enabled) {
-- 
1.9.3

[Qemu-devel] [PATCH v2 7/9] balloon: Inline qemu_balloon(), qemu_balloon_status()

[Markus Armbruster]

... and simplify a bit.  Permits factoring out common error checks in
the next commit.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 balloon.c | 42 +++++++++++++-----------------------------
 1 file changed, 13 insertions(+), 29 deletions(-)

diff --git a/balloon.c b/balloon.c
index b70da4f..2884c2d 100644
--- a/balloon.c
+++ b/balloon.c
@@ -62,25 +62,6 @@ void qemu_remove_balloon_handler(void *opaque)
     balloon_opaque = NULL;
 }
 
-static int qemu_balloon(ram_addr_t target)
-{
-    if (!balloon_event_fn) {
-        return 0;
-    }
-    trace_balloon_event(balloon_opaque, target);
-    balloon_event_fn(balloon_opaque, target);
-    return 1;
-}
-
-static int qemu_balloon_status(BalloonInfo *info)
-{
-    if (!balloon_stat_fn) {
-        return 0;
-    }
-    balloon_stat_fn(balloon_opaque, info);
-    return 1;
-}
-
 BalloonInfo *qmp_query_balloon(Error **errp)
 {
     BalloonInfo *info;
@@ -90,30 +71,33 @@ BalloonInfo *qmp_query_balloon(Error **errp)
         return NULL;
     }
 
+    if (!balloon_stat_fn) {
+        error_set(errp, QERR_DEVICE_NOT_ACTIVE, "balloon");
+        return NULL;
+    }
+
     info = g_malloc0(sizeof(*info));
-
-    if (qemu_balloon_status(info) == 0) {
-        error_set(errp, QERR_DEVICE_NOT_ACTIVE, "balloon");
-        qapi_free_BalloonInfo(info);
-        return NULL;
-    }
-
+    balloon_stat_fn(balloon_opaque, info);
     return info;
 }
 
-void qmp_balloon(int64_t value, Error **errp)
+void qmp_balloon(int64_t target, Error **errp)
 {
     if (kvm_enabled() && !kvm_has_sync_mmu()) {
         error_set(errp, QERR_KVM_MISSING_CAP, "synchronous MMU", "balloon");
         return;
     }
 
-    if (value <= 0) {
+    if (target <= 0) {
         error_set(errp, QERR_INVALID_PARAMETER_VALUE, "target", "a size");
         return;
     }
     
-    if (qemu_balloon(value) == 0) {
+    if (!balloon_event_fn) {
         error_set(errp, QERR_DEVICE_NOT_ACTIVE, "balloon");
+        return;
     }
+
+    trace_balloon_event(balloon_opaque, target);
+    balloon_event_fn(balloon_opaque, target);
 }
-- 
1.9.3

[Qemu-devel] [PATCH v2 8/9] balloon: Factor out common "is balloon active" test

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 balloon.c | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/balloon.c b/balloon.c
index 2884c2d..728bb70 100644
--- a/balloon.c
+++ b/balloon.c
@@ -36,6 +36,19 @@ static QEMUBalloonEvent *balloon_event_fn;
 static QEMUBalloonStatus *balloon_stat_fn;
 static void *balloon_opaque;
 
+static bool have_ballon(Error **errp)
+{
+    if (kvm_enabled() && !kvm_has_sync_mmu()) {
+        error_set(errp, QERR_KVM_MISSING_CAP, "synchronous MMU", "balloon");
+        return false;
+    }
+    if (!balloon_event_fn) {
+        error_set(errp, QERR_DEVICE_NOT_ACTIVE, "balloon");
+        return false;
+    }
+    return true;
+}
+
 int qemu_add_balloon_handler(QEMUBalloonEvent *event_func,
                              QEMUBalloonStatus *stat_func, void *opaque)
 {
@@ -66,13 +79,7 @@ BalloonInfo *qmp_query_balloon(Error **errp)
 {
     BalloonInfo *info;
 
-    if (kvm_enabled() && !kvm_has_sync_mmu()) {
-        error_set(errp, QERR_KVM_MISSING_CAP, "synchronous MMU", "balloon");
-        return NULL;
-    }
-
-    if (!balloon_stat_fn) {
-        error_set(errp, QERR_DEVICE_NOT_ACTIVE, "balloon");
+    if (!have_ballon(errp)) {
         return NULL;
     }
 
@@ -83,8 +90,7 @@ BalloonInfo *qmp_query_balloon(Error **errp)
 
 void qmp_balloon(int64_t target, Error **errp)
 {
-    if (kvm_enabled() && !kvm_has_sync_mmu()) {
-        error_set(errp, QERR_KVM_MISSING_CAP, "synchronous MMU", "balloon");
+    if (!have_ballon(errp)) {
         return;
     }
 
@@ -92,11 +98,6 @@ void qmp_balloon(int64_t target, Error **errp)
         error_set(errp, QERR_INVALID_PARAMETER_VALUE, "target", "a size");
         return;
     }
-    
-    if (!balloon_event_fn) {
-        error_set(errp, QERR_DEVICE_NOT_ACTIVE, "balloon");
-        return;
-    }
 
     trace_balloon_event(balloon_opaque, target);
     balloon_event_fn(balloon_opaque, target);
-- 
1.9.3

[Qemu-devel] [PATCH v2 9/9] balloon: Eliminate silly QERR_ macros

[Markus Armbruster]

The QERR_ macros are leftovers from the days of "rich" error objects.
They're used with error_set() and qerror_report(), and expand into the
first *two* arguments.  This trickiness has become pointless.  Clean
up the balloon ones.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 balloon.c                 | 6 ++++--
 include/qapi/qmp/qerror.h | 6 ------
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/balloon.c b/balloon.c
index 728bb70..dea19a4 100644
--- a/balloon.c
+++ b/balloon.c
@@ -39,11 +39,13 @@ static void *balloon_opaque;
 static bool have_ballon(Error **errp)
 {
     if (kvm_enabled() && !kvm_has_sync_mmu()) {
-        error_set(errp, QERR_KVM_MISSING_CAP, "synchronous MMU", "balloon");
+        error_set(errp, ERROR_CLASS_KVM_MISSING_CAP,
+                  "Using KVM without synchronous MMU, balloon unavailable");
         return false;
     }
     if (!balloon_event_fn) {
-        error_set(errp, QERR_DEVICE_NOT_ACTIVE, "balloon");
+        error_set(errp, ERROR_CLASS_DEVICE_NOT_ACTIVE,
+                  "No balloon device has been activated");
         return false;
     }
     return true;
diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
index 28f980e..eeaf0cb 100644
--- a/include/qapi/qmp/qerror.h
+++ b/include/qapi/qmp/qerror.h
@@ -70,9 +70,6 @@ void qerror_report_err(Error *err);
 #define QERR_DEVICE_NO_HOTPLUG \
     ERROR_CLASS_GENERIC_ERROR, "Device '%s' does not support hotplugging"
 
-#define QERR_DEVICE_NOT_ACTIVE \
-    ERROR_CLASS_DEVICE_NOT_ACTIVE, "No %s device has been activated"
-
 #define QERR_DEVICE_NOT_ENCRYPTED \
     ERROR_CLASS_GENERIC_ERROR, "Device '%s' is not encrypted"
 
@@ -109,9 +106,6 @@ void qerror_report_err(Error *err);
 #define QERR_JSON_PARSING \
     ERROR_CLASS_GENERIC_ERROR, "Invalid JSON syntax"
 
-#define QERR_KVM_MISSING_CAP \
-    ERROR_CLASS_KVM_MISSING_CAP, "Using KVM without %s, %s unavailable"
-
 #define QERR_MIGRATION_ACTIVE \
     ERROR_CLASS_GENERIC_ERROR, "There's a migration process in progress"
 
-- 
1.9.3

Re: [Qemu-devel] [PATCH v2 9/9] balloon: Eliminate silly QERR_ macros

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 593 bytes --]

On 01/29/2015 02:27 AM, Markus Armbruster wrote:
> The QERR_ macros are leftovers from the days of "rich" error objects.
> They're used with error_set() and qerror_report(), and expand into the
> first *two* arguments.  This trickiness has become pointless.  Clean
> up the balloon ones.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

double S-o-b looks odd

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]