[Qemu-devel] [RFC 00/13]: convert device_add to the qapi

[Qemu-devel] [RFC 00/13]: convert device_add to the qapi

[Luiz Capitulino]

This is an RFC because it's not 100% finished yet and I'm not sure if some
of the changes are the way to go.

Here's a summary of the series its issues, more details can be found in
the patches:

 o Patches 1 to 11 convert several qemu-option and qemu-config functions
   from qerror_report() to error_set(). Note that not all users of those
   functions are converted, so it might be needed to call qerror_report_err()
   somewhere in the call stack to keep QError semantics.

   Also, new versions of qemu_opt_set() and qemu_find_opts() that take an
   Error parameter are introduced. This allows for incremental conversion
   (vs. converting 100% of their users in one series)

 o Patch 12 introduces support for allowing QAPI functions to accept a
   variable list of arguments, which are unchecked and passed "as-is"
   to the QAPI function. My implementation here is very hacky, please
   check the patch for details

 o Patch 13 does the QAPI conversion itself, but the HMP version of
   device_add lacks its help text (ie. '?'). The problem here is that hmp.c
   is really a QMP client and can only use what's provided by QMP. There are
   two ways to solve this:

        1. Anthony told me that he has this implemented in his tree, but
           I couldn't find it. Anthony, can you point me to your
           implementation?

        2. Add a query-drivers command or similar to get a list of drivers,
           so that the HMP command can use it to implement the help text

 blockdev.c               |    2 +-
 hmp-commands.hx          |    3 +-
 hmp.c                    |   24 +++++++
 hmp.h                    |    1 +
 hw/qdev-monitor.c        |   70 ++++++++++++++-----
 hw/qdev.h                |    2 +-
 hw/usb/dev-storage.c     |    2 +-
 hw/watchdog.c            |    2 +-
 qapi-schema.json         |   38 ++++++++++
 qemu-char.c              |    8 ++-
 qemu-config.c            |   43 +++++++++---
 qemu-config.h            |    3 +
 qemu-option.c            |  173 +++++++++++++++++++++++++++++++---------------
 qemu-option.h            |    6 +-
 qemu-sockets.c           |    8 +--
 qerror.c                 |    4 ++
 qerror.h                 |    3 +
 qmp-commands.hx          |    3 +-
 scripts/qapi-commands.py |   31 ++++++++-
 scripts/qapi.py          |    2 +
 vl.c                     |   25 ++++---
 21 files changed, 342 insertions(+), 111 deletions(-)

[Qemu-devel] [PATCH 01/13] qemu-option: qemu_opts_create(): use error_set()

[Luiz Capitulino]

This commit converts qemu_opts_create() from qerror_report() to
error_set().

This means that qemu_opts_create() now takes an Error argument and
callers need to pass it if they're interested in getting error
information.

Currently, most calls to qemu_opts_create() can't fail, so most
callers don't need any changes.

The two cases where code checks for qemu_opts_create() erros are:

 1. Initialization code in vl.c. All of them print their own
    error messages directly to stderr, no need to pass the Error
    object

 2. The functions opts_parse(), qemu_opts_from_qdict() and
    qemu_chr_parse_compat() make use of the error information and
    they can be called from HMP or QMP. In this case, to allow for
    incremental conversion, we propagate the error up using
    qerror_report_err(), which keep the QError semantics

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 blockdev.c           |    2 +-
 hw/usb/dev-storage.c |    2 +-
 hw/watchdog.c        |    2 +-
 qemu-char.c          |    8 ++++++--
 qemu-config.c        |    6 +++---
 qemu-option.c        |   37 +++++++++++++++++++++++++++----------
 qemu-option.h        |    4 +++-
 qemu-sockets.c       |    8 ++++----
 vl.c                 |   22 +++++++++++++---------
 9 files changed, 59 insertions(+), 32 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 1a500b8..77be66a 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -565,7 +565,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
         break;
     case IF_VIRTIO:
         /* add virtio block device */
-        opts = qemu_opts_create(qemu_find_opts("device"), NULL, 0);
+        opts = qemu_opts_create(qemu_find_opts("device"), NULL, 0, NULL);
         if (arch_type == QEMU_ARCH_S390X) {
             qemu_opt_set(opts, "driver", "virtio-blk-s390");
         } else {
diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index bdbe7bd..c9c59be 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -580,7 +580,7 @@ static USBDevice *usb_msd_init(USBBus *bus, const char *filename)
 
     /* parse -usbdevice disk: syntax into drive opts */
     snprintf(id, sizeof(id), "usb%d", nr++);
-    opts = qemu_opts_create(qemu_find_opts("drive"), id, 0);
+    opts = qemu_opts_create(qemu_find_opts("drive"), id, 0, NULL);
 
     p1 = strchr(filename, ':');
     if (p1++) {
diff --git a/hw/watchdog.c b/hw/watchdog.c
index 4c18965..a42124d 100644
--- a/hw/watchdog.c
+++ b/hw/watchdog.c
@@ -66,7 +66,7 @@ int select_watchdog(const char *p)
     QLIST_FOREACH(model, &watchdog_list, entry) {
         if (strcasecmp(model->wdt_name, p) == 0) {
             /* add the device */
-            opts = qemu_opts_create(qemu_find_opts("device"), NULL, 0);
+            opts = qemu_opts_create(qemu_find_opts("device"), NULL, 0, NULL);
             qemu_opt_set(opts, "driver", p);
             return 0;
         }
diff --git a/qemu-char.c b/qemu-char.c
index bb9e3f5..67a5ff0 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2581,10 +2581,14 @@ QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename)
     int pos;
     const char *p;
     QemuOpts *opts;
+    Error *local_err = NULL;
 
-    opts = qemu_opts_create(qemu_find_opts("chardev"), label, 1);
-    if (NULL == opts)
+    opts = qemu_opts_create(qemu_find_opts("chardev"), label, 1, &local_err);
+    if (error_is_set(&local_err)) {
+        qerror_report_err(local_err);
+        error_free(local_err);
         return NULL;
+    }
 
     if (strstart(filename, "mon:", &p)) {
         filename = p;
diff --git a/qemu-config.c b/qemu-config.c
index be84a03..f876646 100644
--- a/qemu-config.c
+++ b/qemu-config.c
@@ -709,7 +709,7 @@ int qemu_global_option(const char *str)
         return -1;
     }
 
-    opts = qemu_opts_create(&qemu_global_opts, NULL, 0);
+    opts = qemu_opts_create(&qemu_global_opts, NULL, 0, NULL);
     qemu_opt_set(opts, "driver", driver);
     qemu_opt_set(opts, "property", property);
     qemu_opt_set(opts, "value", str+offset+1);
@@ -781,7 +781,7 @@ int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname)
             list = find_list(lists, group);
             if (list == NULL)
                 goto out;
-            opts = qemu_opts_create(list, id, 1);
+            opts = qemu_opts_create(list, id, 1, NULL);
             continue;
         }
         if (sscanf(line, "[%63[^]]]", group) == 1) {
@@ -789,7 +789,7 @@ int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname)
             list = find_list(lists, group);
             if (list == NULL)
                 goto out;
-            opts = qemu_opts_create(list, NULL, 0);
+            opts = qemu_opts_create(list, NULL, 0, NULL);
             continue;
         }
         if (sscanf(line, " %63s = \"%1023[^\"]\"", arg, value) == 2) {
diff --git a/qemu-option.c b/qemu-option.c
index 35cd609..9f531c8 100644
--- a/qemu-option.c
+++ b/qemu-option.c
@@ -30,6 +30,7 @@
 #include "qemu-error.h"
 #include "qemu-objects.h"
 #include "qemu-option.h"
+#include "error.h"
 #include "qerror.h"
 
 /*
@@ -729,20 +730,21 @@ static int id_wellformed(const char *id)
     return 1;
 }
 
-QemuOpts *qemu_opts_create(QemuOptsList *list, const char *id, int fail_if_exists)
+QemuOpts *qemu_opts_create(QemuOptsList *list, const char *id,
+                           int fail_if_exists, Error **errp)
 {
     QemuOpts *opts = NULL;
 
     if (id) {
         if (!id_wellformed(id)) {
-            qerror_report(QERR_INVALID_PARAMETER_VALUE, "id", "an identifier");
+            error_set(errp,QERR_INVALID_PARAMETER_VALUE, "id", "an identifier");
             error_printf_unless_qmp("Identifiers consist of letters, digits, '-', '.', '_', starting with a letter.\n");
             return NULL;
         }
         opts = qemu_opts_find(list, id);
         if (opts != NULL) {
             if (fail_if_exists && !list->merge_lists) {
-                qerror_report(QERR_DUPLICATE_ID, id, list->name);
+                error_set(errp, QERR_DUPLICATE_ID, id, list->name);
                 return NULL;
             } else {
                 return opts;
@@ -783,9 +785,12 @@ int qemu_opts_set(QemuOptsList *list, const char *id,
                   const char *name, const char *value)
 {
     QemuOpts *opts;
+    Error *local_err = NULL;
 
-    opts = qemu_opts_create(list, id, 1);
-    if (opts == NULL) {
+    opts = qemu_opts_create(list, id, 1, &local_err);
+    if (error_is_set(&local_err)) {
+        qerror_report_err(local_err);
+        error_free(local_err);
         return -1;
     }
     return qemu_opt_set(opts, name, value);
@@ -883,6 +888,7 @@ static QemuOpts *opts_parse(QemuOptsList *list, const char *params,
     char value[1024], *id = NULL;
     const char *p;
     QemuOpts *opts;
+    Error *local_err = NULL;
 
     assert(!permit_abbrev || list->implied_opt_name);
     firstname = permit_abbrev ? list->implied_opt_name : NULL;
@@ -898,13 +904,18 @@ static QemuOpts *opts_parse(QemuOptsList *list, const char *params,
         if (!id && !QTAILQ_EMPTY(&list->head)) {
             opts = qemu_opts_find(list, NULL);
         } else {
-            opts = qemu_opts_create(list, id, 0);
+            opts = qemu_opts_create(list, id, 0, &local_err);
         }
     } else {
-        opts = qemu_opts_create(list, id, 1);
+        opts = qemu_opts_create(list, id, 1, &local_err);
     }
-    if (opts == NULL)
+    if (opts == NULL) {
+        if (error_is_set(&local_err)) {
+            qerror_report_err(local_err);
+            error_free(local_err);
+        }
         return NULL;
+    }
 
     if (opts_do_parse(opts, params, firstname, defaults) != 0) {
         qemu_opts_del(opts);
@@ -975,11 +986,17 @@ static void qemu_opts_from_qdict_1(const char *key, QObject *obj, void *opaque)
 QemuOpts *qemu_opts_from_qdict(QemuOptsList *list, const QDict *qdict)
 {
     QemuOpts *opts;
+    Error *local_err = NULL;
 
-    opts = qemu_opts_create(list, qdict_get_try_str(qdict, "id"), 1);
-    if (opts == NULL)
+    opts = qemu_opts_create(list, qdict_get_try_str(qdict, "id"), 1,
+                            &local_err);
+    if (error_is_set(&local_err)) {
+        qerror_report_err(local_err);
+        error_free(local_err);
         return NULL;
+    }
 
+    assert(opts != NULL);
     qdict_iter(qdict, qemu_opts_from_qdict_1, opts);
     return opts;
 }
diff --git a/qemu-option.h b/qemu-option.h
index 3ca00c3..4d5b3d3 100644
--- a/qemu-option.h
+++ b/qemu-option.h
@@ -28,6 +28,7 @@
 
 #include <stdint.h>
 #include "qemu-queue.h"
+#include "error.h"
 #include "qdict.h"
 
 enum QEMUOptionParType {
@@ -116,7 +117,8 @@ int qemu_opt_foreach(QemuOpts *opts, qemu_opt_loopfunc func, void *opaque,
                      int abort_on_failure);
 
 QemuOpts *qemu_opts_find(QemuOptsList *list, const char *id);
-QemuOpts *qemu_opts_create(QemuOptsList *list, const char *id, int fail_if_exists);
+QemuOpts *qemu_opts_create(QemuOptsList *list, const char *id,
+                           int fail_if_exists, Error **errp);
 void qemu_opts_reset(QemuOptsList *list);
 void qemu_opts_loc_restore(QemuOpts *opts);
 int qemu_opts_set(QemuOptsList *list, const char *id,
diff --git a/qemu-sockets.c b/qemu-sockets.c
index 6bcb8e3..5966d5f 100644
--- a/qemu-sockets.c
+++ b/qemu-sockets.c
@@ -427,7 +427,7 @@ int inet_listen(const char *str, char *ostr, int olen,
     char *optstr;
     int sock = -1;
 
-    opts = qemu_opts_create(&dummy_opts, NULL, 0);
+    opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
     if (inet_parse(opts, str) == 0) {
         sock = inet_listen_opts(opts, port_offset);
         if (sock != -1 && ostr) {
@@ -454,7 +454,7 @@ int inet_connect(const char *str, int socktype)
     QemuOpts *opts;
     int sock = -1;
 
-    opts = qemu_opts_create(&dummy_opts, NULL, 0);
+    opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
     if (inet_parse(opts, str) == 0)
         sock = inet_connect_opts(opts);
     qemu_opts_del(opts);
@@ -547,7 +547,7 @@ int unix_listen(const char *str, char *ostr, int olen)
     char *path, *optstr;
     int sock, len;
 
-    opts = qemu_opts_create(&dummy_opts, NULL, 0);
+    opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
 
     optstr = strchr(str, ',');
     if (optstr) {
@@ -575,7 +575,7 @@ int unix_connect(const char *path)
     QemuOpts *opts;
     int sock;
 
-    opts = qemu_opts_create(&dummy_opts, NULL, 0);
+    opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
     qemu_opt_set(opts, "path", path);
     sock = unix_connect_opts(opts);
     qemu_opts_del(opts);
diff --git a/vl.c b/vl.c
index 0fccf50..6467d6b 100644
--- a/vl.c
+++ b/vl.c
@@ -1772,7 +1772,7 @@ static int balloon_parse(const char *arg)
                 return  -1;
         } else {
             /* create empty opts */
-            opts = qemu_opts_create(qemu_find_opts("device"), NULL, 0);
+            opts = qemu_opts_create(qemu_find_opts("device"), NULL, 0, NULL);
         }
         qemu_opt_set(opts, "driver", "virtio-balloon");
         return 0;
