[Qemu-devel] [PATCH 00/13] QemuOpts: Convert various setters to Error

[Qemu-devel] [PATCH 00/13] QemuOpts: Convert various setters to Error

[Markus Armbruster]

Bonus: improve qemu-img error reporting slightly.

Perhaps the block maintainers want to pick this up.  If not, I'm happy
to route it through my tree.

Markus Armbruster (13):
  QemuOpts: Convert qemu_opt_set_bool() to Error, fix its use
  QemuOpts: Convert qemu_opt_set_number() to Error, fix its use
  QemuOpts: Convert qemu_opts_set() to Error, fix its use
  qemu-img: Suppress unhelpful extra errors in convert, resize
  block: Suppress unhelpful extra errors in bdrv_img_create()
  QemuOpts: Drop qemu_opt_set(), rename qemu_opt_set_err(), fix use
  QemuOpts: Propagate errors through opts_do_parse()
  QemuOpts: Propagate errors through opts_parse()
  qemu-img: Suppress unhelpful extra errors in convert, amend
  block: Simplify setting numeric options
  qemu-sockets: Simplify setting numeric and boolean options
  pc: Use qemu_opt_set() instead of qemu_opts_parse()
  qtest: Use qemu_opt_set() instead of qemu_opts_parse()

 block.c                  |  17 +++++---
 block/nbd.c              |   3 +-
 block/qcow2.c            |   5 ++-
 block/vvfat.c            |   5 ++-
 blockdev.c               |  25 ++++++------
 hw/i386/pc.c             |  11 ++---
 hw/pci/pci-hotplug-old.c |   2 +-
 hw/usb/dev-network.c     |   4 +-
 hw/usb/dev-storage.c     |   6 +--
 hw/watchdog/watchdog.c   |   2 +-
 include/qemu/option.h    |  18 +++++----
 net/net.c                |   6 +--
 qdev-monitor.c           |   6 +--
 qemu-char.c              |  68 +++++++++++++++++--------------
 qemu-img.c               |  44 ++++++++++++++------
 qtest.c                  |  11 ++---
 tests/test-qemu-opts.c   |  38 +++++++++---------
 util/qemu-config.c       |   9 ++++-
 util/qemu-option.c       | 100 +++++++++++++++++++++++-----------------------
 util/qemu-sockets.c      |  42 +++++++++----------
 vl.c                     | 102 ++++++++++++++++++++++++++---------------------
 21 files changed, 284 insertions(+), 240 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH 01/13] QemuOpts: Convert qemu_opt_set_bool() to Error, fix its use

[Markus Armbruster]

Return the Error object instead of reporting it with
qerror_report_err().

Change callers that assume the function can't fail to pass
&error_abort, so that should the assumption ever break, it'll break
noisily.

Turns out all callers outside its unit test assume that.  We could
drop the Error ** argument, but that would make the interface less
regular, so don't.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 blockdev.c             |  6 +++---
 include/qemu/option.h  |  3 ++-
 tests/test-qemu-opts.c | 10 +++++-----
 util/qemu-option.c     |  9 ++++-----
 util/qemu-sockets.c    |  4 ++--
 vl.c                   |  5 +++--
 6 files changed, 19 insertions(+), 18 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 5437955..b279ce9 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -745,15 +745,15 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
         /* Specific options take precedence */
         if (!qemu_opt_get(all_opts, "cache.writeback")) {
             qemu_opt_set_bool(all_opts, "cache.writeback",
-                              !!(flags & BDRV_O_CACHE_WB));
+                              !!(flags & BDRV_O_CACHE_WB), &error_abort);
         }
         if (!qemu_opt_get(all_opts, "cache.direct")) {
             qemu_opt_set_bool(all_opts, "cache.direct",
-                              !!(flags & BDRV_O_NOCACHE));
+                              !!(flags & BDRV_O_NOCACHE), &error_abort);
         }
         if (!qemu_opt_get(all_opts, "cache.no-flush")) {
             qemu_opt_set_bool(all_opts, "cache.no-flush",
-                              !!(flags & BDRV_O_NO_FLUSH));
+                              !!(flags & BDRV_O_NO_FLUSH), &error_abort);
         }
         qemu_opt_unset(all_opts, "cache");
     }
diff --git a/include/qemu/option.h b/include/qemu/option.h
index 58c0157..3206874 100644
--- a/include/qemu/option.h
+++ b/include/qemu/option.h
@@ -97,7 +97,8 @@ int qemu_opt_unset(QemuOpts *opts, const char *name);
 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);
+void qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val,
+                       Error **errp);
 int qemu_opt_set_number(QemuOpts *opts, const char *name, int64_t 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,
diff --git a/tests/test-qemu-opts.c b/tests/test-qemu-opts.c
index ca08ac5..6b0ae33 100644
--- a/tests/test-qemu-opts.c
+++ b/tests/test-qemu-opts.c
@@ -169,10 +169,10 @@ static void test_qemu_opt_get(void)
 
 static void test_qemu_opt_get_bool(void)
 {
+    Error *err = NULL;
     QemuOptsList *list;
     QemuOpts *opts;
     bool opt;
-    int ret;
 
     list = qemu_find_opts("opts_list_02");
     g_assert(list != NULL);
@@ -192,16 +192,16 @@ static void test_qemu_opt_get_bool(void)
     opt = qemu_opt_get_bool(opts, "bool1", false);
     g_assert(opt == false);
 
-    ret = qemu_opt_set_bool(opts, "bool1", true);
-    g_assert(ret == 0);
+    qemu_opt_set_bool(opts, "bool1", true, &err);
+    g_assert(!err);
 
     /* now we have set bool1, should know about it */
     opt = qemu_opt_get_bool(opts, "bool1", false);
     g_assert(opt == true);
 
     /* having reset the value, opt should be the reset one not defval */
-    ret = qemu_opt_set_bool(opts, "bool1", false);
-    g_assert(ret == 0);
+    qemu_opt_set_bool(opts, "bool1", false, &err);
+    g_assert(!err);
 
     opt = qemu_opt_get_bool(opts, "bool1", true);
     g_assert(opt == false);
diff --git a/util/qemu-option.c b/util/qemu-option.c
index d3ab65d..4de8326 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -568,7 +568,8 @@ void qemu_opt_set_err(QemuOpts *opts, const char *name, const char *value,
     opt_set(opts, name, value, false, errp);
 }
 
-int qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val)
+void qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val,
+                       Error **errp)
 {
     QemuOpt *opt;
     const QemuOptDesc *desc = opts->list->desc;
@@ -576,9 +577,9 @@ int qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val)
     opt = g_malloc0(sizeof(*opt));
     opt->desc = find_desc_by_name(desc, name);
     if (!opt->desc && !opts_accepts_any(opts)) {
-        qerror_report(QERR_INVALID_PARAMETER, name);
+        error_set(errp, QERR_INVALID_PARAMETER, name);
         g_free(opt);
-        return -1;
+        return;
     }
 
     opt->name = g_strdup(name);
@@ -586,8 +587,6 @@ int qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val)
     opt->value.boolean = !!val;
     opt->str = g_strdup(val ? "on" : "off");
     QTAILQ_INSERT_TAIL(&opts->head, opt, next);
-
-    return 0;
 }
 
 int qemu_opt_set_number(QemuOpts *opts, const char *name, int64_t val)
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 61fc3c1..7205dad 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -580,8 +580,8 @@ static void inet_addr_to_opts(QemuOpts *opts, const InetSocketAddress *addr)
     bool ipv6 = addr->ipv6 || !addr->has_ipv6;
 
     if (!ipv4 || !ipv6) {
-        qemu_opt_set_bool(opts, "ipv4", ipv4);
-        qemu_opt_set_bool(opts, "ipv6", ipv6);
+        qemu_opt_set_bool(opts, "ipv4", ipv4, &error_abort);
+        qemu_opt_set_bool(opts, "ipv6", ipv6, &error_abort);
     }
     if (addr->has_to) {
         char to[20];
diff --git a/vl.c b/vl.c
index 2a58c6b..d557d11 100644
--- a/vl.c
+++ b/vl.c
@@ -2222,7 +2222,7 @@ static void monitor_parse(const char *optarg, const char *mode, bool pretty)
     }
     qemu_opt_set(opts, "mode", mode);
     qemu_opt_set(opts, "chardev", label);
-    qemu_opt_set_bool(opts, "pretty", pretty);
+    qemu_opt_set_bool(opts, "pretty", pretty, &error_abort);
     if (def)
         qemu_opt_set(opts, "default", "on");
     monitor_device_index++;
@@ -3287,7 +3287,8 @@ int main(int argc, char **argv, char **envp)
                 }
 
                 qemu_opt_set_bool(fsdev, "readonly",
-                                qemu_opt_get_bool(opts, "readonly", 0));
+                                  qemu_opt_get_bool(opts, "readonly", 0),
+                                  &error_abort);
                 device = qemu_opts_create(qemu_find_opts("device"), NULL, 0,
                                           &error_abort);
                 qemu_opt_set(device, "driver", "virtio-9p-pci");
-- 
1.9.3

Re: [Qemu-devel] [PATCH 01/13] QemuOpts: Convert qemu_opt_set_bool() to Error, fix its use

[Eric Blake]

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

On 02/16/2015 07:44 AM, Markus Armbruster wrote:
> Return the Error object instead of reporting it with
> qerror_report_err().
> 
> Change callers that assume the function can't fail to pass
> &error_abort, so that should the assumption ever break, it'll break
> noisily.
> 
> Turns out all callers outside its unit test assume that.  We could
> drop the Error ** argument, but that would make the interface less
> regular, so don't.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  blockdev.c             |  6 +++---
>  include/qemu/option.h  |  3 ++-
>  tests/test-qemu-opts.c | 10 +++++-----
>  util/qemu-option.c     |  9 ++++-----
>  util/qemu-sockets.c    |  4 ++--
>  vl.c                   |  5 +++--
>  6 files changed, 19 insertions(+), 18 deletions(-)

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

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


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

[Qemu-devel] [PATCH 02/13] QemuOpts: Convert qemu_opt_set_number() to Error, fix its use

[Markus Armbruster]

Return the Error object instead of reporting it with
qerror_report_err().

Change callers that assume the function can't fail to pass
&error_abort, so that should the assumption ever break, it'll break
noisily.

Turns out all callers outside its unit test assume that.  We could
drop the Error ** argument, but that would make the interface less
regular, so don't.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block.c                |  6 +++---
 block/nbd.c            |  3 ++-
 block/qcow2.c          |  2 +-
 block/vvfat.c          |  3 ++-
 include/qemu/option.h  |  3 ++-
 qemu-img.c             |  3 ++-
 tests/test-qemu-opts.c | 16 ++++++++--------
 util/qemu-option.c     |  9 ++++-----
 vl.c                   |  2 +-
 9 files changed, 25 insertions(+), 22 deletions(-)

diff --git a/block.c b/block.c
index 210fd5f..be28845 100644
--- a/block.c
+++ b/block.c
@@ -1364,7 +1364,7 @@ int bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp)
 
     opts = qemu_opts_create(bdrv_qcow2.create_opts, NULL, 0,
                             &error_abort);
-    qemu_opt_set_number(opts, BLOCK_OPT_SIZE, total_size);
+    qemu_opt_set_number(opts, BLOCK_OPT_SIZE, total_size, &error_abort);
     ret = bdrv_create(&bdrv_qcow2, tmp_filename, opts, &local_err);
     qemu_opts_del(opts);
     if (ret < 0) {
@@ -5646,7 +5646,7 @@ void bdrv_img_create(const char *filename, const char *fmt,
 
     /* Create parameter list with default values */
     opts = qemu_opts_create(create_opts, NULL, 0, &error_abort);
-    qemu_opt_set_number(opts, BLOCK_OPT_SIZE, img_size);
+    qemu_opt_set_number(opts, BLOCK_OPT_SIZE, img_size, &error_abort);
 
     /* Parse -o options */
     if (options) {
@@ -5728,7 +5728,7 @@ void bdrv_img_create(const char *filename, const char *fmt,
                 goto out;
             }
 
-            qemu_opt_set_number(opts, BLOCK_OPT_SIZE, size);
+            qemu_opt_set_number(opts, BLOCK_OPT_SIZE, size, &error_abort);
 
             bdrv_unref(bs);
         } else {
diff --git a/block/nbd.c b/block/nbd.c
index b05d1d0..13a4bae 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -215,7 +215,8 @@ static void nbd_config(BDRVNBDState *s, QDict *options, char **export,
     }
 
     if (!qemu_opt_get(s->socket_opts, "port")) {
-        qemu_opt_set_number(s->socket_opts, "port", NBD_DEFAULT_PORT);
+        qemu_opt_set_number(s->socket_opts, "port", NBD_DEFAULT_PORT,
+                            &error_abort);
     }
 
     *export = g_strdup(qdict_get_try_str(options, "export"));
diff --git a/block/qcow2.c b/block/qcow2.c
index 7e614d7..c83e0ff 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1858,7 +1858,7 @@ static int qcow2_create2(const char *filename, int64_t total_size,
         meta_size += nreftablee * sizeof(uint64_t);
 
         qemu_opt_set_number(opts, BLOCK_OPT_SIZE,
-                            aligned_total_size + meta_size);
+                            aligned_total_size + meta_size, &error_abort);
         qemu_opt_set(opts, BLOCK_OPT_PREALLOC, PreallocMode_lookup[prealloc]);
     }
 
diff --git a/block/vvfat.c b/block/vvfat.c
index a1a44f0..5e32d77 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -2924,7 +2924,8 @@ static int enable_write_target(BDRVVVFATState *s, Error **errp)
     }
 
     opts = qemu_opts_create(bdrv_qcow->create_opts, NULL, 0, &error_abort);
-    qemu_opt_set_number(opts, BLOCK_OPT_SIZE, s->sector_count * 512);
+    qemu_opt_set_number(opts, BLOCK_OPT_SIZE, s->sector_count * 512,
+                        &error_abort);
     qemu_opt_set(opts, BLOCK_OPT_BACKING_FILE, "fat:");
 
     ret = bdrv_create(bdrv_qcow, s->qcow_filename, opts, errp);
diff --git a/include/qemu/option.h b/include/qemu/option.h
index 3206874..7422cc2 100644
--- a/include/qemu/option.h
+++ b/include/qemu/option.h
@@ -99,7 +99,8 @@ void qemu_opt_set_err(QemuOpts *opts, const char *name, const char *value,
                       Error **errp);
 void qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val,
                        Error **errp);
