[Qemu-devel] [PATCH qmp-next v4 00/16]: qapi: convert netdev_add & netdev_del

[Qemu-devel] [PATCH qmp-next v4 00/16]: qapi: convert netdev_add & netdev_del

[Luiz Capitulino]

v4

- Fix vde build [Laszlo]
- Rebase on top of qmp-next (which contains latest master) [Me]

 blockdev.c           |    2 +-
 hmp-commands.hx      |    6 +-
 hmp.c                |   30 +++++++++
 hmp.h                |    2 +
 hw/pci-hotplug.c     |    8 ++-
 hw/qdev-monitor.c    |    7 +-
 hw/usb/dev-network.c |    7 +-
 hw/usb/dev-storage.c |    2 +-
 hw/watchdog.c        |    2 +-
 net.c                |  104 +++++++++++++++++++----------
 net.h                |    6 +-
 net/dump.c           |    2 +-
 net/dump.h           |    3 +-
 net/slirp.c          |    5 +-
 net/slirp.h          |    5 +-
 net/socket.c         |    8 +--
 net/socket.h         |    3 +-
 net/tap-win32.c      |    2 +-
 net/tap.c            |    9 ++-
 net/tap.h            |    5 +-
 net/vde.c            |    2 +-
 net/vde.h            |    2 +-
 qapi-schema.json     |   42 ++++++++++++
 qemu-char.c          |    8 ++-
 qemu-config.c        |   43 +++++++++---
 qemu-config.h        |    3 +
 qemu-option.c        |  176 ++++++++++++++++++++++++++++++++++----------------
 qemu-option.h        |   11 +++-
 qemu-sockets.c       |    8 +--
 qerror.c             |    4 ++
 qerror.h             |    3 +
 qmp-commands.hx      |   10 +--
 vl.c                 |   22 ++++---
 33 files changed, 382 insertions(+), 170 deletions(-)

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

[Luiz Capitulino]

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

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 keeps 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 67895b2..622ecba 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -569,7 +569,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 ae22fb1..a96c0b9 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -584,7 +584,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 fe1126f..0bd903f 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2584,10 +2584,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 46c7619..2ae715d 100644
--- a/qemu-sockets.c
+++ b/qemu-sockets.c
@@ -461,7 +461,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, errp);
         if (sock != -1 && ostr) {
@@ -490,7 +490,7 @@ int inet_connect(const char *str, bool block, Error **errp)
     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) {
         if (block) {
             qemu_opt_set(opts, "block", "on");
@@ -589,7 +589,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) {
@@ -617,7 +617,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 23ab3a3..1485426 100644
--- a/vl.c
+++ b/vl.c
@@ -1786,7 +1786,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;
@@ -1921,7 +1921,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);
@@ -2035,14 +2035,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);
@@ -2065,7 +2065,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);
@@ -2813,7 +2813,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"));
@@ -2845,7 +2846,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"));
@@ -2857,14 +2859,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/16] 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/16] 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/16] 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/16] 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 |   36 +++++++++++++++++-------------------
 1 file changed, 17 insertions(+), 19 deletions(-)

diff --git a/qemu-option.c b/qemu-option.c
index 61354af..d79acbd 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++) {
@@ -661,10 +651,14 @@ 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)) {
+        qerror_report_err(local_err);
+        error_free(local_err);
         qemu_opt_del(opt);
         return -1;
     }
+
     return 0;
 }
 
@@ -1053,6 +1047,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);
 
@@ -1071,7 +1066,10 @@ int qemu_opts_validate(QemuOpts *opts, const QemuOptDesc *desc)
 
         opt->desc = &desc[i];
 
-        if (qemu_opt_parse(opt) < 0) {
+        qemu_opt_parse(opt, &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 06/16] qemu-option: qemu_opts_validate(): use error_set()

[Luiz Capitulino]

net_client_init() propagates the error up by calling qerror_report_err(),
because its users expect QError semantics.

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

diff --git a/net.c b/net.c
index 1922d8a..f5d9cc7 100644
--- a/net.c
+++ b/net.c
@@ -1136,10 +1136,14 @@ int net_client_init(Monitor *mon, QemuOpts *opts, int is_netdev)
     for (i = 0; i < NET_CLIENT_TYPE_MAX; i++) {
         if (net_client_types[i].type != NULL &&
             !strcmp(net_client_types[i].type, type)) {
+            Error *local_err = NULL;
             VLANState *vlan = NULL;
             int ret;
 
-            if (qemu_opts_validate(opts, &net_client_types[i].desc[0]) == -1) {
+            qemu_opts_validate(opts, &net_client_types[i].desc[0], &local_err);
+            if (error_is_set(&local_err)) {
+                qerror_report_err(local_err);
+                error_free(local_err);
                 return -1;
             }
 
diff --git a/qemu-option.c b/qemu-option.c
index d79acbd..3a9a5f8 100644
--- a/qemu-option.c
+++ b/qemu-option.c
@@ -1044,7 +1044,7 @@ QDict *qemu_opts_to_qdict(QemuOpts *opts, QDict *qdict)
 /* Validate parsed opts against descriptions where no
  * descriptions were provided in the QemuOptsList.
  */
-int qemu_opts_validate(QemuOpts *opts, const QemuOptDesc *desc)
+void qemu_opts_validate(QemuOpts *opts, const QemuOptDesc *desc, Error **errp)
 {
     QemuOpt *opt;
     Error *local_err = NULL;
@@ -1060,21 +1060,18 @@ 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(errp, QERR_INVALID_PARAMETER, opt->name);
+            return;
         }
 
         opt->desc = &desc[i];
 
         qemu_opt_parse(opt, &local_err);
         if (error_is_set(&local_err)) {
-            qerror_report_err(local_err);
-            error_free(local_err);
-            return -1;
+            error_propagate(errp, local_err);
+            return;
         }
     }
-
-    return 0;
 }
 
 int qemu_opts_foreach(QemuOptsList *list, qemu_opts_loopfunc func, void *opaque,
diff --git a/qemu-option.h b/qemu-option.h
index 4d5b3d3..e9fbbb5 100644
--- a/qemu-option.h
+++ b/qemu-option.h
@@ -125,7 +125,7 @@ int qemu_opts_set(QemuOptsList *list, const char *id,
                   const char *name, const char *value);
 const char *qemu_opts_id(QemuOpts *opts);
 void qemu_opts_del(QemuOpts *opts);
-int qemu_opts_validate(QemuOpts *opts, const QemuOptDesc *desc);
+void qemu_opts_validate(QemuOpts *opts, const QemuOptDesc *desc, Error **errp);
 int qemu_opts_do_parse(QemuOpts *opts, const char *params, const char *firstname);
 QemuOpts *qemu_opts_parse(QemuOptsList *list, const char *params, int permit_abbrev);
 void qemu_opts_set_defaults(QemuOptsList *list, const char *params,
-- 
1.7.9.2.384.g4a92a

[Qemu-devel] [PATCH 07/16] 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 |   31 ++++++++++++++++++++-----------
 1 file changed, 20 insertions(+), 11 deletions(-)

diff --git a/qemu-option.c b/qemu-option.c
index 3a9a5f8..c54bb21 100644
--- a/qemu-option.c
+++ b/qemu-option.c
@@ -615,8 +615,8 @@ 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;
@@ -632,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(errp, QERR_INVALID_PARAMETER, name);
+            return;
         }
     }
 
@@ -653,18 +653,23 @@ static int opt_set(QemuOpts *opts, const char *name, const char *value,
     }
     qemu_opt_parse(opt, &local_err);
     if (error_is_set(&local_err)) {
-        qerror_report_err(local_err);
-        error_free(local_err);
+        error_propagate(errp, local_err);
         qemu_opt_del(opt);
-        return -1;
     }
-
-    return 0;
 }
 
 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)
@@ -850,6 +855,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, '=');
@@ -881,7 +887,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 08/16] 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 c54bb21..afee3fb 100644
--- a/qemu-option.c
+++ b/qemu-option.c
@@ -672,6 +672,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 e9fbbb5..c0e022b 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/16] qemu-option: qemu_opts_from_qdict(): use error_set()

[Luiz Capitulino]

do_device_add() and do_netdev_add() call qerror_report_err() to maintain
their QError semantics.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 hw/qdev-monitor.c |    7 +++++--
 net.c             |    5 ++++-
 qemu-option.c     |   31 ++++++++++++++++++++++++-------
 qemu-option.h     |    3 ++-
 4 files changed, 35 insertions(+), 11 deletions(-)

diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
index eed781d..b01ef06 100644
--- a/hw/qdev-monitor.c
+++ b/hw/qdev-monitor.c
@@ -554,10 +554,13 @@ void do_info_qdm(Monitor *mon)
 
 int do_device_add(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
+    Error *local_err = NULL;
     QemuOpts *opts;
 
-    opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict);
-    if (!opts) {
+    opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict, &local_err);
+    if (error_is_set(&local_err)) {
+        qerror_report_err(local_err);
+        error_free(local_err);
         return -1;
     }
     if (!monitor_cur_is_qmp() && qdev_device_help(opts)) {
diff --git a/net.c b/net.c
index f5d9cc7..246209f 100644
--- a/net.c
+++ b/net.c
@@ -1237,11 +1237,14 @@ void net_host_device_remove(Monitor *mon, const QDict *qdict)
 
 int do_netdev_add(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
+    Error *local_err = NULL;
     QemuOpts *opts;
     int res;
 
-    opts = qemu_opts_from_qdict(qemu_find_opts("netdev"), qdict);
+    opts = qemu_opts_from_qdict(qemu_find_opts("netdev"), qdict, &local_err);
     if (!opts) {
+        qerror_report_err(local_err);
+        error_free(local_err);
         return -1;
     }
 
diff --git a/qemu-option.c b/qemu-option.c
index afee3fb..a26c40a 100644
--- a/qemu-option.c
+++ b/qemu-option.c
@@ -971,13 +971,19 @@ void qemu_opts_set_defaults(QemuOptsList *list, const char *params,
     assert(opts);
 }
 
+typedef struct OptsFromQDictState {
+    QemuOpts *opts;
+    Error **errp;
+} OptsFromQDictState;
+
 static void qemu_opts_from_qdict_1(const char *key, QObject *obj, void *opaque)
 {
+    OptsFromQDictState *state = opaque;
     char buf[32];
     const char *value;
     int n;
 
-    if (!strcmp(key, "id")) {
+    if (!strcmp(key, "id") || error_is_set(state->errp)) {
         return;
     }
 
@@ -1005,7 +1011,8 @@ static void qemu_opts_from_qdict_1(const char *key, QObject *obj, void *opaque)
     default:
         return;
     }
-    qemu_opt_set(opaque, key, value);
+
+    qemu_opt_set_err(state->opts, key, value, state->errp);
 }
 
 /*
@@ -1014,21 +1021,31 @@ static void qemu_opts_from_qdict_1(const char *key, QObject *obj, void *opaque)
  * Only QStrings, QInts, QFloats and QBools are copied.  Entries with
  * other types are silently ignored.
  */
-QemuOpts *qemu_opts_from_qdict(QemuOptsList *list, const QDict *qdict)
+QemuOpts *qemu_opts_from_qdict(QemuOptsList *list, const QDict *qdict,
+                               Error **errp)
 {
-    QemuOpts *opts;
+    OptsFromQDictState state;
     Error *local_err = NULL;
+    QemuOpts *opts;
 
     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);
+        error_propagate(errp, local_err);
         return NULL;
     }
 
     assert(opts != NULL);
-    qdict_iter(qdict, qemu_opts_from_qdict_1, opts);
+
+    state.errp = &local_err;
+    state.opts = opts;
+    qdict_iter(qdict, qemu_opts_from_qdict_1, &state);
+    if (error_is_set(&local_err)) {
+        error_propagate(errp, local_err);
+        qemu_opts_del(opts);
+        return NULL;
+    }
+
     return opts;
 }
 