@@ -1907,7 +1907,7 @@ static void monitor_parse(const char *optarg, const char *mode)
         }
     }
 
-    opts = qemu_opts_create(qemu_find_opts("mon"), label, 1);
+    opts = qemu_opts_create(qemu_find_opts("mon"), label, 1, NULL);
     if (!opts) {
         fprintf(stderr, "duplicate chardev: %s\n", label);
         exit(1);
@@ -2021,14 +2021,14 @@ static int virtcon_parse(const char *devname)
         exit(1);
     }
 
-    bus_opts = qemu_opts_create(device, NULL, 0);
+    bus_opts = qemu_opts_create(device, NULL, 0, NULL);
     if (arch_type == QEMU_ARCH_S390X) {
         qemu_opt_set(bus_opts, "driver", "virtio-serial-s390");
     } else {
         qemu_opt_set(bus_opts, "driver", "virtio-serial-pci");
     } 
 
-    dev_opts = qemu_opts_create(device, NULL, 0);
+    dev_opts = qemu_opts_create(device, NULL, 0, NULL);
     qemu_opt_set(dev_opts, "driver", "virtconsole");
 
     snprintf(label, sizeof(label), "virtcon%d", index);
@@ -2051,7 +2051,7 @@ static int debugcon_parse(const char *devname)
     if (!qemu_chr_new("debugcon", devname, NULL)) {
         exit(1);
     }
-    opts = qemu_opts_create(qemu_find_opts("device"), "debugcon", 1);
+    opts = qemu_opts_create(qemu_find_opts("device"), "debugcon", 1, NULL);
     if (!opts) {
         fprintf(stderr, "qemu: already have a debugcon device\n");
         exit(1);
@@ -2799,7 +2799,8 @@ int main(int argc, char **argv, char **envp)
                     exit(1);
                 }
                 fsdev = qemu_opts_create(qemu_find_opts("fsdev"),
-                                         qemu_opt_get(opts, "mount_tag"), 1);
+                                         qemu_opt_get(opts, "mount_tag"),
+                                         1, NULL);
                 if (!fsdev) {
                     fprintf(stderr, "duplicate fsdev id: %s\n",
                             qemu_opt_get(opts, "mount_tag"));
@@ -2831,7 +2832,8 @@ int main(int argc, char **argv, char **envp)
 
                 qemu_opt_set_bool(fsdev, "readonly",
                                 qemu_opt_get_bool(opts, "readonly", 0));
-                device = qemu_opts_create(qemu_find_opts("device"), NULL, 0);
+                device = qemu_opts_create(qemu_find_opts("device"), NULL, 0,
+                                          NULL);
                 qemu_opt_set(device, "driver", "virtio-9p-pci");
                 qemu_opt_set(device, "fsdev",
                              qemu_opt_get(opts, "mount_tag"));
@@ -2843,14 +2845,16 @@ int main(int argc, char **argv, char **envp)
                 QemuOpts *fsdev;
                 QemuOpts *device;
 
-                fsdev = qemu_opts_create(qemu_find_opts("fsdev"), "v_synth", 1);
+                fsdev = qemu_opts_create(qemu_find_opts("fsdev"), "v_synth",
+                                         1, NULL);
                 if (!fsdev) {
                     fprintf(stderr, "duplicate option: %s\n", "virtfs_synth");
                     exit(1);
                 }
                 qemu_opt_set(fsdev, "fsdriver", "synth");
 
-                device = qemu_opts_create(qemu_find_opts("device"), NULL, 0);
+                device = qemu_opts_create(qemu_find_opts("device"), NULL, 0,
+                                          NULL);
                 qemu_opt_set(device, "driver", "virtio-9p-pci");
                 qemu_opt_set(device, "fsdev", "v_synth");
                 qemu_opt_set(device, "mount_tag", "v_synth");
-- 
1.7.9.2.384.g4a92a

[Qemu-devel] [PATCH 02/13] qemu-option: parse_option_number(): use error_set()

[Luiz Capitulino]

Note that qemu_opt_parse() callers still expect automatic error reporting
with QError, so qemu_opts_parse() calls qerror_report_err() to keep the
same semantics.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qemu-option.c |   26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/qemu-option.c b/qemu-option.c
index 9f531c8..72dcb56 100644
--- a/qemu-option.c
+++ b/qemu-option.c
@@ -186,7 +186,8 @@ static int parse_option_bool(const char *name, const char *value, bool *ret)
     return 0;
 }
 
-static int parse_option_number(const char *name, const char *value, uint64_t *ret)
+static void parse_option_number(const char *name, const char *value,
+                                uint64_t *ret, Error **errp)
 {
     char *postfix;
     uint64_t number;
@@ -194,15 +195,13 @@ static int parse_option_number(const char *name, const char *value, uint64_t *re
     if (value != NULL) {
         number = strtoull(value, &postfix, 0);
         if (*postfix != '\0') {
-            qerror_report(QERR_INVALID_PARAMETER_VALUE, name, "a number");
-            return -1;
+            error_set(errp, QERR_INVALID_PARAMETER_VALUE, name, "a number");
+            return;
         }
         *ret = number;
     } else {
-        qerror_report(QERR_INVALID_PARAMETER_VALUE, name, "a number");
-        return -1;
+        error_set(errp, QERR_INVALID_PARAMETER_VALUE, name, "a number");
     }
-    return 0;
 }
 
 static int parse_option_size(const char *name, const char *value, uint64_t *ret)
@@ -579,8 +578,11 @@ uint64_t qemu_opt_get_size(QemuOpts *opts, const char *name, uint64_t defval)
 
 static int qemu_opt_parse(QemuOpt *opt)
 {
+    Error *local_err = NULL;
+
     if (opt->desc == NULL)
         return 0;
+
     switch (opt->desc->type) {
     case QEMU_OPT_STRING:
         /* nothing */
@@ -588,12 +590,22 @@ static int qemu_opt_parse(QemuOpt *opt)
     case QEMU_OPT_BOOL:
         return parse_option_bool(opt->name, opt->str, &opt->value.boolean);
     case QEMU_OPT_NUMBER:
-        return parse_option_number(opt->name, opt->str, &opt->value.uint);
+        parse_option_number(opt->name, opt->str, &opt->value.uint,
+                            &local_err);
+        break;
     case QEMU_OPT_SIZE:
         return parse_option_size(opt->name, opt->str, &opt->value.uint);
     default:
         abort();
     }
+
+    if (error_is_set(&local_err)) {
+        qerror_report_err(local_err);
+        error_free(local_err);
+        return -1;
+    }
+
+    return 0;
 }
 
 static void qemu_opt_del(QemuOpt *opt)
-- 
1.7.9.2.384.g4a92a

[Qemu-devel] [PATCH 03/13] qemu-option: parse_option_bool(): use error_set()

[Luiz Capitulino]

Note that set_option_parameter() callers still expect automatic error
reporting with QError, so set_option_parameter() calls
qerror_report_err() to keep the same semantics.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qemu-option.c |   22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/qemu-option.c b/qemu-option.c
index 72dcb56..a8b50af 100644
--- a/qemu-option.c
+++ b/qemu-option.c
@@ -169,7 +169,8 @@ QEMUOptionParameter *get_option_parameter(QEMUOptionParameter *list,
     return NULL;
 }
 
-static int parse_option_bool(const char *name, const char *value, bool *ret)
+static void parse_option_bool(const char *name, const char *value, bool *ret,
+                              Error **errp)
 {
     if (value != NULL) {
         if (!strcmp(value, "on")) {
@@ -177,13 +178,11 @@ static int parse_option_bool(const char *name, const char *value, bool *ret)
         } else if (!strcmp(value, "off")) {
             *ret = 0;
         } else {
-            qerror_report(QERR_INVALID_PARAMETER_VALUE, name, "'on' or 'off'");
-            return -1;
+            error_set(errp,QERR_INVALID_PARAMETER_VALUE, name, "'on' or 'off'");
         }
     } else {
         *ret = 1;
     }
-    return 0;
 }
 
 static void parse_option_number(const char *name, const char *value,
@@ -263,6 +262,7 @@ int set_option_parameter(QEMUOptionParameter *list, const char *name,
     const char *value)
 {
     bool flag;
+    Error *local_err = NULL;
 
     // Find a matching parameter
     list = get_option_parameter(list, name);
@@ -274,8 +274,10 @@ int set_option_parameter(QEMUOptionParameter *list, const char *name,
     // Process parameter
     switch (list->type) {
     case OPT_FLAG:
-        if (parse_option_bool(name, value, &flag) == -1)
-            return -1;
+        parse_option_bool(name, value, &flag, &local_err);
+        if (error_is_set(&local_err)) {
+            goto exit_err;
+        }
         list->value.n = flag;
         break;
 
@@ -299,6 +301,11 @@ int set_option_parameter(QEMUOptionParameter *list, const char *name,
     }
 
     return 0;
+
+exit_err:
+    qerror_report_err(local_err);
+    error_free(local_err);
+    return -1;
 }
 
 /*
@@ -588,7 +595,8 @@ static int qemu_opt_parse(QemuOpt *opt)
         /* nothing */
         return 0;
     case QEMU_OPT_BOOL:
-        return parse_option_bool(opt->name, opt->str, &opt->value.boolean);
+        parse_option_bool(opt->name, opt->str, &opt->value.boolean, &local_err);
+        break;
     case QEMU_OPT_NUMBER:
         parse_option_number(opt->name, opt->str, &opt->value.uint,
                             &local_err);
-- 
1.7.9.2.384.g4a92a

[Qemu-devel] [PATCH 04/13] qemu-option: parse_option_size(): use error_set()

[Luiz Capitulino]

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qemu-option.c |   20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/qemu-option.c b/qemu-option.c
index a8b50af..61354af 100644
--- a/qemu-option.c
+++ b/qemu-option.c
@@ -203,7 +203,8 @@ static void parse_option_number(const char *name, const char *value,
     }
 }
 
-static int parse_option_size(const char *name, const char *value, uint64_t *ret)
+static void parse_option_size(const char *name, const char *value,
+                              uint64_t *ret, Error **errp)
 {
     char *postfix;
     double sizef;
@@ -229,16 +230,14 @@ static int parse_option_size(const char *name, const char *value, uint64_t *ret)
             *ret = (uint64_t) sizef;
             break;
         default:
-            qerror_report(QERR_INVALID_PARAMETER_VALUE, name, "a size");
+            error_set(errp, QERR_INVALID_PARAMETER_VALUE, name, "a size");
             error_printf_unless_qmp("You may use k, M, G or T suffixes for "
                     "kilobytes, megabytes, gigabytes and terabytes.\n");
-            return -1;
+            return;
         }
     } else {
-        qerror_report(QERR_INVALID_PARAMETER_VALUE, name, "a size");
-        return -1;
+        error_set(errp, QERR_INVALID_PARAMETER_VALUE, name, "a size");
     }
-    return 0;
 }
 
 /*
@@ -291,8 +290,10 @@ int set_option_parameter(QEMUOptionParameter *list, const char *name,
         break;
 
     case OPT_SIZE:
-        if (parse_option_size(name, value, &list->value.n) == -1)
-            return -1;
+        parse_option_size(name, value, &list->value.n, &local_err);
+        if (error_is_set(&local_err)) {
+            goto exit_err;
+        }
         break;
 
     default:
@@ -602,7 +603,8 @@ static int qemu_opt_parse(QemuOpt *opt)
                             &local_err);
         break;
     case QEMU_OPT_SIZE:
-        return parse_option_size(opt->name, opt->str, &opt->value.uint);
+        parse_option_size(opt->name, opt->str, &opt->value.uint, &local_err);
+        break;
     default:
         abort();
     }
-- 
1.7.9.2.384.g4a92a

[Qemu-devel] [PATCH 05/13] qemu-option: qemu_opt_parse(): use error_set()

[Luiz Capitulino]

The functions opt_set() and qemu_opts_validate() both call qemu_opt_parse(),
but their callers expect QError semantics. Thus, both functions call
qerro_report_err() to keep the expected semantics.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qemu-option.c |   54 +++++++++++++++++++++++++++++-------------------------
 1 file changed, 29 insertions(+), 25 deletions(-)

diff --git a/qemu-option.c b/qemu-option.c
index 61354af..4829de5 100644
--- a/qemu-option.c
+++ b/qemu-option.c
@@ -584,38 +584,27 @@ uint64_t qemu_opt_get_size(QemuOpts *opts, const char *name, uint64_t defval)
     return opt->value.uint;
 }
 
-static int qemu_opt_parse(QemuOpt *opt)
+static void qemu_opt_parse(QemuOpt *opt, Error **errp)
 {
-    Error *local_err = NULL;
-
     if (opt->desc == NULL)
-        return 0;
+        return;
 
     switch (opt->desc->type) {
     case QEMU_OPT_STRING:
         /* nothing */
-        return 0;
+        return;
     case QEMU_OPT_BOOL:
-        parse_option_bool(opt->name, opt->str, &opt->value.boolean, &local_err);
+        parse_option_bool(opt->name, opt->str, &opt->value.boolean, errp);
         break;
     case QEMU_OPT_NUMBER:
-        parse_option_number(opt->name, opt->str, &opt->value.uint,
-                            &local_err);
+        parse_option_number(opt->name, opt->str, &opt->value.uint, errp);
         break;
     case QEMU_OPT_SIZE:
-        parse_option_size(opt->name, opt->str, &opt->value.uint, &local_err);
+        parse_option_size(opt->name, opt->str, &opt->value.uint, errp);
         break;
     default:
         abort();
     }
