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

[Qemu-devel] [PATCH 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.

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 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>
---
 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..b3f2679 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 int qemu_using_spice(Error **errp)
+{
+    if (!using_spice) {
+        /* correct one? spice isn't a device ,,, */
+        error_set(errp, QERR_DEVICE_NOT_ACTIVE, "spice");
+        return 0;
+    }
+    return 1;
+}
+
 #endif /* QEMU_SPICE_H */
diff --git a/monitor.c b/monitor.c
index 1808e41..cd81289 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 0b4f131..f01ae7d 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);
@@ -562,8 +558,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 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>
---
 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 b3f2679..99bb622 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 int 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 0;
     }
     return 1;
-- 
1.9.3

[Qemu-devel] [PATCH 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>
---
 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 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>
---
 qmp.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/qmp.c b/qmp.c
index f01ae7d..54624ef 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 5/9] qmp: Simplify recognition of capability negotiation command

[Markus Armbruster]

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

diff --git a/monitor.c b/monitor.c
index cd81289..f8497b6 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 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>
---
 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 f8497b6..0293769 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 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>
---
 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 8/9] balloon: Factor out common "is balloon active" test

[Markus Armbruster]

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

diff --git a/balloon.c b/balloon.c
index 2884c2d..aa30617 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 int have_ballon(Error **errp)
+{
+    if (kvm_enabled() && !kvm_has_sync_mmu()) {
+        error_set(errp, QERR_KVM_MISSING_CAP, "synchronous MMU", "balloon");
+        return 0;
+    }
+    if (!balloon_event_fn) {
+        error_set(errp, QERR_DEVICE_NOT_ACTIVE, "balloon");
+        return 0;
+    }
+    return 1;
+}
+
 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 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.

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 aa30617..dcf5b9c 100644
--- a/balloon.c
+++ b/balloon.c
@@ -39,11 +39,13 @@ static void *balloon_opaque;
 static int 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 0;
     }
     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 0;
     }
     return 1;
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 0/9] qmp hmp balloon: Cleanups around error reporting

[Gerd Hoffmann]

On Di, 2015-01-13 at 18:50 +0100, Markus Armbruster wrote:
>   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

Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>

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

[Eric Blake]

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

On 01/13/2015 10:50 AM, Markus Armbruster wrote:
> 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>
> ---
>  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..b3f2679 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 int qemu_using_spice(Error **errp)

Why not s/int/bool/

> +{
> +    if (!using_spice) {
> +        /* correct one? spice isn't a device ,,, */
> +        error_set(errp, QERR_DEVICE_NOT_ACTIVE, "spice");
> +        return 0;
> +    }
> +    return 1;

and use true/false here? All callers use it only in a boolean context.

Either way,
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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 --]

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

[Eric Blake]

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

On 01/13/2015 10:50 AM, Markus Armbruster wrote:
> 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.

Libvirt has a few places that look explicitly for "DeviceNotActive" when
deciding how to handle failure reporting; I didn't audit closely enough
to see if it actually changes behavior if the class were to disappear.
Your approach in this patch is safest, at any rate.

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

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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 --]

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

[Eric Blake]

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

On 01/13/2015 10:50 AM, Markus Armbruster wrote:
> 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>
> ---

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

There's probably several commands that are only conditionally compiled
in qmp-commands.hx, but where the qapi has no way to express that they
are conditional on how the binary was compiled.  I'm not sure if it
would help the user to document that a command listed in the .json might
not actually exist, but if enhancing the qapi code generator to
understand conditionals made for less maintenance, then that alone would
be worth cleaning up this FIXME.

In the meantime, your patch is just fine.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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 --]

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

[Eric Blake]

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

On 01/13/2015 10:50 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  balloon.c | 29 +++++++++++++++--------------
>  1 file changed, 15 insertions(+), 14 deletions(-)
> 
> diff --git a/balloon.c b/balloon.c
> index 2884c2d..aa30617 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 int have_ballon(Error **errp)
> +{
> +    if (kvm_enabled() && !kvm_has_sync_mmu()) {
> +        error_set(errp, QERR_KVM_MISSING_CAP, "synchronous MMU", "balloon");
> +        return 0;
> +    }
> +    if (!balloon_event_fn) {
> +        error_set(errp, QERR_DEVICE_NOT_ACTIVE, "balloon");
> +        return 0;
> +    }
> +    return 1;

Another case where I would have used bool, true, false.

Either way,
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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 --]

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

[Eric Blake]

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

On 01/13/2015 10:50 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.

I like your wording in 6/9 better ("Clean this one up"), as it is more
obvious that you are only fixing a subset and not eliminating all such
macros.

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