-int qemu_opt_set_number(QemuOpts *opts, const char *name, int64_t val);
+void qemu_opt_set_number(QemuOpts *opts, const char *name, int64_t val,
+                         Error **errp);
 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,
                      int abort_on_failure);
diff --git a/qemu-img.c b/qemu-img.c
index 25b1369..7eea84a 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1554,7 +1554,8 @@ static int img_convert(int argc, char **argv)
         goto out;
     }
 
-    qemu_opt_set_number(opts, BLOCK_OPT_SIZE, total_sectors * 512);
+    qemu_opt_set_number(opts, BLOCK_OPT_SIZE, total_sectors * 512,
+                        &error_abort);
     ret = add_old_style_options(out_fmt, opts, out_baseimg, NULL);
     if (ret < 0) {
         goto out;
diff --git a/tests/test-qemu-opts.c b/tests/test-qemu-opts.c
index 6b0ae33..3fea96a 100644
--- a/tests/test-qemu-opts.c
+++ b/tests/test-qemu-opts.c
@@ -215,10 +215,10 @@ static void test_qemu_opt_get_bool(void)
 
 static void test_qemu_opt_get_number(void)
 {
+    Error *err = NULL;
     QemuOptsList *list;
     QemuOpts *opts;
     uint64_t opt;
-    int ret;
 
     list = qemu_find_opts("opts_list_01");
     g_assert(list != NULL);
@@ -238,16 +238,16 @@ static void test_qemu_opt_get_number(void)
     opt = qemu_opt_get_number(opts, "number1", 5);
     g_assert(opt == 5);
 
-    ret = qemu_opt_set_number(opts, "number1", 10);
-    g_assert(ret == 0);
+    qemu_opt_set_number(opts, "number1", 10, &err);
+    g_assert(!err);
 
     /* now we have set number1, should know about it */
     opt = qemu_opt_get_number(opts, "number1", 5);
     g_assert(opt == 10);
 
     /* having reset it, the returned should be the reset one not defval */
-    ret = qemu_opt_set_number(opts, "number1", 15);
-    g_assert(ret == 0);
+    qemu_opt_set_number(opts, "number1", 15, &err);
+    g_assert(!err);
 
     opt = qemu_opt_get_number(opts, "number1", 5);
     g_assert(opt == 15);
@@ -349,10 +349,10 @@ static void test_qemu_opt_unset(void)
 
 static void test_qemu_opts_reset(void)
 {
+    Error *err = NULL;
     QemuOptsList *list;
     QemuOpts *opts;
     uint64_t opt;
-    int ret;
 
     list = qemu_find_opts("opts_list_01");
     g_assert(list != NULL);
@@ -372,8 +372,8 @@ static void test_qemu_opts_reset(void)
     opt = qemu_opt_get_number(opts, "number1", 5);
     g_assert(opt == 5);
 
-    ret = qemu_opt_set_number(opts, "number1", 10);
-    g_assert(ret == 0);
+    qemu_opt_set_number(opts, "number1", 10, &err);
+    g_assert(!err);
 
     /* now we have set number1, should know about it */
     opt = qemu_opt_get_number(opts, "number1", 5);
diff --git a/util/qemu-option.c b/util/qemu-option.c
index 4de8326..c873b6c 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -589,7 +589,8 @@ void qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val,
     QTAILQ_INSERT_TAIL(&opts->head, opt, next);
 }
 
-int qemu_opt_set_number(QemuOpts *opts, const char *name, int64_t val)
+void qemu_opt_set_number(QemuOpts *opts, const char *name, int64_t val,
+                         Error **errp)
 {
     QemuOpt *opt;
     const QemuOptDesc *desc = opts->list->desc;
@@ -597,9 +598,9 @@ int qemu_opt_set_number(QemuOpts *opts, const char *name, int64_t val)
     opt = g_malloc0(sizeof(*opt));
     opt->desc = find_desc_by_name(desc, name);
     if (!opt->desc && !opts_accepts_any(opts)) {
-        qerror_report(QERR_INVALID_PARAMETER, name);
+        error_set(errp, QERR_INVALID_PARAMETER, name);
         g_free(opt);
-        return -1;
+        return;
     }
 
     opt->name = g_strdup(name);
@@ -607,8 +608,6 @@ int qemu_opt_set_number(QemuOpts *opts, const char *name, int64_t val)
     opt->value.uint = val;
     opt->str = g_strdup_printf("%" PRId64, val);
     QTAILQ_INSERT_TAIL(&opts->head, opt, next);
-
-    return 0;
 }
 
 int qemu_opt_foreach(QemuOpts *opts, qemu_opt_loopfunc func, void *opaque,
diff --git a/vl.c b/vl.c
index d557d11..395793e 100644
--- a/vl.c
+++ b/vl.c
@@ -2684,7 +2684,7 @@ static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size)
     }
 
     /* store value for the future use */
-    qemu_opt_set_number(opts, "size", ram_size);
+    qemu_opt_set_number(opts, "size", ram_size, &error_abort);
     *maxram_size = ram_size;
 
     maxmem_str = qemu_opt_get(opts, "maxmem");
-- 
1.9.3

Re: [Qemu-devel] [PATCH 02/13] QemuOpts: Convert qemu_opt_set_number() to Error, fix its use

[Eric Blake]

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

On 02/16/2015 07:44 AM, Markus Armbruster wrote:
> Return the Error object instead of reporting it with
> qerror_report_err().
> 
> Change callers that assume the function can't fail to pass
> &error_abort, so that should the assumption ever break, it'll break
> noisily.
> 
> Turns out all callers outside its unit test assume that.  We could
> drop the Error ** argument, but that would make the interface less
> regular, so don't.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block.c                |  6 +++---
>  block/nbd.c            |  3 ++-
>  block/qcow2.c          |  2 +-
>  block/vvfat.c          |  3 ++-
>  include/qemu/option.h  |  3 ++-
>  qemu-img.c             |  3 ++-
>  tests/test-qemu-opts.c | 16 ++++++++--------
>  util/qemu-option.c     |  9 ++++-----
>  vl.c                   |  2 +-
>  9 files changed, 25 insertions(+), 22 deletions(-)
> 

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

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


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

[Qemu-devel] [PATCH 03/13] QemuOpts: Convert qemu_opts_set() to Error, fix its use

[Markus Armbruster]

Return the Error object instead of reporting it with
qerror_report_err().

Change callers that assume the function can't fail to pass
&error_abort, so that should the assumption ever break, it'll break
noisily.

Turns out all callers outside its unit test assume that.  We could
drop the Error ** argument, but that would make the interface less
regular, so don't.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/qemu/option.h  |  4 ++--
 net/net.c              |  4 ++--
 tests/test-qemu-opts.c |  6 +++---
 util/qemu-option.c     | 11 +++++------
 vl.c                   | 15 ++++++++++-----
 5 files changed, 22 insertions(+), 18 deletions(-)

diff --git a/include/qemu/option.h b/include/qemu/option.h
index 7422cc2..7d6addc 100644
--- a/include/qemu/option.h
+++ b/include/qemu/option.h
@@ -110,8 +110,8 @@ 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,
-                  const char *name, const char *value);
+void qemu_opts_set(QemuOptsList *list, const char *id,
+                   const char *name, const char *value, Error **errp);
 const char *qemu_opts_id(QemuOpts *opts);
 void qemu_opts_set_id(QemuOpts *opts, char *id);
 void qemu_opts_del(QemuOpts *opts);
diff --git a/net/net.c b/net/net.c
index ec6e581..f0499c0 100644
--- a/net/net.c
+++ b/net/net.c
@@ -1296,9 +1296,9 @@ int net_init_clients(void)
 
     if (default_net) {
         /* if no clients, we use a default config */
-        qemu_opts_set(net, NULL, "type", "nic");
+        qemu_opts_set(net, NULL, "type", "nic", &error_abort);
 #ifdef CONFIG_SLIRP
-        qemu_opts_set(net, NULL, "type", "user");
+        qemu_opts_set(net, NULL, "type", "user", &error_abort);
 #endif
     }
 
diff --git a/tests/test-qemu-opts.c b/tests/test-qemu-opts.c
index 3fea96a..11cd1bd 100644
--- a/tests/test-qemu-opts.c
+++ b/tests/test-qemu-opts.c
@@ -388,9 +388,9 @@ static void test_qemu_opts_reset(void)
 
 static void test_qemu_opts_set(void)
 {
+    Error *err = NULL;
     QemuOptsList *list;
     QemuOpts *opts;
-    int ret;
     const char *opt;
 
     list = qemu_find_opts("opts_list_01");
@@ -403,8 +403,8 @@ static void test_qemu_opts_set(void)
     g_assert(opts == NULL);
 
     /* implicitly create opts and set str3 value */
-    ret = qemu_opts_set(list, NULL, "str3", "value");
-    g_assert(ret == 0);
+    qemu_opts_set(list, NULL, "str3", "value", &err);
+    g_assert(!err);
     g_assert(!QTAILQ_EMPTY(&list->head));
 
     /* get the just created opts */
diff --git a/util/qemu-option.c b/util/qemu-option.c
index c873b6c..56711ca 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -690,19 +690,18 @@ void qemu_opts_loc_restore(QemuOpts *opts)
     loc_restore(&opts->loc);
 }
 
-int qemu_opts_set(QemuOptsList *list, const char *id,
-                  const char *name, const char *value)
+void qemu_opts_set(QemuOptsList *list, const char *id,
+                   const char *name, const char *value, Error **errp)
 {
     QemuOpts *opts;
     Error *local_err = NULL;
 
     opts = qemu_opts_create(list, id, 1, &local_err);
     if (local_err) {
-        qerror_report_err(local_err);
-        error_free(local_err);
-        return -1;
+        error_propagate(errp, local_err);
+        return;
     }
-    return qemu_opt_set(opts, name, value);
+    qemu_opt_set_err(opts, name, value, errp);
 }
 
 const char *qemu_opts_id(QemuOpts *opts)
diff --git a/vl.c b/vl.c
index 395793e..5921add 100644
--- a/vl.c
+++ b/vl.c
@@ -3023,16 +3023,20 @@ int main(int argc, char **argv, char **envp)
                 }
                 break;
             case QEMU_OPTION_kernel:
-                qemu_opts_set(qemu_find_opts("machine"), 0, "kernel", optarg);
+                qemu_opts_set(qemu_find_opts("machine"), 0, "kernel", optarg,
+                              &error_abort);
                 break;
             case QEMU_OPTION_initrd:
-                qemu_opts_set(qemu_find_opts("machine"), 0, "initrd", optarg);
+                qemu_opts_set(qemu_find_opts("machine"), 0, "initrd", optarg,
+                              &error_abort);
                 break;
             case QEMU_OPTION_append:
-                qemu_opts_set(qemu_find_opts("machine"), 0, "append", optarg);
+                qemu_opts_set(qemu_find_opts("machine"), 0, "append", optarg,
+                              &error_abort);
                 break;
             case QEMU_OPTION_dtb:
-                qemu_opts_set(qemu_find_opts("machine"), 0, "dtb", optarg);
+                qemu_opts_set(qemu_find_opts("machine"), 0, "dtb", optarg,
+                              &error_abort);
                 break;
             case QEMU_OPTION_cdrom:
                 drive_add(IF_DEFAULT, 2, optarg, CDROM_OPTS);
@@ -3136,7 +3140,8 @@ int main(int argc, char **argv, char **envp)
                 }
                 break;
             case QEMU_OPTION_bios:
-                qemu_opts_set(qemu_find_opts("machine"), 0, "firmware", optarg);
+                qemu_opts_set(qemu_find_opts("machine"), 0, "firmware", optarg,
+                              &error_abort);
                 break;
             case QEMU_OPTION_singlestep:
                 singlestep = 1;