-
-    if (error_is_set(&local_err)) {
-        qerror_report_err(local_err);
-        error_free(local_err);
-        return -1;
-    }
-
-    return 0;
 }
 
 static void qemu_opt_del(QemuOpt *opt)
@@ -631,6 +620,7 @@ static int opt_set(QemuOpts *opts, const char *name, const char *value,
 {
     QemuOpt *opt;
     const QemuOptDesc *desc = opts->list->desc;
+    Error *local_err = NULL;
     int i;
 
     for (i = 0; desc[i].name != NULL; i++) {
@@ -642,8 +632,8 @@ static int opt_set(QemuOpts *opts, const char *name, const char *value,
         if (i == 0) {
             /* empty list -> allow any */;
         } else {
-            qerror_report(QERR_INVALID_PARAMETER, name);
-            return -1;
+            error_set(&local_err, QERR_INVALID_PARAMETER, name);
+            goto exit_err;
         }
     }
 
@@ -661,11 +651,18 @@ static int opt_set(QemuOpts *opts, const char *name, const char *value,
     if (value) {
         opt->str = g_strdup(value);
     }
-    if (qemu_opt_parse(opt) < 0) {
+    qemu_opt_parse(opt, &local_err);
+    if (error_is_set(&local_err)) {
         qemu_opt_del(opt);
-        return -1;
+        goto exit_err;
     }
+
     return 0;
+
+exit_err:
+    qerror_report_err(local_err);
+    error_free(local_err);
+    return -1;
 }
 
 int qemu_opt_set(QemuOpts *opts, const char *name, const char *value)
@@ -1053,6 +1050,7 @@ QDict *qemu_opts_to_qdict(QemuOpts *opts, QDict *qdict)
 int qemu_opts_validate(QemuOpts *opts, const QemuOptDesc *desc)
 {
     QemuOpt *opt;
+    Error *local_err = NULL;
 
     assert(opts->list->desc[0].name == NULL);
 
@@ -1065,18 +1063,24 @@ int qemu_opts_validate(QemuOpts *opts, const QemuOptDesc *desc)
             }
         }
         if (desc[i].name == NULL) {
-            qerror_report(QERR_INVALID_PARAMETER, opt->name);
-            return -1;
+            error_set(&local_err, QERR_INVALID_PARAMETER, opt->name);
+            goto exit_err;
         }
 
         opt->desc = &desc[i];
 
-        if (qemu_opt_parse(opt) < 0) {
-            return -1;
+        qemu_opt_parse(opt, &local_err);
+        if (error_is_set(&local_err)) {
+            goto exit_err;
         }
     }
 
     return 0;
+
+exit_err:
+    qerror_report_err(local_err);
+    error_free(local_err);
+    return -1;
 }
 
 int qemu_opts_foreach(QemuOptsList *list, qemu_opts_loopfunc func, void *opaque,
-- 
1.7.9.2.384.g4a92a

[Qemu-devel] [PATCH 06/13] qemu-option: opt_set(): use error_set()

[Luiz Capitulino]

The functions qemu_opt_set() and opts_do_parse() both call opt_set(),
but their callers expect QError semantics. Thus, both functions call
qerro_report_err() to keep the expected semantics.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qemu-option.c |   38 +++++++++++++++++++++-----------------
 1 file changed, 21 insertions(+), 17 deletions(-)

diff --git a/qemu-option.c b/qemu-option.c
index 4829de5..38461d4 100644
--- a/qemu-option.c
+++ b/qemu-option.c
@@ -615,12 +615,11 @@ static void qemu_opt_del(QemuOpt *opt)
     g_free(opt);
 }
 
-static int opt_set(QemuOpts *opts, const char *name, const char *value,
-                   bool prepend)
+static void opt_set(QemuOpts *opts, const char *name, const char *value,
+                    bool prepend, Error **errp)
 {
     QemuOpt *opt;
     const QemuOptDesc *desc = opts->list->desc;
-    Error *local_err = NULL;
     int i;
 
     for (i = 0; desc[i].name != NULL; i++) {
@@ -632,8 +631,8 @@ static int opt_set(QemuOpts *opts, const char *name, const char *value,
         if (i == 0) {
             /* empty list -> allow any */;
         } else {
-            error_set(&local_err, QERR_INVALID_PARAMETER, name);
-            goto exit_err;
+            error_set(errp, QERR_INVALID_PARAMETER, name);
+            return;
         }
     }
 
@@ -651,23 +650,24 @@ static int opt_set(QemuOpts *opts, const char *name, const char *value,
     if (value) {
         opt->str = g_strdup(value);
     }
-    qemu_opt_parse(opt, &local_err);
-    if (error_is_set(&local_err)) {
+    qemu_opt_parse(opt, errp);
+    if (error_is_set(errp)) {
         qemu_opt_del(opt);
-        goto exit_err;
     }
-
-    return 0;
-
-exit_err:
-    qerror_report_err(local_err);
-    error_free(local_err);
-    return -1;
 }
 
 int qemu_opt_set(QemuOpts *opts, const char *name, const char *value)
 {
-    return opt_set(opts, name, value, false);
+    Error *local_err = NULL;
+
+    opt_set(opts, name, value, false, &local_err);
+    if (error_is_set(&local_err)) {
+        qerror_report_err(local_err);
+        error_free(local_err);
+        return -1;
+    }
+
+    return 0;
 }
 
 int qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val)
@@ -853,6 +853,7 @@ static int opts_do_parse(QemuOpts *opts, const char *params,
 {
     char option[128], value[1024];
     const char *p,*pe,*pc;
+    Error *local_err = NULL;
 
     for (p = params; *p != '\0'; p++) {
         pe = strchr(p, '=');
@@ -884,7 +885,10 @@ static int opts_do_parse(QemuOpts *opts, const char *params,
         }
         if (strcmp(option, "id") != 0) {
             /* store and parse */
-            if (opt_set(opts, option, value, prepend) == -1) {
+            opt_set(opts, option, value, prepend, &local_err);
+            if (error_is_set(&local_err)) {
+                qerror_report_err(local_err);
+                error_free(local_err);
                 return -1;
             }
         }
-- 
1.7.9.2.384.g4a92a

[Qemu-devel] [PATCH 07/13] qemu-option: opts_do_parse(): use error_set()

[Luiz Capitulino]

The functions qemu_opts_do_parse() and opts_parse() both call
opts_do_parse(), but their callers expect QError semantics. Thus,
both functions call qerro_report_err() to keep the expected semantics.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qemu-option.c |   38 ++++++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 14 deletions(-)

diff --git a/qemu-option.c b/qemu-option.c
index 38461d4..666eacd 100644
--- a/qemu-option.c
+++ b/qemu-option.c
@@ -848,12 +848,11 @@ int qemu_opts_print(QemuOpts *opts, void *dummy)
     return 0;
 }
 
-static int opts_do_parse(QemuOpts *opts, const char *params,
-                         const char *firstname, bool prepend)
+static void opts_do_parse(QemuOpts *opts, const char *params,
+                          const char *firstname, bool prepend, Error **errp)
 {
     char option[128], value[1024];
     const char *p,*pe,*pc;
-    Error *local_err = NULL;
 
     for (p = params; *p != '\0'; p++) {
         pe = strchr(p, '=');
@@ -885,23 +884,29 @@ static int opts_do_parse(QemuOpts *opts, const char *params,
         }
         if (strcmp(option, "id") != 0) {
             /* store and parse */
-            opt_set(opts, option, value, prepend, &local_err);
-            if (error_is_set(&local_err)) {
-                qerror_report_err(local_err);
-                error_free(local_err);
-                return -1;
+            opt_set(opts, option, value, prepend, errp);
+            if (error_is_set(errp)) {
+                return;
             }
         }
         if (*p != ',') {
             break;
         }
     }
-    return 0;
 }
 
 int qemu_opts_do_parse(QemuOpts *opts, const char *params, const char *firstname)
 {
-    return opts_do_parse(opts, params, firstname, false);
+    Error *local_err = NULL;
+
+    opts_do_parse(opts, params, firstname, false, &local_err);
+    if (error_is_set(&local_err)) {
+        qerror_report_err(local_err);
+        error_free(local_err);
+        return -1;
+    }
+
+    return 0;
 }
 
 static QemuOpts *opts_parse(QemuOptsList *list, const char *params,
@@ -934,18 +939,23 @@ static QemuOpts *opts_parse(QemuOptsList *list, const char *params,
     }
     if (opts == NULL) {
         if (error_is_set(&local_err)) {
-            qerror_report_err(local_err);
-            error_free(local_err);
+            goto exit_err;
         }
         return NULL;
     }
 
-    if (opts_do_parse(opts, params, firstname, defaults) != 0) {
+    opts_do_parse(opts, params, firstname, defaults, &local_err);
+    if (error_is_set(&local_err)) {
         qemu_opts_del(opts);
-        return NULL;
+        goto exit_err;
     }
 
     return opts;
+
+exit_err:
+    qerror_report_err(local_err);
+    error_free(local_err);
+    return NULL;
 }
 
 QemuOpts *qemu_opts_parse(QemuOptsList *list, const char *params,
-- 
1.7.9.2.384.g4a92a

[Qemu-devel] [PATCH 08/13] qemu-option: introduce qemu_opt_set_err()

[Luiz Capitulino]

This is like qemu_opt_set(), except that it takes an Error argument.

This new function allows for a incremental conversion of code using
qemu_opt_set().

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qemu-option.c |    6 ++++++
 qemu-option.h |    2 ++
 2 files changed, 8 insertions(+)

diff --git a/qemu-option.c b/qemu-option.c
index 666eacd..881467d 100644
--- a/qemu-option.c
+++ b/qemu-option.c
@@ -670,6 +670,12 @@ int qemu_opt_set(QemuOpts *opts, const char *name, const char *value)
     return 0;
 }
 
+void qemu_opt_set_err(QemuOpts *opts, const char *name, const char *value,
+                      Error **errp)
+{
+    opt_set(opts, name, value, false, errp);
+}
+
 int qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val)
 {
     QemuOpt *opt;
diff --git a/qemu-option.h b/qemu-option.h
index 4d5b3d3..52e9ec1 100644
--- a/qemu-option.h
+++ b/qemu-option.h
@@ -111,6 +111,8 @@ bool qemu_opt_get_bool(QemuOpts *opts, const char *name, bool defval);
 uint64_t qemu_opt_get_number(QemuOpts *opts, const char *name, uint64_t defval);
 uint64_t qemu_opt_get_size(QemuOpts *opts, const char *name, uint64_t defval);
 int qemu_opt_set(QemuOpts *opts, const char *name, const char *value);
+void qemu_opt_set_err(QemuOpts *opts, const char *name, const char *value,
+                      Error **errp);
 int qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val);
 typedef int (*qemu_opt_loopfunc)(const char *name, const char *value, void *opaque);
 int qemu_opt_foreach(QemuOpts *opts, qemu_opt_loopfunc func, void *opaque,
-- 
1.7.9.2.384.g4a92a

[Qemu-devel] [PATCH 09/13] qerror: introduce QERR_INVALID_OPTION_GROUP

[Luiz Capitulino]

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

diff --git a/qerror.c b/qerror.c
index 41c729a..02e3a14 100644
--- a/qerror.c
+++ b/qerror.c
@@ -156,6 +156,10 @@ static const QErrorStringTable qerror_table[] = {
         .desc      = "Invalid block format '%(name)'",
     },
     {
+        .error_fmt = QERR_INVALID_OPTION_GROUP,
+        .desc      = "There is no option group '%(group)'",
+    },
+    {
         .error_fmt = QERR_INVALID_PARAMETER,
         .desc      = "Invalid parameter '%(name)'",
     },
diff --git a/qerror.h b/qerror.h
index e16f9c2..c7eeee9 100644
--- a/qerror.h
+++ b/qerror.h
@@ -139,6 +139,9 @@ QError *qobject_to_qerror(const QObject *obj);
 #define QERR_INVALID_BLOCK_FORMAT \
     "{ 'class': 'InvalidBlockFormat', 'data': { 'name': %s } }"
 
+#define QERR_INVALID_OPTION_GROUP \
+    "{ 'class': 'InvalidOptionGroup', 'data': { 'group': %s } }"
+
 #define QERR_INVALID_PARAMETER \
     "{ 'class': 'InvalidParameter', 'data': { 'name': %s } }"
 
-- 
1.7.9.2.384.g4a92a

[Qemu-devel] [PATCH 10/13] qemu-config: find_list(): use error_set()

[Luiz Capitulino]

Note that qemu_find_opts() callers still expect automatic error reporting
with QError, so qemu_find_opts() calls qerror_report_err() to keep the
same semantics.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qemu-config.c |   32 +++++++++++++++++++++++++-------
 1 file changed, 25 insertions(+), 7 deletions(-)

diff --git a/qemu-config.c b/qemu-config.c
index f876646..bdb381d 100644
--- a/qemu-config.c
+++ b/qemu-config.c
@@ -3,6 +3,7 @@
 #include "qemu-option.h"
 #include "qemu-config.h"
 #include "hw/qdev.h"
+#include "error.h"
 
 static QemuOptsList qemu_drive_opts = {
     .name = "drive",
@@ -631,7 +632,8 @@ static QemuOptsList *vm_config_groups[32] = {
     NULL,
 };
 
-static QemuOptsList *find_list(QemuOptsList **lists, const char *group)
+static QemuOptsList *find_list(QemuOptsList **lists, const char *group,
+                               Error **errp)
 {
     int i;
 
@@ -640,14 +642,23 @@ static QemuOptsList *find_list(QemuOptsList **lists, const char *group)
             break;
     }
     if (lists[i] == NULL) {
-        error_report("there is no option group \"%s\"", group);
+        error_set(errp, QERR_INVALID_OPTION_GROUP, group);
     }
     return lists[i];
 }
 
 QemuOptsList *qemu_find_opts(const char *group)
 {
-    return find_list(vm_config_groups, group);
+    QemuOptsList *ret;
+    Error *local_err = NULL;
+
+    ret = find_list(vm_config_groups, group, &local_err);
+    if (error_is_set(&local_err)) {
+        error_report("%s\n", error_get_pretty(local_err));
+        error_free(local_err);
+    }
+
+    return ret;
 }
 
 void qemu_add_opts(QemuOptsList *list)
@@ -762,6 +773,7 @@ int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname)
     char line[1024], group[64], id[64], arg[64], value[1024];
     Location loc;
     QemuOptsList *list = NULL;
+    Error *local_err = NULL;
     QemuOpts *opts = NULL;
     int res = -1, lno = 0;
 
@@ -778,17 +790,23 @@ int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname)
         }
         if (sscanf(line, "[%63s \"%63[^\"]\"]", group, id) == 2) {
             /* group with id */
-            list = find_list(lists, group);
-            if (list == NULL)
+            list = find_list(lists, group, &local_err);
+            if (error_is_set(&local_err)) {
+                error_report("%s\n", error_get_pretty(local_err));
+                error_free(local_err);
                 goto out;
+            }
             opts = qemu_opts_create(list, id, 1, NULL);
             continue;
         }
         if (sscanf(line, "[%63[^]]]", group) == 1) {
             /* group without id */
-            list = find_list(lists, group);
-            if (list == NULL)
+            list = find_list(lists, group, &local_err);
+            if (error_is_set(&local_err)) {
+                error_report("%s\n", error_get_pretty(local_err));
+                error_free(local_err);
                 goto out;
+            }
             opts = qemu_opts_create(list, NULL, 0, NULL);
             continue;
         }
-- 
1.7.9.2.384.g4a92a

[Qemu-devel] [PATCH 11/13] qemu-config: introduce qemu_find_opts_err()

[Luiz Capitulino]

This is like qemu_find_opts(), except that it takes an Error argument.

This new function allows for a incremental conversion of code using
qemu_find_opts().

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qemu-config.c |    5 +++++
 qemu-config.h |    3 +++
 2 files changed, 8 insertions(+)

diff --git a/qemu-config.c b/qemu-config.c
index bdb381d..bb3bff4 100644
--- a/qemu-config.c
+++ b/qemu-config.c
@@ -661,6 +661,11 @@ QemuOptsList *qemu_find_opts(const char *group)
     return ret;
 }
 
+QemuOptsList *qemu_find_opts_err(const char *group, Error **errp)
+{
+    return find_list(vm_config_groups, group, errp);
+}
+
 void qemu_add_opts(QemuOptsList *list)
 {
     int entries, i;
diff --git a/qemu-config.h b/qemu-config.h
index 20d707f..a093e3f 100644
--- a/qemu-config.h
+++ b/qemu-config.h
@@ -1,11 +1,14 @@
 #ifndef QEMU_CONFIG_H
 #define QEMU_CONFIG_H
 
+#include "error.h"
+
 extern QemuOptsList qemu_fsdev_opts;
 extern QemuOptsList qemu_virtfs_opts;
 extern QemuOptsList qemu_spice_opts;
 
 QemuOptsList *qemu_find_opts(const char *group);
+QemuOptsList *qemu_find_opts_err(const char *group, Error **errp);
 void qemu_add_opts(QemuOptsList *list);
 int qemu_set_option(const char *str);
 int qemu_global_option(const char *str);
-- 
1.7.9.2.384.g4a92a

[Qemu-devel] [PATCH 12/13] qapi: support for keyworded variable-length argument list

[Luiz Capitulino]

This allows for QAPI functions to receive a variable-length argument
list. This is going to be used by device_add and netdev_add commands.

In the schema, the argument list is represented by type name '**',
like this example:

    { 'command': 'foo', 'data': { 'arg-list': '**' } }

Each argument is represented by the KeyValues type and the C
implementation should expect a KeyValuesList, like:

    void qmp_foo(KeyValuesList *values_list, Error **errp);

XXX: This implementation is simple but very hacky. We just iterate
     through all arguments and build the KeyValuesList list to be
     passed to the QAPI function.

     Maybe we could have a kwargs type, that does exactly this but
     through a visitor instead?

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qapi-schema.json         |   15 +++++++++++++++
 scripts/qapi-commands.py |   31 ++++++++++++++++++++++++++++---
 scripts/qapi.py          |    2 ++
 3 files changed, 45 insertions(+), 3 deletions(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index 0d11d6e..25bd487 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1701,3 +1701,18 @@
 # Since: 1.1
 ##
 { 'command': 'xen-save-devices-state', 'data': {'filename': 'str'} }
+
+##
+# @KeyValues:
+#
+# A generic representation of a key value pair.
+#
+# @key: the name of the item
+#
+# @value: the string representation of the item value.  This typically follows
+#         QEMU's command line parsing format.  See the man pages for more
+#         information.
+#
+# Since: 0.14.0
+##
+{ 'type': 'KeyValues', 'data': {'key': 'str', 'value': 'str'} }
diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index 30a24d2..75a6e81 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -146,19 +146,44 @@ v = qmp_input_get_visitor(mi);
                      obj=obj)
 
     for argname, argtype, optional, structured in parse_args(args):
-        if optional:
+        if optional and not '**':
             ret += mcgen('''
 visit_start_optional(v, &has_%(c_name)s, "%(name)s", errp);
 if (has_%(c_name)s) {
 ''',
                          c_name=c_var(argname), name=argname)
             push_indent()
-        ret += mcgen('''
+        if argtype == '**':
+            if dealloc:
+                ret += mcgen('''
+qapi_free_KeyValuesList(%(obj)s);
+''',
+                        obj=c_var(argname))
+            else:
+                ret += mcgen('''
+{
+    const QDictEntry *entry;
+    v = v; /* fix me baby */
+    
+    for (entry = qdict_first(args); entry; entry = qdict_next(qdict, entry)) {
+        KeyValuesList *item = g_malloc0(sizeof(*item));
+        item->value = g_malloc0(sizeof(*item->value));
+        item->value->key = g_strdup(qdict_entry_key(entry));
+        item->value->value = g_strdup(qstring_get_str(qobject_to_qstring(qdict_entry_value(entry))));
+    
+        item->next = %(obj)s;
+        %(obj)s = item;
+    }
+}
+''',
+                        obj=c_var(argname))
+        else:
+            ret += mcgen('''
 %(visitor)s(v, &%(c_name)s, "%(name)s", errp);
 ''',
                      c_name=c_var(argname), name=argname, argtype=argtype,
                      visitor=type_visitor(argtype))
-        if optional:
+        if optional and not '**':
             pop_indent()
             ret += mcgen('''
 }
diff --git a/scripts/qapi.py b/scripts/qapi.py
index e062336..87b9ee6 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -163,6 +163,8 @@ def c_type(name):
         return 'bool'
     elif name == 'number':
         return 'double'
+    elif name == '**':
+        return 'KeyValuesList *'
     elif type(name) == list:
         return '%s *' % c_list_type(name[0])
     elif is_enum(name):
-- 
1.7.9.2.384.g4a92a

[Qemu-devel] [PATCH 13/13] qapi: convert device_add

[Luiz Capitulino]

FIXME: HMP's help text (device_add ?) is not implemented. We need a way to
       export this information in QMP.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 hmp-commands.hx   |    3 +--
 hmp.c             |   24 ++++++++++++++++++
 hmp.h             |    1 +
 hw/qdev-monitor.c |   70 +++++++++++++++++++++++++++++++++++++++--------------
 hw/qdev.h         |    2 +-
 qapi-schema.json  |   23 ++++++++++++++++++
 qmp-commands.hx   |    3 +--
 vl.c              |    3 ++-
 8 files changed, 105 insertions(+), 24 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index bd35a3e..376dc4d 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -606,8 +606,7 @@ ETEXI
         .args_type  = "device:O",
         .params     = "driver[,prop=value][,...]",
         .help       = "add device, like -device on the command line",
-        .user_print = monitor_user_noop,
-        .mhandler.cmd_new = do_device_add,
+        .mhandler.cmd = hmp_device_add,
     },
 
 STEXI
diff --git a/hmp.c b/hmp.c
index 9cf2d13..23c06ec 100644
--- a/hmp.c
+++ b/hmp.c
@@ -934,3 +934,27 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)
         qemu_mod_timer(status->timer, qemu_get_clock_ms(rt_clock));
     }
 }
+
+void hmp_device_add(Monitor *mon, const QDict *qdict)
+{
+    KeyValuesList *opts_list = NULL;
+    const QDictEntry *entry;
+    Error *err = NULL;
+
+    for (entry = qdict_first(qdict); entry; entry = qdict_next(qdict, entry)) {
+        const char *key = qdict_entry_key(entry);
+        const char *value = qstring_get_str(qobject_to_qstring(qdict_entry_value(entry)));
+        KeyValuesList *kv;
+
+        kv = g_malloc0(sizeof(*kv));
+        kv->value = g_malloc0(sizeof(*kv->value));
+        kv->value->key = g_strdup(key);
+        kv->value->value = g_strdup(value);
+        kv->next = opts_list;
+        opts_list = kv;
+    }
+
+    qmp_device_add(opts_list, &err);
+    qapi_free_KeyValuesList(opts_list);
+    hmp_handle_error(mon, &err);
+}
diff --git a/hmp.h b/hmp.h
index 8807853..3e75d4c 100644
--- a/hmp.h
+++ b/hmp.h
@@ -60,5 +60,6 @@ void hmp_block_stream(Monitor *mon, const QDict *qdict);
 void hmp_block_job_set_speed(Monitor *mon, const QDict *qdict);
 void hmp_block_job_cancel(Monitor *mon, const QDict *qdict);
 void hmp_migrate(Monitor *mon, const QDict *qdict);
+void hmp_device_add(Monitor *mon, const QDict *qdict);
 
 #endif
diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
index a310cc7..0661f55 100644
--- a/hw/qdev-monitor.c
+++ b/hw/qdev-monitor.c
@@ -19,6 +19,7 @@
 
 #include "qdev.h"
 #include "monitor.h"
+#include "qmp-commands.h"
 
 /*
  * Aliases were a bad idea from the start.  Let's keep them
@@ -388,7 +389,7 @@ static BusState *qbus_find(const char *path)
     }
 }
 
-DeviceState *qdev_device_add(QemuOpts *opts)
+DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
 {
     ObjectClass *obj;
     DeviceClass *k;
@@ -398,7 +399,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
 
     driver = qemu_opt_get(opts, "driver");
     if (!driver) {
-        qerror_report(QERR_MISSING_PARAMETER, "driver");
+        error_set(errp, QERR_MISSING_PARAMETER, "driver");
         return NULL;
     }
 
@@ -414,7 +415,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
     }
 
     if (!obj) {
-        qerror_report(QERR_INVALID_PARAMETER_VALUE, "driver", "device type");
+        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "driver", "device type");
         return NULL;
     }
 
@@ -428,20 +429,18 @@ DeviceState *qdev_device_add(QemuOpts *opts)
             return NULL;
         }
         if (bus->info != k->bus_info) {
-            qerror_report(QERR_BAD_BUS_FOR_DEVICE,
-                           driver, bus->info->name);
+            error_set(errp, QERR_BAD_BUS_FOR_DEVICE, driver, bus->info->name);
             return NULL;
         }
     } else {
         bus = qbus_find_recursive(sysbus_get_default(), NULL, k->bus_info);
         if (!bus) {
-            qerror_report(QERR_NO_BUS_FOR_DEVICE,
-                          driver, k->bus_info->name);
+            error_set(errp, QERR_NO_BUS_FOR_DEVICE, driver, k->bus_info->name);
             return NULL;
         }
     }
     if (qdev_hotplug && !bus->allow_hotplug) {
-        qerror_report(QERR_BUS_NO_HOTPLUG, bus->name);
+        error_set(errp, QERR_BUS_NO_HOTPLUG, bus->name);
         return NULL;
     }
 
@@ -463,7 +462,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
         return NULL;
     }
     if (qdev_init(qdev) < 0) {
-        qerror_report(QERR_DEVICE_INIT_FAILED, driver);
+        error_set(errp, QERR_DEVICE_INIT_FAILED, driver);
         return NULL;
     }
     if (qdev->id) {
@@ -555,23 +554,58 @@ void do_info_qdm(Monitor *mon)
     object_class_foreach(qdev_print_devinfo, TYPE_DEVICE, false, NULL);
 }
 
-int do_device_add(Monitor *mon, const QDict *qdict, QObject **ret_data)
+void qmp_device_add(KeyValuesList *opts, Error **errp)
 {
-    QemuOpts *opts;
+    Error *local_err = NULL;
+    QemuOptsList *opts_list;
+    QemuOpts *qopts = NULL;
+    const char *id = NULL;
+    KeyValuesList *kv;
+
+    for (kv = opts; kv; kv = kv->next) {
+        if (!strcmp(kv->value->key, "id")) {
+            id = kv->value->key;
+            break;
+        }
+    }
 
-    opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict);
-    if (!opts) {
-        return -1;
+    opts_list = qemu_find_opts_err("device", &local_err);
+    if (error_is_set(&local_err)) {
+        goto exit_err;
+    }
+
+    qopts = qemu_opts_create(opts_list, id, 1, &local_err);
+    if (local_err) {
+        goto exit_err;
+    }
+
+    for (kv = opts; kv; kv = kv->next) {
+        qemu_opt_set_err(qopts, kv->value->key, kv->value->value, &local_err);
+        if (error_is_set(&local_err)) {
+            goto exit_err;
+        }
     }
+
+#if 0
+    FIXME: move the help to the HMP counterpart
     if (!monitor_cur_is_qmp() && qdev_device_help(opts)) {
         qemu_opts_del(opts);
         return 0;
     }
-    if (!qdev_device_add(opts)) {
-        qemu_opts_del(opts);
-        return -1;
+#endif
+
+    qdev_device_add(qopts, &local_err);
+    if (error_is_set(&local_err)) {
+        goto exit_err;
     }
-    return 0;
+
+    return;
+
+exit_err:
+    if (qopts) {
+        qemu_opts_del(qopts);
+    }
+    error_propagate(errp, local_err);
 }
 
 int do_device_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
diff --git a/hw/qdev.h b/hw/qdev.h
index 9cc3f98..3b938e2 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -143,7 +143,7 @@ DeviceState *qdev_create(BusState *bus, const char *name);
 DeviceState *qdev_try_create(BusState *bus, const char *name);
 bool qdev_exists(const char *name);
 int qdev_device_help(QemuOpts *opts);
-DeviceState *qdev_device_add(QemuOpts *opts);
+DeviceState *qdev_device_add(QemuOpts *opts, Error **errp);
 int qdev_init(DeviceState *dev) QEMU_WARN_UNUSED_RESULT;
 void qdev_init_nofail(DeviceState *dev);
 void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
diff --git a/qapi-schema.json b/qapi-schema.json
index 25bd487..68e2515 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1716,3 +1716,26 @@
 # Since: 0.14.0
 ##
 { 'type': 'KeyValues', 'data': {'key': 'str', 'value': 'str'} }
+
+##
+# @device_add:
+#
+# Add a new device to the guest
+#
+# @device-optoins: a list of KeyValues of options specific to each device.
+#                  There are two driver indepedent options: 'driver' (which is
+#                  actually _required_) and 'id'.
+#
+# Returns: Nothing on success
+#          If @driver is not a valid device type, DeviceNotFound
+#          If @opts contains an invalid parameter for this device,
+#            InvalidParameter
+#
+# Notes: The semantics of @opts is not well defined.  Future commands will be
+#        introduced that provide stronger typing for device creation.
+#        This command performs hotplug of devices for guests.  When the command
+#        returns, the device may not be visible to the guest.
+#
+# Since: 0.14.0
+##
+{ 'command': 'device_add', 'data': {'device-options': '**' } }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index c626ba8..829a043 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -279,8 +279,7 @@ EQMP
         .args_type  = "device:O",
         .params     = "driver[,prop=value][,...]",
         .help       = "add device, like -device on the command line",
-        .user_print = monitor_user_noop,
-        .mhandler.cmd_new = do_device_add,
+        .mhandler.cmd_new = qmp_marshal_input_device_add,
     },
 
 SQMP
diff --git a/vl.c b/vl.c
index 6467d6b..976126f 100644
--- a/vl.c
+++ b/vl.c
@@ -1821,7 +1821,8 @@ static int device_init_func(QemuOpts *opts, void *opaque)
 {
     DeviceState *dev;
 
-    dev = qdev_device_add(opts);
+    /* FIXME: */
+    dev = qdev_device_add(opts, NULL);
     if (!dev)
         return -1;
     return 0;
-- 
1.7.9.2.384.g4a92a

Re: [Qemu-devel] [PATCH 12/13] qapi: support for keyworded variable-length argument list

[Anthony Liguori]

On 03/29/2012 12:26 PM, Luiz Capitulino wrote:
> This allows for QAPI functions to receive a variable-length argument
> list. This is going to be used by device_add and netdev_add commands.
>
> In the schema, the argument list is represented by type name '**',
> like this example:
>
>      { 'command': 'foo', 'data': { 'arg-list': '**' } }
>
> Each argument is represented by the KeyValues type and the C
> implementation should expect a KeyValuesList, like:
>
>      void qmp_foo(KeyValuesList *values_list, Error **errp);
>
> XXX: This implementation is simple but very hacky. We just iterate
>       through all arguments and build the KeyValuesList list to be
>       passed to the QAPI function.
>
>       Maybe we could have a kwargs type, that does exactly this but
>       through a visitor instead?
>
> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>

What about just treating '**' as "marshal remaining arguments to a string" and 
then pass that string to device_add?  qmp_device_add can then parse that string 
with QemuOpts.

It's a bit ugly, but that's how things worked.  When we introduce qom_add, this 
problem goes away because you would make multiple calls to qom_set to set all of 
the properties.

Regards,

Anthony Liguori

> ---
>   qapi-schema.json         |   15 +++++++++++++++
>   scripts/qapi-commands.py |   31 ++++++++++++++++++++++++++++---
>   scripts/qapi.py          |    2 ++
>   3 files changed, 45 insertions(+), 3 deletions(-)
>
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 0d11d6e..25bd487 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1701,3 +1701,18 @@
>   # Since: 1.1
>   ##
>   { 'command': 'xen-save-devices-state', 'data': {'filename': 'str'} }
> +
> +##
> +# @KeyValues:
> +#
> +# A generic representation of a key value pair.
> +#
> +# @key: the name of the item
> +#
> +# @value: the string representation of the item value.  This typically follows
> +#         QEMU's command line parsing format.  See the man pages for more
> +#         information.
> +#
> +# Since: 0.14.0
> +##
> +{ 'type': 'KeyValues', 'data': {'key': 'str', 'value': 'str'} }
> diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
> index 30a24d2..75a6e81 100644
> --- a/scripts/qapi-commands.py
> +++ b/scripts/qapi-commands.py
> @@ -146,19 +146,44 @@ v = qmp_input_get_visitor(mi);
>                        obj=obj)
>
>       for argname, argtype, optional, structured in parse_args(args):
> -        if optional:
> +        if optional and not '**':
>               ret += mcgen('''
>   visit_start_optional(v,&has_%(c_name)s, "%(name)s", errp);
>   if (has_%(c_name)s) {
>   ''',
>                            c_name=c_var(argname), name=argname)
>               push_indent()
> -        ret += mcgen('''
> +        if argtype == '**':
> +            if dealloc:
> +                ret += mcgen('''
> +qapi_free_KeyValuesList(%(obj)s);
> +''',
> +                        obj=c_var(argname))
> +            else:
> +                ret += mcgen('''
> +{
> +    const QDictEntry *entry;
> +    v = v; /* fix me baby */
> +
> +    for (entry = qdict_first(args); entry; entry = qdict_next(qdict, entry)) {
> +        KeyValuesList *item = g_malloc0(sizeof(*item));
> +        item->value = g_malloc0(sizeof(*item->value));
> +        item->value->key = g_strdup(qdict_entry_key(entry));
> +        item->value->value = g_strdup(qstring_get_str(qobject_to_qstring(qdict_entry_value(entry))));
> +
> +        item->next = %(obj)s;
> +        %(obj)s = item;
> +    }
> +}
> +''',
> +                        obj=c_var(argname))
> +        else:
> +            ret += mcgen('''
>   %(visitor)s(v,&%(c_name)s, "%(name)s", errp);
>   ''',
>                        c_name=c_var(argname), name=argname, argtype=argtype,
>                        visitor=type_visitor(argtype))
> -        if optional:
> +        if optional and not '**':
>               pop_indent()
>               ret += mcgen('''
>   }
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index e062336..87b9ee6 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -163,6 +163,8 @@ def c_type(name):
>           return 'bool'
>       elif name == 'number':
>           return 'double'
> +    elif name == '**':
> +        return 'KeyValuesList *'
>       elif type(name) == list:
>           return '%s *' % c_list_type(name[0])
>       elif is_enum(name):

Re: [Qemu-devel] [PATCH 12/13] qapi: support for keyworded variable-length argument list

[Luiz Capitulino]

On Thu, 29 Mar 2012 13:26:34 -0500
Anthony Liguori <aliguori@us.ibm.com> wrote:

> On 03/29/2012 12:26 PM, Luiz Capitulino wrote:
> > This allows for QAPI functions to receive a variable-length argument
> > list. This is going to be used by device_add and netdev_add commands.
> >
> > In the schema, the argument list is represented by type name '**',
> > like this example:
> >
> >      { 'command': 'foo', 'data': { 'arg-list': '**' } }
> >
> > Each argument is represented by the KeyValues type and the C
> > implementation should expect a KeyValuesList, like:
> >
> >      void qmp_foo(KeyValuesList *values_list, Error **errp);
> >
> > XXX: This implementation is simple but very hacky. We just iterate
> >       through all arguments and build the KeyValuesList list to be
> >       passed to the QAPI function.
> >
> >       Maybe we could have a kwargs type, that does exactly this but
> >       through a visitor instead?
> >
> > Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
> 
> What about just treating '**' as "marshal remaining arguments to a string" and 
> then pass that string to device_add?  qmp_device_add can then parse that string 
> with QemuOpts.

If this turns out to be simple enough, I'm fine with it.

> It's a bit ugly, but that's how things worked.  When we introduce qom_add, this 
> problem goes away because you would make multiple calls to qom_set to set all of 
> the properties.

Just out of curiosity, is qom_add going to supersede device_add?

Re: [Qemu-devel] [PATCH 12/13] qapi: support for keyworded variable-length argument list

[Anthony Liguori]

On 03/29/2012 01:42 PM, Luiz Capitulino wrote:
> On Thu, 29 Mar 2012 13:26:34 -0500
> Anthony Liguori<aliguori@us.ibm.com>  wrote:
>
>> On 03/29/2012 12:26 PM, Luiz Capitulino wrote:
>>> This allows for QAPI functions to receive a variable-length argument
>>> list. This is going to be used by device_add and netdev_add commands.
>>>
>>> In the schema, the argument list is represented by type name '**',
>>> like this example:
>>>
>>>       { 'command': 'foo', 'data': { 'arg-list': '**' } }
>>>
>>> Each argument is represented by the KeyValues type and the C
>>> implementation should expect a KeyValuesList, like:
>>>
>>>       void qmp_foo(KeyValuesList *values_list, Error **errp);
>>>
>>> XXX: This implementation is simple but very hacky. We just iterate
>>>        through all arguments and build the KeyValuesList list to be
>>>        passed to the QAPI function.
>>>
>>>        Maybe we could have a kwargs type, that does exactly this but
>>>        through a visitor instead?
>>>
>>> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
>>
>> What about just treating '**' as "marshal remaining arguments to a string" and
>> then pass that string to device_add?  qmp_device_add can then parse that string
>> with QemuOpts.
>
> If this turns out to be simple enough, I'm fine with it.

I don't love doing this sort of double conversion but it's really the only 
practical way to do it I think.  device_add has a weird semantic where 
printing/parsing is implied so I think it's unavoidable.

>> It's a bit ugly, but that's how things worked.  When we introduce qom_add, this
>> problem goes away because you would make multiple calls to qom_set to set all of
>> the properties.
>
> Just out of curiosity, is qom_add going to supersede device_add?

Eventually...

I'd like to see qom_add as the low level interface but then I'd like to see nice 
high level interfaces like 'block_add' which took a parameter of virtio-blk and 
did all of the magic to create a block device in such a way that's compliant to 
the current machine type.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 12/13] qapi: support for keyworded variable-length argument list