diff --git a/qemu-option.h b/qemu-option.h
index c0e022b..951dec3 100644
--- a/qemu-option.h
+++ b/qemu-option.h
@@ -132,7 +132,8 @@ int qemu_opts_do_parse(QemuOpts *opts, const char *params, const char *firstname
 QemuOpts *qemu_opts_parse(QemuOptsList *list, const char *params, int permit_abbrev);
 void qemu_opts_set_defaults(QemuOptsList *list, const char *params,
                             int permit_abbrev);
-QemuOpts *qemu_opts_from_qdict(QemuOptsList *list, const QDict *qdict);
+QemuOpts *qemu_opts_from_qdict(QemuOptsList *list, const QDict *qdict,
+                               Error **errp);
 QDict *qemu_opts_to_qdict(QemuOpts *opts, QDict *qdict);
 
 typedef int (*qemu_opts_loopfunc)(QemuOpts *opts, void *opaque);
-- 
1.7.9.2.384.g4a92a

[Qemu-devel] [PATCH 10/16] 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 5092fe7..92c4eff 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 4cbba48..b4c8758 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 11/16] qemu-config: find_list(): use error_set()

[Luiz Capitulino]

Note that qemu_find_opts() and qemu_config_parse() need to call
error_report() to maintain their semantics on error.

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 12/16] 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 6d7365d..e9f2ef4 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 13/16] net: purge the monitor object from all init functions

[Luiz Capitulino]

The only backend that really uses it is the socket one, which calls
monitor_get_fd(). But it can use 'cur_mon' instead.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 hw/pci-hotplug.c     |    2 +-
 hw/usb/dev-network.c |    2 +-
 net.c                |   18 +++++++-----------
 net.h                |    2 +-
 net/dump.c           |    2 +-
 net/dump.h           |    3 +--
 net/slirp.c          |    5 +----
 net/slirp.h          |    5 +----
 net/socket.c         |    8 +++-----
 net/socket.h         |    3 +--
 net/tap-win32.c      |    2 +-
 net/tap.c            |    9 ++++-----
 net/tap.h            |    5 ++---
 net/vde.c            |    2 +-
 net/vde.h            |    2 +-
 15 files changed, 27 insertions(+), 43 deletions(-)