-- 
1.9.3

Re: [Qemu-devel] [PATCH 03/13] QemuOpts: Convert qemu_opts_set() to Error, fix its use

[Eric Blake]

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

On 02/16/2015 07:44 AM, Markus Armbruster wrote:
> Return the Error object instead of reporting it with
> qerror_report_err().
> 
> Change callers that assume the function can't fail to pass
> &error_abort, so that should the assumption ever break, it'll break
> noisily.
> 
> Turns out all callers outside its unit test assume that.  We could
> drop the Error ** argument, but that would make the interface less
> regular, so don't.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  include/qemu/option.h  |  4 ++--
>  net/net.c              |  4 ++--
>  tests/test-qemu-opts.c |  6 +++---
>  util/qemu-option.c     | 11 +++++------
>  vl.c                   | 15 ++++++++++-----
>  5 files changed, 22 insertions(+), 18 deletions(-)
> 

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

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


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

[Qemu-devel] [PATCH 04/13] qemu-img: Suppress unhelpful extra errors in convert, resize

[Markus Armbruster]

add_old_style_options() for img_convert() and img_resize() use
qemu_opt_set(), which reports errors with qerror_report_err().  Its
error messages aren't helpful here, the caller reports one that
actually makes sense.  Reproducer:

    $ qemu-img convert -B raw in.img out.img
    qemu-img: Invalid parameter 'backing_file'
    qemu-img: Backing file not supported for file format 'raw'