[Michael Roth]

On Thu, Mar 29, 2012 at 01:26:34PM -0500, Anthony Liguori wrote:
> On 03/29/2012 12:26 PM, Luiz Capitulino wrote:
> >This allows for QAPI functions to receive a variable-length argument
> >list. This is going to be used by device_add and netdev_add commands.
> >
> >In the schema, the argument list is represented by type name '**',
> >like this example:
> >
> >     { 'command': 'foo', 'data': { 'arg-list': '**' } }
> >
> >Each argument is represented by the KeyValues type and the C
> >implementation should expect a KeyValuesList, like:
> >
> >     void qmp_foo(KeyValuesList *values_list, Error **errp);
> >
> >XXX: This implementation is simple but very hacky. We just iterate
> >      through all arguments and build the KeyValuesList list to be
> >      passed to the QAPI function.
> >
> >      Maybe we could have a kwargs type, that does exactly this but
> >      through a visitor instead?
> >
> >Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
> 
> What about just treating '**' as "marshal remaining arguments to a
> string" and then pass that string to device_add?  qmp_device_add can
> then parse that string with QemuOpts.

Since currently we explicitly point qmp to the marshaller anyway, we
could also just treat '**' as an indicator to not generate a marshaller.
Then, we open-code the marshaller to process the QDict, rather than embedding
it in the script or passing it through to qmp_device_add().