For this patch, and for all others in the series where I did not
explicitly reply,
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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 --]

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

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 01/13/2015 10:50 AM, Markus Armbruster wrote:
>> 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.
>
> Libvirt has a few places that look explicitly for "DeviceNotActive" when
> deciding how to handle failure reporting; I didn't audit closely enough
> to see if it actually changes behavior if the class were to disappear.

I studied libvirt's use of error classes other than GenericError before
my Christmas break.  My findings:

* CommandNotFound: used; libvirt could perhaps rely on query-commands
  instead

* DeviceEncrypted: not used (surprise, surprise)

* DeviceNotActive: used with balloon and block jobs, not used with spice

* DeviceNotFound: used to detect active commit support, with
  device-list-properties and with qom-list (which isn't about devices at
  all)

* KVMMissingCap: used; occurs only with balloon, and it shandled just
  like DeviceNotActive

> Your approach in this patch is safest, at any rate.

I'm resigned to keeping these error classes around.  I'm still fighting
their proliferation to new code.

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

Thanks!

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

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 01/13/2015 10:50 AM, Markus Armbruster wrote:
>> 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>
>> ---
>>  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..b3f2679 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 int qemu_using_spice(Error **errp)
>
> Why not s/int/bool/
>
>> +{
>> +    if (!using_spice) {
>> +        /* correct one? spice isn't a device ,,, */
>> +        error_set(errp, QERR_DEVICE_NOT_ACTIVE, "spice");
>> +        return 0;
>> +    }
>> +    return 1;
>
> and use true/false here? All callers use it only in a boolean context.

I wrapped around using_spice, which is int, and didn't think of
switching to bool.  Old habits die hard...

Luiz, would you be willing to fix this up on commit?

> Either way,
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

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

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 01/13/2015 10:50 AM, Markus Armbruster wrote:
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  balloon.c | 29 +++++++++++++++--------------
>>  1 file changed, 15 insertions(+), 14 deletions(-)
>> 
>> diff --git a/balloon.c b/balloon.c
>> index 2884c2d..aa30617 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 int have_ballon(Error **errp)
>> +{
>> +    if (kvm_enabled() && !kvm_has_sync_mmu()) {
>> +        error_set(errp, QERR_KVM_MISSING_CAP, "synchronous MMU", "balloon");
>> +        return 0;
>> +    }
>> +    if (!balloon_event_fn) {
>> +        error_set(errp, QERR_DEVICE_NOT_ACTIVE, "balloon");
>> +        return 0;
>> +    }
>> +    return 1;
>
> Another case where I would have used bool, true, false.

Me too now that you mention it %-)

> Either way,
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

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

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  balloon.c | 29 +++++++++++++++--------------
>  1 file changed, 15 insertions(+), 14 deletions(-)
> 
> diff --git a/balloon.c b/balloon.c
> index 2884c2d..aa30617 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 int have_ballon(Error **errp)

                       ^

An 'o' escaped from your balloon.

Dave

> +{
> +    if (kvm_enabled() && !kvm_has_sync_mmu()) {
> +        error_set(errp, QERR_KVM_MISSING_CAP, "synchronous MMU", "balloon");
> +        return 0;
> +    }
> +    if (!balloon_event_fn) {
> +        error_set(errp, QERR_DEVICE_NOT_ACTIVE, "balloon");
> +        return 0;
> +    }
> +    return 1;
> +}
> +
>  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
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Eric Blake]

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

On 02/10/2015 11:39 AM, Dr. David Alan Gilbert wrote:
> * Markus Armbruster (armbru@redhat.com) wrote:
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  balloon.c | 29 +++++++++++++++--------------
>>  1 file changed, 15 insertions(+), 14 deletions(-)
>>
>> diff --git a/balloon.c b/balloon.c
>> index 2884c2d..aa30617 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 int have_ballon(Error **errp)
> 
>                        ^
> 
> An 'o' escaped from your balloon.

Oops - it also escaped from my R-b.  I've sent the obvious patch, as
penance for my "deflated ego" :)

-- 
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 --]

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

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 02/10/2015 11:39 AM, Dr. David Alan Gilbert wrote:
>> * Markus Armbruster (armbru@redhat.com) wrote:
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>>  balloon.c | 29 +++++++++++++++--------------
>>>  1 file changed, 15 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/balloon.c b/balloon.c
>>> index 2884c2d..aa30617 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 int have_ballon(Error **errp)
>> 
>>                        ^
>> 
>> An 'o' escaped from your balloon.
>
> Oops - it also escaped from my R-b.  I've sent the obvious patch, as
> penance for my "deflated ego" :)

Thank you both!