Switch to qemu_opt_set_err() to get rid of the unwanted messages.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qemu-img.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 7eea84a..7a806bc 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -338,17 +338,23 @@ static int add_old_style_options(const char *fmt, QemuOpts *opts,
                                  const char *base_filename,
                                  const char *base_fmt)
 {
+    Error *err = NULL;
+
     if (base_filename) {
-        if (qemu_opt_set(opts, BLOCK_OPT_BACKING_FILE, base_filename)) {
+        qemu_opt_set_err(opts, BLOCK_OPT_BACKING_FILE, base_filename, &err);
+        if (err) {
             error_report("Backing file not supported for file format '%s'",
                          fmt);
+            error_free(err);
             return -1;
         }
     }
     if (base_fmt) {
-        if (qemu_opt_set(opts, BLOCK_OPT_BACKING_FMT, base_fmt)) {
+        qemu_opt_set_err(opts, BLOCK_OPT_BACKING_FMT, base_fmt, &err);
+        if (err) {
             error_report("Backing file format not supported for file "
                          "format '%s'", fmt);
+            error_free(err);
             return -1;
         }
     }
@@ -2758,6 +2764,7 @@ out:
 
 static int img_resize(int argc, char **argv)
 {
+    Error *err = NULL;
     int c, ret, relative;
     const char *filename, *fmt, *size;
     int64_t n, total_size;
@@ -2830,8 +2837,9 @@ static int img_resize(int argc, char **argv)
 
     /* Parse size */
     param = qemu_opts_create(&resize_options, NULL, 0, &error_abort);
-    if (qemu_opt_set(param, BLOCK_OPT_SIZE, size)) {
-        /* Error message already printed when size parsing fails */
+    qemu_opt_set_err(param, BLOCK_OPT_SIZE, size, &err);
+    if (err) {
+        error_report_err(err);
         ret = -1;
         qemu_opts_del(param);
         goto out;
-- 
1.9.3

Re: [Qemu-devel] [PATCH 04/13] qemu-img: Suppress unhelpful extra errors in convert, resize

[John Snow]

On 02/16/2015 09:44 AM, Markus Armbruster wrote:
> add_old_style_options() for img_convert() and img_resize() use
> qemu_opt_set(), which reports errors with qerror_report_err().  Its
> error messages aren't helpful here, the caller reports one that
> actually makes sense.  Reproducer:
>
>      $ qemu-img convert -B raw in.img out.img
>      qemu-img: Invalid parameter 'backing_file'
>      qemu-img: Backing file not supported for file format 'raw'
>
> Switch to qemu_opt_set_err() to get rid of the unwanted messages.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   qemu-img.c | 16 ++++++++++++----
>   1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/qemu-img.c b/qemu-img.c
> index 7eea84a..7a806bc 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -338,17 +338,23 @@ static int add_old_style_options(const char *fmt, QemuOpts *opts,
>                                    const char *base_filename,
>                                    const char *base_fmt)
>   {
> +    Error *err = NULL;
> +
>       if (base_filename) {
> -        if (qemu_opt_set(opts, BLOCK_OPT_BACKING_FILE, base_filename)) {
> +        qemu_opt_set_err(opts, BLOCK_OPT_BACKING_FILE, base_filename, &err);
> +        if (err) {
>               error_report("Backing file not supported for file format '%s'",
>                            fmt);
> +            error_free(err);
>               return -1;
>           }
>       }
>       if (base_fmt) {
> -        if (qemu_opt_set(opts, BLOCK_OPT_BACKING_FMT, base_fmt)) {
> +        qemu_opt_set_err(opts, BLOCK_OPT_BACKING_FMT, base_fmt, &err);
> +        if (err) {
>               error_report("Backing file format not supported for file "
>                            "format '%s'", fmt);
> +            error_free(err);
>               return -1;
>           }
>       }
> @@ -2758,6 +2764,7 @@ out:
>
>   static int img_resize(int argc, char **argv)
>   {
> +    Error *err = NULL;
>       int c, ret, relative;
>       const char *filename, *fmt, *size;
>       int64_t n, total_size;
> @@ -2830,8 +2837,9 @@ static int img_resize(int argc, char **argv)
>
>       /* Parse size */
>       param = qemu_opts_create(&resize_options, NULL, 0, &error_abort);
> -    if (qemu_opt_set(param, BLOCK_OPT_SIZE, size)) {
> -        /* Error message already printed when size parsing fails */
> +    qemu_opt_set_err(param, BLOCK_OPT_SIZE, size, &err);
> +    if (err) {
> +        error_report_err(err);

Creates a new warning/failure for me, if basing off of origin/master or 
kevin/block:

   CC    qemu-img.o
/home/bos/jhuston/src/qemu/qemu-img.c: In function ‘img_resize’:
/home/bos/jhuston/src/qemu/qemu-img.c:2844:9: error: implicit 
declaration of function ‘error_report_err’ 
[-Werror=implicit-function-declaration]
          error_report_err(err);
          ^
/home/bos/jhuston/src/qemu/qemu-img.c:2844:9: error: nested extern 
declaration of ‘error_report_err’ [-Werror=nested-externs]
cc1: all warnings being treated as errors
make: *** [qemu-img.o] Error 1
make: *** Waiting for unfinished jobs....


>           ret = -1;
>           qemu_opts_del(param);
>           goto out;
>

Re: [Qemu-devel] [PATCH 04/13] qemu-img: Suppress unhelpful extra errors in convert, resize

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 02/16/2015 09:44 AM, Markus Armbruster wrote:
>> add_old_style_options() for img_convert() and img_resize() use
>> qemu_opt_set(), which reports errors with qerror_report_err().  Its
>> error messages aren't helpful here, the caller reports one that
>> actually makes sense.  Reproducer:
>>
>>      $ qemu-img convert -B raw in.img out.img
>>      qemu-img: Invalid parameter 'backing_file'
>>      qemu-img: Backing file not supported for file format 'raw'
>>
>> Switch to qemu_opt_set_err() to get rid of the unwanted messages.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>   qemu-img.c | 16 ++++++++++++----
>>   1 file changed, 12 insertions(+), 4 deletions(-)
>>
>> diff --git a/qemu-img.c b/qemu-img.c
>> index 7eea84a..7a806bc 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
[...]
>> @@ -2830,8 +2837,9 @@ static int img_resize(int argc, char **argv)
>>
>>       /* Parse size */
>>       param = qemu_opts_create(&resize_options, NULL, 0, &error_abort);
>> -    if (qemu_opt_set(param, BLOCK_OPT_SIZE, size)) {
>> -        /* Error message already printed when size parsing fails */
>> +    qemu_opt_set_err(param, BLOCK_OPT_SIZE, size, &err);
>> +    if (err) {
>> +        error_report_err(err);
>
> Creates a new warning/failure for me, if basing off of origin/master
> or kevin/block:
>
>   CC    qemu-img.o
> /home/bos/jhuston/src/qemu/qemu-img.c: In function ‘img_resize’:
> /home/bos/jhuston/src/qemu/qemu-img.c:2844:9: error: implicit
> declaration of function ‘error_report_err’
> [-Werror=implicit-function-declaration]
>          error_report_err(err);
>          ^
> /home/bos/jhuston/src/qemu/qemu-img.c:2844:9: error: nested extern
> declaration of ‘error_report_err’ [-Werror=nested-externs]
> cc1: all warnings being treated as errors
> make: *** [qemu-img.o] Error 1
> make: *** Waiting for unfinished jobs....

You need my "[PATCH v2 01/10] error: New convenience function
error_report_err()", but I forgot to mention it in my cover letter.  My
apologies.

[...]

Re: [Qemu-devel] [PATCH 04/13] qemu-img: Suppress unhelpful extra errors in convert, resize

[Eric Blake]

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

On 02/16/2015 07:44 AM, Markus Armbruster wrote:
> add_old_style_options() for img_convert() and img_resize() use
> qemu_opt_set(), which reports errors with qerror_report_err().  Its
> error messages aren't helpful here, the caller reports one that
> actually makes sense.  Reproducer:
> 
>     $ qemu-img convert -B raw in.img out.img
>     qemu-img: Invalid parameter 'backing_file'
>     qemu-img: Backing file not supported for file format 'raw'
> 
> Switch to qemu_opt_set_err() to get rid of the unwanted messages.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  qemu-img.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)

With pre-req patch in place,
Reviewed-by: Eric Blake <eblake@redhat.com>

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


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

[Qemu-devel] [PATCH 05/13] block: Suppress unhelpful extra errors in bdrv_img_create()

[Markus Armbruster]

bdrv_img_create() uses qemu_opt_set(), which reports errors with
qerror_report_err().  Its error messages aren't helpful here, the
caller reports one that actually makes sense.  I don't know how to
trigger the error conditions, though.

Switch to qemu_opt_set_err() to get rid of the unwanted messages.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index be28845..09352cb 100644
--- a/block.c
+++ b/block.c
@@ -5657,7 +5657,9 @@ void bdrv_img_create(const char *filename, const char *fmt,
     }
 
     if (base_filename) {
-        if (qemu_opt_set(opts, BLOCK_OPT_BACKING_FILE, base_filename)) {
+        qemu_opt_set_err(opts, BLOCK_OPT_BACKING_FILE, base_filename,
+                         &local_err);
+        if (local_err) {
             error_setg(errp, "Backing file not supported for file format '%s'",
                        fmt);
             goto out;
@@ -5665,7 +5667,8 @@ void bdrv_img_create(const char *filename, const char *fmt,
     }
 
     if (base_fmt) {
-        if (qemu_opt_set(opts, BLOCK_OPT_BACKING_FMT, base_fmt)) {
+        qemu_opt_set_err(opts, BLOCK_OPT_BACKING_FMT, base_fmt, &local_err);
+        if (local_err) {
             error_setg(errp, "Backing file format not supported for file "
                              "format '%s'", fmt);
             goto out;
-- 
1.9.3

Re: [Qemu-devel] [PATCH 05/13] block: Suppress unhelpful extra errors in bdrv_img_create()

[Eric Blake]

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

On 02/16/2015 07:44 AM, Markus Armbruster wrote:
> bdrv_img_create() uses qemu_opt_set(), which reports errors with
> qerror_report_err().  Its error messages aren't helpful here, the
> caller reports one that actually makes sense.  I don't know how to
> trigger the error conditions, though.
> 
> Switch to qemu_opt_set_err() to get rid of the unwanted messages.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 

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

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


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

[Qemu-devel] [PATCH 06/13] QemuOpts: Drop qemu_opt_set(), rename qemu_opt_set_err(), fix use

[Markus Armbruster]

qemu_opt_set() is a wrapper around qemu_opt_set() that reports the
error with qerror_report_err().

Most of its users assume the function can't fail.  Make them use
qemu_opt_set_err() with &error_abort, so that should the assumption
ever break, it'll break noisily.

Just two users remain, in util/qemu-config.c.  Switch them to
qemu_opt_set_err() as well, then rename qemu_opt_set_err() to
qemu_opt_set().

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block.c                  |  5 ++--
 block/qcow2.c            |  3 +-
 block/vvfat.c            |  2 +-
 blockdev.c               | 17 +++++------
 hw/pci/pci-hotplug-old.c |  2 +-
 hw/usb/dev-network.c     |  4 +--
 hw/usb/dev-storage.c     |  6 ++--
 hw/watchdog/watchdog.c   |  2 +-
 include/qemu/option.h    |  5 ++--
 net/net.c                |  2 +-
 qdev-monitor.c           |  6 ++--
 qemu-char.c              | 58 +++++++++++++++++++-------------------
 qemu-img.c               |  6 ++--
 tests/test-qemu-opts.c   |  6 ++--
 util/qemu-config.c       |  9 ++++--
 util/qemu-option.c       | 22 +++------------
 util/qemu-sockets.c      | 34 +++++++++++-----------
 vl.c                     | 73 ++++++++++++++++++++++++++----------------------
 18 files changed, 131 insertions(+), 131 deletions(-)

diff --git a/block.c b/block.c
index 09352cb..71d3bcc 100644
--- a/block.c
+++ b/block.c
@@ -5657,8 +5657,7 @@ void bdrv_img_create(const char *filename, const char *fmt,
     }
 
     if (base_filename) {
-        qemu_opt_set_err(opts, BLOCK_OPT_BACKING_FILE, base_filename,
-                         &local_err);
+        qemu_opt_set(opts, BLOCK_OPT_BACKING_FILE, base_filename, &local_err);
         if (local_err) {
             error_setg(errp, "Backing file not supported for file format '%s'",
                        fmt);
@@ -5667,7 +5666,7 @@ void bdrv_img_create(const char *filename, const char *fmt,
     }
 
     if (base_fmt) {
-        qemu_opt_set_err(opts, BLOCK_OPT_BACKING_FMT, base_fmt, &local_err);
+        qemu_opt_set(opts, BLOCK_OPT_BACKING_FMT, base_fmt, &local_err);
         if (local_err) {
             error_setg(errp, "Backing file format not supported for file "
                              "format '%s'", fmt);
diff --git a/block/qcow2.c b/block/qcow2.c
index c83e0ff..45225e5 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1859,7 +1859,8 @@ static int qcow2_create2(const char *filename, int64_t total_size,
 
         qemu_opt_set_number(opts, BLOCK_OPT_SIZE,
                             aligned_total_size + meta_size, &error_abort);
-        qemu_opt_set(opts, BLOCK_OPT_PREALLOC, PreallocMode_lookup[prealloc]);
+        qemu_opt_set(opts, BLOCK_OPT_PREALLOC, PreallocMode_lookup[prealloc],
+                     &error_abort);
     }
 
     ret = bdrv_create_file(filename, opts, &local_err);
diff --git a/block/vvfat.c b/block/vvfat.c
index 5e32d77..9be632f 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -2926,7 +2926,7 @@ static int enable_write_target(BDRVVVFATState *s, Error **errp)
     opts = qemu_opts_create(bdrv_qcow->create_opts, NULL, 0, &error_abort);
     qemu_opt_set_number(opts, BLOCK_OPT_SIZE, s->sector_count * 512,
                         &error_abort);
-    qemu_opt_set(opts, BLOCK_OPT_BACKING_FILE, "fat:");
+    qemu_opt_set(opts, BLOCK_OPT_BACKING_FILE, "fat:", &error_abort);
 
     ret = bdrv_create(bdrv_qcow, s->qcow_filename, opts, errp);
     qemu_opts_del(opts);
diff --git a/blockdev.c b/blockdev.c
index b279ce9..f4c6e92 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -187,14 +187,14 @@ QemuOpts *drive_add(BlockInterfaceType type, int index, const char *file,
         return NULL;
     }
     if (type != IF_DEFAULT) {
-        qemu_opt_set(opts, "if", if_name[type]);
+        qemu_opt_set(opts, "if", if_name[type], &error_abort);
     }
     if (index >= 0) {
         snprintf(buf, sizeof(buf), "%d", index);
-        qemu_opt_set(opts, "index", buf);
+        qemu_opt_set(opts, "index", buf, &error_abort);
     }
     if (file)
-        qemu_opt_set(opts, "file", file);
+        qemu_opt_set(opts, "file", file, &error_abort);
     return opts;
 }
 
@@ -592,7 +592,7 @@ static void qemu_opt_rename(QemuOpts *opts, const char *from, const char *to,
 
     /* rename all items in opts */
     while ((value = qemu_opt_get(opts, from))) {
-        qemu_opt_set(opts, to, value);
+        qemu_opt_set(opts, to, value, &error_abort);
         qemu_opt_unset(opts, from);
     }
 }
@@ -943,13 +943,14 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
         devopts = qemu_opts_create(qemu_find_opts("device"), NULL, 0,
                                    &error_abort);
         if (arch_type == QEMU_ARCH_S390X) {
-            qemu_opt_set(devopts, "driver", "virtio-blk-s390");
+            qemu_opt_set(devopts, "driver", "virtio-blk-s390", &error_abort);
         } else {
-            qemu_opt_set(devopts, "driver", "virtio-blk-pci");
+            qemu_opt_set(devopts, "driver", "virtio-blk-pci", &error_abort);
         }
-        qemu_opt_set(devopts, "drive", qdict_get_str(bs_opts, "id"));
+        qemu_opt_set(devopts, "drive", qdict_get_str(bs_opts, "id"),
+                     &error_abort);
         if (devaddr) {
-            qemu_opt_set(devopts, "addr", devaddr);
+            qemu_opt_set(devopts, "addr", devaddr, &error_abort);
         }
     }
 
diff --git a/hw/pci/pci-hotplug-old.c b/hw/pci/pci-hotplug-old.c
index d07db25..23d51bd 100644
--- a/hw/pci/pci-hotplug-old.c
+++ b/hw/pci/pci-hotplug-old.c
@@ -87,7 +87,7 @@ static PCIDevice *qemu_pci_hot_add_nic(Monitor *mon,
         return NULL;
     }
 
-    qemu_opt_set(opts, "type", "nic");
+    qemu_opt_set(opts, "type", "nic", &error_abort);
 
     ret = net_client_init(opts, 0, &local_err);
     if (local_err) {
diff --git a/hw/usb/dev-network.c b/hw/usb/dev-network.c
index c49fde8..dcf6f79 100644
--- a/hw/usb/dev-network.c
+++ b/hw/usb/dev-network.c
@@ -1394,8 +1394,8 @@ static USBDevice *usb_net_init(USBBus *bus, const char *cmdline)
     if (!opts) {
         return NULL;
     }
-    qemu_opt_set(opts, "type", "nic");
-    qemu_opt_set(opts, "model", "usb");
+    qemu_opt_set(opts, "type", "nic", &error_abort);
+    qemu_opt_set(opts, "model", "usb", &error_abort);
 
     idx = net_client_init(opts, 0, &local_err);
     if (local_err) {
diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index 4539733..65a7e92 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -683,7 +683,7 @@ static USBDevice *usb_msd_init(USBBus *bus, const char *filename)
         if (strstart(filename, "format=", &p2)) {
             int len = MIN(p1 - p2, sizeof(fmt));
             pstrcpy(fmt, len, p2);
-            qemu_opt_set(opts, "format", fmt);
+            qemu_opt_set(opts, "format", fmt, &error_abort);
         } else if (*filename != ':') {
             error_report("unrecognized USB mass-storage option %s", filename);
             return NULL;
@@ -694,8 +694,8 @@ static USBDevice *usb_msd_init(USBBus *bus, const char *filename)
         error_report("block device specification needed");
         return NULL;
     }
-    qemu_opt_set(opts, "file", filename);
-    qemu_opt_set(opts, "if", "none");
+    qemu_opt_set(opts, "file", filename, &error_abort);
+    qemu_opt_set(opts, "if", "none", &error_abort);
 
     /* create host drive */
     dinfo = drive_new(opts, 0);
diff --git a/hw/watchdog/watchdog.c b/hw/watchdog/watchdog.c
index c307f9b..54440c9 100644
--- a/hw/watchdog/watchdog.c
+++ b/hw/watchdog/watchdog.c
@@ -68,7 +68,7 @@ int select_watchdog(const char *p)
             /* add the device */
             opts = qemu_opts_create(qemu_find_opts("device"), NULL, 0,
                                     &error_abort);
-            qemu_opt_set(opts, "driver", p);
+            qemu_opt_set(opts, "driver", p, &error_abort);
             return 0;
         }
     }
diff --git a/include/qemu/option.h b/include/qemu/option.h
index 7d6addc..a41ee92 100644
--- a/include/qemu/option.h
+++ b/include/qemu/option.h
@@ -94,9 +94,8 @@ uint64_t qemu_opt_get_number_del(QemuOpts *opts, const char *name,
 uint64_t qemu_opt_get_size_del(QemuOpts *opts, const char *name,
                                uint64_t defval);
 int qemu_opt_unset(QemuOpts *opts, const char *name);
-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);
+void qemu_opt_set(QemuOpts *opts, const char *name, const char *value,
+                  Error **errp);
 void qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val,
                        Error **errp);
 void qemu_opt_set_number(QemuOpts *opts, const char *name, int64_t val,
diff --git a/net/net.c b/net/net.c
index f0499c0..4a0f3fd 100644
--- a/net/net.c
+++ b/net/net.c
@@ -970,7 +970,7 @@ void net_host_device_add(Monitor *mon, const QDict *qdict)
         return;
     }
 
-    qemu_opt_set(opts, "type", device);
+    qemu_opt_set(opts, "type", device, &error_abort);
 
     net_client_init(opts, 0, &local_err);
     if (local_err) {
diff --git a/qdev-monitor.c b/qdev-monitor.c
index ebfa701..c31f4fd 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -772,8 +772,8 @@ int qemu_global_option(const char *str)
     }
 
     opts = qemu_opts_create(&qemu_global_opts, NULL, 0, &error_abort);
-    qemu_opt_set(opts, "driver", driver);
-    qemu_opt_set(opts, "property", property);
-    qemu_opt_set(opts, "value", str+offset+1);
+    qemu_opt_set(opts, "driver", driver, &error_abort);
+    qemu_opt_set(opts, "property", property, &error_abort);
+    qemu_opt_set(opts, "value", str + offset + 1, &error_abort);
     return 0;
 }
diff --git a/qemu-char.c b/qemu-char.c
index 8159462..3ed46ee 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -3312,14 +3312,14 @@ QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename)
 
     if (strstart(filename, "mon:", &p)) {
         filename = p;
-        qemu_opt_set(opts, "mux", "on");
+        qemu_opt_set(opts, "mux", "on", &error_abort);
         if (strcmp(filename, "stdio") == 0) {
             /* Monitor is muxed to stdio: do not exit on Ctrl+C by default
              * but pass it to the guest.  Handle this only for compat syntax,
              * for -chardev syntax we have special option for this.
              * This is what -nographic did, redirecting+muxing serial+monitor
              * to stdio causing Ctrl+C to be passed to guest. */
-            qemu_opt_set(opts, "signal", "off");
+            qemu_opt_set(opts, "signal", "off", &error_abort);
         }
     }
 
@@ -3329,20 +3329,20 @@ QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename)
         strcmp(filename, "braille") == 0 ||
         strcmp(filename, "testdev") == 0 ||
         strcmp(filename, "stdio")   == 0) {
-        qemu_opt_set(opts, "backend", filename);
+        qemu_opt_set(opts, "backend", filename, &error_abort);
         return opts;
     }
     if (strstart(filename, "vc", &p)) {
-        qemu_opt_set(opts, "backend", "vc");
+        qemu_opt_set(opts, "backend", "vc", &error_abort);
         if (*p == ':') {
             if (sscanf(p+1, "%7[0-9]x%7[0-9]", width, height) == 2) {
                 /* pixels */
-                qemu_opt_set(opts, "width", width);
-                qemu_opt_set(opts, "height", height);
+                qemu_opt_set(opts, "width", width, &error_abort);
+                qemu_opt_set(opts, "height", height, &error_abort);
             } else if (sscanf(p+1, "%7[0-9]Cx%7[0-9]C", width, height) == 2) {
                 /* chars */
-                qemu_opt_set(opts, "cols", width);
-                qemu_opt_set(opts, "rows", height);
+                qemu_opt_set(opts, "cols", width, &error_abort);
+                qemu_opt_set(opts, "rows", height, &error_abort);
             } else {
                 goto fail;
             }
@@ -3350,22 +3350,22 @@ QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename)
         return opts;
     }
     if (strcmp(filename, "con:") == 0) {
-        qemu_opt_set(opts, "backend", "console");
+        qemu_opt_set(opts, "backend", "console", &error_abort);
         return opts;
     }
     if (strstart(filename, "COM", NULL)) {
-        qemu_opt_set(opts, "backend", "serial");
-        qemu_opt_set(opts, "path", filename);
+        qemu_opt_set(opts, "backend", "serial", &error_abort);
+        qemu_opt_set(opts, "path", filename, &error_abort);
         return opts;
     }
     if (strstart(filename, "file:", &p)) {
-        qemu_opt_set(opts, "backend", "file");
-        qemu_opt_set(opts, "path", p);
+        qemu_opt_set(opts, "backend", "file", &error_abort);
+        qemu_opt_set(opts, "path", p, &error_abort);
         return opts;
     }
     if (strstart(filename, "pipe:", &p)) {
-        qemu_opt_set(opts, "backend", "pipe");
-        qemu_opt_set(opts, "path", p);
+        qemu_opt_set(opts, "backend", "pipe", &error_abort);
+        qemu_opt_set(opts, "path", p, &error_abort);
         return opts;
     }
     if (strstart(filename, "tcp:", &p) ||
@@ -3375,27 +3375,27 @@ QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename)
             if (sscanf(p, ":%32[^,]%n", port, &pos) < 1)
                 goto fail;
         }
-        qemu_opt_set(opts, "backend", "socket");
-        qemu_opt_set(opts, "host", host);
-        qemu_opt_set(opts, "port", port);
+        qemu_opt_set(opts, "backend", "socket", &error_abort);
+        qemu_opt_set(opts, "host", host, &error_abort);
+        qemu_opt_set(opts, "port", port, &error_abort);
         if (p[pos] == ',') {
             if (qemu_opts_do_parse(opts, p+pos+1, NULL) != 0)
                 goto fail;
         }
         if (strstart(filename, "telnet:", &p))
-            qemu_opt_set(opts, "telnet", "on");
+            qemu_opt_set(opts, "telnet", "on", &error_abort);
         return opts;
     }
     if (strstart(filename, "udp:", &p)) {
-        qemu_opt_set(opts, "backend", "udp");
+        qemu_opt_set(opts, "backend", "udp", &error_abort);
         if (sscanf(p, "%64[^:]:%32[^@,]%n", host, port, &pos) < 2) {
             host[0] = 0;
             if (sscanf(p, ":%32[^@,]%n", port, &pos) < 1) {
                 goto fail;
             }
         }
-        qemu_opt_set(opts, "host", host);
-        qemu_opt_set(opts, "port", port);
+        qemu_opt_set(opts, "host", host, &error_abort);
+        qemu_opt_set(opts, "port", port, &error_abort);
         if (p[pos] == '@') {
             p += pos + 1;
             if (sscanf(p, "%64[^:]:%32[^,]%n", host, port, &pos) < 2) {
@@ -3404,26 +3404,26 @@ QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename)
                     goto fail;
                 }
             }
-            qemu_opt_set(opts, "localaddr", host);
-            qemu_opt_set(opts, "localport", port);
+            qemu_opt_set(opts, "localaddr", host, &error_abort);
+            qemu_opt_set(opts, "localport", port, &error_abort);
         }
         return opts;
     }
     if (strstart(filename, "unix:", &p)) {
-        qemu_opt_set(opts, "backend", "socket");
+        qemu_opt_set(opts, "backend", "socket", &error_abort);
         if (qemu_opts_do_parse(opts, p, "path") != 0)
             goto fail;
         return opts;
     }
     if (strstart(filename, "/dev/parport", NULL) ||
         strstart(filename, "/dev/ppi", NULL)) {
-        qemu_opt_set(opts, "backend", "parport");
-        qemu_opt_set(opts, "path", filename);
+        qemu_opt_set(opts, "backend", "parport", &error_abort);
+        qemu_opt_set(opts, "path", filename, &error_abort);
         return opts;
     }
     if (strstart(filename, "/dev/", NULL)) {
-        qemu_opt_set(opts, "backend", "tty");
-        qemu_opt_set(opts, "path", filename);
+        qemu_opt_set(opts, "backend", "tty", &error_abort);
+        qemu_opt_set(opts, "path", filename, &error_abort);
         return opts;
     }
 
diff --git a/qemu-img.c b/qemu-img.c
index 7a806bc..cb99cc2 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -341,7 +341,7 @@ static int add_old_style_options(const char *fmt, QemuOpts *opts,
     Error *err = NULL;
 
     if (base_filename) {
-        qemu_opt_set_err(opts, BLOCK_OPT_BACKING_FILE, base_filename, &err);
+        qemu_opt_set(opts, BLOCK_OPT_BACKING_FILE, base_filename, &err);
         if (err) {
             error_report("Backing file not supported for file format '%s'",
                          fmt);
@@ -350,7 +350,7 @@ static int add_old_style_options(const char *fmt, QemuOpts *opts,
         }
     }
     if (base_fmt) {
-        qemu_opt_set_err(opts, BLOCK_OPT_BACKING_FMT, base_fmt, &err);
+        qemu_opt_set(opts, BLOCK_OPT_BACKING_FMT, base_fmt, &err);
         if (err) {
             error_report("Backing file format not supported for file "
                          "format '%s'", fmt);
@@ -2837,7 +2837,7 @@ static int img_resize(int argc, char **argv)
 
     /* Parse size */
     param = qemu_opts_create(&resize_options, NULL, 0, &error_abort);
-    qemu_opt_set_err(param, BLOCK_OPT_SIZE, size, &err);
+    qemu_opt_set(param, BLOCK_OPT_SIZE, size, &err);
     if (err) {
         error_report_err(err);
         ret = -1;
diff --git a/tests/test-qemu-opts.c b/tests/test-qemu-opts.c
index 11cd1bd..da56492 100644
--- a/tests/test-qemu-opts.c
+++ b/tests/test-qemu-opts.c
@@ -148,13 +148,13 @@ static void test_qemu_opt_get(void)
     opt = qemu_opt_get(opts, "str2");
     g_assert(opt == NULL);
 
-    qemu_opt_set(opts, "str2", "value");
+    qemu_opt_set(opts, "str2", "value", &error_abort);
 
     /* now we have set str2, should know about it */
     opt = qemu_opt_get(opts, "str2");
     g_assert_cmpstr(opt, ==, "value");
 
-    qemu_opt_set(opts, "str2", "value2");
+    qemu_opt_set(opts, "str2", "value2", &error_abort);
 
     /* having reset the value, the returned should be the reset one */
     opt = qemu_opt_get(opts, "str2");
@@ -331,7 +331,7 @@ static void test_qemu_opt_unset(void)
     g_assert_cmpstr(value, ==, "value");
 
     /* reset it to value2 */
-    qemu_opt_set(opts, "key", "value2");
+    qemu_opt_set(opts, "key", "value2", &error_abort);
 
     value = qemu_opt_get(opts, "key");
     g_assert_cmpstr(value, ==, "value2");
diff --git a/util/qemu-config.c b/util/qemu-config.c
index b13efe2..203dc18 100644
--- a/util/qemu-config.c
+++ b/util/qemu-config.c
@@ -219,6 +219,7 @@ void qemu_add_opts(QemuOptsList *list)
 
 int qemu_set_option(const char *str)
 {
+    Error *local_err = NULL;
     char group[64], id[64], arg[64];
     QemuOptsList *list;
     QemuOpts *opts;
@@ -242,7 +243,9 @@ int qemu_set_option(const char *str)
         return -1;
     }
 
-    if (qemu_opt_set(opts, arg, str+offset+1) == -1) {
+    qemu_opt_set(opts, arg, str+offset+1, &local_err);
+    if (local_err) {
+        error_report_err(local_err);
         return -1;
     }
     return 0;
@@ -335,7 +338,9 @@ int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname)
                 error_report("no group defined");
                 goto out;
             }
-            if (qemu_opt_set(opts, arg, value) != 0) {
+            qemu_opt_set(opts, arg, value, &local_err);
+            if (local_err) {
+                error_report_err(local_err);
                 goto out;
             }
             continue;
diff --git a/util/qemu-option.c b/util/qemu-option.c
index 56711ca..928b86f 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -548,22 +548,8 @@ static void opt_set(QemuOpts *opts, const char *name, const char *value,
     }
 }
 
-int qemu_opt_set(QemuOpts *opts, const char *name, const char *value)
-{
-    Error *local_err = NULL;
-
-    opt_set(opts, name, value, false, &local_err);
-    if (local_err) {
-        qerror_report_err(local_err);
-        error_free(local_err);
-        return -1;
-    }
-
-    return 0;
-}
-
-void qemu_opt_set_err(QemuOpts *opts, const char *name, const char *value,
-                      Error **errp)
+void qemu_opt_set(QemuOpts *opts, const char *name, const char *value,
+                  Error **errp)
 {
     opt_set(opts, name, value, false, errp);
 }
@@ -701,7 +687,7 @@ void qemu_opts_set(QemuOptsList *list, const char *id,
         error_propagate(errp, local_err);
         return;
     }
-    qemu_opt_set_err(opts, name, value, errp);
+    qemu_opt_set(opts, name, value, errp);
 }
 
 const char *qemu_opts_id(QemuOpts *opts)
@@ -921,7 +907,7 @@ static void qemu_opts_from_qdict_1(const char *key, QObject *obj, void *opaque)
         return;
     }
 
-    qemu_opt_set_err(state->opts, key, value, state->errp);
+    qemu_opt_set(state->opts, key, value, state->errp);
 }
 
 /*
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 7205dad..e908523 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -200,10 +200,12 @@ listen:
         return -1;
     }
     snprintf(uport, sizeof(uport), "%d", inet_getport(e) - port_offset);
-    qemu_opt_set(opts, "host", uaddr);
-    qemu_opt_set(opts, "port", uport);
-    qemu_opt_set(opts, "ipv6", (e->ai_family == PF_INET6) ? "on" : "off");
-    qemu_opt_set(opts, "ipv4", (e->ai_family != PF_INET6) ? "on" : "off");
+    qemu_opt_set(opts, "host", uaddr, &error_abort);
+    qemu_opt_set(opts, "port", uport, &error_abort);
+    qemu_opt_set(opts, "ipv6", (e->ai_family == PF_INET6) ? "on" : "off",
+                 &error_abort);
+    qemu_opt_set(opts, "ipv4", (e->ai_family != PF_INET6) ? "on" : "off",
+                 &error_abort);
     freeaddrinfo(res);
     return slisten;
 }
@@ -586,10 +588,10 @@ static void inet_addr_to_opts(QemuOpts *opts, const InetSocketAddress *addr)
     if (addr->has_to) {
         char to[20];
         snprintf(to, sizeof(to), "%d", addr->to);
-        qemu_opt_set(opts, "to", to);
+        qemu_opt_set(opts, "to", to, &error_abort);
     }
-    qemu_opt_set(opts, "host", addr->host);
-    qemu_opt_set(opts, "port", addr->port);
+    qemu_opt_set(opts, "host", addr->host, &error_abort);
+    qemu_opt_set(opts, "port", addr->port, &error_abort);
 }
 
 int inet_listen(const char *str, char *ostr, int olen,
@@ -726,7 +728,7 @@ int unix_listen_opts(QemuOpts *opts, Error **errp)
             goto err;
         }
         close(fd);
-        qemu_opt_set(opts, "path", un.sun_path);
+        qemu_opt_set(opts, "path", un.sun_path, &error_abort);
     }
 
     unlink(un.sun_path);
@@ -838,11 +840,11 @@ int unix_listen(const char *str, char *ostr, int olen, Error **errp)
         if (len) {
             path = g_malloc(len+1);
             snprintf(path, len+1, "%.*s", len, str);
-            qemu_opt_set(opts, "path", path);
+            qemu_opt_set(opts, "path", path, &error_abort);
             g_free(path);
         }
     } else {
-        qemu_opt_set(opts, "path", str);
+        qemu_opt_set(opts, "path", str, &error_abort);
     }
 
     sock = unix_listen_opts(opts, errp);
@@ -859,7 +861,7 @@ int unix_connect(const char *path, Error **errp)
     int sock;
 
     opts = qemu_opts_create(&socket_optslist, NULL, 0, &error_abort);
-    qemu_opt_set(opts, "path", path);
+    qemu_opt_set(opts, "path", path, &error_abort);
     sock = unix_connect_opts(opts, errp, NULL, NULL);
     qemu_opts_del(opts);
     return sock;
@@ -876,7 +878,7 @@ int unix_nonblocking_connect(const char *path,
     g_assert(callback != NULL);
 
     opts = qemu_opts_create(&socket_optslist, NULL, 0, &error_abort);
-    qemu_opt_set(opts, "path", path);
+    qemu_opt_set(opts, "path", path, &error_abort);
     sock = unix_connect_opts(opts, errp, callback, opaque);
     qemu_opts_del(opts);
     return sock;
@@ -933,7 +935,7 @@ int socket_connect(SocketAddress *addr, Error **errp,
         break;
 
     case SOCKET_ADDRESS_KIND_UNIX:
-        qemu_opt_set(opts, "path", addr->q_unix->path);
+        qemu_opt_set(opts, "path", addr->q_unix->path, &error_abort);
         fd = unix_connect_opts(opts, errp, callback, opaque);
         break;
 
@@ -965,7 +967,7 @@ int socket_listen(SocketAddress *addr, Error **errp)
         break;
 
     case SOCKET_ADDRESS_KIND_UNIX:
-        qemu_opt_set(opts, "path", addr->q_unix->path);
+        qemu_opt_set(opts, "path", addr->q_unix->path, &error_abort);
         fd = unix_listen_opts(opts, errp);
         break;
 
@@ -990,8 +992,8 @@ int socket_dgram(SocketAddress *remote, SocketAddress *local, Error **errp)
     case SOCKET_ADDRESS_KIND_INET:
         inet_addr_to_opts(opts, remote->inet);
         if (local) {
-            qemu_opt_set(opts, "localaddr", local->inet->host);
-            qemu_opt_set(opts, "localport", local->inet->port);
+            qemu_opt_set(opts, "localaddr", local->inet->host, &error_abort);
+            qemu_opt_set(opts, "localport", local->inet->port, &error_abort);
         }
         fd = inet_dgram_opts(opts, errp);
         break;
diff --git a/vl.c b/vl.c
index 5921add..a80d36f 100644
--- a/vl.c
+++ b/vl.c
@@ -1096,7 +1096,7 @@ static int drive_init_func(QemuOpts *opts, void *opaque)
 static int drive_enable_snapshot(QemuOpts *opts, void *opaque)
 {
     if (qemu_opt_get(opts, "snapshot") == NULL) {
-        qemu_opt_set(opts, "snapshot", "on");
+        qemu_opt_set(opts, "snapshot", "on", &error_abort);
     }
     return 0;
 }
@@ -2074,7 +2074,7 @@ static int balloon_parse(const char *arg)
             opts = qemu_opts_create(qemu_find_opts("device"), NULL, 0,
                                     &error_abort);
         }
-        qemu_opt_set(opts, "driver", "virtio-balloon");
+        qemu_opt_set(opts, "driver", "virtio-balloon", &error_abort);
         return 0;
     }
 
@@ -2220,11 +2220,11 @@ static void monitor_parse(const char *optarg, const char *mode, bool pretty)
         error_report_err(local_err);
         exit(1);
     }
-    qemu_opt_set(opts, "mode", mode);
-    qemu_opt_set(opts, "chardev", label);
+    qemu_opt_set(opts, "mode", mode, &error_abort);
+    qemu_opt_set(opts, "chardev", label, &error_abort);
     qemu_opt_set_bool(opts, "pretty", pretty, &error_abort);
     if (def)
-        qemu_opt_set(opts, "default", "on");
+        qemu_opt_set(opts, "default", "on", &error_abort);
     monitor_device_index++;
 }
 
@@ -2336,13 +2336,13 @@ static int virtcon_parse(const char *devname)
 
     bus_opts = qemu_opts_create(device, NULL, 0, &error_abort);
     if (arch_type == QEMU_ARCH_S390X) {
-        qemu_opt_set(bus_opts, "driver", "virtio-serial-s390");
+        qemu_opt_set(bus_opts, "driver", "virtio-serial-s390", &error_abort);
     } else {
-        qemu_opt_set(bus_opts, "driver", "virtio-serial-pci");
+        qemu_opt_set(bus_opts, "driver", "virtio-serial-pci", &error_abort);
     }
 
     dev_opts = qemu_opts_create(device, NULL, 0, &error_abort);
-    qemu_opt_set(dev_opts, "driver", "virtconsole");
+    qemu_opt_set(dev_opts, "driver", "virtconsole", &error_abort);
 
     snprintf(label, sizeof(label), "virtcon%d", index);
     virtcon_hds[index] = qemu_chr_new(label, devname, NULL);
@@ -2351,7 +2351,7 @@ static int virtcon_parse(const char *devname)
                 " to character backend '%s'\n", devname);
         return -1;
     }
-    qemu_opt_set(dev_opts, "chardev", label);
+    qemu_opt_set(dev_opts, "chardev", label, &error_abort);
 
     index++;
     return 0;
@@ -2375,7 +2375,7 @@ static int sclp_parse(const char *devname)
     assert(arch_type == QEMU_ARCH_S390X);
 
     dev_opts = qemu_opts_create(device, NULL, 0, NULL);
-    qemu_opt_set(dev_opts, "driver", "sclpconsole");
+    qemu_opt_set(dev_opts, "driver", "sclpconsole", &error_abort);
 
     snprintf(label, sizeof(label), "sclpcon%d", index);
     sclp_hds[index] = qemu_chr_new(label, devname, NULL);
@@ -2384,7 +2384,7 @@ static int sclp_parse(const char *devname)
                 " to character backend '%s'\n", devname);
         return -1;
     }
-    qemu_opt_set(dev_opts, "chardev", label);
+    qemu_opt_set(dev_opts, "chardev", label, &error_abort);
 
     index++;
     return 0;
@@ -2402,8 +2402,8 @@ static int debugcon_parse(const char *devname)
         fprintf(stderr, "qemu: already have a debugcon device\n");
         exit(1);
     }
-    qemu_opt_set(opts, "driver", "isa-debugcon");
-    qemu_opt_set(opts, "chardev", "debugcon");
+    qemu_opt_set(opts, "driver", "isa-debugcon", &error_abort);
+    qemu_opt_set(opts, "chardev", "debugcon", &error_abort);
     return 0;
 }
 
@@ -2973,19 +2973,23 @@ int main(int argc, char **argv, char **envp)
                     if (hda_opts != NULL) {
                         char num[16];
                         snprintf(num, sizeof(num), "%d", cyls);
-                        qemu_opt_set(hda_opts, "cyls", num);
+                        qemu_opt_set(hda_opts, "cyls", num, &error_abort);
                         snprintf(num, sizeof(num), "%d", heads);
-                        qemu_opt_set(hda_opts, "heads", num);
+                        qemu_opt_set(hda_opts, "heads", num, &error_abort);
                         snprintf(num, sizeof(num), "%d", secs);
-                        qemu_opt_set(hda_opts, "secs", num);
+                        qemu_opt_set(hda_opts, "secs", num, &error_abort);
                         if (translation == BIOS_ATA_TRANSLATION_LARGE) {
-                            qemu_opt_set(hda_opts, "trans", "large");
+                            qemu_opt_set(hda_opts, "trans", "large",
+                                         &error_abort);
                         } else if (translation == BIOS_ATA_TRANSLATION_RECHS) {
-                            qemu_opt_set(hda_opts, "trans", "rechs");
+                            qemu_opt_set(hda_opts, "trans", "rechs",
+                                         &error_abort);
                         } else if (translation == BIOS_ATA_TRANSLATION_LBA) {
-                            qemu_opt_set(hda_opts, "trans", "lba");
+                            qemu_opt_set(hda_opts, "trans", "lba",
+                                         &error_abort);
                         } else if (translation == BIOS_ATA_TRANSLATION_NONE) {
-                            qemu_opt_set(hda_opts, "trans", "none");
+                            qemu_opt_set(hda_opts, "trans", "none",
+                                         &error_abort);
                         }
                     }
                 }
@@ -3271,24 +3275,27 @@ int main(int argc, char **argv, char **envp)
                 writeout = qemu_opt_get(opts, "writeout");
                 if (writeout) {
 #ifdef CONFIG_SYNC_FILE_RANGE
-                    qemu_opt_set(fsdev, "writeout", writeout);
+                    qemu_opt_set(fsdev, "writeout", writeout, &error_abort);
 #else
                     fprintf(stderr, "writeout=immediate not supported on "
                             "this platform\n");
                     exit(1);
 #endif
                 }
-                qemu_opt_set(fsdev, "fsdriver", qemu_opt_get(opts, "fsdriver"));
-                qemu_opt_set(fsdev, "path", qemu_opt_get(opts, "path"));
+                qemu_opt_set(fsdev, "fsdriver",
+                             qemu_opt_get(opts, "fsdriver"), &error_abort);
+                qemu_opt_set(fsdev, "path", qemu_opt_get(opts, "path"),
+                             &error_abort);
                 qemu_opt_set(fsdev, "security_model",
-                             qemu_opt_get(opts, "security_model"));
+                             qemu_opt_get(opts, "security_model"),
+                             &error_abort);
                 socket = qemu_opt_get(opts, "socket");
                 if (socket) {
-                    qemu_opt_set(fsdev, "socket", socket);
+                    qemu_opt_set(fsdev, "socket", socket, &error_abort);
                 }
                 sock_fd = qemu_opt_get(opts, "sock_fd");
                 if (sock_fd) {
-                    qemu_opt_set(fsdev, "sock_fd", sock_fd);
+                    qemu_opt_set(fsdev, "sock_fd", sock_fd, &error_abort);
                 }
 
                 qemu_opt_set_bool(fsdev, "readonly",
@@ -3296,11 +3303,11 @@ int main(int argc, char **argv, char **envp)
                                   &error_abort);
                 device = qemu_opts_create(qemu_find_opts("device"), NULL, 0,
                                           &error_abort);
-                qemu_opt_set(device, "driver", "virtio-9p-pci");
+                qemu_opt_set(device, "driver", "virtio-9p-pci", &error_abort);
                 qemu_opt_set(device, "fsdev",
-                             qemu_opt_get(opts, "mount_tag"));
+                             qemu_opt_get(opts, "mount_tag"), &error_abort);
                 qemu_opt_set(device, "mount_tag",
-                             qemu_opt_get(opts, "mount_tag"));
+                             qemu_opt_get(opts, "mount_tag"), &error_abort);
                 break;
             }
             case QEMU_OPTION_virtfs_synth: {
@@ -3313,13 +3320,13 @@ int main(int argc, char **argv, char **envp)
                     fprintf(stderr, "duplicate option: %s\n", "virtfs_synth");
                     exit(1);
                 }
-                qemu_opt_set(fsdev, "fsdriver", "synth");
+                qemu_opt_set(fsdev, "fsdriver", "synth", &error_abort);
 
                 device = qemu_opts_create(qemu_find_opts("device"), NULL, 0,
                                           &error_abort);
-                qemu_opt_set(device, "driver", "virtio-9p-pci");
-                qemu_opt_set(device, "fsdev", "v_synth");
-                qemu_opt_set(device, "mount_tag", "v_synth");
+                qemu_opt_set(device, "driver", "virtio-9p-pci", &error_abort);
+                qemu_opt_set(device, "fsdev", "v_synth", &error_abort);
+                qemu_opt_set(device, "mount_tag", "v_synth", &error_abort);
                 break;
             }
             case QEMU_OPTION_serial:
-- 
1.9.3

Re: [Qemu-devel] [PATCH 06/13] QemuOpts: Drop qemu_opt_set(), rename qemu_opt_set_err(), fix use

[Eric Blake]

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

On 02/16/2015 07:44 AM, Markus Armbruster wrote:
> qemu_opt_set() is a wrapper around qemu_opt_set() that reports the
> error with qerror_report_err().
> 
> Most of its users assume the function can't fail.  Make them use
> qemu_opt_set_err() with &error_abort, so that should the assumption
> ever break, it'll break noisily.
> 
> Just two users remain, in util/qemu-config.c.  Switch them to
> qemu_opt_set_err() as well, then rename qemu_opt_set_err() to
> qemu_opt_set().

Might be better to split this into fixing the two users in one patch,
then doing the rename in another (since the rename touches more than two
callers).  On the other hand, it would represent more churn with
changing existing qemu_opt_set() to qemu_opt_set_err(,&error_abort) just
to change back to qemu_opt_set later.

> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block.c                  |  5 ++--
>  block/qcow2.c            |  3 +-
>  block/vvfat.c            |  2 +-
>  blockdev.c               | 17 +++++------
>  hw/pci/pci-hotplug-old.c |  2 +-
>  hw/usb/dev-network.c     |  4 +--
>  hw/usb/dev-storage.c     |  6 ++--
>  hw/watchdog/watchdog.c   |  2 +-
>  include/qemu/option.h    |  5 ++--
>  net/net.c                |  2 +-
>  qdev-monitor.c           |  6 ++--
>  qemu-char.c              | 58 +++++++++++++++++++-------------------
>  qemu-img.c               |  6 ++--
>  tests/test-qemu-opts.c   |  6 ++--
>  util/qemu-config.c       |  9 ++++--
>  util/qemu-option.c       | 22 +++------------
>  util/qemu-sockets.c      | 34 +++++++++++-----------
>  vl.c                     | 73 ++++++++++++++++++++++++++----------------------
>  18 files changed, 131 insertions(+), 131 deletions(-)

Also, the patch is fairly obvious that it is mechanical, so even if you
don't split, I could live with:

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

> +++ b/util/qemu-config.c
> @@ -219,6 +219,7 @@ void qemu_add_opts(QemuOptsList *list)
>  
>  int qemu_set_option(const char *str)
>  {
> +    Error *local_err = NULL;
>      char group[64], id[64], arg[64];
>      QemuOptsList *list;
>      QemuOpts *opts;
> @@ -242,7 +243,9 @@ int qemu_set_option(const char *str)
>          return -1;
>      }
>  
> -    if (qemu_opt_set(opts, arg, str+offset+1) == -1) {
> +    qemu_opt_set(opts, arg, str+offset+1, &local_err);

Worth adding whitespace around the 2 '+' operators while touching this?

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


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

Re: [Qemu-devel] [PATCH 06/13] QemuOpts: Drop qemu_opt_set(), rename qemu_opt_set_err(), fix use

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 02/16/2015 07:44 AM, Markus Armbruster wrote:
>> qemu_opt_set() is a wrapper around qemu_opt_set() that reports the
>> error with qerror_report_err().
>> 
>> Most of its users assume the function can't fail.  Make them use
>> qemu_opt_set_err() with &error_abort, so that should the assumption
>> ever break, it'll break noisily.
>> 
>> Just two users remain, in util/qemu-config.c.  Switch them to
>> qemu_opt_set_err() as well, then rename qemu_opt_set_err() to
>> qemu_opt_set().
>
> Might be better to split this into fixing the two users in one patch,
> then doing the rename in another (since the rename touches more than two
> callers).  On the other hand, it would represent more churn with
> changing existing qemu_opt_set() to qemu_opt_set_err(,&error_abort) just
> to change back to qemu_opt_set later.

Yeah, that's why I chose to do it in one go.

>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  block.c                  |  5 ++--
>>  block/qcow2.c            |  3 +-
>>  block/vvfat.c            |  2 +-
>>  blockdev.c               | 17 +++++------
>>  hw/pci/pci-hotplug-old.c |  2 +-
>>  hw/usb/dev-network.c     |  4 +--
>>  hw/usb/dev-storage.c     |  6 ++--
>>  hw/watchdog/watchdog.c   |  2 +-
>>  include/qemu/option.h    |  5 ++--
>>  net/net.c                |  2 +-
>>  qdev-monitor.c           |  6 ++--
>>  qemu-char.c              | 58 +++++++++++++++++++-------------------
>>  qemu-img.c               |  6 ++--
>>  tests/test-qemu-opts.c   |  6 ++--
>>  util/qemu-config.c       |  9 ++++--
>>  util/qemu-option.c       | 22 +++------------
>>  util/qemu-sockets.c      | 34 +++++++++++-----------
>>  vl.c | 73 ++++++++++++++++++++++++++----------------------
>>  18 files changed, 131 insertions(+), 131 deletions(-)
>
> Also, the patch is fairly obvious that it is mechanical, so even if you
> don't split, I could live with:
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
>> +++ b/util/qemu-config.c
>> @@ -219,6 +219,7 @@ void qemu_add_opts(QemuOptsList *list)
>>  
>>  int qemu_set_option(const char *str)
>>  {
>> +    Error *local_err = NULL;
>>      char group[64], id[64], arg[64];
>>      QemuOptsList *list;
>>      QemuOpts *opts;
>> @@ -242,7 +243,9 @@ int qemu_set_option(const char *str)
>>          return -1;
>>      }
>>  
>> -    if (qemu_opt_set(opts, arg, str+offset+1) == -1) {
>> +    qemu_opt_set(opts, arg, str+offset+1, &local_err);
>
> Worth adding whitespace around the 2 '+' operators while touching this?

Yes, if I need to respin or send a pull request.

Thanks!

[Qemu-devel] [PATCH 07/13] QemuOpts: Propagate errors through opts_do_parse()

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 util/qemu-option.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/util/qemu-option.c b/util/qemu-option.c
index 928b86f..8a0f69e 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -750,8 +750,8 @@ void qemu_opts_print(QemuOpts *opts, const char *sep)
     }
 }
 
-static int opts_do_parse(QemuOpts *opts, const char *params,
-                         const char *firstname, bool prepend)
+static void opts_do_parse(QemuOpts *opts, const char *params,
+                          const char *firstname, bool prepend, Error **errp)
 {
     char option[128], value[1024];
     const char *p,*pe,*pc;
@@ -789,21 +789,27 @@ static int opts_do_parse(QemuOpts *opts, const char *params,
             /* store and parse */
             opt_set(opts, option, value, prepend, &local_err);
             if (local_err) {
-                qerror_report_err(local_err);
-                error_free(local_err);
-                return -1;
+                error_propagate(errp, local_err);
+                return;
             }
         }
         if (*p != ',') {
             break;
         }
     }
-    return 0;
 }
 
 int qemu_opts_do_parse(QemuOpts *opts, const char *params, const char *firstname)
 {
-    return opts_do_parse(opts, params, firstname, false);
+    Error *err = NULL;
+
+    opts_do_parse(opts, params, firstname, false, &err);
+    if (err) {
+        qerror_report_err(err);
+        error_free(err);
+        return -1;
+    }
+    return 0;
 }
 
 static QemuOpts *opts_parse(QemuOptsList *list, const char *params,
@@ -843,7 +849,10 @@ static QemuOpts *opts_parse(QemuOptsList *list, const char *params,
         return NULL;
     }
 
-    if (opts_do_parse(opts, params, firstname, defaults) != 0) {
+    opts_do_parse(opts, params, firstname, defaults, &local_err);
+    if (local_err) {
+        qerror_report_err(local_err);
+        error_free(local_err);
         qemu_opts_del(opts);
         return NULL;
     }
-- 
1.9.3

Re: [Qemu-devel] [PATCH 07/13] QemuOpts: Propagate errors through opts_do_parse()

[Eric Blake]

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

On 02/16/2015 07:44 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  util/qemu-option.c | 25 +++++++++++++++++--------
>  1 file changed, 17 insertions(+), 8 deletions(-)
> 

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

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


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

[Qemu-devel] [PATCH 08/13] QemuOpts: Propagate errors through opts_parse()

[Markus Armbruster]

Since I'm touching qemu_opts_parse() anyway, write a function comment
for it.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 util/qemu-option.c | 29 ++++++++++++++++++++---------
 1 file changed, 20 insertions(+), 9 deletions(-)

diff --git a/util/qemu-option.c b/util/qemu-option.c
index 8a0f69e..95f242e 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -813,7 +813,7 @@ int qemu_opts_do_parse(QemuOpts *opts, const char *params, const char *firstname
 }
 
 static QemuOpts *opts_parse(QemuOptsList *list, const char *params,
-                            int permit_abbrev, bool defaults)
+                            int permit_abbrev, bool defaults, Error **errp)
 {
     const char *firstname;
     char value[1024], *id = NULL;
@@ -842,17 +842,13 @@ static QemuOpts *opts_parse(QemuOptsList *list, const char *params,
     assert(!defaults || list->merge_lists);
     opts = qemu_opts_create(list, id, !defaults, &local_err);
     if (opts == NULL) {
-        if (local_err) {
-            qerror_report_err(local_err);
-            error_free(local_err);
-        }
+        error_propagate(errp, local_err);
         return NULL;
     }
 
     opts_do_parse(opts, params, firstname, defaults, &local_err);
     if (local_err) {
-        qerror_report_err(local_err);
-        error_free(local_err);
+        error_propagate(errp, local_err);
         qemu_opts_del(opts);
         return NULL;
     }
@@ -860,10 +856,25 @@ static QemuOpts *opts_parse(QemuOptsList *list, const char *params,
     return opts;
 }
 
+/**
+ * Create a QemuOpts in @list and with options parsed from @params.
+ * If @permit_abbrev, the first key=value in @params may omit key=,
+ * and is treated as if key was @list->implied_opt_name.
+ * Report errors with qerror_report_err().
+ * Return the new QemuOpts on success, null pointer on error.
+ */
 QemuOpts *qemu_opts_parse(QemuOptsList *list, const char *params,
                           int permit_abbrev)
 {
-    return opts_parse(list, params, permit_abbrev, false);
+    Error *err = NULL;
+    QemuOpts *opts;
+
+    opts = opts_parse(list, params, permit_abbrev, false, &err);
+    if (!opts) {
+        qerror_report_err(err);
+        error_free(err);
+    }
+    return opts;
 }
 
 void qemu_opts_set_defaults(QemuOptsList *list, const char *params,
@@ -871,7 +882,7 @@ void qemu_opts_set_defaults(QemuOptsList *list, const char *params,
 {
     QemuOpts *opts;
 
-    opts = opts_parse(list, params, permit_abbrev, true);
+    opts = opts_parse(list, params, permit_abbrev, true, NULL);
     assert(opts);
 }
 
-- 
1.9.3

Re: [Qemu-devel] [PATCH 08/13] QemuOpts: Propagate errors through opts_parse()

[Eric Blake]

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

On 02/16/2015 07:44 AM, Markus Armbruster wrote:
> Since I'm touching qemu_opts_parse() anyway, write a function comment
> for it.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  util/qemu-option.c | 29 ++++++++++++++++++++---------
>  1 file changed, 20 insertions(+), 9 deletions(-)
> 

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

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


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

[Qemu-devel] [PATCH 09/13] qemu-img: Suppress unhelpful extra errors in convert, amend

[Markus Armbruster]

img_convert() and img_amend() use qemu_opts_do_parse(), which reports
errors with qerror_report_err().  Its error messages aren't helpful
here, the caller reports one that actually makes sense.  Reproducer:

    $ qemu-img convert -o backing_format=raw in.img out.img
    qemu-img: Invalid parameter 'backing_format'
    qemu-img: Invalid options for file format 'raw'

To fix, propagate errors through qemu_opts_do_parse().  This lifts the
error reporting into callers.  Drop it from img_convert() and
img_amend(), keep it in qemu_chr_parse_compat(), bdrv_img_create().

Since I'm touching qemu_opts_do_parse() anyway, write a function
comment for it.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block.c               |  5 ++++-
 include/qemu/option.h |  3 ++-
 qemu-char.c           | 10 ++++++++--
 qemu-img.c            | 25 +++++++++++++++++--------
 util/qemu-option.c    | 19 +++++++++----------
 5 files changed, 40 insertions(+), 22 deletions(-)

diff --git a/block.c b/block.c
index 71d3bcc..e596330 100644
--- a/block.c
+++ b/block.c
@@ -5650,7 +5650,10 @@ void bdrv_img_create(const char *filename, const char *fmt,
 
     /* Parse -o options */
     if (options) {
-        if (qemu_opts_do_parse(opts, options, NULL) != 0) {
+        qemu_opts_do_parse(opts, options, NULL, &local_err);
+        if (local_err) {
+            error_report_err(local_err);
+            local_err = NULL;
             error_setg(errp, "Invalid options for file format '%s'", fmt);
             goto out;
         }
diff --git a/include/qemu/option.h b/include/qemu/option.h
index a41ee92..f88b545 100644
--- a/include/qemu/option.h
+++ b/include/qemu/option.h
@@ -115,7 +115,8 @@ const char *qemu_opts_id(QemuOpts *opts);
 void qemu_opts_set_id(QemuOpts *opts, char *id);
 void qemu_opts_del(QemuOpts *opts);
 void qemu_opts_validate(QemuOpts *opts, const QemuOptDesc *desc, Error **errp);
-int qemu_opts_do_parse(QemuOpts *opts, const char *params, const char *firstname);
+void qemu_opts_do_parse(QemuOpts *opts, const char *params,
+                        const char *firstname, Error **errp);
 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);
diff --git a/qemu-char.c b/qemu-char.c
index 3ed46ee..a405d76 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -3379,8 +3379,11 @@ QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename)
         qemu_opt_set(opts, "host", host, &error_abort);
         qemu_opt_set(opts, "port", port, &error_abort);
         if (p[pos] == ',') {
-            if (qemu_opts_do_parse(opts, p+pos+1, NULL) != 0)
+            qemu_opts_do_parse(opts, p+pos+1, NULL, &local_err);
+            if (local_err) {
+                error_report_err(local_err);
                 goto fail;
+            }
         }
         if (strstart(filename, "telnet:", &p))
             qemu_opt_set(opts, "telnet", "on", &error_abort);
@@ -3411,8 +3414,11 @@ QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename)
     }
     if (strstart(filename, "unix:", &p)) {
         qemu_opt_set(opts, "backend", "socket", &error_abort);
-        if (qemu_opts_do_parse(opts, p, "path") != 0)
+        qemu_opts_do_parse(opts, p, "path", &local_err);
+        if (local_err) {
+            error_report_err(local_err);
             goto fail;
+        }
         return opts;
     }
     if (strstart(filename, "/dev/parport", NULL) ||
diff --git a/qemu-img.c b/qemu-img.c
index cb99cc2..eadebd7 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1554,10 +1554,14 @@ static int img_convert(int argc, char **argv)
     create_opts = qemu_opts_append(create_opts, proto_drv->create_opts);
 
     opts = qemu_opts_create(create_opts, NULL, 0, &error_abort);
-    if (options && qemu_opts_do_parse(opts, options, NULL)) {
-        error_report("Invalid options for file format '%s'", out_fmt);
-        ret = -1;
-        goto out;
+    if (options) {
+        qemu_opts_do_parse(opts, options, NULL, &local_err);
+        if (local_err) {
+            error_report("Invalid options for file format '%s'", out_fmt);
+            error_free(local_err);
+            ret = -1;
+            goto out;
+        }
     }
 
     qemu_opt_set_number(opts, BLOCK_OPT_SIZE, total_sectors * 512,
@@ -2897,6 +2901,7 @@ static void amend_status_cb(BlockDriverState *bs,
 
 static int img_amend(int argc, char **argv)
 {
+    Error *err = NULL;
     int c, ret = 0;
     char *options = NULL;
     QemuOptsList *create_opts = NULL;
@@ -3002,10 +3007,14 @@ static int img_amend(int argc, char **argv)
 
     create_opts = qemu_opts_append(create_opts, bs->drv->create_opts);
     opts = qemu_opts_create(create_opts, NULL, 0, &error_abort);
-    if (options && qemu_opts_do_parse(opts, options, NULL)) {
-        error_report("Invalid options for file format '%s'", fmt);
-        ret = -1;
-        goto out;
+    if (options) {
+        qemu_opts_do_parse(opts, options, NULL, &err);
+        if (err) {
+            error_report("Invalid options for file format '%s'", fmt);
+            error_free(err);
+            ret = -1;
+            goto out;
+        }
     }
 
     /* In case the driver does not call amend_status_cb() */
diff --git a/util/qemu-option.c b/util/qemu-option.c
index 95f242e..ea49140 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -799,17 +799,16 @@ static void opts_do_parse(QemuOpts *opts, const char *params,
     }
 }
 
-int qemu_opts_do_parse(QemuOpts *opts, const char *params, const char *firstname)
+/**
+ * Store into @opts options parsed from @params.
+ * If @firstname is non-null, the first key=value in @params may omit
+ * key=, and is treated as if key was @firstname.
+ * On error, store an error object through @errp if non-null.
+ */
+void qemu_opts_do_parse(QemuOpts *opts, const char *params,
+                       const char *firstname, Error **errp)
 {
-    Error *err = NULL;
-
-    opts_do_parse(opts, params, firstname, false, &err);
-    if (err) {
-        qerror_report_err(err);
-        error_free(err);
-        return -1;
-    }
-    return 0;
+    opts_do_parse(opts, params, firstname, false, errp);
 }
 
 static QemuOpts *opts_parse(QemuOptsList *list, const char *params,
-- 
1.9.3

Re: [Qemu-devel] [PATCH 09/13] qemu-img: Suppress unhelpful extra errors in convert, amend

[Eric Blake]

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

On 02/16/2015 07:44 AM, Markus Armbruster wrote:
> img_convert() and img_amend() use qemu_opts_do_parse(), which reports
> errors with qerror_report_err().  Its error messages aren't helpful
> here, the caller reports one that actually makes sense.  Reproducer:
> 
>     $ qemu-img convert -o backing_format=raw in.img out.img
>     qemu-img: Invalid parameter 'backing_format'
>     qemu-img: Invalid options for file format 'raw'
> 
> To fix, propagate errors through qemu_opts_do_parse().  This lifts the
> error reporting into callers.  Drop it from img_convert() and
> img_amend(), keep it in qemu_chr_parse_compat(), bdrv_img_create().
> 
> Since I'm touching qemu_opts_do_parse() anyway, write a function
> comment for it.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block.c               |  5 ++++-
>  include/qemu/option.h |  3 ++-
>  qemu-char.c           | 10 ++++++++--
>  qemu-img.c            | 25 +++++++++++++++++--------
>  util/qemu-option.c    | 19 +++++++++----------
>  5 files changed, 40 insertions(+), 22 deletions(-)
> 

> -int qemu_opts_do_parse(QemuOpts *opts, const char *params, const char *firstname)
> +/**
> + * Store into @opts options parsed from @params.

Reads a bit awkwardly; maybe:

Store options parsed from @params into @opts.

> + * If @firstname is non-null, the first key=value in @params may omit
> + * key=, and is treated as if key was @firstname.
> + * On error, store an error object through @errp if non-null.
> + */
> +void qemu_opts_do_parse(QemuOpts *opts, const char *params,
> +                       const char *firstname, Error **errp)
>  {

Up to you if you want to improve that; either way, it's in a comment, so
it doesn't affect correctness, so:

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

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


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

Re: [Qemu-devel] [PATCH 09/13] qemu-img: Suppress unhelpful extra errors in convert, amend

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 02/16/2015 07:44 AM, Markus Armbruster wrote:
>> img_convert() and img_amend() use qemu_opts_do_parse(), which reports
>> errors with qerror_report_err().  Its error messages aren't helpful
>> here, the caller reports one that actually makes sense.  Reproducer:
>> 
>>     $ qemu-img convert -o backing_format=raw in.img out.img
>>     qemu-img: Invalid parameter 'backing_format'
>>     qemu-img: Invalid options for file format 'raw'
>> 
>> To fix, propagate errors through qemu_opts_do_parse().  This lifts the
>> error reporting into callers.  Drop it from img_convert() and
>> img_amend(), keep it in qemu_chr_parse_compat(), bdrv_img_create().
>> 
>> Since I'm touching qemu_opts_do_parse() anyway, write a function
>> comment for it.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  block.c               |  5 ++++-
>>  include/qemu/option.h |  3 ++-
>>  qemu-char.c           | 10 ++++++++--
>>  qemu-img.c            | 25 +++++++++++++++++--------
>>  util/qemu-option.c    | 19 +++++++++----------
>>  5 files changed, 40 insertions(+), 22 deletions(-)
>> 
>
>> -int qemu_opts_do_parse(QemuOpts *opts, const char *params, const char *firstname)
>> +/**
>> + * Store into @opts options parsed from @params.
>
> Reads a bit awkwardly; maybe:
>
> Store options parsed from @params into @opts.

Mentioning parameters in order.  Old habits die hard...

>> + * If @firstname is non-null, the first key=value in @params may omit
>> + * key=, and is treated as if key was @firstname.
>> + * On error, store an error object through @errp if non-null.
>> + */
>> +void qemu_opts_do_parse(QemuOpts *opts, const char *params,
>> +                       const char *firstname, Error **errp)
>>  {
>
> Up to you if you want to improve that; either way, it's in a comment, so
> it doesn't affect correctness, so:
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

[Qemu-devel] [PATCH 10/13] block: Simplify setting numeric options

[Markus Armbruster]

Don't convert numbers to strings for use with qemu_opt_set(), simply
use qemu_opt_set_number() instead.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 blockdev.c |  4 +---
 vl.c       | 13 ++++++-------
 2 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index f4c6e92..48c668c 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -180,7 +180,6 @@ QemuOpts *drive_add(BlockInterfaceType type, int index, const char *file,
                     const char *optstr)
 {
     QemuOpts *opts;
-    char buf[32];
 
     opts = drive_def(optstr);
     if (!opts) {
@@ -190,8 +189,7 @@ QemuOpts *drive_add(BlockInterfaceType type, int index, const char *file,
         qemu_opt_set(opts, "if", if_name[type], &error_abort);
     }
     if (index >= 0) {
-        snprintf(buf, sizeof(buf), "%d", index);
-        qemu_opt_set(opts, "index", buf, &error_abort);
+        qemu_opt_set_number(opts, "index", index, &error_abort);
     }
     if (file)
         qemu_opt_set(opts, "file", file, &error_abort);
diff --git a/vl.c b/vl.c
index a80d36f..dcb359a 100644
--- a/vl.c
+++ b/vl.c
@@ -2971,13 +2971,12 @@ int main(int argc, char **argv, char **envp)
                         exit(1);
                     }
                     if (hda_opts != NULL) {
-                        char num[16];
-                        snprintf(num, sizeof(num), "%d", cyls);
-                        qemu_opt_set(hda_opts, "cyls", num, &error_abort);
-                        snprintf(num, sizeof(num), "%d", heads);
-                        qemu_opt_set(hda_opts, "heads", num, &error_abort);
-                        snprintf(num, sizeof(num), "%d", secs);
-                        qemu_opt_set(hda_opts, "secs", num, &error_abort);
+                        qemu_opt_set_number(hda_opts, "cyls", cyls,
+                                            &error_abort);
+                        qemu_opt_set_number(hda_opts, "heads", heads,
+                                            &error_abort);
+                        qemu_opt_set_number(hda_opts, "secs", secs,
+                                            &error_abort);
                         if (translation == BIOS_ATA_TRANSLATION_LARGE) {
                             qemu_opt_set(hda_opts, "trans", "large",
                                          &error_abort);
-- 
1.9.3

Re: [Qemu-devel] [PATCH 10/13] block: Simplify setting numeric options

[Eric Blake]

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

On 02/16/2015 07:44 AM, Markus Armbruster wrote:
> Don't convert numbers to strings for use with qemu_opt_set(), simply
> use qemu_opt_set_number() instead.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  blockdev.c |  4 +---
>  vl.c       | 13 ++++++-------
>  2 files changed, 7 insertions(+), 10 deletions(-)
> 

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

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


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

[Qemu-devel] [PATCH 11/13] qemu-sockets: Simplify setting numeric and boolean options

[Markus Armbruster]

Don't convert numbers or bools to strings for use with qemu_opt_set(),
simply use qemu_opt_set_number() or qemu_opt_set_bool() instead.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 util/qemu-sockets.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index e908523..87c9bc6 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -199,13 +199,13 @@ listen:
         freeaddrinfo(res);
         return -1;
     }
-    snprintf(uport, sizeof(uport), "%d", inet_getport(e) - port_offset);
     qemu_opt_set(opts, "host", uaddr, &error_abort);
-    qemu_opt_set(opts, "port", uport, &error_abort);
-    qemu_opt_set(opts, "ipv6", (e->ai_family == PF_INET6) ? "on" : "off",
-                 &error_abort);
-    qemu_opt_set(opts, "ipv4", (e->ai_family != PF_INET6) ? "on" : "off",
-                 &error_abort);
+    qemu_opt_set_number(opts, "port", inet_getport(e) - port_offset,
+                        &error_abort);
+    qemu_opt_set_bool(opts, "ipv6", e->ai_family == PF_INET6,
+                      &error_abort);
+    qemu_opt_set_bool(opts, "ipv4", e->ai_family != PF_INET6,
+                      &error_abort);
     freeaddrinfo(res);
     return slisten;
 }
@@ -586,9 +586,7 @@ static void inet_addr_to_opts(QemuOpts *opts, const InetSocketAddress *addr)
         qemu_opt_set_bool(opts, "ipv6", ipv6, &error_abort);
     }
     if (addr->has_to) {
-        char to[20];
-        snprintf(to, sizeof(to), "%d", addr->to);
-        qemu_opt_set(opts, "to", to, &error_abort);
+        qemu_opt_set_number(opts, "to", addr->to, &error_abort);
     }
     qemu_opt_set(opts, "host", addr->host, &error_abort);
     qemu_opt_set(opts, "port", addr->port, &error_abort);
-- 
1.9.3

Re: [Qemu-devel] [PATCH 11/13] qemu-sockets: Simplify setting numeric and boolean options

[Eric Blake]

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

On 02/16/2015 07:44 AM, Markus Armbruster wrote:
> Don't convert numbers or bools to strings for use with qemu_opt_set(),
> simply use qemu_opt_set_number() or qemu_opt_set_bool() instead.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  util/qemu-sockets.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
> 

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

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


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

[Qemu-devel] [PATCH 12/13] pc: Use qemu_opt_set() instead of qemu_opts_parse()

[Markus Armbruster]

Less code, same result.

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

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 05008cb..51b4cf5 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1137,15 +1137,11 @@ void pc_acpi_init(const char *default_dsdt)
     if (filename == NULL) {
         fprintf(stderr, "WARNING: failed to find %s\n", default_dsdt);
     } else {
-        char *arg;
-        QemuOpts *opts;
+        QemuOpts *opts = qemu_opts_create(qemu_find_opts("acpi"), NULL, 0,
+                                          &error_abort);
         Error *err = NULL;
 
-        arg = g_strdup_printf("file=%s", filename);
-
-        /* creates a deep copy of "arg" */
-        opts = qemu_opts_parse(qemu_find_opts("acpi"), arg, 0);
-        g_assert(opts != NULL);
+        qemu_opt_set(opts, "file", filename, &error_abort);
 
         acpi_table_add_builtin(opts, &err);
         if (err) {
@@ -1153,7 +1149,6 @@ void pc_acpi_init(const char *default_dsdt)
                          error_get_pretty(err));
             error_free(err);
         }
-        g_free(arg);
         g_free(filename);
     }
 }
-- 
1.9.3

Re: [Qemu-devel] [PATCH 12/13] pc: Use qemu_opt_set() instead of qemu_opts_parse()

[Eric Blake]

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

On 02/16/2015 07:44 AM, Markus Armbruster wrote:
> Less code, same result.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/i386/pc.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 

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

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


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

[Qemu-devel] [PATCH 13/13] qtest: Use qemu_opt_set() instead of qemu_opts_parse()

[Markus Armbruster]

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

diff --git a/qtest.c b/qtest.c
index 2bca04e..8d1e66c 100644
--- a/qtest.c
+++ b/qtest.c
@@ -520,16 +520,13 @@ static void qtest_event(void *opaque, int event)
     }
 }
 
-static void configure_qtest_icount(const char *options)
+static int qtest_init_accel(MachineState *ms)
 {
-    QemuOpts *opts  = qemu_opts_parse(qemu_find_opts("icount"), options, 1);
+    QemuOpts *opts = qemu_opts_create(qemu_find_opts("icount"), NULL, 0,
+                                      &error_abort);
+    qemu_opt_set(opts, "shift", "0", &error_abort);
     configure_icount(opts, &error_abort);
     qemu_opts_del(opts);
-}
-
-static int qtest_init_accel(MachineState *ms)
-{
-    configure_qtest_icount("0");
     return 0;
 }
 
-- 
1.9.3

Re: [Qemu-devel] [PATCH 13/13] qtest: Use qemu_opt_set() instead of qemu_opts_parse()

[Eric Blake]

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

On 02/16/2015 07:44 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  qtest.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 

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

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


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

Re: [Qemu-devel] [PATCH 00/13] QemuOpts: Convert various setters to Error

[Markus Armbruster]

Forgot to mention: based on my "[PATCH v2 00/10] Clean up around
error_get_pretty(), qerror_report_err()" series, because it needs its
"[PATCH v2 01/10] error: New convenience function error_report_err()".