>From the perspective of qmp_device_add() it then just looks like any
other qmp command.

> 
> It's a bit ugly, but that's how things worked.  When we introduce
> qom_add, this problem goes away because you would make multiple
> calls to qom_set to set all of the properties.
> 
> Regards,
> 
> Anthony Liguori
> 
> >---
> >  qapi-schema.json         |   15 +++++++++++++++
> >  scripts/qapi-commands.py |   31 ++++++++++++++++++++++++++++---
> >  scripts/qapi.py          |    2 ++
> >  3 files changed, 45 insertions(+), 3 deletions(-)
> >
> >diff --git a/qapi-schema.json b/qapi-schema.json
> >index 0d11d6e..25bd487 100644
> >--- a/qapi-schema.json
> >+++ b/qapi-schema.json
> >@@ -1701,3 +1701,18 @@
> >  # Since: 1.1
> >  ##
> >  { 'command': 'xen-save-devices-state', 'data': {'filename': 'str'} }
> >+
> >+##
> >+# @KeyValues:
> >+#
> >+# A generic representation of a key value pair.
> >+#
> >+# @key: the name of the item
> >+#
> >+# @value: the string representation of the item value.  This typically follows
> >+#         QEMU's command line parsing format.  See the man pages for more
> >+#         information.
> >+#
> >+# Since: 0.14.0
> >+##
> >+{ 'type': 'KeyValues', 'data': {'key': 'str', 'value': 'str'} }
> >diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
> >index 30a24d2..75a6e81 100644
> >--- a/scripts/qapi-commands.py
> >+++ b/scripts/qapi-commands.py
> >@@ -146,19 +146,44 @@ v = qmp_input_get_visitor(mi);
> >                       obj=obj)
> >
> >      for argname, argtype, optional, structured in parse_args(args):
> >-        if optional:
> >+        if optional and not '**':
> >              ret += mcgen('''
> >  visit_start_optional(v,&has_%(c_name)s, "%(name)s", errp);
> >  if (has_%(c_name)s) {
> >  ''',
> >                           c_name=c_var(argname), name=argname)
> >              push_indent()
> >-        ret += mcgen('''
> >+        if argtype == '**':
> >+            if dealloc:
> >+                ret += mcgen('''
> >+qapi_free_KeyValuesList(%(obj)s);
> >+''',
> >+                        obj=c_var(argname))
> >+            else:
> >+                ret += mcgen('''
> >+{
> >+    const QDictEntry *entry;
> >+    v = v; /* fix me baby */
> >+
> >+    for (entry = qdict_first(args); entry; entry = qdict_next(qdict, entry)) {
> >+        KeyValuesList *item = g_malloc0(sizeof(*item));
> >+        item->value = g_malloc0(sizeof(*item->value));
> >+        item->value->key = g_strdup(qdict_entry_key(entry));
> >+        item->value->value = g_strdup(qstring_get_str(qobject_to_qstring(qdict_entry_value(entry))));
> >+
> >+        item->next = %(obj)s;
> >+        %(obj)s = item;
> >+    }
> >+}
> >+''',
> >+                        obj=c_var(argname))
> >+        else:
> >+            ret += mcgen('''
> >  %(visitor)s(v,&%(c_name)s, "%(name)s", errp);
> >  ''',
> >                       c_name=c_var(argname), name=argname, argtype=argtype,
> >                       visitor=type_visitor(argtype))
> >-        if optional:
> >+        if optional and not '**':
> >              pop_indent()
> >              ret += mcgen('''
> >  }
> >diff --git a/scripts/qapi.py b/scripts/qapi.py
> >index e062336..87b9ee6 100644
> >--- a/scripts/qapi.py
> >+++ b/scripts/qapi.py
> >@@ -163,6 +163,8 @@ def c_type(name):
> >          return 'bool'
> >      elif name == 'number':
> >          return 'double'
> >+    elif name == '**':
> >+        return 'KeyValuesList *'
> >      elif type(name) == list:
> >          return '%s *' % c_list_type(name[0])
> >      elif is_enum(name):
> 