diff --git a/hw/pci-hotplug.c b/hw/pci-hotplug.c
index c55d8b9..785eb3d 100644
--- a/hw/pci-hotplug.c
+++ b/hw/pci-hotplug.c
@@ -60,7 +60,7 @@ static PCIDevice *qemu_pci_hot_add_nic(Monitor *mon,
 
     qemu_opt_set(opts, "type", "nic");
 
-    ret = net_client_init(mon, opts, 0);
+    ret = net_client_init(opts, 0);
     if (ret < 0)
         return NULL;
     if (nd_table[ret].devaddr) {
diff --git a/hw/usb/dev-network.c b/hw/usb/dev-network.c
index b238a09..4e26e64 100644
--- a/hw/usb/dev-network.c
+++ b/hw/usb/dev-network.c
@@ -1367,7 +1367,7 @@ static USBDevice *usb_net_init(USBBus *bus, const char *cmdline)
     qemu_opt_set(opts, "type", "nic");
     qemu_opt_set(opts, "model", "usb");
 
-    idx = net_client_init(NULL, opts, 0);
+    idx = net_client_init(opts, 0);
     if (idx == -1) {
         return NULL;
     }
diff --git a/net.c b/net.c
index 246209f..adb7e20 100644
--- a/net.c
+++ b/net.c
@@ -745,10 +745,7 @@ int net_handle_fd_param(Monitor *mon, const char *param)
     return fd;
 }
 
-static int net_init_nic(QemuOpts *opts,
-                        Monitor *mon,
-                        const char *name,
-                        VLANState *vlan)
+static int net_init_nic(QemuOpts *opts, const char *name, VLANState *vlan)
 {
     int idx;
     NICInfo *nd;
@@ -821,7 +818,6 @@ static int net_init_nic(QemuOpts *opts,
      }
 
 typedef int (*net_client_init_func)(QemuOpts *opts,
-                                    Monitor *mon,
                                     const char *name,
                                     VLANState *vlan);
 
@@ -1085,7 +1081,7 @@ static const struct {
 #endif /* CONFIG_NET_BRIDGE */
 };
 
-int net_client_init(Monitor *mon, QemuOpts *opts, int is_netdev)
+int net_client_init(QemuOpts *opts, int is_netdev)
 {
     const char *name;
     const char *type;
@@ -1156,7 +1152,7 @@ int net_client_init(Monitor *mon, QemuOpts *opts, int is_netdev)
 
             ret = 0;
             if (net_client_types[i].init) {
-                ret = net_client_types[i].init(opts, mon, name, vlan);
+                ret = net_client_types[i].init(opts, name, vlan);
                 if (ret < 0) {
                     /* TODO push error reporting into init() methods */
                     qerror_report(QERR_DEVICE_INIT_FAILED, type);
@@ -1213,7 +1209,7 @@ void net_host_device_add(Monitor *mon, const QDict *qdict)
 
     qemu_opt_set(opts, "type", device);
 
-    if (net_client_init(mon, opts, 0) < 0) {
+    if (net_client_init(opts, 0) < 0) {
         monitor_printf(mon, "adding host network device %s failed\n", device);
     }
 }
@@ -1248,7 +1244,7 @@ int do_netdev_add(Monitor *mon, const QDict *qdict, QObject **ret_data)
         return -1;
     }
 
-    res = net_client_init(mon, opts, 1);
+    res = net_client_init(opts, 1);
     if (res < 0) {
         qemu_opts_del(opts);
     }
@@ -1431,14 +1427,14 @@ void net_check_clients(void)
 
 static int net_init_client(QemuOpts *opts, void *dummy)
 {
-    if (net_client_init(NULL, opts, 0) < 0)
+    if (net_client_init(opts, 0) < 0)
         return -1;
     return 0;
 }
 
 static int net_init_netdev(QemuOpts *opts, void *dummy)
 {
-    return net_client_init(NULL, opts, 1);
+    return net_client_init(opts, 1);
 }
 
 int net_init_clients(void)
diff --git a/net.h b/net.h
index 64993b4..9d1ed93 100644
--- a/net.h
+++ b/net.h
@@ -163,7 +163,7 @@ struct HCIInfo *qemu_next_hci(void);
 extern const char *legacy_tftp_prefix;
 extern const char *legacy_bootp_filename;
 
-int net_client_init(Monitor *mon, QemuOpts *opts, int is_netdev);
+int net_client_init(QemuOpts *opts, int is_netdev);
 int net_client_parse(QemuOptsList *opts_list, const char *str);
 int net_init_clients(void);
 void net_check_clients(void);
diff --git a/net/dump.c b/net/dump.c
index 4b48d48..f835c51 100644
--- a/net/dump.c
+++ b/net/dump.c
@@ -144,7 +144,7 @@ static int net_dump_init(VLANState *vlan, const char *device,
     return 0;
 }
 
-int net_init_dump(QemuOpts *opts, Monitor *mon, const char *name, VLANState *vlan)
+int net_init_dump(QemuOpts *opts, const char *name, VLANState *vlan)
 {
     int len;
     const char *file;
diff --git a/net/dump.h b/net/dump.h
index fdc91ad..2b5d9ba 100644
--- a/net/dump.h
+++ b/net/dump.h
@@ -27,7 +27,6 @@
 #include "net.h"
 #include "qemu-common.h"
 
-int net_init_dump(QemuOpts *opts, Monitor *mon,
-                  const char *name, VLANState *vlan);
+int net_init_dump(QemuOpts *opts, const char *name, VLANState *vlan);
 
 #endif /* QEMU_NET_DUMP_H */
diff --git a/net/slirp.c b/net/slirp.c
index 96f5032..37b6ccf 100644
--- a/net/slirp.c
+++ b/net/slirp.c
@@ -676,10 +676,7 @@ static int net_init_slirp_configs(const char *name, const char *value, void *opa
     return 0;
 }
 
-int net_init_slirp(QemuOpts *opts,
-                   Monitor *mon,
-                   const char *name,
-                   VLANState *vlan)
+int net_init_slirp(QemuOpts *opts, const char *name, VLANState *vlan)
 {
     struct slirp_config_str *config;
     const char *vhost;
diff --git a/net/slirp.h b/net/slirp.h
index c17de8e..53fe95d 100644
--- a/net/slirp.h
+++ b/net/slirp.h
@@ -30,10 +30,7 @@
 
 #ifdef CONFIG_SLIRP
 
-int net_init_slirp(QemuOpts *opts,
-                   Monitor *mon,
-                   const char *name,
-                   VLANState *vlan);
+int net_init_slirp(QemuOpts *opts, const char *name, VLANState *vlan);
 
 void net_slirp_hostfwd_add(Monitor *mon, const QDict *qdict);
 void net_slirp_hostfwd_remove(Monitor *mon, const QDict *qdict);
diff --git a/net/socket.c b/net/socket.c
index 0bcf229..fcd0a3c 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -26,6 +26,7 @@
 #include "config-host.h"
 
 #include "net.h"
+#include "monitor.h"
 #include "qemu-char.h"
 #include "qemu-common.h"
 #include "qemu-error.h"
@@ -585,10 +586,7 @@ static int net_socket_udp_init(VLANState *vlan,
     return 0;
 }
 
-int net_init_socket(QemuOpts *opts,
-                    Monitor *mon,
-                    const char *name,
-                    VLANState *vlan)
+int net_init_socket(QemuOpts *opts, const char *name, VLANState *vlan)
 {
     if (qemu_opt_get(opts, "fd")) {
         int fd;
@@ -601,7 +599,7 @@ int net_init_socket(QemuOpts *opts,
             return -1;
         }
 
-        fd = net_handle_fd_param(mon, qemu_opt_get(opts, "fd"));
+        fd = net_handle_fd_param(cur_mon, qemu_opt_get(opts, "fd"));
         if (fd == -1) {
             return -1;
         }
diff --git a/net/socket.h b/net/socket.h
index ea46f02..e1fe959 100644
--- a/net/socket.h
+++ b/net/socket.h
@@ -27,7 +27,6 @@
 #include "net.h"
 #include "qemu-common.h"
 
-int net_init_socket(QemuOpts *opts, Monitor *mon,
-                    const char *name, VLANState *vlan);
+int net_init_socket(QemuOpts *opts, const char *name, VLANState *vlan);
 
 #endif /* QEMU_NET_SOCKET_H */
diff --git a/net/tap-win32.c b/net/tap-win32.c
index 596132e..a801a55 100644
--- a/net/tap-win32.c
+++ b/net/tap-win32.c
@@ -699,7 +699,7 @@ static int tap_win32_init(VLANState *vlan, const char *model,
     return 0;
 }
 
-int net_init_tap(QemuOpts *opts, Monitor *mon, const char *name, VLANState *vlan)
+int net_init_tap(QemuOpts *opts, const char *name, VLANState *vlan)
 {
     const char *ifname;
 
diff --git a/net/tap.c b/net/tap.c
index f240028..5ac4ba3 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -512,8 +512,7 @@ static int net_bridge_run_helper(const char *helper, const char *bridge)
     return -1;
 }
 
-int net_init_bridge(QemuOpts *opts, Monitor *mon, const char *name,
-                    VLANState *vlan)
+int net_init_bridge(QemuOpts *opts, const char *name, VLANState *vlan)
 {
     TAPState *s;
     int fd, vnet_hdr;
@@ -583,7 +582,7 @@ static int net_tap_init(QemuOpts *opts, int *vnet_hdr)
     return fd;
 }
 
-int net_init_tap(QemuOpts *opts, Monitor *mon, const char *name, VLANState *vlan)
+int net_init_tap(QemuOpts *opts, const char *name, VLANState *vlan)
 {
     TAPState *s;
     int fd, vnet_hdr = 0;
@@ -600,7 +599,7 @@ int net_init_tap(QemuOpts *opts, Monitor *mon, const char *name, VLANState *vlan
             return -1;
         }
 
-        fd = net_handle_fd_param(mon, qemu_opt_get(opts, "fd"));
+        fd = net_handle_fd_param(cur_mon, qemu_opt_get(opts, "fd"));
         if (fd == -1) {
             return -1;
         }
@@ -687,7 +686,7 @@ int net_init_tap(QemuOpts *opts, Monitor *mon, const char *name, VLANState *vlan
         int vhostfd, r;
         bool force = qemu_opt_get_bool(opts, "vhostforce", false);
         if (qemu_opt_get(opts, "vhostfd")) {
-            r = net_handle_fd_param(mon, qemu_opt_get(opts, "vhostfd"));
+            r = net_handle_fd_param(cur_mon, qemu_opt_get(opts, "vhostfd"));
             if (r == -1) {
                 return -1;
             }
diff --git a/net/tap.h b/net/tap.h
index 56c591f..b2a9450 100644
--- a/net/tap.h
+++ b/net/tap.h
@@ -32,7 +32,7 @@
 #define DEFAULT_NETWORK_SCRIPT "/etc/qemu-ifup"
 #define DEFAULT_NETWORK_DOWN_SCRIPT "/etc/qemu-ifdown"
 
-int net_init_tap(QemuOpts *opts, Monitor *mon, const char *name, VLANState *vlan);
+int net_init_tap(QemuOpts *opts, const char *name, VLANState *vlan);
 
 int tap_open(char *ifname, int ifname_size, int *vnet_hdr, int vnet_hdr_required);
 
@@ -57,7 +57,6 @@ int tap_get_fd(VLANClientState *vc);
 struct vhost_net;
 struct vhost_net *tap_get_vhost_net(VLANClientState *vc);
 
-int net_init_bridge(QemuOpts *opts, Monitor *mon, const char *name,
-                    VLANState *vlan);
+int net_init_bridge(QemuOpts *opts, const char *name, VLANState *vlan);
 
 #endif /* QEMU_NET_TAP_H */
diff --git a/net/vde.c b/net/vde.c
index ac48ab2..6b9d452 100644
--- a/net/vde.c
+++ b/net/vde.c
@@ -110,7 +110,7 @@ static int net_vde_init(VLANState *vlan, const char *model,
     return 0;
 }
 
-int net_init_vde(QemuOpts *opts, Monitor *mon, const char *name, VLANState *vlan)
+int net_init_vde(QemuOpts *opts, const char *name, VLANState *vlan)
 {
     const char *sock;
     const char *group;
diff --git a/net/vde.h b/net/vde.h
index 3e6ca3e..732e575 100644
--- a/net/vde.h
+++ b/net/vde.h
@@ -29,7 +29,7 @@
 
 #ifdef CONFIG_VDE
 
-int net_init_vde(QemuOpts *opts, Monitor *mon, const char *name, VLANState *vlan);
+int net_init_vde(QemuOpts *opts, const char *name, VLANState *vlan);
 
 #endif /* CONFIG_VDE */
 
-- 
1.7.9.2.384.g4a92a

[Qemu-devel] [PATCH 14/16] net: net_client_init(): use error_set()

[Luiz Capitulino]

Callers are changed to use qerror_report_err() to keep their QError
semantics.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 hw/pci-hotplug.c     |    8 ++++++--
 hw/usb/dev-network.c |    7 +++++--
 net.c                |   53 +++++++++++++++++++++++++++++++++++---------------
 net.h                |    2 +-
 4 files changed, 49 insertions(+), 21 deletions(-)

diff --git a/hw/pci-hotplug.c b/hw/pci-hotplug.c
index 785eb3d..61257f4 100644
--- a/hw/pci-hotplug.c
+++ b/hw/pci-hotplug.c
@@ -39,6 +39,7 @@ static PCIDevice *qemu_pci_hot_add_nic(Monitor *mon,
                                        const char *devaddr,
                                        const char *opts_str)
 {
+    Error *local_err = NULL;
     QemuOpts *opts;
     PCIBus *bus;
     int ret, devfn;
@@ -60,9 +61,12 @@ static PCIDevice *qemu_pci_hot_add_nic(Monitor *mon,
 
     qemu_opt_set(opts, "type", "nic");
 
-    ret = net_client_init(opts, 0);
-    if (ret < 0)
+    ret = net_client_init(opts, 0, &local_err);
+    if (error_is_set(&local_err)) {
+        qerror_report_err(local_err);
+        error_free(local_err);
         return NULL;
+    }
     if (nd_table[ret].devaddr) {
         monitor_printf(mon, "Parameter addr not supported\n");
         return NULL;
diff --git a/hw/usb/dev-network.c b/hw/usb/dev-network.c
index 4e26e64..5d2f098 100644
--- a/hw/usb/dev-network.c
+++ b/hw/usb/dev-network.c
@@ -1356,6 +1356,7 @@ static int usb_net_initfn(USBDevice *dev)
 
 static USBDevice *usb_net_init(USBBus *bus, const char *cmdline)
 {
+    Error *local_err = NULL;
     USBDevice *dev;
     QemuOpts *opts;
     int idx;
@@ -1367,8 +1368,10 @@ static USBDevice *usb_net_init(USBBus *bus, const char *cmdline)
     qemu_opt_set(opts, "type", "nic");
     qemu_opt_set(opts, "model", "usb");
 
-    idx = net_client_init(opts, 0);
-    if (idx == -1) {
+    idx = net_client_init(opts, 0, &local_err);
+    if (error_is_set(&local_err)) {
+        qerror_report_err(local_err);
+        error_free(local_err);
         return NULL;
     }
 
diff --git a/net.c b/net.c
index adb7e20..7e9d0c5 100644
--- a/net.c
+++ b/net.c
@@ -1081,7 +1081,7 @@ static const struct {
 #endif /* CONFIG_NET_BRIDGE */
 };
 
-int net_client_init(QemuOpts *opts, int is_netdev)
+int net_client_init(QemuOpts *opts, int is_netdev, Error **errp)
 {
     const char *name;
     const char *type;
@@ -1089,7 +1089,7 @@ int net_client_init(QemuOpts *opts, int is_netdev)
 
     type = qemu_opt_get(opts, "type");
     if (!type) {
-        qerror_report(QERR_MISSING_PARAMETER, "type");
+        error_set(errp, QERR_MISSING_PARAMETER, "type");
         return -1;
     }
 
@@ -1105,21 +1105,21 @@ int net_client_init(QemuOpts *opts, int is_netdev)
             strcmp(type, "vde") != 0 &&
 #endif
             strcmp(type, "socket") != 0) {
-            qerror_report(QERR_INVALID_PARAMETER_VALUE, "type",
-                          "a netdev backend type");
+            error_set(errp, QERR_INVALID_PARAMETER_VALUE, "type",
+                      "a netdev backend type");
             return -1;
         }
 
         if (qemu_opt_get(opts, "vlan")) {
-            qerror_report(QERR_INVALID_PARAMETER, "vlan");
+            error_set(errp, QERR_INVALID_PARAMETER, "vlan");
             return -1;
         }
         if (qemu_opt_get(opts, "name")) {
-            qerror_report(QERR_INVALID_PARAMETER, "name");
+            error_set(errp, QERR_INVALID_PARAMETER, "name");
             return -1;
         }
         if (!qemu_opts_id(opts)) {
-            qerror_report(QERR_MISSING_PARAMETER, "id");
+            error_set(errp, QERR_MISSING_PARAMETER, "id");
             return -1;
         }
     }
@@ -1138,8 +1138,7 @@ int net_client_init(QemuOpts *opts, int is_netdev)
 
             qemu_opts_validate(opts, &net_client_types[i].desc[0], &local_err);
             if (error_is_set(&local_err)) {
-                qerror_report_err(local_err);
-                error_free(local_err);
+                error_propagate(errp, local_err);
                 return -1;
             }
 
@@ -1155,7 +1154,7 @@ int net_client_init(QemuOpts *opts, int is_netdev)
                 ret = net_client_types[i].init(opts, name, vlan);
                 if (ret < 0) {
                     /* TODO push error reporting into init() methods */
-                    qerror_report(QERR_DEVICE_INIT_FAILED, type);
+                    error_set(errp, QERR_DEVICE_INIT_FAILED, type);
                     return -1;
                 }
             }
@@ -1163,8 +1162,8 @@ int net_client_init(QemuOpts *opts, int is_netdev)
         }
     }
 
-    qerror_report(QERR_INVALID_PARAMETER_VALUE, "type",
-                  "a network client type");
+    error_set(errp, QERR_INVALID_PARAMETER_VALUE, "type",
+              "a network client type");
     return -1;
 }
 
@@ -1195,6 +1194,7 @@ void net_host_device_add(Monitor *mon, const QDict *qdict)
 {
     const char *device = qdict_get_str(qdict, "device");
     const char *opts_str = qdict_get_try_str(qdict, "opts");
+    Error *local_err = NULL;
     QemuOpts *opts;
 
     if (!net_host_check_device(device)) {
@@ -1209,7 +1209,10 @@ void net_host_device_add(Monitor *mon, const QDict *qdict)
 
     qemu_opt_set(opts, "type", device);
 
-    if (net_client_init(opts, 0) < 0) {
+    net_client_init(opts, 0, &local_err);
+    if (error_is_set(&local_err)) {
+        qerror_report_err(local_err);
+        error_free(local_err);
         monitor_printf(mon, "adding host network device %s failed\n", device);
     }
 }
@@ -1244,8 +1247,10 @@ int do_netdev_add(Monitor *mon, const QDict *qdict, QObject **ret_data)
         return -1;
     }
 
-    res = net_client_init(opts, 1);
+    res = net_client_init(opts, 1, &local_err);
     if (res < 0) {
+        qerror_report_err(local_err);
+        error_free(local_err);
         qemu_opts_del(opts);
     }
 
@@ -1427,14 +1432,30 @@ void net_check_clients(void)
 
 static int net_init_client(QemuOpts *opts, void *dummy)
 {
-    if (net_client_init(opts, 0) < 0)
+    Error *local_err = NULL;
+
+    net_client_init(opts, 0, &local_err);
+    if (error_is_set(&local_err)) {
+        qerror_report_err(local_err);
+        error_free(local_err);
         return -1;
+    }
+
     return 0;
 }
 
 static int net_init_netdev(QemuOpts *opts, void *dummy)
 {
-    return net_client_init(opts, 1);
+    Error *local_err = NULL;
+
+    net_client_init(opts, 1, &local_err);
+    if (error_is_set(&local_err)) {
+        qerror_report_err(local_err);
+        error_free(local_err);
+        return -1;
+    }
+
+    return 0;
 }
 
 int net_init_clients(void)
diff --git a/net.h b/net.h
index 9d1ed93..7ee97e9 100644
--- a/net.h
+++ b/net.h
@@ -163,7 +163,7 @@ struct HCIInfo *qemu_next_hci(void);
 extern const char *legacy_tftp_prefix;
 extern const char *legacy_bootp_filename;
 
-int net_client_init(QemuOpts *opts, int is_netdev);
+int net_client_init(QemuOpts *opts, int is_netdev, Error **errp);
 int net_client_parse(QemuOptsList *opts_list, const char *str);
 int net_init_clients(void);
 void net_check_clients(void);
-- 
1.7.9.2.384.g4a92a

[Qemu-devel] [PATCH 15/16] qapi: convert netdev_add

[Luiz Capitulino]

This is not a full QAPI conversion, but an intermediate step.

In essence, do_netdev_add() is split into three functions:

 1. netdev_add(): performs the actual work. This function is fully
    converted to Error (thus, it's "qapi-friendly")

 2. qmp_netdev_add(): the QMP front-end for netdev_add(). This is
    coded by hand and not auto-generated (gen=no in the schema). The
    reason for this it's a lot easier and simpler to with QemuOpts
    this way

 3. hmp_netdev_add(): HMP front-end.

This design was suggested by Paolo Bonzini.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 hmp-commands.hx  |    3 +--
 hmp.c            |   21 +++++++++++++++++++++
 hmp.h            |    1 +
 net.c            |   41 +++++++++++++++++++++++++++--------------
 net.h            |    3 ++-
 qapi-schema.json |   28 ++++++++++++++++++++++++++++
 qmp-commands.hx  |    5 +----
 7 files changed, 81 insertions(+), 21 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 81723c8..d0ce6a5 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1037,8 +1037,7 @@ ETEXI
         .args_type  = "netdev:O",
         .params     = "[user|tap|socket],id=str[,prop=value][,...]",
         .help       = "add host network device",
-        .user_print = monitor_user_noop,
-        .mhandler.cmd_new = do_netdev_add,
+        .mhandler.cmd = hmp_netdev_add,
     },
 
 STEXI
diff --git a/hmp.c b/hmp.c
index 42ced2a..7a4e25f 100644
--- a/hmp.c
+++ b/hmp.c
@@ -14,6 +14,8 @@
  */
 
 #include "hmp.h"
+#include "net.h"
+#include "qemu-option.h"
 #include "qemu-timer.h"
 #include "qmp-commands.h"
 
@@ -969,3 +971,22 @@ void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict)
                           &errp);
     hmp_handle_error(mon, &errp);
 }
+
+void hmp_netdev_add(Monitor *mon, const QDict *qdict)
+{
+    Error *err = NULL;
+    QemuOpts *opts;
+
+    opts = qemu_opts_from_qdict(qemu_find_opts("netdev"), qdict, &err);
+    if (error_is_set(&err)) {
+        goto out;
+    }
+
+    netdev_add(opts, &err);
+    if (error_is_set(&err)) {
+        qemu_opts_del(opts);
+    }
+
+out:
+    hmp_handle_error(mon, &err);
+}
diff --git a/hmp.h b/hmp.h
index 5cf3241..017df87 100644
--- a/hmp.h
+++ b/hmp.h
@@ -62,5 +62,6 @@ void hmp_block_job_cancel(Monitor *mon, const QDict *qdict);
 void hmp_migrate(Monitor *mon, const QDict *qdict);
 void hmp_device_del(Monitor *mon, const QDict *qdict);
 void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict);
+void hmp_netdev_add(Monitor *mon, const QDict *qdict);
 
 #endif
diff --git a/net.c b/net.c
index 7e9d0c5..5f0c53c 100644
--- a/net.c
+++ b/net.c
@@ -1234,27 +1234,39 @@ void net_host_device_remove(Monitor *mon, const QDict *qdict)
     qemu_del_vlan_client(vc);
 }
 
-int do_netdev_add(Monitor *mon, const QDict *qdict, QObject **ret_data)
+void netdev_add(QemuOpts *opts, Error **errp)
+{
+    net_client_init(opts, 1, errp);
+}
+
+int qmp_netdev_add(Monitor *mon, const QDict *qdict, QObject **ret)
 {
     Error *local_err = NULL;
+    QemuOptsList *opts_list;
     QemuOpts *opts;
-    int res;
 
-    opts = qemu_opts_from_qdict(qemu_find_opts("netdev"), qdict, &local_err);
-    if (!opts) {
-        qerror_report_err(local_err);
-        error_free(local_err);
-        return -1;
+    opts_list = qemu_find_opts_err("netdev", &local_err);
+    if (error_is_set(&local_err)) {
+        goto exit_err;
     }
 
-    res = net_client_init(opts, 1, &local_err);
-    if (res < 0) {
-        qerror_report_err(local_err);
-        error_free(local_err);
+    opts = qemu_opts_from_qdict(opts_list, qdict, &local_err);
+    if (error_is_set(&local_err)) {
+        goto exit_err;
+    }
+
+    netdev_add(opts, &local_err);
+    if (error_is_set(&local_err)) {
         qemu_opts_del(opts);
+        goto exit_err;
     }
 
-    return res;
+    return 0;
+
+exit_err:
+    qerror_report_err(local_err);
+    error_free(local_err);
+    return -1;
 }
 
 int do_netdev_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
@@ -1447,15 +1459,16 @@ static int net_init_client(QemuOpts *opts, void *dummy)
 static int net_init_netdev(QemuOpts *opts, void *dummy)
 {
     Error *local_err = NULL;
+    int ret;
 
-    net_client_init(opts, 1, &local_err);
+    ret = net_client_init(opts, 1, &local_err);
     if (error_is_set(&local_err)) {
         qerror_report_err(local_err);
         error_free(local_err);
         return -1;
     }
 
-    return 0;
+    return ret;
 }
 
 int net_init_clients(void)
diff --git a/net.h b/net.h
index 7ee97e9..1eb9280 100644
--- a/net.h
+++ b/net.h
@@ -170,7 +170,8 @@ void net_check_clients(void);
 void net_cleanup(void);
 void net_host_device_add(Monitor *mon, const QDict *qdict);
 void net_host_device_remove(Monitor *mon, const QDict *qdict);
-int do_netdev_add(Monitor *mon, const QDict *qdict, QObject **ret_data);
+void netdev_add(QemuOpts *opts, Error **errp);
+int qmp_netdev_add(Monitor *mon, const QDict *qdict, QObject **ret);
 int do_netdev_del(Monitor *mon, const QDict *qdict, QObject **ret_data);
 
 #define DEFAULT_NETWORK_SCRIPT "/etc/qemu-ifup"
diff --git a/qapi-schema.json b/qapi-schema.json
index dfa0e1f..69fcd8e 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1798,3 +1798,31 @@
 { 'command': 'dump-guest-memory',
   'data': { 'paging': 'bool', 'protocol': 'str', '*begin': 'int',
             '*length': 'int' } }
+##
+# @netdev_add:
+#
+# Add a network backend.
+#
+# @type: the type of network backend.  Current valid values are 'user', 'tap',
+#        'vde', 'socket', 'dump' and 'bridge'
+#
+# @id: the name of the new network backend
+#
+# @props: #optional a list of properties to be passed to the backend in
+#         the format 'name=value', like 'ifname=tap0,script=no'
+#
+# Notes: The semantics of @props is not well defined.  Future commands will be
+#        introduced that provide stronger typing for backend creation.
+#
+# Since: 0.14.0
+#
+# Returns: Nothing on success
+#          If @type is not a valid network backend, DeviceNotFound
+#          If @id is not a valid identifier, InvalidParameterValue
+#          if @id already exists, DuplicateId
+#          If @props contains an invalid parameter for this backend,
+#            InvalidParameter
+##
+{ 'command': 'netdev_add',
+  'data': {'type': 'str', 'id': 'str', '*props': '**'},
+  'gen': 'no' }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 2aa64ad..f6550cb 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -642,10 +642,7 @@ EQMP
     {
         .name       = "netdev_add",
         .args_type  = "netdev:O",
-        .params     = "[user|tap|socket],id=str[,prop=value][,...]",
-        .help       = "add host network device",
-        .user_print = monitor_user_noop,
-        .mhandler.cmd_new = do_netdev_add,
+        .mhandler.cmd_new = qmp_netdev_add,
     },
 
 SQMP
-- 
1.7.9.2.384.g4a92a

[Qemu-devel] [PATCH 16/16] qapi: convert netdev_del

[Luiz Capitulino]

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 hmp-commands.hx  |    3 +--
 hmp.c            |    9 +++++++++
 hmp.h            |    1 +
 net.c            |   11 +++++------
 net.h            |    1 -
 qapi-schema.json |   14 ++++++++++++++
 qmp-commands.hx  |    5 +----
 7 files changed, 31 insertions(+), 13 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index d0ce6a5..f5d9d91 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1051,8 +1051,7 @@ ETEXI
         .args_type  = "id:s",
         .params     = "id",
         .help       = "remove host network device",
-        .user_print = monitor_user_noop,
-        .mhandler.cmd_new = do_netdev_del,
+        .mhandler.cmd = hmp_netdev_del,
     },
 
 STEXI
diff --git a/hmp.c b/hmp.c
index 7a4e25f..2ce8cb9 100644
--- a/hmp.c
+++ b/hmp.c
@@ -990,3 +990,12 @@ void hmp_netdev_add(Monitor *mon, const QDict *qdict)
 out:
     hmp_handle_error(mon, &err);
 }
+
+void hmp_netdev_del(Monitor *mon, const QDict *qdict)
+{
+    const char *id = qdict_get_str(qdict, "id");
+    Error *err = NULL;
+
+    qmp_netdev_del(id, &err);
+    hmp_handle_error(mon, &err);
+}
diff --git a/hmp.h b/hmp.h
index 017df87..79d138d 100644
--- a/hmp.h
+++ b/hmp.h
@@ -63,5 +63,6 @@ void hmp_migrate(Monitor *mon, const QDict *qdict);
 void hmp_device_del(Monitor *mon, const QDict *qdict);
 void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict);
 void hmp_netdev_add(Monitor *mon, const QDict *qdict);
+void hmp_netdev_del(Monitor *mon, const QDict *qdict);
 
 #endif
diff --git a/net.c b/net.c
index 5f0c53c..4aa416c 100644
--- a/net.c
+++ b/net.c
@@ -1269,19 +1269,18 @@ exit_err:
     return -1;
 }
 
-int do_netdev_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
+void qmp_netdev_del(const char *id, Error **errp)
 {
-    const char *id = qdict_get_str(qdict, "id");
     VLANClientState *vc;
 
     vc = qemu_find_netdev(id);
     if (!vc) {
-        qerror_report(QERR_DEVICE_NOT_FOUND, id);
-        return -1;
+        error_set(errp, QERR_DEVICE_NOT_FOUND, id);
+        return;
     }
+
     qemu_del_vlan_client(vc);
-    qemu_opts_del(qemu_opts_find(qemu_find_opts("netdev"), id));
-    return 0;
+    qemu_opts_del(qemu_opts_find(qemu_find_opts_err("netdev", errp), id));
 }
 
 static void print_net_client(Monitor *mon, VLANClientState *vc)
diff --git a/net.h b/net.h
index 1eb9280..bdc2a06 100644
--- a/net.h
+++ b/net.h
@@ -172,7 +172,6 @@ void net_host_device_add(Monitor *mon, const QDict *qdict);
 void net_host_device_remove(Monitor *mon, const QDict *qdict);
 void netdev_add(QemuOpts *opts, Error **errp);
 int qmp_netdev_add(Monitor *mon, const QDict *qdict, QObject **ret);
-int do_netdev_del(Monitor *mon, const QDict *qdict, QObject **ret_data);
 
 #define DEFAULT_NETWORK_SCRIPT "/etc/qemu-ifup"
 #define DEFAULT_NETWORK_DOWN_SCRIPT "/etc/qemu-ifdown"
diff --git a/qapi-schema.json b/qapi-schema.json
index 69fcd8e..bb1f806 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1826,3 +1826,17 @@
 { 'command': 'netdev_add',
   'data': {'type': 'str', 'id': 'str', '*props': '**'},
   'gen': 'no' }
+
+##
+# @netdev_del:
+#
+# Remove a network backend.
+#
+# @id: the name of the network backend to remove
+#
+# Returns: Nothing on success
+#          If @id is not a valid network backend, DeviceNotFound
+#
+# Since: 0.14.0
+##
+{ 'command': 'netdev_del', 'data': {'id': 'str'} }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index f6550cb..57ea803 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -671,10 +671,7 @@ EQMP
     {
         .name       = "netdev_del",
         .args_type  = "id:s",
-        .params     = "id",
-        .help       = "remove host network device",
-        .user_print = monitor_user_noop,
-        .mhandler.cmd_new = do_netdev_del,
+        .mhandler.cmd_new = qmp_marshal_input_netdev_del,
     },
 
 SQMP
-- 
1.7.9.2.384.g4a92a

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

[Laszlo Ersek]

On 05/17/12 16:33, Luiz Capitulino wrote:
> 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;
>  }

I think this may have been simpler (= goto-less) like this:

    case OPT_FLAG:
        parse_option_bool(name, value, &flag, &local_err);
        if (!error_is_set(&local_err)) {
            list->value.n = flag;
        }
        break;

and in the end, before the "return 0", another check with error_is_set().

But it appears correct this way too, of course.

Laszlo

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

[Laszlo Ersek]

On 05/17/12 16:33, Luiz Capitulino wrote:

> @@ -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();
>      }

... and then the set_option_parameter() change would look like the
qemu_opt_parse() change (no if or goto). But it's fine.

Laszlo

Re: [Qemu-devel] [PATCH 06/16] qemu-option: qemu_opts_validate(): use error_set()

[Laszlo Ersek]

On 05/17/12 16:33, Luiz Capitulino wrote:

> @@ -1060,21 +1060,18 @@ 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(errp, QERR_INVALID_PARAMETER, opt->name);
> +            return;
>          }
>  
>          opt->desc = &desc[i];
>  
>          qemu_opt_parse(opt, &local_err);
>          if (error_is_set(&local_err)) {
> -            qerror_report_err(local_err);
> -            error_free(local_err);
> -            return -1;
> +            error_propagate(errp, local_err);
> +            return;
>          }
>      }
> -
> -    return 0;
>  }

(I *almost* suggested to drop "local_err" and pass "errp" directly to
qemu_opt_parse(), since the "if" body consists of nothing more than
error_propagate() now. But then I noticed the "return" that aborts the
QTAILQ_FOREACH(), and so we do have to rely on "local_err" -- "errp"
could be NULL, and we could not check *errp for loop-exit purposes. Good.)

Laszlo

Re: [Qemu-devel] [PATCH 09/16] qemu-option: qemu_opts_from_qdict(): use error_set()

[Laszlo Ersek]

comments in-line

On 05/17/12 16:33, Luiz Capitulino wrote:
> do_device_add() and do_netdev_add() call qerror_report_err() to maintain
> their QError semantics.
> 
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  hw/qdev-monitor.c |    7 +++++--
>  net.c             |    5 ++++-
>  qemu-option.c     |   31 ++++++++++++++++++++++++-------
>  qemu-option.h     |    3 ++-
>  4 files changed, 35 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
> index eed781d..b01ef06 100644
> --- a/hw/qdev-monitor.c
> +++ b/hw/qdev-monitor.c
> @@ -554,10 +554,13 @@ void do_info_qdm(Monitor *mon)
>  
>  int do_device_add(Monitor *mon, const QDict *qdict, QObject **ret_data)
>  {
> +    Error *local_err = NULL;
>      QemuOpts *opts;
>  
> -    opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict);
> -    if (!opts) {
> +    opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict, &local_err);
> +    if (error_is_set(&local_err)) {
> +        qerror_report_err(local_err);
> +        error_free(local_err);
>          return -1;
>      }
>      if (!monitor_cur_is_qmp() && qdev_device_help(opts)) {
> diff --git a/net.c b/net.c
> index f5d9cc7..246209f 100644
> --- a/net.c
> +++ b/net.c
> @@ -1237,11 +1237,14 @@ void net_host_device_remove(Monitor *mon, const QDict *qdict)
>  
>  int do_netdev_add(Monitor *mon, const QDict *qdict, QObject **ret_data)
>  {
> +    Error *local_err = NULL;
>      QemuOpts *opts;
>      int res;
>  
> -    opts = qemu_opts_from_qdict(qemu_find_opts("netdev"), qdict);
> +    opts = qemu_opts_from_qdict(qemu_find_opts("netdev"), qdict, &local_err);
>      if (!opts) {
> +        qerror_report_err(local_err);
> +        error_free(local_err);
>          return -1;
>      }

AFAICS the error condition is not the same in these two callers, but
looking at the new qemu_opts_from_qdict() below, they should be
equivalent. OK.


>  
> diff --git a/qemu-option.c b/qemu-option.c
> index afee3fb..a26c40a 100644
> --- a/qemu-option.c
> +++ b/qemu-option.c
> @@ -971,13 +971,19 @@ void qemu_opts_set_defaults(QemuOptsList *list, const char *params,
>      assert(opts);
>  }
>  
> +typedef struct OptsFromQDictState {
> +    QemuOpts *opts;
> +    Error **errp;
> +} OptsFromQDictState;
> +
>  static void qemu_opts_from_qdict_1(const char *key, QObject *obj, void *opaque)
>  {
> +    OptsFromQDictState *state = opaque;
>      char buf[32];
>      const char *value;
>      int n;
>  
> -    if (!strcmp(key, "id")) {
> +    if (!strcmp(key, "id") || error_is_set(state->errp)) {
>          return;
>      }

(Could have reversed the order, but I'm splitting hairs.)


>  
> @@ -1005,7 +1011,8 @@ static void qemu_opts_from_qdict_1(const char *key, QObject *obj, void *opaque)
>      default:
>          return;
>      }
> -    qemu_opt_set(opaque, key, value);
> +
> +    qemu_opt_set_err(state->opts, key, value, state->errp);
>  }

Aha. qemu_opt_set() did report errors, we just didn't care in
qemu_opts_from_qdict_1(), and certainly didn't try to pass those
outwards, for stopping the iteration (or at least making the rest of the
iteration a no-op) or otherwise. This changed now.


>  
>  /*
> @@ -1014,21 +1021,31 @@ static void qemu_opts_from_qdict_1(const char *key, QObject *obj, void *opaque)
>   * Only QStrings, QInts, QFloats and QBools are copied.  Entries with
>   * other types are silently ignored.
>   */
> -QemuOpts *qemu_opts_from_qdict(QemuOptsList *list, const QDict *qdict)
> +QemuOpts *qemu_opts_from_qdict(QemuOptsList *list, const QDict *qdict,
> +                               Error **errp)
>  {
> -    QemuOpts *opts;
> +    OptsFromQDictState state;
>      Error *local_err = NULL;
> +    QemuOpts *opts;
>  
>      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);
> +        error_propagate(errp, local_err);
>          return NULL;
>      }
>  
>      assert(opts != NULL);
> -    qdict_iter(qdict, qemu_opts_from_qdict_1, opts);
> +
> +    state.errp = &local_err;
> +    state.opts = opts;
> +    qdict_iter(qdict, qemu_opts_from_qdict_1, &state);
> +    if (error_is_set(&local_err)) {
> +        error_propagate(errp, local_err);
> +        qemu_opts_del(opts);
> +        return NULL;
> +    }
> +
>      return opts;
>  }

Yes, this is new error handling here (and another possibility to return
NULL to our callers). Seems correct. Our callers had to handle NULL before.

>  
> diff --git a/qemu-option.h b/qemu-option.h
> index c0e022b..951dec3 100644
> --- a/qemu-option.h
> +++ b/qemu-option.h
> @@ -132,7 +132,8 @@ int qemu_opts_do_parse(QemuOpts *opts, const char *params, const char *firstname
>  QemuOpts *qemu_opts_parse(QemuOptsList *list, const char *params, int permit_abbrev);
>  void qemu_opts_set_defaults(QemuOptsList *list, const char *params,
>                              int permit_abbrev);
> -QemuOpts *qemu_opts_from_qdict(QemuOptsList *list, const QDict *qdict);
> +QemuOpts *qemu_opts_from_qdict(QemuOptsList *list, const QDict *qdict,
> +                               Error **errp);
>  QDict *qemu_opts_to_qdict(QemuOpts *opts, QDict *qdict);
>  
>  typedef int (*qemu_opts_loopfunc)(QemuOpts *opts, void *opaque);

Good.

Laszlo

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

[Luiz Capitulino]

On Fri, 18 May 2012 16:03:30 +0200
Laszlo Ersek <lersek@redhat.com> wrote:

> On 05/17/12 16:33, Luiz Capitulino wrote:
> 
> > @@ -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();
> >      }
> 
> ... and then the set_option_parameter() change would look like the
> qemu_opt_parse() change (no if or goto). But it's fine.

I'll simplify if I respin.

Re: [Qemu-devel] [PATCH 09/16] qemu-option: qemu_opts_from_qdict(): use error_set()

[Luiz Capitulino]

On Fri, 18 May 2012 16:43:16 +0200
Laszlo Ersek <lersek@redhat.com> wrote:

> comments in-line
> 
> On 05/17/12 16:33, Luiz Capitulino wrote:
> > do_device_add() and do_netdev_add() call qerror_report_err() to maintain
> > their QError semantics.
> > 
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >  hw/qdev-monitor.c |    7 +++++--
> >  net.c             |    5 ++++-
> >  qemu-option.c     |   31 ++++++++++++++++++++++++-------
> >  qemu-option.h     |    3 ++-
> >  4 files changed, 35 insertions(+), 11 deletions(-)
> > 
> > diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
> > index eed781d..b01ef06 100644
> > --- a/hw/qdev-monitor.c
> > +++ b/hw/qdev-monitor.c
> > @@ -554,10 +554,13 @@ void do_info_qdm(Monitor *mon)
> >  
> >  int do_device_add(Monitor *mon, const QDict *qdict, QObject **ret_data)
> >  {
> > +    Error *local_err = NULL;
> >      QemuOpts *opts;
> >  
> > -    opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict);
> > -    if (!opts) {
> > +    opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict, &local_err);
> > +    if (error_is_set(&local_err)) {
> > +        qerror_report_err(local_err);
> > +        error_free(local_err);
> >          return -1;
> >      }
> >      if (!monitor_cur_is_qmp() && qdev_device_help(opts)) {
> > diff --git a/net.c b/net.c
> > index f5d9cc7..246209f 100644
> > --- a/net.c
> > +++ b/net.c
> > @@ -1237,11 +1237,14 @@ void net_host_device_remove(Monitor *mon, const QDict *qdict)
> >  
> >  int do_netdev_add(Monitor *mon, const QDict *qdict, QObject **ret_data)
> >  {
> > +    Error *local_err = NULL;
> >      QemuOpts *opts;
> >      int res;
> >  
> > -    opts = qemu_opts_from_qdict(qemu_find_opts("netdev"), qdict);
> > +    opts = qemu_opts_from_qdict(qemu_find_opts("netdev"), qdict, &local_err);
> >      if (!opts) {
> > +        qerror_report_err(local_err);
> > +        error_free(local_err);
> >          return -1;
> >      }
> 
> AFAICS the error condition is not the same in these two callers, but
> looking at the new qemu_opts_from_qdict() below, they should be
> equivalent. OK.
> 
> 
> >  
> > diff --git a/qemu-option.c b/qemu-option.c
> > index afee3fb..a26c40a 100644
> > --- a/qemu-option.c
> > +++ b/qemu-option.c
> > @@ -971,13 +971,19 @@ void qemu_opts_set_defaults(QemuOptsList *list, const char *params,
> >      assert(opts);
> >  }
> >  
> > +typedef struct OptsFromQDictState {
> > +    QemuOpts *opts;
> > +    Error **errp;
> > +} OptsFromQDictState;
> > +
> >  static void qemu_opts_from_qdict_1(const char *key, QObject *obj, void *opaque)
> >  {
> > +    OptsFromQDictState *state = opaque;
> >      char buf[32];
> >      const char *value;
> >      int n;
> >  
> > -    if (!strcmp(key, "id")) {
> > +    if (!strcmp(key, "id") || error_is_set(state->errp)) {
> >          return;
> >      }
> 
> (Could have reversed the order, but I'm splitting hairs.)
> 
> 
> >  
> > @@ -1005,7 +1011,8 @@ static void qemu_opts_from_qdict_1(const char *key, QObject *obj, void *opaque)
> >      default:
> >          return;
> >      }
> > -    qemu_opt_set(opaque, key, value);
> > +
> > +    qemu_opt_set_err(state->opts, key, value, state->errp);
> >  }
> 
> Aha. qemu_opt_set() did report errors, we just didn't care in
> qemu_opts_from_qdict_1(), and certainly didn't try to pass those
> outwards, for stopping the iteration (or at least making the rest of the
> iteration a no-op) or otherwise. This changed now.

Yes, because we can't override state->errp. So we either, report the first or
the last error.

> 
> 
> >  
> >  /*
> > @@ -1014,21 +1021,31 @@ static void qemu_opts_from_qdict_1(const char *key, QObject *obj, void *opaque)
> >   * Only QStrings, QInts, QFloats and QBools are copied.  Entries with
> >   * other types are silently ignored.
> >   */
> > -QemuOpts *qemu_opts_from_qdict(QemuOptsList *list, const QDict *qdict)
> > +QemuOpts *qemu_opts_from_qdict(QemuOptsList *list, const QDict *qdict,
> > +                               Error **errp)
> >  {
> > -    QemuOpts *opts;
> > +    OptsFromQDictState state;
> >      Error *local_err = NULL;
> > +    QemuOpts *opts;
> >  
> >      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);
> > +        error_propagate(errp, local_err);
> >          return NULL;
> >      }
> >  
> >      assert(opts != NULL);
> > -    qdict_iter(qdict, qemu_opts_from_qdict_1, opts);
> > +
> > +    state.errp = &local_err;
> > +    state.opts = opts;
> > +    qdict_iter(qdict, qemu_opts_from_qdict_1, &state);
> > +    if (error_is_set(&local_err)) {
> > +        error_propagate(errp, local_err);
> > +        qemu_opts_del(opts);
> > +        return NULL;
> > +    }
> > +
> >      return opts;
> >  }
> 
> Yes, this is new error handling here (and another possibility to return
> NULL to our callers). Seems correct. Our callers had to handle NULL before.
> 
> >  
> > diff --git a/qemu-option.h b/qemu-option.h
> > index c0e022b..951dec3 100644
> > --- a/qemu-option.h
> > +++ b/qemu-option.h
> > @@ -132,7 +132,8 @@ int qemu_opts_do_parse(QemuOpts *opts, const char *params, const char *firstname
> >  QemuOpts *qemu_opts_parse(QemuOptsList *list, const char *params, int permit_abbrev);
> >  void qemu_opts_set_defaults(QemuOptsList *list, const char *params,
> >                              int permit_abbrev);
> > -QemuOpts *qemu_opts_from_qdict(QemuOptsList *list, const QDict *qdict);
> > +QemuOpts *qemu_opts_from_qdict(QemuOptsList *list, const QDict *qdict,
> > +                               Error **errp);
> >  QDict *qemu_opts_to_qdict(QemuOpts *opts, QDict *qdict);
> >  
> >  typedef int (*qemu_opts_loopfunc)(QemuOpts *opts, void *opaque);
> 
> Good.
> 
> Laszlo
> 

Re: [Qemu-devel] [PATCH 14/16] net: net_client_init(): use error_set()

[Laszlo Ersek]

On 05/17/12 16:33, Luiz Capitulino wrote:
> @@ -1244,8 +1247,10 @@ int do_netdev_add(Monitor *mon, const QDict *qdict, QObject **ret_data)
>          return -1;
>      }
>  
> -    res = net_client_init(opts, 1);
> +    res = net_client_init(opts, 1, &local_err);
>      if (res < 0) {
> +        qerror_report_err(local_err);
> +        error_free(local_err);
>          qemu_opts_del(opts);
>      }
>  

I think this is the only net_client_init() caller hunk where we rely on
the retval instead of error_is_set(); but again, they seem to be
equivalent -- all branches in net_client_init() have been converted AFAICS.

(I can see that do_netdev_add() might set "local_err" higher up, and
then check "opts" again, instead of error_is_set(), but
qemu_opts_from_qdict() returns/sets those equivalently as well, so we
could use error_is_set() both times in do_netdev_add(), I think. Anyway,
it's OK.)

Laszlo

Re: [Qemu-devel] [PATCH 15/16] qapi: convert netdev_add

[Laszlo Ersek]

Comments in-line.

On 05/17/12 16:33, Luiz Capitulino wrote:
> This is not a full QAPI conversion, but an intermediate step.
> 
> In essence, do_netdev_add() is split into three functions:
> 
>  1. netdev_add(): performs the actual work. This function is fully
>     converted to Error (thus, it's "qapi-friendly")
> 
>  2. qmp_netdev_add(): the QMP front-end for netdev_add(). This is
>     coded by hand and not auto-generated (gen=no in the schema). The
>     reason for this it's a lot easier and simpler to with QemuOpts
>     this way
> 
>  3. hmp_netdev_add(): HMP front-end.
> 
> This design was suggested by Paolo Bonzini.
> 
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  hmp-commands.hx  |    3 +--
>  hmp.c            |   21 +++++++++++++++++++++
>  hmp.h            |    1 +
>  net.c            |   41 +++++++++++++++++++++++++++--------------
>  net.h            |    3 ++-
>  qapi-schema.json |   28 ++++++++++++++++++++++++++++
>  qmp-commands.hx  |    5 +----
>  7 files changed, 81 insertions(+), 21 deletions(-)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 81723c8..d0ce6a5 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1037,8 +1037,7 @@ ETEXI
>          .args_type  = "netdev:O",
>          .params     = "[user|tap|socket],id=str[,prop=value][,...]",
>          .help       = "add host network device",
> -        .user_print = monitor_user_noop,
> -        .mhandler.cmd_new = do_netdev_add,
> +        .mhandler.cmd = hmp_netdev_add,
>      },
>  
>  STEXI
> diff --git a/hmp.c b/hmp.c
> index 42ced2a..7a4e25f 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -14,6 +14,8 @@
>   */
>  
>  #include "hmp.h"
> +#include "net.h"
> +#include "qemu-option.h"
>  #include "qemu-timer.h"
>  #include "qmp-commands.h"
>  
> @@ -969,3 +971,22 @@ void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict)
>                            &errp);
>      hmp_handle_error(mon, &errp);
>  }
> +
> +void hmp_netdev_add(Monitor *mon, const QDict *qdict)
> +{
> +    Error *err = NULL;
> +    QemuOpts *opts;
> +
> +    opts = qemu_opts_from_qdict(qemu_find_opts("netdev"), qdict, &err);

I note we trust qemu_find_opts("netdev") to succeed, as we did in
do_netdev_add() before.


> +    if (error_is_set(&err)) {
> +        goto out;
> +    }
> +
> +    netdev_add(opts, &err);

OK this takes on the task of net_client_init().

> +    if (error_is_set(&err)) {
> +        qemu_opts_del(opts);
> +    }
> +
> +out:
> +    hmp_handle_error(mon, &err);
> +}

Basically the HMP function does the same as do_netdev_add() did before. OK.


> diff --git a/hmp.h b/hmp.h
> index 5cf3241..017df87 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -62,5 +62,6 @@ void hmp_block_job_cancel(Monitor *mon, const QDict *qdict);
>  void hmp_migrate(Monitor *mon, const QDict *qdict);
>  void hmp_device_del(Monitor *mon, const QDict *qdict);
>  void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict);
> +void hmp_netdev_add(Monitor *mon, const QDict *qdict);
>  
>  #endif
> diff --git a/net.c b/net.c
> index 7e9d0c5..5f0c53c 100644
> --- a/net.c
> +++ b/net.c
> @@ -1234,27 +1234,39 @@ void net_host_device_remove(Monitor *mon, const QDict *qdict)
>      qemu_del_vlan_client(vc);
>  }
>  
> -int do_netdev_add(Monitor *mon, const QDict *qdict, QObject **ret_data)
> +void netdev_add(QemuOpts *opts, Error **errp)
> +{
> +    net_client_init(opts, 1, errp);
> +}
> +
> +int qmp_netdev_add(Monitor *mon, const QDict *qdict, QObject **ret)
>  {
>      Error *local_err = NULL;
> +    QemuOptsList *opts_list;
>      QemuOpts *opts;
> -    int res;
>  
> -    opts = qemu_opts_from_qdict(qemu_find_opts("netdev"), qdict, &local_err);
> -    if (!opts) {
> -        qerror_report_err(local_err);
> -        error_free(local_err);
> -        return -1;
> +    opts_list = qemu_find_opts_err("netdev", &local_err);

I think we're a bit too paranoid here (and a bit inconsistent with
hmp_netdev_add()), but that's just a superficial observation.


> +    if (error_is_set(&local_err)) {
> +        goto exit_err;
>      }
>  
> -    res = net_client_init(opts, 1, &local_err);
> -    if (res < 0) {
> -        qerror_report_err(local_err);
> -        error_free(local_err);
> +    opts = qemu_opts_from_qdict(opts_list, qdict, &local_err);
> +    if (error_is_set(&local_err)) {
> +        goto exit_err;
> +    }
> +
> +    netdev_add(opts, &local_err);
> +    if (error_is_set(&local_err)) {
>          qemu_opts_del(opts);
> +        goto exit_err;
>      }
>  
> -    return res;
> +    return 0;
> +
> +exit_err:
> +    qerror_report_err(local_err);
> +    error_free(local_err);
> +    return -1;
>  }

OK. We migrate to error_is_set() checks now, move the error reporting to
the end, into a common block. Also drop "res", since netdev_add()
ignores net_client_init()'s retval anyway.

>  
>  int do_netdev_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
> @@ -1447,15 +1459,16 @@ static int net_init_client(QemuOpts *opts, void *dummy)
>  static int net_init_netdev(QemuOpts *opts, void *dummy)
>  {
>      Error *local_err = NULL;
> +    int ret;
>  
> -    net_client_init(opts, 1, &local_err);
> +    ret = net_client_init(opts, 1, &local_err);
>      if (error_is_set(&local_err)) {
>          qerror_report_err(local_err);
>          error_free(local_err);
>          return -1;
>      }
>  
> -    return 0;
> +    return ret;
>  }

Hmmm. How is this modification related to this patch, and why is it
necessary in general?

net_client_init() can return -1 in eight places, and it propagates /
sets errors too in those places. There's another place in there where
the type-specific init function can return a positive value (and no
error is set or reported). Until now we seemed to handle that no
differently from 0.

This change will allow net_init_netdev() to pass this positive value to
its only caller, net_init_clients():

net_init_clients()
  qemu_opts_foreach() -- called with abort_on_failure == 1
    net_init_netdev() -- now returning positive values
      net_client_init()
        type-specific init

In effect a positive retval from the type-specific init function will
now abort the loop in qemu_opts_foreach(), but it won't cause
net_init_clients() itself to fail (= return -1 early), because rc will
be positive in qemu_opts_foreach().

What are we trying to achieve?

The rest seems fine.

Thanks,
Laszlo

>  
>  int net_init_clients(void)
> diff --git a/net.h b/net.h
> index 7ee97e9..1eb9280 100644
> --- a/net.h
> +++ b/net.h
> @@ -170,7 +170,8 @@ void net_check_clients(void);
>  void net_cleanup(void);
>  void net_host_device_add(Monitor *mon, const QDict *qdict);
>  void net_host_device_remove(Monitor *mon, const QDict *qdict);
> -int do_netdev_add(Monitor *mon, const QDict *qdict, QObject **ret_data);
> +void netdev_add(QemuOpts *opts, Error **errp);
> +int qmp_netdev_add(Monitor *mon, const QDict *qdict, QObject **ret);
>  int do_netdev_del(Monitor *mon, const QDict *qdict, QObject **ret_data);
>  
>  #define DEFAULT_NETWORK_SCRIPT "/etc/qemu-ifup"
> diff --git a/qapi-schema.json b/qapi-schema.json
> index dfa0e1f..69fcd8e 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1798,3 +1798,31 @@
>  { 'command': 'dump-guest-memory',
>    'data': { 'paging': 'bool', 'protocol': 'str', '*begin': 'int',
>              '*length': 'int' } }
> +##
> +# @netdev_add:
> +#
> +# Add a network backend.
> +#
> +# @type: the type of network backend.  Current valid values are 'user', 'tap',
> +#        'vde', 'socket', 'dump' and 'bridge'
> +#
> +# @id: the name of the new network backend
> +#
> +# @props: #optional a list of properties to be passed to the backend in
> +#         the format 'name=value', like 'ifname=tap0,script=no'
> +#
> +# Notes: The semantics of @props is not well defined.  Future commands will be
> +#        introduced that provide stronger typing for backend creation.
> +#
> +# Since: 0.14.0
> +#
> +# Returns: Nothing on success
> +#          If @type is not a valid network backend, DeviceNotFound
> +#          If @id is not a valid identifier, InvalidParameterValue
> +#          if @id already exists, DuplicateId
> +#          If @props contains an invalid parameter for this backend,
> +#            InvalidParameter
> +##
> +{ 'command': 'netdev_add',
> +  'data': {'type': 'str', 'id': 'str', '*props': '**'},
> +  'gen': 'no' }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 2aa64ad..f6550cb 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -642,10 +642,7 @@ EQMP
>      {
>          .name       = "netdev_add",
>          .args_type  = "netdev:O",
> -        .params     = "[user|tap|socket],id=str[,prop=value][,...]",
> -        .help       = "add host network device",
> -        .user_print = monitor_user_noop,
> -        .mhandler.cmd_new = do_netdev_add,
> +        .mhandler.cmd_new = qmp_netdev_add,
>      },
>  
>  SQMP

Re: [Qemu-devel] [PATCH 16/16] qapi: convert netdev_del

[Laszlo Ersek]

On 05/17/12 16:33, Luiz Capitulino wrote:

> diff --git a/hmp.c b/hmp.c
> index 7a4e25f..2ce8cb9 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -990,3 +990,12 @@ void hmp_netdev_add(Monitor *mon, const QDict *qdict)
>  out:
>      hmp_handle_error(mon, &err);
>  }
> +
> +void hmp_netdev_del(Monitor *mon, const QDict *qdict)
> +{
> +    const char *id = qdict_get_str(qdict, "id");
> +    Error *err = NULL;
> +
> +    qmp_netdev_del(id, &err);
> +    hmp_handle_error(mon, &err);
> +}