Re: [Qemu-devel] [PATCH 12/13] qapi: support for keyworded variable-length argument list

[Anthony Liguori]

On 03/29/2012 02:28 PM, Michael Roth wrote:
> On Thu, Mar 29, 2012 at 01:26:34PM -0500, Anthony Liguori wrote:
>> On 03/29/2012 12:26 PM, Luiz Capitulino wrote:
>>> This allows for QAPI functions to receive a variable-length argument
>>> list. This is going to be used by device_add and netdev_add commands.
>>>
>>> In the schema, the argument list is represented by type name '**',
>>> like this example:
>>>
>>>      { 'command': 'foo', 'data': { 'arg-list': '**' } }
>>>
>>> Each argument is represented by the KeyValues type and the C
>>> implementation should expect a KeyValuesList, like:
>>>
>>>      void qmp_foo(KeyValuesList *values_list, Error **errp);
>>>
>>> XXX: This implementation is simple but very hacky. We just iterate
>>>       through all arguments and build the KeyValuesList list to be
>>>       passed to the QAPI function.
>>>
>>>       Maybe we could have a kwargs type, that does exactly this but
>>>       through a visitor instead?
>>>
>>> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
>>
>> What about just treating '**' as "marshal remaining arguments to a
>> string" and then pass that string to device_add?  qmp_device_add can
>> then parse that string with QemuOpts.
>
> Since currently we explicitly point qmp to the marshaller anyway, we
> could also just treat '**' as an indicator to not generate a marshaller.
> Then, we open-code the marshaller to process the QDict, rather than embedding
> it in the script or passing it through to qmp_device_add().