I'm not sure what invariants "qdict" satisfies on entry to this
function... Does it certainly contain "id", and is it a string? If not,
even qdict_get_str() (or something below it) would crash, so I'll assume
we do set "id" to non-NULL here -- hmp_device_del() does the same.


> diff --git a/hmp.h b/hmp.h
> index 017df87..79d138d 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -63,5 +63,6 @@ void hmp_migrate(Monitor *mon, const QDict *qdict);
>  void hmp_device_del(Monitor *mon, const QDict *qdict);
>  void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict);
>  void hmp_netdev_add(Monitor *mon, const QDict *qdict);
> +void hmp_netdev_del(Monitor *mon, const QDict *qdict);
>  
>  #endif
> diff --git a/net.c b/net.c
> index 5f0c53c..4aa416c 100644
> --- a/net.c
> +++ b/net.c
> @@ -1269,19 +1269,18 @@ exit_err:
>      return -1;
>  }
>  
> -int do_netdev_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
> +void qmp_netdev_del(const char *id, Error **errp)
>  {
> -    const char *id = qdict_get_str(qdict, "id");
>      VLANClientState *vc;
>  
>      vc = qemu_find_netdev(id);

OK, "id" must be non-NULL either way.

>      if (!vc) {
> -        qerror_report(QERR_DEVICE_NOT_FOUND, id);
> -        return -1;
> +        error_set(errp, QERR_DEVICE_NOT_FOUND, id);
> +        return;
>      }
> +
>      qemu_del_vlan_client(vc);
> -    qemu_opts_del(qemu_opts_find(qemu_find_opts("netdev"), id));
> -    return 0;
> +    qemu_opts_del(qemu_opts_find(qemu_find_opts_err("netdev", errp), id));
>  }