You could also just do gen=False...

But I don't think open coding the marshaller is the right thing here.  You have 
to convert to strings and reparse anyway.  The code needs to be shared between 
device_add and netdev_add too.

Regards,

Anthony Liguori

>
>> From the perspective of qmp_device_add() it then just looks like any
> other qmp command.
>
>>
>> It's a bit ugly, but that's how things worked.  When we introduce
>> qom_add, this problem goes away because you would make multiple
>> calls to qom_set to set all of the properties.
>>
>> Regards,
>>
>> Anthony Liguori
>>
>>> ---
>>>   qapi-schema.json         |   15 +++++++++++++++
>>>   scripts/qapi-commands.py |   31 ++++++++++++++++++++++++++++---
>>>   scripts/qapi.py          |    2 ++
>>>   3 files changed, 45 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/qapi-schema.json b/qapi-schema.json
>>> index 0d11d6e..25bd487 100644
>>> --- a/qapi-schema.json
>>> +++ b/qapi-schema.json
>>> @@ -1701,3 +1701,18 @@
>>>   # Since: 1.1
>>>   ##
>>>   { 'command': 'xen-save-devices-state', 'data': {'filename': 'str'} }
>>> +
>>> +##
>>> +# @KeyValues:
>>> +#
>>> +# A generic representation of a key value pair.
>>> +#
>>> +# @key: the name of the item
>>> +#
>>> +# @value: the string representation of the item value.  This typically follows
>>> +#         QEMU's command line parsing format.  See the man pages for more
>>> +#         information.
>>> +#
>>> +# Since: 0.14.0
>>> +##
>>> +{ 'type': 'KeyValues', 'data': {'key': 'str', 'value': 'str'} }
>>> diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
>>> index 30a24d2..75a6e81 100644
>>> --- a/scripts/qapi-commands.py
>>> +++ b/scripts/qapi-commands.py
>>> @@ -146,19 +146,44 @@ v = qmp_input_get_visitor(mi);
>>>                        obj=obj)
>>>
>>>       for argname, argtype, optional, structured in parse_args(args):
>>> -        if optional:
>>> +        if optional and not '**':
>>>               ret += mcgen('''
>>>   visit_start_optional(v,&has_%(c_name)s, "%(name)s", errp);
>>>   if (has_%(c_name)s) {
>>>   ''',
>>>                            c_name=c_var(argname), name=argname)
>>>               push_indent()
>>> -        ret += mcgen('''
>>> +        if argtype == '**':
>>> +            if dealloc:
>>> +                ret += mcgen('''
>>> +qapi_free_KeyValuesList(%(obj)s);
>>> +''',
>>> +                        obj=c_var(argname))
>>> +            else:
>>> +                ret += mcgen('''
>>> +{
>>> +    const QDictEntry *entry;
>>> +    v = v; /* fix me baby */
>>> +
>>> +    for (entry = qdict_first(args); entry; entry = qdict_next(qdict, entry)) {
>>> +        KeyValuesList *item = g_malloc0(sizeof(*item));
>>> +        item->value = g_malloc0(sizeof(*item->value));
>>> +        item->value->key = g_strdup(qdict_entry_key(entry));
>>> +        item->value->value = g_strdup(qstring_get_str(qobject_to_qstring(qdict_entry_value(entry))));
>>> +
>>> +        item->next = %(obj)s;
>>> +        %(obj)s = item;
>>> +    }
>>> +}
>>> +''',
>>> +                        obj=c_var(argname))
>>> +        else:
>>> +            ret += mcgen('''
>>>   %(visitor)s(v,&%(c_name)s, "%(name)s", errp);
>>>   ''',
>>>                        c_name=c_var(argname), name=argname, argtype=argtype,
>>>                        visitor=type_visitor(argtype))
>>> -        if optional:
>>> +        if optional and not '**':
>>>               pop_indent()
>>>               ret += mcgen('''
>>>   }
>>> diff --git a/scripts/qapi.py b/scripts/qapi.py
>>> index e062336..87b9ee6 100644
>>> --- a/scripts/qapi.py
>>> +++ b/scripts/qapi.py
>>> @@ -163,6 +163,8 @@ def c_type(name):
>>>           return 'bool'
>>>       elif name == 'number':
>>>           return 'double'
>>> +    elif name == '**':
>>> +        return 'KeyValuesList *'
>>>       elif type(name) == list:
>>>           return '%s *' % c_list_type(name[0])
>>>       elif is_enum(name):
>>
>

Re: [Qemu-devel] [PATCH 12/13] qapi: support for keyworded variable-length argument list

[Paolo Bonzini]

Il 29/03/2012 21:28, Michael Roth ha scritto:
> Since currently we explicitly point qmp to the marshaller anyway, we
> could also just treat '**' as an indicator to not generate a marshaller.
> Then, we open-code the marshaller to process the QDict, rather than embedding
> it in the script or passing it through to qmp_device_add().
> 
> From the perspective of qmp_device_add() it then just looks like any
> other qmp command.

That's what no_gen does, right?

Paolo

Re: [Qemu-devel] [PATCH 12/13] qapi: support for keyworded variable-length argument list

[Michael Roth]

On Thu, Mar 29, 2012 at 03:01:16PM -0500, Anthony Liguori wrote:
> On 03/29/2012 02:28 PM, Michael Roth wrote:
> >On Thu, Mar 29, 2012 at 01:26:34PM -0500, Anthony Liguori wrote:
> >>On 03/29/2012 12:26 PM, Luiz Capitulino wrote:
> >>>This allows for QAPI functions to receive a variable-length argument
> >>>list. This is going to be used by device_add and netdev_add commands.
> >>>
> >>>In the schema, the argument list is represented by type name '**',
> >>>like this example:
> >>>
> >>>     { 'command': 'foo', 'data': { 'arg-list': '**' } }
> >>>
> >>>Each argument is represented by the KeyValues type and the C
> >>>implementation should expect a KeyValuesList, like:
> >>>
> >>>     void qmp_foo(KeyValuesList *values_list, Error **errp);
> >>>
> >>>XXX: This implementation is simple but very hacky. We just iterate
> >>>      through all arguments and build the KeyValuesList list to be
> >>>      passed to the QAPI function.
> >>>
> >>>      Maybe we could have a kwargs type, that does exactly this but
> >>>      through a visitor instead?
> >>>
> >>>Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
> >>
> >>What about just treating '**' as "marshal remaining arguments to a
> >>string" and then pass that string to device_add?  qmp_device_add can
> >>then parse that string with QemuOpts.
> >
> >Since currently we explicitly point qmp to the marshaller anyway, we
> >could also just treat '**' as an indicator to not generate a marshaller.
> >Then, we open-code the marshaller to process the QDict, rather than embedding
> >it in the script or passing it through to qmp_device_add().
> 
> You could also just do gen=False...

Ahh, yes we could. Nice :)

> 
> But I don't think open coding the marshaller is the right thing
> here.  You have to convert to strings and reparse anyway.  The code
> needs to be shared between device_add and netdev_add too.

The only thing the marshallers need to do is call qemu_opts_from_qdict()
and pass them on to qdev_device_add()/net_client_init()/etc. We'd basically
be taking the current qmp implementations and modifying their call signatures
to be compatible with qmp-dispatch.c in the future. It's not really
qapi-ish, admittedly, but neither is a built-in varargs sort of type.

I'd just prefer not to bake legacy hooks into the code generators if we
don't have to. If we absolutely have to do this in the future, it would be more
sense to define such fields as being string arguments from the get-go.

> 
> Regards,
> 
> Anthony Liguori
> 
> >
> >>From the perspective of qmp_device_add() it then just looks like any
> >other qmp command.
> >
> >>
> >>It's a bit ugly, but that's how things worked.  When we introduce
> >>qom_add, this problem goes away because you would make multiple
> >>calls to qom_set to set all of the properties.
> >>
> >>Regards,
> >>
> >>Anthony Liguori
> >>
> >>>---
> >>>  qapi-schema.json         |   15 +++++++++++++++
> >>>  scripts/qapi-commands.py |   31 ++++++++++++++++++++++++++++---
> >>>  scripts/qapi.py          |    2 ++
> >>>  3 files changed, 45 insertions(+), 3 deletions(-)
> >>>
> >>>diff --git a/qapi-schema.json b/qapi-schema.json
> >>>index 0d11d6e..25bd487 100644
> >>>--- a/qapi-schema.json
> >>>+++ b/qapi-schema.json
> >>>@@ -1701,3 +1701,18 @@
> >>>  # Since: 1.1
> >>>  ##
> >>>  { 'command': 'xen-save-devices-state', 'data': {'filename': 'str'} }
> >>>+
> >>>+##
> >>>+# @KeyValues:
> >>>+#
> >>>+# A generic representation of a key value pair.
> >>>+#
> >>>+# @key: the name of the item
> >>>+#
> >>>+# @value: the string representation of the item value.  This typically follows
> >>>+#         QEMU's command line parsing format.  See the man pages for more
> >>>+#         information.
> >>>+#
> >>>+# Since: 0.14.0
> >>>+##
> >>>+{ 'type': 'KeyValues', 'data': {'key': 'str', 'value': 'str'} }
> >>>diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
> >>>index 30a24d2..75a6e81 100644
> >>>--- a/scripts/qapi-commands.py
> >>>+++ b/scripts/qapi-commands.py
> >>>@@ -146,19 +146,44 @@ v = qmp_input_get_visitor(mi);
> >>>                       obj=obj)
> >>>
> >>>      for argname, argtype, optional, structured in parse_args(args):
> >>>-        if optional:
> >>>+        if optional and not '**':
> >>>              ret += mcgen('''
> >>>  visit_start_optional(v,&has_%(c_name)s, "%(name)s", errp);
> >>>  if (has_%(c_name)s) {
> >>>  ''',
> >>>                           c_name=c_var(argname), name=argname)
> >>>              push_indent()
> >>>-        ret += mcgen('''
> >>>+        if argtype == '**':
> >>>+            if dealloc:
> >>>+                ret += mcgen('''
> >>>+qapi_free_KeyValuesList(%(obj)s);
> >>>+''',
> >>>+                        obj=c_var(argname))
> >>>+            else:
> >>>+                ret += mcgen('''
> >>>+{
> >>>+    const QDictEntry *entry;
> >>>+    v = v; /* fix me baby */
> >>>+
> >>>+    for (entry = qdict_first(args); entry; entry = qdict_next(qdict, entry)) {
> >>>+        KeyValuesList *item = g_malloc0(sizeof(*item));
> >>>+        item->value = g_malloc0(sizeof(*item->value));
> >>>+        item->value->key = g_strdup(qdict_entry_key(entry));
> >>>+        item->value->value = g_strdup(qstring_get_str(qobject_to_qstring(qdict_entry_value(entry))));
> >>>+
> >>>+        item->next = %(obj)s;
> >>>+        %(obj)s = item;
> >>>+    }
> >>>+}
> >>>+''',
> >>>+                        obj=c_var(argname))
> >>>+        else:
> >>>+            ret += mcgen('''
> >>>  %(visitor)s(v,&%(c_name)s, "%(name)s", errp);
> >>>  ''',
> >>>                       c_name=c_var(argname), name=argname, argtype=argtype,
> >>>                       visitor=type_visitor(argtype))
> >>>-        if optional:
> >>>+        if optional and not '**':
> >>>              pop_indent()
> >>>              ret += mcgen('''
> >>>  }
> >>>diff --git a/scripts/qapi.py b/scripts/qapi.py
> >>>index e062336..87b9ee6 100644
> >>>--- a/scripts/qapi.py
> >>>+++ b/scripts/qapi.py
> >>>@@ -163,6 +163,8 @@ def c_type(name):
> >>>          return 'bool'
> >>>      elif name == 'number':
> >>>          return 'double'
> >>>+    elif name == '**':
> >>>+        return 'KeyValuesList *'
> >>>      elif type(name) == list:
> >>>          return '%s *' % c_list_type(name[0])
> >>>      elif is_enum(name):
> >>
> >
> 

Re: [Qemu-devel] [PATCH 12/13] qapi: support for keyworded variable-length argument list

[Michael Roth]

On Thu, Mar 29, 2012 at 05:39:10PM -0500, Michael Roth wrote:
> On Thu, Mar 29, 2012 at 03:01:16PM -0500, Anthony Liguori wrote:
> > On 03/29/2012 02:28 PM, Michael Roth wrote:
> > >On Thu, Mar 29, 2012 at 01:26:34PM -0500, Anthony Liguori wrote:
> > >>On 03/29/2012 12:26 PM, Luiz Capitulino wrote:
> > >>>This allows for QAPI functions to receive a variable-length argument
> > >>>list. This is going to be used by device_add and netdev_add commands.
> > >>>
> > >>>In the schema, the argument list is represented by type name '**',
> > >>>like this example:
> > >>>
> > >>>     { 'command': 'foo', 'data': { 'arg-list': '**' } }
> > >>>
> > >>>Each argument is represented by the KeyValues type and the C
> > >>>implementation should expect a KeyValuesList, like:
> > >>>
> > >>>     void qmp_foo(KeyValuesList *values_list, Error **errp);
> > >>>
> > >>>XXX: This implementation is simple but very hacky. We just iterate
> > >>>      through all arguments and build the KeyValuesList list to be
> > >>>      passed to the QAPI function.
> > >>>
> > >>>      Maybe we could have a kwargs type, that does exactly this but
> > >>>      through a visitor instead?
> > >>>
> > >>>Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
> > >>
> > >>What about just treating '**' as "marshal remaining arguments to a
> > >>string" and then pass that string to device_add?  qmp_device_add can
> > >>then parse that string with QemuOpts.
> > >
> > >Since currently we explicitly point qmp to the marshaller anyway, we
> > >could also just treat '**' as an indicator to not generate a marshaller.
> > >Then, we open-code the marshaller to process the QDict, rather than embedding
> > >it in the script or passing it through to qmp_device_add().
> > 
> > You could also just do gen=False...
> 
> Ahh, yes we could. Nice :)
> 
> > 
> > But I don't think open coding the marshaller is the right thing
> > here.  You have to convert to strings and reparse anyway.  The code
> > needs to be shared between device_add and netdev_add too.
> 
> The only thing the marshallers need to do is call qemu_opts_from_qdict()
> and pass them on to qdev_device_add()/net_client_init()/etc. We'd basically
> be taking the current qmp implementations and modifying their call signatures
> to be compatible with qmp-dispatch.c in the future. It's not really

Err, scratch that... I've been living in qemu-ga land too long. For
QMP this basically amounts to a no-op for now (other than documenting the
command in the schema and doing the error-handling cleanups), and when we
switch to the new qmp server/dispatch stuff we just massage the function
signature to match the marshaller qapi expects and continue to skip
marshaller generation.

> qapi-ish, admittedly, but neither is a built-in varargs sort of type.
> 
> I'd just prefer not to bake legacy hooks into the code generators if we
> don't have to. If we absolutely have to do this in the future, it would be more
> sense to define such fields as being string arguments from the get-go.
> 
> > 
> > Regards,
> > 
> > Anthony Liguori
> > 
> > >
> > >>From the perspective of qmp_device_add() it then just looks like any
> > >other qmp command.
> > >
> > >>
> > >>It's a bit ugly, but that's how things worked.  When we introduce
> > >>qom_add, this problem goes away because you would make multiple
> > >>calls to qom_set to set all of the properties.
> > >>
> > >>Regards,
> > >>
> > >>Anthony Liguori
> > >>
> > >>>---
> > >>>  qapi-schema.json         |   15 +++++++++++++++
> > >>>  scripts/qapi-commands.py |   31 ++++++++++++++++++++++++++++---
> > >>>  scripts/qapi.py          |    2 ++
> > >>>  3 files changed, 45 insertions(+), 3 deletions(-)
> > >>>
> > >>>diff --git a/qapi-schema.json b/qapi-schema.json
> > >>>index 0d11d6e..25bd487 100644
> > >>>--- a/qapi-schema.json
> > >>>+++ b/qapi-schema.json
> > >>>@@ -1701,3 +1701,18 @@
> > >>>  # Since: 1.1
> > >>>  ##
> > >>>  { 'command': 'xen-save-devices-state', 'data': {'filename': 'str'} }
> > >>>+
> > >>>+##
> > >>>+# @KeyValues:
> > >>>+#
> > >>>+# A generic representation of a key value pair.
> > >>>+#
> > >>>+# @key: the name of the item
> > >>>+#
> > >>>+# @value: the string representation of the item value.  This typically follows
> > >>>+#         QEMU's command line parsing format.  See the man pages for more
> > >>>+#         information.
> > >>>+#
> > >>>+# Since: 0.14.0
> > >>>+##
> > >>>+{ 'type': 'KeyValues', 'data': {'key': 'str', 'value': 'str'} }
> > >>>diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
> > >>>index 30a24d2..75a6e81 100644
> > >>>--- a/scripts/qapi-commands.py
> > >>>+++ b/scripts/qapi-commands.py
> > >>>@@ -146,19 +146,44 @@ v = qmp_input_get_visitor(mi);
> > >>>                       obj=obj)
> > >>>
> > >>>      for argname, argtype, optional, structured in parse_args(args):
> > >>>-        if optional:
> > >>>+        if optional and not '**':
> > >>>              ret += mcgen('''
> > >>>  visit_start_optional(v,&has_%(c_name)s, "%(name)s", errp);
> > >>>  if (has_%(c_name)s) {
> > >>>  ''',
> > >>>                           c_name=c_var(argname), name=argname)
> > >>>              push_indent()
> > >>>-        ret += mcgen('''
> > >>>+        if argtype == '**':
> > >>>+            if dealloc:
> > >>>+                ret += mcgen('''
> > >>>+qapi_free_KeyValuesList(%(obj)s);
> > >>>+''',
> > >>>+                        obj=c_var(argname))
> > >>>+            else:
> > >>>+                ret += mcgen('''
> > >>>+{
> > >>>+    const QDictEntry *entry;
> > >>>+    v = v; /* fix me baby */
> > >>>+
> > >>>+    for (entry = qdict_first(args); entry; entry = qdict_next(qdict, entry)) {
> > >>>+        KeyValuesList *item = g_malloc0(sizeof(*item));
> > >>>+        item->value = g_malloc0(sizeof(*item->value));
> > >>>+        item->value->key = g_strdup(qdict_entry_key(entry));
> > >>>+        item->value->value = g_strdup(qstring_get_str(qobject_to_qstring(qdict_entry_value(entry))));
> > >>>+
> > >>>+        item->next = %(obj)s;
> > >>>+        %(obj)s = item;
> > >>>+    }
> > >>>+}
> > >>>+''',
> > >>>+                        obj=c_var(argname))
> > >>>+        else:
> > >>>+            ret += mcgen('''
> > >>>  %(visitor)s(v,&%(c_name)s, "%(name)s", errp);
> > >>>  ''',
> > >>>                       c_name=c_var(argname), name=argname, argtype=argtype,
> > >>>                       visitor=type_visitor(argtype))
> > >>>-        if optional:
> > >>>+        if optional and not '**':
> > >>>              pop_indent()
> > >>>              ret += mcgen('''
> > >>>  }
> > >>>diff --git a/scripts/qapi.py b/scripts/qapi.py
> > >>>index e062336..87b9ee6 100644
> > >>>--- a/scripts/qapi.py
> > >>>+++ b/scripts/qapi.py
> > >>>@@ -163,6 +163,8 @@ def c_type(name):
> > >>>          return 'bool'
> > >>>      elif name == 'number':
> > >>>          return 'double'
> > >>>+    elif name == '**':
> > >>>+        return 'KeyValuesList *'
> > >>>      elif type(name) == list:
> > >>>          return '%s *' % c_list_type(name[0])
> > >>>      elif is_enum(name):
> > >>
> > >
> > 

Re: [Qemu-devel] [PATCH 12/13] qapi: support for keyworded variable-length argument list

[Paolo Bonzini]

Il 29/03/2012 22:01, Anthony Liguori ha scritto:
>> Then, we open-code the marshaller to process the QDict, rather than
>> embedding
>> it in the script or passing it through to qmp_device_add().
> 
> You could also just do gen=False...
> 
> But I don't think open coding the marshaller is the right thing here. 
> You have to convert to strings and reparse anyway.

device_add expects strings, you don't have to convert no?  There is a
single parse point.

(Instead, a qom_add would accept the right types, but then it would also
use a more QAPI-friendly syntax with lists insteads of varargs).

Regarding device_add ? and device_add foo,? I would implement it as
separate QMP commands hooking into QOM, such as qom_list_types (taking
the superclass as an optional argument) and qom_properties.  But the
latter first needs static properties to move up from devices to objects.
 I'll take a look at that.

Paolo

Re: [Qemu-devel] [PATCH 12/13] qapi: support for keyworded variable-length argument list

[Luiz Capitulino]

On Fri, 30 Mar 2012 09:49:20 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Regarding device_add ? and device_add foo,? I would implement it as
> separate QMP commands hooking into QOM, such as qom_list_types (taking
> the superclass as an optional argument) and qom_properties.  But the
> latter first needs static properties to move up from devices to objects.

Yes, after I suggested query-devices I remembered about qom_list_types.

I think it's ok for hmp.c to use it even if it's not declared stable yet,
right?

Anthony, you said you did it in your tree differently, did you? I think
using qom_list_types is the way to go here...