I think this last change is both unnecessary and ineffective.
qemu_find_opts("netdev") should always succeed.

If not, then -- even though qemu_find_opts_err() -> find_list() sets the
error -- we call qemu_opts_find(NULL, id), which I think crashes.

Anyway it seems harmless.

>  
>  static void print_net_client(Monitor *mon, VLANClientState *vc)
> diff --git a/net.h b/net.h
> index 1eb9280..bdc2a06 100644
> --- a/net.h
> +++ b/net.h
> @@ -172,7 +172,6 @@ void net_host_device_add(Monitor *mon, const QDict *qdict);
>  void net_host_device_remove(Monitor *mon, const QDict *qdict);
>  void netdev_add(QemuOpts *opts, Error **errp);
>  int qmp_netdev_add(Monitor *mon, const QDict *qdict, QObject **ret);
> -int do_netdev_del(Monitor *mon, const QDict *qdict, QObject **ret_data);
>  
>  #define DEFAULT_NETWORK_SCRIPT "/etc/qemu-ifup"
>  #define DEFAULT_NETWORK_DOWN_SCRIPT "/etc/qemu-ifdown"
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 69fcd8e..bb1f806 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1826,3 +1826,17 @@
>  { 'command': 'netdev_add',
>    'data': {'type': 'str', 'id': 'str', '*props': '**'},

Yep, "id" is mandatory, and I think it ensures that "id"'s value is at
worst an empty string in qmp_netdev_del(), not a NULL pointer.
(qmp_input_type_str() calls g_strdup(), if I'm not lost.)

>    'gen': 'no' }
> +
> +##
> +# @netdev_del:
> +#
> +# Remove a network backend.
> +#
> +# @id: the name of the network backend to remove
> +#
> +# Returns: Nothing on success
> +#          If @id is not a valid network backend, DeviceNotFound
> +#
> +# Since: 0.14.0
> +##
> +{ 'command': 'netdev_del', 'data': {'id': 'str'} }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index f6550cb..57ea803 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -671,10 +671,7 @@ EQMP
>      {
>          .name       = "netdev_del",
>          .args_type  = "id:s",
> -        .params     = "id",
> -        .help       = "remove host network device",
> -        .user_print = monitor_user_noop,
> -        .mhandler.cmd_new = do_netdev_del,
> +        .mhandler.cmd_new = qmp_marshal_input_netdev_del,
>      },
>  
>  SQMP

I'm ready to ACK the series, but I'm intrigued by the net_init_netdev()
change in 15/16... :)

Thanks!
Laszlo

Re: [Qemu-devel] [PATCH 15/16] qapi: convert netdev_add

[Luiz Capitulino]

On Fri, 18 May 2012 17:51:01 +0200
Laszlo Ersek <lersek@redhat.com> wrote:

> > +void hmp_netdev_add(Monitor *mon, const QDict *qdict)
> > +{
> > +    Error *err = NULL;
> > +    QemuOpts *opts;
> > +
> > +    opts = qemu_opts_from_qdict(qemu_find_opts("netdev"), qdict, &err);
> 
> I note we trust qemu_find_opts("netdev") to succeed, as we did in
> do_netdev_add() before.

Yes, as far as I can see a failure is impossible. We could add an assert(),
but this patch doesn't change the current code in that aspect.

> >  int do_netdev_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
> > @@ -1447,15 +1459,16 @@ static int net_init_client(QemuOpts *opts, void *dummy)
> >  static int net_init_netdev(QemuOpts *opts, void *dummy)
> >  {
> >      Error *local_err = NULL;
> > +    int ret;
> >  
> > -    net_client_init(opts, 1, &local_err);
> > +    ret = net_client_init(opts, 1, &local_err);
> >      if (error_is_set(&local_err)) {
> >          qerror_report_err(local_err);
> >          error_free(local_err);
> >          return -1;
> >      }
> >  
> > -    return 0;
> > +    return ret;
> >  }
> 
> Hmmm. How is this modification related to this patch, and why is it
> necessary in general?

I think you found two problems here. The first one is that this hunk doesn't
pertain to this patch, it should be in the previous one.

The other problem is this:

> net_client_init() can return -1 in eight places, and it propagates /
> sets errors too in those places. There's another place in there where
> the type-specific init function can return a positive value (and no
> error is set or reported). Until now we seemed to handle that no
> differently from 0.
> 
> This change will allow net_init_netdev() to pass this positive value to
> its only caller, net_init_clients():
> 
> net_init_clients()
>   qemu_opts_foreach() -- called with abort_on_failure == 1
>     net_init_netdev() -- now returning positive values
>       net_client_init()
>         type-specific init
> 
> In effect a positive retval from the type-specific init function will
> now abort the loop in qemu_opts_foreach(), but it won't cause
> net_init_clients() itself to fail (= return -1 early), because rc will
> be positive in qemu_opts_foreach().

Your theory is right, but this is not related to this series, as the current
code does it too. But more importantly, it seems that that condition can
never happen in practice.

The only type init function that returns a positive value is net_init_nic().
However, net_init_netdev() calls net_client_init() with is_netdev=1, meaning
that it wants to initialize a backend. This also means that net_init_nic()
will never be called in this context.

We have two options here:

 1. Just maintain that behavior. This was my original intention, but then
    the hunk changing net_init_netdev() should be moved to the previous patch

 2. The value returned by net_init_nic() doesn't seem to be used outside
    net_init_nic(), so I could add a patch to this series dropping it

Michael, Markus, any suggestion?

Other than that I think we should have a better separation between front-ends
and back-ends in that code (we seem to call everything a "type").

Re: [Qemu-devel] [PATCH 15/16] qapi: convert netdev_add

[Paolo Bonzini]

Il 18/05/2012 19:41, Luiz Capitulino ha scritto:
> Other than that I think we should have a better separation between front-ends
> and back-ends in that code (we seem to call everything a "type").

That makes sense if you consider the way -net originally worked with
VLANs...

Paolo

Re: [Qemu-devel] [PATCH 16/16] qapi: convert netdev_del

[Luiz Capitulino]

On Fri, 18 May 2012 18:15:34 +0200
Laszlo Ersek <lersek@redhat.com> wrote:

> On 05/17/12 16:33, Luiz Capitulino wrote:
> 
> > diff --git a/hmp.c b/hmp.c
> > index 7a4e25f..2ce8cb9 100644
> > --- a/hmp.c
> > +++ b/hmp.c
> > @@ -990,3 +990,12 @@ void hmp_netdev_add(Monitor *mon, const QDict *qdict)
> >  out:
> >      hmp_handle_error(mon, &err);
> >  }
> > +
> > +void hmp_netdev_del(Monitor *mon, const QDict *qdict)
> > +{
> > +    const char *id = qdict_get_str(qdict, "id");
> > +    Error *err = NULL;
> > +
> > +    qmp_netdev_del(id, &err);
> > +    hmp_handle_error(mon, &err);
> > +}
> 
> I'm not sure what invariants "qdict" satisfies on entry to this
> function... Does it certainly contain "id", and is it a string? If not,
> even qdict_get_str() (or something below it) would crash, so I'll assume
> we do set "id" to non-NULL here -- hmp_device_del() does the same.

qdict is a dictionary containing all options passed by the user. In this
context, "id" is an required option so it must exist at this point (if the
user doesn't pass it HMP (the human monitor) will fail before getting here).

> I'm ready to ACK the series, but I'm intrigued by the net_init_netdev()
> change in 15/16... :)

Thanks a lot for the review!

[Qemu-devel] [PATCH 03/16] 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 |   24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/qemu-option.c b/qemu-option.c
index 72dcb56..b5da116 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,9 +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;
-        list->value.n = flag;
+        parse_option_bool(name, value, &flag, &local_err);
+        if (!error_is_set(&local_err)) {
+            list->value.n = flag;
+        }
         break;
 
     case OPT_STRING:
@@ -298,6 +299,12 @@ int set_option_parameter(QEMUOptionParameter *list, const char *name,
         return -1;
     }
 
+    if (error_is_set(&local_err)) {
+        qerror_report_err(local_err);
+        error_free(local_err);
+        return -1;
+    }
+
     return 0;
 }
 
@@ -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