[Qemu-devel] [RFC PATCH v1 0/5] Add error_abort and associated cleanups

qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed

* [Qemu-devel] [RFC PATCH v1 0/5] Add error_abort and associated cleanups
@ 2013-12-03  5:49 Peter Crosthwaite
  2013-12-03  5:49 ` [Qemu-devel] [RFC PATCH v1 1/5] error: Add error_abort Peter Crosthwaite
                   ` (5 more replies)
  0 siblings, 6 replies; 25+ messages in thread
From: Peter Crosthwaite @ 2013-12-03  5:49 UTC (permalink / raw)
  To: qemu-devel; +Cc: imammedo, afaerber, armbru, pbonzini

Following our discussion RE self asserting API calls, here is a spin of
my proposal. This series obsoletes the need for _nofail variants for
Error ** accepting APIs. Is also greately reduces the verbosity of calls
sites that are currently asserting against errors.

Patch 1 is the main event - addition of error_abort. The following
patches then cleanup uses of _nofail and assert_no_error().

To give it a smoke test, I introduce a (critical) bug into QOM:

--- a/qom/object.c
+++ b/qom/object.c
@@ -797,7 +797,7 @@ void object_property_set(Object *obj, Visitor *v,
const char *name,
         return;
     }

-    if (!prop->set) {
+    if (prop->set) {
         error_set(errp, QERR_PERMISSION_DENIED);
     } else {
         prop->set(obj, v, prop->opaque, name, errp);

And run QEMU:

$ gdb --args ./microblazeel-softmmu/qemu-system-microblazeel -M petalogix-ml605 -nographic

Without application of this series, the bug manifests as:

qemu-system-microblazeel: Insufficient permission to perform this operation

Program received signal SIGABRT, Aborted.
(gdb) bt
0  0x00007ffff55f7425 in __GI_raise (sig=<optimized out>) at ../nptl/sysdeps/unix/sysv/linux/raise.c:64
1  0x00007ffff55fab8b in __GI_abort () at abort.c:91
2  0x00005555557073da in assert_no_error (err=<optimized out>) at qobject/qerror.c:128
3  0x0000555555615159 in qdev_property_add_static (dev=0x555555e9e6a0, prop=0x5555559ea4c0, errp=0x7fffffffe3c0) at hw/core/qdev.c:666
4  0x0000555555615355 in device_initfn (obj=<optimized out>) at hw/core/qdev.c:744

With this series, we now get a the full backtrace into the QOM API when
the assertion occurs (note stack frames 2-4 giving visibility into the
broken QOM API):

qemu-system-microblazeel: Insufficient permission to perform this operation

Program received signal SIGABRT, Aborted.
(gdb) bt
0  0x00007ffff55f7425 in __GI_raise (sig=<optimized out>) at ../nptl/sysdeps/unix/sysv/linux/raise.c:64
1  0x00007ffff55fab8b in __GI_abort () at abort.c:91
2  0x000055555570c5a4 in error_set (errp=0x555555e1b5f0, err_class=ERROR_CLASS_GENERIC_ERROR, fmt=0x55555571f5c0 "Insufficient permission to perform this operation") at util/error.c:47
3  0x00005555556778e5 in object_property_set_qobject (obj=0x555555e9e6a0, value=<optimized out>, name=0x55555574e7a6 "xlnx.base-vectors", errp=0x555555e1b5f0) at qom/qom-qobject.c:24
4  0x000055555567674d in object_property_set_int (obj=0x555555e9e6a0, value=<optimized out>, name=0x55555574e7a6 "xlnx.base-vectors", errp=0x555555e1b5f0) at qom/object.c:898
5  0x0000555555615192 in qdev_property_add_static (dev=0x555555e9e6a0, prop=0x5555559ea4c0, errp=0x555555e1b5f0) at hw/core/qdev.c:664
6  0x000055555561534f in device_initfn (obj=<optimized out>) at hw/core/qdev.c:741


Peter Crosthwaite (5):
  error: Add error_abort
  hw: Remove assert_no_error usages
  target-i386: Remove assert_no_error usage
  qemu-option: Remove qemu_opts_create_nofail
  qerror: Remove assert_no_error()

 block/blkdebug.c                 |  2 +-
 block/blkverify.c                |  2 +-
 block/curl.c                     |  2 +-
 block/gluster.c                  |  2 +-
 block/iscsi.c                    |  2 +-
 block/nbd.c                      |  2 +-
 block/qcow2.c                    |  2 +-
 block/raw-posix.c                |  2 +-
 block/raw-win32.c                |  4 ++--
 block/rbd.c                      |  2 +-
 block/sheepdog.c                 |  2 +-
 block/vvfat.c                    |  2 +-
 blockdev.c                       |  6 ++++--
 hw/core/qdev-properties-system.c |  8 ++------
 hw/core/qdev-properties.c        | 40 ++++++++++------------------------------
 hw/core/qdev.c                   | 28 +++++++---------------------
 hw/dma/xilinx_axidma.c           | 13 ++++---------
 hw/net/xilinx_axienet.c          | 13 ++++---------
 hw/watchdog/watchdog.c           |  3 ++-
 include/hw/xilinx.h              | 14 ++++----------
 include/qapi/error.h             |  6 ++++++
 include/qapi/qmp/qerror.h        |  1 -
 include/qemu/option.h            |  1 -
 qdev-monitor.c                   |  2 +-
 qemu-img.c                       |  2 +-
 qobject/qerror.c                 |  8 --------
 target-i386/cpu.c                |  4 +---
 util/error.c                     | 22 +++++++++++++++++++++-
 util/qemu-config.c               |  2 +-
 util/qemu-option.c               |  9 ---------
 util/qemu-sockets.c              | 18 +++++++++---------
 vl.c                             | 17 ++++++++++-------
 32 files changed, 100 insertions(+), 143 deletions(-)

-- 
1.8.4.4

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [Qemu-devel] [RFC PATCH v1 1/5] error: Add error_abort
  2013-12-03  5:49 [Qemu-devel] [RFC PATCH v1 0/5] Add error_abort and associated cleanups Peter Crosthwaite
@ 2013-12-03  5:49 ` Peter Crosthwaite
  2013-12-03  5:50 ` [Qemu-devel] [RFC PATCH v1 2/5] hw: Remove assert_no_error usages Peter Crosthwaite
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 25+ messages in thread
From: Peter Crosthwaite @ 2013-12-03  5:49 UTC (permalink / raw)
  To: qemu-devel; +Cc: imammedo, afaerber, armbru, pbonzini

Add a special Error * that can be passed to error handling APIs to
signal that any errors are fatal and should abort QEMU. There are two
advantages to this:

- allows for brevity when wishing to assert success of Error **
  accepting APIs. No need for this pattern:
        Error * local_err = NULL;
        api_call(foo, bar, &local_err);
        assert_no_error(local_err);
  This also removes the need for _nofail variants of APIs with
  asserting call sites now reduced to 1LOC.
- SIGABRT happens from within the offending API. When a fatal error
  occurs in an API call (when the caller is asserting sucess) failure
  often means the API itself is broken. With the abort happening in the
  API call now, the stack frames into the call are available at debug
  time. In the assert_no_error scheme the abort happens after the fact.

The exact semantic is that when an error is raised, if the argument
Error ** matches &error_abort, then the abort occurs immediately. The
error messaged is reported.

For error_propagate, if the destination error is &error_abort, then
the abort happens at propagation time.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 include/qapi/error.h |  6 ++++++
 util/error.c         | 22 +++++++++++++++++++++-
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/include/qapi/error.h b/include/qapi/error.h
index 7d4c696..c0f0c3b 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -95,4 +95,10 @@ void error_propagate(Error **dst_err, Error *local_err);
  */
 void error_free(Error *err);
 
+/**
+ * If passed to error_set and friends, abort().
+ */
+
+extern Error *error_abort;
+
 #endif
diff --git a/util/error.c b/util/error.c
index ec0faa6..856c7db 100644
--- a/util/error.c
+++ b/util/error.c
@@ -23,6 +23,8 @@ struct Error
     ErrorClass err_class;
 };
 
+Error *error_abort;
+
 void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
 {
     Error *err;
@@ -40,6 +42,11 @@ void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
     va_end(ap);
     err->err_class = err_class;
 
+    if (errp == &error_abort) {
+        error_report("%s", error_get_pretty(err));
+        abort();
+    }
+
     *errp = err;
 }
 
@@ -68,6 +75,11 @@ void error_set_errno(Error **errp, int os_errno, ErrorClass err_class,
     va_end(ap);
     err->err_class = err_class;
 
+    if (errp == &error_abort) {
+        error_report("%s", error_get_pretty(err));
+        abort();
+    }
+
     *errp = err;
 }
 
@@ -106,6 +118,11 @@ void error_set_win32(Error **errp, int win32_err, ErrorClass err_class,
     va_end(ap);
     err->err_class = err_class;
 
+    if (errp == &error_abort) {
+        error_report("%s", error_get_pretty(err));
+        abort();
+    }
+
     *errp = err;
 }
 
@@ -147,7 +164,10 @@ void error_free(Error *err)
 
 void error_propagate(Error **dst_err, Error *local_err)
 {
-    if (dst_err && !*dst_err) {
+    if (local_err && dst_err == &error_abort) {
+        error_report("%s", error_get_pretty(local_err));
+        abort();
+    } else if (dst_err && !*dst_err) {
         *dst_err = local_err;
     } else if (local_err) {
         error_free(local_err);
-- 
1.8.4.4

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [Qemu-devel] [RFC PATCH v1 2/5] hw: Remove assert_no_error usages
  2013-12-03  5:49 [Qemu-devel] [RFC PATCH v1 0/5] Add error_abort and associated cleanups Peter Crosthwaite
  2013-12-03  5:49 ` [Qemu-devel] [RFC PATCH v1 1/5] error: Add error_abort Peter Crosthwaite
@ 2013-12-03  5:50 ` Peter Crosthwaite
  2013-12-03  9:35   ` Markus Armbruster
  2013-12-03  5:51 ` [Qemu-devel] [RFC PATCH v1 3/5] target-i386: Remove assert_no_error usage Peter Crosthwaite
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 25+ messages in thread
From: Peter Crosthwaite @ 2013-12-03  5:50 UTC (permalink / raw)
  To: qemu-devel; +Cc: imammedo, afaerber, armbru, pbonzini

Replace assert_no_error() usages with the error_abort system.
&error_abort is passed into API calls to signal to the Error sub-system
that any errors are fatal. Removes need for caller assertions.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/core/qdev-properties-system.c |  8 ++------
 hw/core/qdev-properties.c        | 40 ++++++++++------------------------------
 hw/core/qdev.c                   | 28 +++++++---------------------
 hw/dma/xilinx_axidma.c           | 13 ++++---------
 hw/net/xilinx_axienet.c          | 13 ++++---------
 include/hw/xilinx.h              | 14 ++++----------
 6 files changed, 31 insertions(+), 85 deletions(-)

diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index 729efa8..3f29b49 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -352,21 +352,17 @@ void qdev_prop_set_drive_nofail(DeviceState *dev, const char *name,
 void qdev_prop_set_chr(DeviceState *dev, const char *name,
                        CharDriverState *value)
 {
-    Error *errp = NULL;
     assert(!value || value->label);
     object_property_set_str(OBJECT(dev),
-                            value ? value->label : "", name, &errp);
-    assert_no_error(errp);
+                            value ? value->label : "", name, &error_abort);
 }
 
 void qdev_prop_set_netdev(DeviceState *dev, const char *name,
                           NetClientState *value)
 {
-    Error *errp = NULL;
     assert(!value || value->name);
     object_property_set_str(OBJECT(dev),
-                            value ? value->name : "", name, &errp);
-    assert_no_error(errp);
+                            value ? value->name : "", name, &error_abort);
 }
 
 void qdev_set_nic_properties(DeviceState *dev, NICInfo *nd)
diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index dc8ae69..b949f0e 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -1003,73 +1003,55 @@ void qdev_prop_parse(DeviceState *dev, const char *name, const char *value,
 
 void qdev_prop_set_bit(DeviceState *dev, const char *name, bool value)
 {
-    Error *errp = NULL;
-    object_property_set_bool(OBJECT(dev), value, name, &errp);
-    assert_no_error(errp);
+    object_property_set_bool(OBJECT(dev), value, name, &error_abort);
 }
 
 void qdev_prop_set_uint8(DeviceState *dev, const char *name, uint8_t value)
 {
-    Error *errp = NULL;
-    object_property_set_int(OBJECT(dev), value, name, &errp);
-    assert_no_error(errp);
+    object_property_set_int(OBJECT(dev), value, name, &error_abort);
 }
 
 void qdev_prop_set_uint16(DeviceState *dev, const char *name, uint16_t value)
 {
-    Error *errp = NULL;
-    object_property_set_int(OBJECT(dev), value, name, &errp);
-    assert_no_error(errp);
+    object_property_set_int(OBJECT(dev), value, name, &error_abort);
 }
 
 void qdev_prop_set_uint32(DeviceState *dev, const char *name, uint32_t value)
 {
-    Error *errp = NULL;
-    object_property_set_int(OBJECT(dev), value, name, &errp);
-    assert_no_error(errp);
+    object_property_set_int(OBJECT(dev), value, name, &error_abort);
 }
 
 void qdev_prop_set_int32(DeviceState *dev, const char *name, int32_t value)
 {
-    Error *errp = NULL;
-    object_property_set_int(OBJECT(dev), value, name, &errp);
-    assert_no_error(errp);
+    object_property_set_int(OBJECT(dev), value, name, &error_abort);
 }
 
 void qdev_prop_set_uint64(DeviceState *dev, const char *name, uint64_t value)
 {
-    Error *errp = NULL;
-    object_property_set_int(OBJECT(dev), value, name, &errp);
-    assert_no_error(errp);
+    object_property_set_int(OBJECT(dev), value, name, &error_abort);
 }
 
 void qdev_prop_set_string(DeviceState *dev, const char *name, const char *value)
 {
-    Error *errp = NULL;
-    object_property_set_str(OBJECT(dev), value, name, &errp);
-    assert_no_error(errp);
+    object_property_set_str(OBJECT(dev), value, name, &error_abort);
 }
 
 void qdev_prop_set_macaddr(DeviceState *dev, const char *name, uint8_t *value)
 {
-    Error *errp = NULL;
     char str[2 * 6 + 5 + 1];
     snprintf(str, sizeof(str), "%02x:%02x:%02x:%02x:%02x:%02x",
              value[0], value[1], value[2], value[3], value[4], value[5]);
 
-    object_property_set_str(OBJECT(dev), str, name, &errp);
-    assert_no_error(errp);
+    object_property_set_str(OBJECT(dev), str, name, &error_abort);
 }
 
 void qdev_prop_set_enum(DeviceState *dev, const char *name, int value)
 {
     Property *prop;
-    Error *errp = NULL;
 
     prop = qdev_prop_find(dev, name);
     object_property_set_str(OBJECT(dev), prop->info->enum_table[value],
-                            name, &errp);
-    assert_no_error(errp);
+                            name, &error_abort);
 }
 
 void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value)
@@ -1161,12 +1143,10 @@ static void set_size(Object *obj, Visitor *v, void *opaque,
 static int parse_size(DeviceState *dev, Property *prop, const char *str)
 {
     uint64_t *ptr = qdev_get_prop_ptr(dev, prop);
-    Error *errp = NULL;
 
     if (str != NULL) {
-        parse_option_size(prop->name, str, ptr, &errp);
+        parse_option_size(prop->name, str, ptr, &error_abort);
     }
-    assert_no_error(errp);
     return 0;
 }
 
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index e374a93..7d869fc 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -656,14 +656,13 @@ void qdev_property_add_static(DeviceState *dev, Property *prop,
     }
 
     if (prop->qtype == QTYPE_QBOOL) {
-        object_property_set_bool(obj, prop->defval, prop->name, &local_err);
+        object_property_set_bool(obj, prop->defval, prop->name, &error_abort);
     } else if (prop->info->enum_table) {
         object_property_set_str(obj, prop->info->enum_table[prop->defval],
-                                prop->name, &local_err);
+                                prop->name, &error_abort);
     } else if (prop->qtype == QTYPE_QINT) {
-        object_property_set_int(obj, prop->defval, prop->name, &local_err);
+        object_property_set_int(obj, prop->defval, prop->name, &error_abort);
     }
-    assert_no_error(local_err);
 }
 
 static bool device_get_realized(Object *obj, Error **err)
@@ -723,7 +722,6 @@ static void device_initfn(Object *obj)
     DeviceState *dev = DEVICE(obj);
     ObjectClass *class;
     Property *prop;
-    Error *err = NULL;
 
     if (qdev_hotplug) {
         dev->hotplugged = 1;
@@ -739,31 +737,19 @@ static void device_initfn(Object *obj)
     class = object_get_class(OBJECT(dev));
     do {
         for (prop = DEVICE_CLASS(class)->props; prop && prop->name; prop++) {
-            qdev_property_add_legacy(dev, prop, &err);
-            assert_no_error(err);
-            qdev_property_add_static(dev, prop, &err);
-            assert_no_error(err);
+            qdev_property_add_legacy(dev, prop, &error_abort);
+            qdev_property_add_static(dev, prop, &error_abort);
         }
         class = object_class_get_parent(class);
     } while (class != object_class_by_name(TYPE_DEVICE));
-    if (err != NULL) {
-        qerror_report_err(err);
-        error_free(err);
-        exit(1);
-    }
 
     object_property_add_link(OBJECT(dev), "parent_bus", TYPE_BUS,
-                             (Object **)&dev->parent_bus, &err);
-    assert_no_error(err);
+                             (Object **)&dev->parent_bus, &error_abort);
 }
 
 static void device_post_init(Object *obj)
 {
-    DeviceState *dev = DEVICE(obj);
-    Error *err = NULL;
-
-    qdev_prop_set_globals(dev, &err);
-    assert_no_error(err);
+    qdev_prop_set_globals(DEVICE(obj), &error_abort);
 }
 
 /* Unlink device from bus and free the structure.  */
diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
index d67c5f1..bb92e41 100644
--- a/hw/dma/xilinx_axidma.c
+++ b/hw/dma/xilinx_axidma.c
@@ -569,26 +569,21 @@ static void xilinx_axidma_init(Object *obj)
 {
     XilinxAXIDMA *s = XILINX_AXI_DMA(obj);
     SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
-    Error *errp = NULL;
 
     object_property_add_link(obj, "axistream-connected", TYPE_STREAM_SLAVE,
-                             (Object **) &s->tx_data_dev, &errp);
-    assert_no_error(errp);
+                             (Object **) &s->tx_data_dev, &error_abort);
     object_property_add_link(obj, "axistream-control-connected",
                              TYPE_STREAM_SLAVE,
-                             (Object **) &s->tx_control_dev, &errp);
-    assert_no_error(errp);
+                             (Object **) &s->tx_control_dev, &error_abort);
 
     object_initialize(&s->rx_data_dev, sizeof(s->rx_data_dev),
                       TYPE_XILINX_AXI_DMA_DATA_STREAM);
     object_initialize(&s->rx_control_dev, sizeof(s->rx_control_dev),
                       TYPE_XILINX_AXI_DMA_CONTROL_STREAM);
     object_property_add_child(OBJECT(s), "axistream-connected-target",
-                              (Object *)&s->rx_data_dev, &errp);
-    assert_no_error(errp);
+                              (Object *)&s->rx_data_dev, &error_abort);
     object_property_add_child(OBJECT(s), "axistream-control-connected-target",
-                              (Object *)&s->rx_control_dev, &errp);
-    assert_no_error(errp);
+                              (Object *)&s->rx_control_dev, &error_abort);
 
     sysbus_init_irq(sbd, &s->streams[0].irq);
     sysbus_init_irq(sbd, &s->streams[1].irq);
diff --git a/hw/net/xilinx_axienet.c b/hw/net/xilinx_axienet.c
index 3eb7715..0bd5eda 100644
--- a/hw/net/xilinx_axienet.c
+++ b/hw/net/xilinx_axienet.c
@@ -980,26 +980,21 @@ static void xilinx_enet_init(Object *obj)
 {
     XilinxAXIEnet *s = XILINX_AXI_ENET(obj);
     SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
-    Error *errp = NULL;
 
     object_property_add_link(obj, "axistream-connected", TYPE_STREAM_SLAVE,
-                             (Object **) &s->tx_data_dev, &errp);
-    assert_no_error(errp);
+                             (Object **) &s->tx_data_dev, &error_abort);
     object_property_add_link(obj, "axistream-control-connected",
                              TYPE_STREAM_SLAVE,
-                             (Object **) &s->tx_control_dev, &errp);
-    assert_no_error(errp);
+                             (Object **) &s->tx_control_dev, &error_abort);
 
     object_initialize(&s->rx_data_dev, sizeof(s->rx_data_dev),
                       TYPE_XILINX_AXI_ENET_DATA_STREAM);
     object_initialize(&s->rx_control_dev, sizeof(s->rx_control_dev),
                       TYPE_XILINX_AXI_ENET_CONTROL_STREAM);
     object_property_add_child(OBJECT(s), "axistream-connected-target",
-                              (Object *)&s->rx_data_dev, &errp);
-    assert_no_error(errp);
+                              (Object *)&s->rx_data_dev, &error_abort);
     object_property_add_child(OBJECT(s), "axistream-control-connected-target",
-                              (Object *)&s->rx_control_dev, &errp);
-    assert_no_error(errp);
+                              (Object *)&s->rx_control_dev, &error_abort);
 
     sysbus_init_irq(sbd, &s->irq);
 
diff --git a/include/hw/xilinx.h b/include/hw/xilinx.h
index 0c0251a..9d6debe 100644
--- a/include/hw/xilinx.h
+++ b/include/hw/xilinx.h
@@ -59,16 +59,13 @@ xilinx_axiethernet_init(DeviceState *dev, NICInfo *nd, StreamSlave *ds,
                         StreamSlave *cs, hwaddr base, qemu_irq irq, int txmem,
                         int rxmem)
 {
-    Error *errp = NULL;
-
     qdev_set_nic_properties(dev, nd);
     qdev_prop_set_uint32(dev, "rxmem", rxmem);
     qdev_prop_set_uint32(dev, "txmem", txmem);
     object_property_set_link(OBJECT(dev), OBJECT(ds),
-                             "axistream-connected", &errp);
+                             "axistream-connected", &error_abort);
     object_property_set_link(OBJECT(dev), OBJECT(cs),
-                             "axistream-control-connected", &errp);
-    assert_no_error(errp);
+                             "axistream-control-connected", &error_abort);
     qdev_init_nofail(dev);
     sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
     sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, irq);
@@ -78,14 +75,11 @@ static inline void
 xilinx_axidma_init(DeviceState *dev, StreamSlave *ds, StreamSlave *cs,
                    hwaddr base, qemu_irq irq, qemu_irq irq2, int freqhz)
 {
-    Error *errp = NULL;
-
     qdev_prop_set_uint32(dev, "freqhz", freqhz);
     object_property_set_link(OBJECT(dev), OBJECT(ds),
-                             "axistream-connected", &errp);
+                             "axistream-connected", &error_abort);
     object_property_set_link(OBJECT(dev), OBJECT(cs),
-                             "axistream-control-connected", &errp);
-    assert_no_error(errp);
+                             "axistream-control-connected", &error_abort);
     qdev_init_nofail(dev);
 
     sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
-- 
1.8.4.4

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [RFC PATCH v1 2/5] hw: Remove assert_no_error usages
  2013-12-03  5:50 ` [Qemu-devel] [RFC PATCH v1 2/5] hw: Remove assert_no_error usages Peter Crosthwaite
@ 2013-12-03  9:35   ` Markus Armbruster
  2013-12-03 10:04     ` Peter Crosthwaite
  0 siblings, 1 reply; 25+ messages in thread
From: Markus Armbruster @ 2013-12-03  9:35 UTC (permalink / raw)
  To: Peter Crosthwaite; +Cc: imammedo, pbonzini, qemu-devel, afaerber

Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes:

> Replace assert_no_error() usages with the error_abort system.
> &error_abort is passed into API calls to signal to the Error sub-system
> that any errors are fatal. Removes need for caller assertions.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
[...]
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index e374a93..7d869fc 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
[...]
> @@ -739,31 +737,19 @@ static void device_initfn(Object *obj)
>      class = object_get_class(OBJECT(dev));
>      do {
>          for (prop = DEVICE_CLASS(class)->props; prop && prop->name; prop++) {
> -            qdev_property_add_legacy(dev, prop, &err);
> -            assert_no_error(err);
> -            qdev_property_add_static(dev, prop, &err);
> -            assert_no_error(err);
> +            qdev_property_add_legacy(dev, prop, &error_abort);
> +            qdev_property_add_static(dev, prop, &error_abort);
>          }
>          class = object_class_get_parent(class);
>      } while (class != object_class_by_name(TYPE_DEVICE));
> -    if (err != NULL) {
> -        qerror_report_err(err);
> -        error_free(err);
> -        exit(1);
> -    }

Removal of these five lines isn't about replacing assert_no_error(),
it's burying dead code: err is initialized to null, and every place
where err can be set is followed by an assert_no_error(), therefore err
must still be null here.  Correct?

>  
>      object_property_add_link(OBJECT(dev), "parent_bus", TYPE_BUS,
> -                             (Object **)&dev->parent_bus, &err);
> -    assert_no_error(err);
> +                             (Object **)&dev->parent_bus, &error_abort);
>  }
>  
>  static void device_post_init(Object *obj)
>  {
> -    DeviceState *dev = DEVICE(obj);
> -    Error *err = NULL;
> -
> -    qdev_prop_set_globals(dev, &err);
> -    assert_no_error(err);
> +    qdev_prop_set_globals(DEVICE(obj), &error_abort);
>  }
>  
>  /* Unlink device from bus and free the structure.  */
> diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
> index d67c5f1..bb92e41 100644
> --- a/hw/dma/xilinx_axidma.c
> +++ b/hw/dma/xilinx_axidma.c
> @@ -569,26 +569,21 @@ static void xilinx_axidma_init(Object *obj)
>  {
>      XilinxAXIDMA *s = XILINX_AXI_DMA(obj);
>      SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> -    Error *errp = NULL;
>  
>      object_property_add_link(obj, "axistream-connected", TYPE_STREAM_SLAVE,
> -                             (Object **) &s->tx_data_dev, &errp);
> -    assert_no_error(errp);
> +                             (Object **) &s->tx_data_dev, &error_abort);

You could use the opportunity and drop the space between cast and its
operand, for consistency with the other casts around here.  No need to
respin just for that, of course.

[...]

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [RFC PATCH v1 2/5] hw: Remove assert_no_error usages
  2013-12-03  9:35   ` Markus Armbruster
@ 2013-12-03 10:04     ` Peter Crosthwaite
  0 siblings, 0 replies; 25+ messages in thread
From: Peter Crosthwaite @ 2013-12-03 10:04 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: Igor Mammedov, qemu-devel@nongnu.org Developers,
	Andreas Färber, Paolo Bonzini

On Tue, Dec 3, 2013 at 7:35 PM, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes:
>
>> Replace assert_no_error() usages with the error_abort system.
>> &error_abort is passed into API calls to signal to the Error sub-system
>> that any errors are fatal. Removes need for caller assertions.
>>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> [...]
>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>> index e374a93..7d869fc 100644
>> --- a/hw/core/qdev.c
>> +++ b/hw/core/qdev.c
> [...]
>> @@ -739,31 +737,19 @@ static void device_initfn(Object *obj)
>>      class = object_get_class(OBJECT(dev));
>>      do {
>>          for (prop = DEVICE_CLASS(class)->props; prop && prop->name; prop++) {
>> -            qdev_property_add_legacy(dev, prop, &err);
>> -            assert_no_error(err);
>> -            qdev_property_add_static(dev, prop, &err);
>> -            assert_no_error(err);
>> +            qdev_property_add_legacy(dev, prop, &error_abort);
>> +            qdev_property_add_static(dev, prop, &error_abort);
>>          }
>>          class = object_class_get_parent(class);
>>      } while (class != object_class_by_name(TYPE_DEVICE));
>> -    if (err != NULL) {
>> -        qerror_report_err(err);
>> -        error_free(err);
>> -        exit(1);
>> -    }
>
> Removal of these five lines isn't about replacing assert_no_error(),
> it's burying dead code: err is initialized to null, and every place
> where err can be set is followed by an assert_no_error(), therefore err
> must still be null here.  Correct?
>

Correct, it's dead code.

>>
>>      object_property_add_link(OBJECT(dev), "parent_bus", TYPE_BUS,
>> -                             (Object **)&dev->parent_bus, &err);
>> -    assert_no_error(err);
>> +                             (Object **)&dev->parent_bus, &error_abort);
>>  }
>>
>>  static void device_post_init(Object *obj)
>>  {
>> -    DeviceState *dev = DEVICE(obj);
>> -    Error *err = NULL;
>> -
>> -    qdev_prop_set_globals(dev, &err);
>> -    assert_no_error(err);
>> +    qdev_prop_set_globals(DEVICE(obj), &error_abort);
>>  }
>>
>>  /* Unlink device from bus and free the structure.  */
>> diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
>> index d67c5f1..bb92e41 100644
>> --- a/hw/dma/xilinx_axidma.c
>> +++ b/hw/dma/xilinx_axidma.c
>> @@ -569,26 +569,21 @@ static void xilinx_axidma_init(Object *obj)
>>  {
>>      XilinxAXIDMA *s = XILINX_AXI_DMA(obj);
>>      SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
>> -    Error *errp = NULL;
>>
>>      object_property_add_link(obj, "axistream-connected", TYPE_STREAM_SLAVE,
>> -                             (Object **) &s->tx_data_dev, &errp);
>> -    assert_no_error(errp);
>> +                             (Object **) &s->tx_data_dev, &error_abort);
>
> You could use the opportunity and drop the space between cast and its
> operand, for consistency with the other casts around here.  No need to
> respin just for that, of course.
>

Will fix on respin.

Regards,
Peter

> [...]
>

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [Qemu-devel] [RFC PATCH v1 3/5] target-i386: Remove assert_no_error usage
  2013-12-03  5:49 [Qemu-devel] [RFC PATCH v1 0/5] Add error_abort and associated cleanups Peter Crosthwaite
  2013-12-03  5:49 ` [Qemu-devel] [RFC PATCH v1 1/5] error: Add error_abort Peter Crosthwaite
  2013-12-03  5:50 ` [Qemu-devel] [RFC PATCH v1 2/5] hw: Remove assert_no_error usages Peter Crosthwaite
@ 2013-12-03  5:51 ` Peter Crosthwaite
  2013-12-03  5:51 ` [Qemu-devel] [RFC PATCH v1 4/5] qemu-option: Remove qemu_opts_create_nofail Peter Crosthwaite
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 25+ messages in thread
From: Peter Crosthwaite @ 2013-12-03  5:51 UTC (permalink / raw)
  To: qemu-devel; +Cc: imammedo, afaerber, armbru, pbonzini

Replace an assert_no_error() usage with the error_abort system.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 target-i386/cpu.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 47af9a8..b930ca5 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1600,7 +1600,6 @@ static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
                                 const char *name)
 {
     x86_def_t *def;
-    Error *err = NULL;
     int i;
 
     if (name == NULL) {
@@ -1608,8 +1607,7 @@ static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
     }
     if (kvm_enabled() && strcmp(name, "host") == 0) {
         kvm_cpu_fill_host(x86_cpu_def);
-        object_property_set_bool(OBJECT(cpu), true, "pmu", &err);
-        assert_no_error(err);
+        object_property_set_bool(OBJECT(cpu), true, "pmu", &error_abort);
         return 0;
     }
 
-- 
1.8.4.4

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [Qemu-devel] [RFC PATCH v1 4/5] qemu-option: Remove qemu_opts_create_nofail
  2013-12-03  5:49 [Qemu-devel] [RFC PATCH v1 0/5] Add error_abort and associated cleanups Peter Crosthwaite
                   ` (2 preceding siblings ...)
  2013-12-03  5:51 ` [Qemu-devel] [RFC PATCH v1 3/5] target-i386: Remove assert_no_error usage Peter Crosthwaite
@ 2013-12-03  5:51 ` Peter Crosthwaite
  2013-12-03  9:42   ` Markus Armbruster
  2013-12-03  5:52 ` [Qemu-devel] [RFC PATCH v1 5/5] qerror: Remove assert_no_error() Peter Crosthwaite
  2013-12-03  9:44 ` [Qemu-devel] [RFC PATCH v1 0/5] Add error_abort and associated cleanups Markus Armbruster
  5 siblings, 1 reply; 25+ messages in thread
From: Peter Crosthwaite @ 2013-12-03  5:51 UTC (permalink / raw)
  To: qemu-devel; +Cc: imammedo, afaerber, armbru, pbonzini

This is a boiler-plate _nofail variant of qemu_opts_create. Remove and
use error_abort in call sites.

A null argument needs to be added for the id field in affected callsites
due to inconsistency between the normal and no_fail variants.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 block/blkdebug.c       |  2 +-
 block/blkverify.c      |  2 +-
 block/curl.c           |  2 +-
 block/gluster.c        |  2 +-
 block/iscsi.c          |  2 +-
 block/nbd.c            |  2 +-
 block/qcow2.c          |  2 +-
 block/raw-posix.c      |  2 +-
 block/raw-win32.c      |  4 ++--
 block/rbd.c            |  2 +-
 block/sheepdog.c       |  2 +-
 block/vvfat.c          |  2 +-
 blockdev.c             |  6 ++++--
 hw/watchdog/watchdog.c |  3 ++-
 include/qemu/option.h  |  1 -
 qdev-monitor.c         |  2 +-
 qemu-img.c             |  2 +-
 util/qemu-config.c     |  2 +-
 util/qemu-option.c     |  9 ---------
 util/qemu-sockets.c    | 18 +++++++++---------
 vl.c                   | 17 ++++++++++-------
 21 files changed, 41 insertions(+), 45 deletions(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index 16d2b91..6b49df8 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -359,7 +359,7 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
     const char *filename, *config;
     int ret;
 
-    opts = qemu_opts_create_nofail(&runtime_opts);
+    opts = qemu_opts_create(&runtime_opts, NULL, 1, &error_abort);
     qemu_opts_absorb_qdict(opts, options, &local_err);
     if (error_is_set(&local_err)) {
         error_propagate(errp, local_err);
diff --git a/block/blkverify.c b/block/blkverify.c
index 3c63528..d901852 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -125,7 +125,7 @@ static int blkverify_open(BlockDriverState *bs, QDict *options, int flags,
     const char *filename, *raw;
     int ret;
 
-    opts = qemu_opts_create_nofail(&runtime_opts);
+    opts = qemu_opts_create(&runtime_opts, NULL, 1, &error_abort);
     qemu_opts_absorb_qdict(opts, options, &local_err);
     if (error_is_set(&local_err)) {
         error_propagate(errp, local_err);
diff --git a/block/curl.c b/block/curl.c
index 5a46f97..25dd433 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -413,7 +413,7 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
         return -EROFS;
     }
 
-    opts = qemu_opts_create_nofail(&runtime_opts);
+    opts = qemu_opts_create(&runtime_opts, NULL, 1, &error_abort);
     qemu_opts_absorb_qdict(opts, options, &local_err);
     if (error_is_set(&local_err)) {
         qerror_report_err(local_err);
diff --git a/block/gluster.c b/block/gluster.c
index 877686a..38c1a72 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -298,7 +298,7 @@ static int qemu_gluster_open(BlockDriverState *bs,  QDict *options,
     Error *local_err = NULL;
     const char *filename;
 
-    opts = qemu_opts_create_nofail(&runtime_opts);
+    opts = qemu_opts_create(&runtime_opts, NULL, 1, &error_abort);
     qemu_opts_absorb_qdict(opts, options, &local_err);
     if (error_is_set(&local_err)) {
         qerror_report_err(local_err);
diff --git a/block/iscsi.c b/block/iscsi.c
index a2d578c..b5a8221 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1241,7 +1241,7 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
         return -EINVAL;
     }
 
-    opts = qemu_opts_create_nofail(&runtime_opts);
+    opts = qemu_opts_create(&runtime_opts, NULL, 1, &error_abort);
     qemu_opts_absorb_qdict(opts, options, &local_err);
     if (error_is_set(&local_err)) {
         qerror_report_err(local_err);
diff --git a/block/nbd.c b/block/nbd.c
index c8deeee..b281b9c 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -234,7 +234,7 @@ static int nbd_config(BDRVNBDState *s, QDict *options)
         return -EINVAL;
     }
 
-    s->socket_opts = qemu_opts_create_nofail(&socket_optslist);
+    s->socket_opts = qemu_opts_create(&socket_optslist, NULL, 1, &error_abort);
 
     qemu_opts_absorb_qdict(s->socket_opts, options, &local_err);
     if (error_is_set(&local_err)) {
diff --git a/block/qcow2.c b/block/qcow2.c
index 6e5d98d..ac63e06 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -669,7 +669,7 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     /* Enable lazy_refcounts according to image and command line options */
-    opts = qemu_opts_create_nofail(&qcow2_runtime_opts);
+    opts = qemu_opts_create(&qcow2_runtime_opts, NULL, 1, &error_abort);
     qemu_opts_absorb_qdict(opts, options, &local_err);
     if (error_is_set(&local_err)) {
         error_propagate(errp, local_err);
diff --git a/block/raw-posix.c b/block/raw-posix.c
index f836c8e..996ec34 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -284,7 +284,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
     const char *filename;
     int fd, ret;
 
-    opts = qemu_opts_create_nofail(&raw_runtime_opts);
+    opts = qemu_opts_create(&raw_runtime_opts, NULL, 1, &error_abort);
     qemu_opts_absorb_qdict(opts, options, &local_err);
     if (error_is_set(&local_err)) {
         error_propagate(errp, local_err);
diff --git a/block/raw-win32.c b/block/raw-win32.c
index 2bad5a3..5881836 100644
--- a/block/raw-win32.c
+++ b/block/raw-win32.c
@@ -248,7 +248,7 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags,
 
     s->type = FTYPE_FILE;
 
-    opts = qemu_opts_create_nofail(&raw_runtime_opts);
+    opts = qemu_opts_create(&raw_runtime_opts, NULL, 1, &error_abort);
     qemu_opts_absorb_qdict(opts, options, &local_err);
     if (error_is_set(&local_err)) {
         error_propagate(errp, local_err);
@@ -550,7 +550,7 @@ static int hdev_open(BlockDriverState *bs, QDict *options, int flags,
     Error *local_err = NULL;
     const char *filename;
 
-    QemuOpts *opts = qemu_opts_create_nofail(&raw_runtime_opts);
+    QemuOpts *opts = qemu_opts_create(&raw_runtime_opts, NULL, 1, &error_abort);
     qemu_opts_absorb_qdict(opts, options, &local_err);
     if (error_is_set(&local_err)) {
         error_propagate(errp, local_err);
diff --git a/block/rbd.c b/block/rbd.c
index 4a1ea5b..e7250dd 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -461,7 +461,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
     const char *filename;
     int r;
 
-    opts = qemu_opts_create_nofail(&runtime_opts);
+    opts = qemu_opts_create(&runtime_opts, NULL, 1, &error_abort);
     qemu_opts_absorb_qdict(opts, options, &local_err);
     if (error_is_set(&local_err)) {
         qerror_report_err(local_err);
diff --git a/block/sheepdog.c b/block/sheepdog.c
index ef387de..12f0b3f 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1375,7 +1375,7 @@ static int sd_open(BlockDriverState *bs, QDict *options, int flags,
 
     s->bs = bs;
 
-    opts = qemu_opts_create_nofail(&runtime_opts);
+    opts = qemu_opts_create(&runtime_opts, NULL, 1, &error_abort);
     qemu_opts_absorb_qdict(opts, options, &local_err);
     if (error_is_set(&local_err)) {
         qerror_report_err(local_err);
diff --git a/block/vvfat.c b/block/vvfat.c
index 3ddaa0b..57bc0f9 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1085,7 +1085,7 @@ DLOG(if (stderr == NULL) {
     setbuf(stderr, NULL);
 })
 
-    opts = qemu_opts_create_nofail(&runtime_opts);
+    opts = qemu_opts_create(&runtime_opts, NULL, 1, &error_abort);
     qemu_opts_absorb_qdict(opts, options, &local_err);
     if (error_is_set(&local_err)) {
         qerror_report_err(local_err);
diff --git a/blockdev.c b/blockdev.c
index 330aa4a..78e8455 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -682,7 +682,8 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
     bs_opts = qdict_new();
     qemu_opts_to_qdict(all_opts, bs_opts);
 
-    legacy_opts = qemu_opts_create_nofail(&qemu_legacy_drive_opts);
+    legacy_opts = qemu_opts_create(&qemu_legacy_drive_opts, NULL, 1,
+                                   &error_abort);
     qemu_opts_absorb_qdict(legacy_opts, bs_opts, &local_err);
     if (error_is_set(&local_err)) {
         qerror_report_err(local_err);
@@ -853,7 +854,8 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
 
     if (type == IF_VIRTIO) {
         QemuOpts *devopts;
-        devopts = qemu_opts_create_nofail(qemu_find_opts("device"));
+        devopts = qemu_opts_create(qemu_find_opts("device"), NULL, 1,
+                                   &error_abort);
         if (arch_type == QEMU_ARCH_S390X) {
             qemu_opt_set(devopts, "driver", "virtio-blk-s390");
         } else {
diff --git a/hw/watchdog/watchdog.c b/hw/watchdog/watchdog.c
index 387962e..0ab2a0e 100644
--- a/hw/watchdog/watchdog.c
+++ b/hw/watchdog/watchdog.c
@@ -66,7 +66,8 @@ int select_watchdog(const char *p)
     QLIST_FOREACH(model, &watchdog_list, entry) {
         if (strcasecmp(model->wdt_name, p) == 0) {
             /* add the device */
-            opts = qemu_opts_create_nofail(qemu_find_opts("device"));
+            opts = qemu_opts_create(qemu_find_opts("device"), NULL, 1,
+                                    &error_abort);
             qemu_opt_set(opts, "driver", p);
             return 0;
         }
diff --git a/include/qemu/option.h b/include/qemu/option.h
index 5c0c6dd..3ea871a 100644
--- a/include/qemu/option.h
+++ b/include/qemu/option.h
@@ -136,7 +136,6 @@ int qemu_opt_foreach(QemuOpts *opts, qemu_opt_loopfunc func, void *opaque,
 QemuOpts *qemu_opts_find(QemuOptsList *list, const char *id);
 QemuOpts *qemu_opts_create(QemuOptsList *list, const char *id,
                            int fail_if_exists, Error **errp);
-QemuOpts *qemu_opts_create_nofail(QemuOptsList *list);
 void qemu_opts_reset(QemuOptsList *list);
 void qemu_opts_loc_restore(QemuOpts *opts);
 int qemu_opts_set(QemuOptsList *list, const char *id,
diff --git a/qdev-monitor.c b/qdev-monitor.c
index dc37a43..dd6e68c 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -730,7 +730,7 @@ int qemu_global_option(const char *str)
         return -1;
     }
 
-    opts = qemu_opts_create_nofail(&qemu_global_opts);
+    opts = qemu_opts_create(&qemu_global_opts, NULL, 1, &error_abort);
     qemu_opt_set(opts, "driver", driver);
     qemu_opt_set(opts, "property", property);
     qemu_opt_set(opts, "value", str+offset+1);
diff --git a/qemu-img.c b/qemu-img.c
index b6b5644..4f6b519 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2477,7 +2477,7 @@ static int img_resize(int argc, char **argv)
     }
 
     /* Parse size */
-    param = qemu_opts_create_nofail(&resize_options);
+    param = qemu_opts_create(&resize_options, NULL, 1, &error_abort);
     if (qemu_opt_set(param, BLOCK_OPT_SIZE, size)) {
         /* Error message already printed when size parsing fails */
         ret = -1;
diff --git a/util/qemu-config.c b/util/qemu-config.c
index 04da942..a803922 100644
--- a/util/qemu-config.c
+++ b/util/qemu-config.c
@@ -311,7 +311,7 @@ int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname)
                 error_free(local_err);
                 goto out;
             }
-            opts = qemu_opts_create_nofail(list);
+            opts = qemu_opts_create(list, NULL, 1, &error_abort);
             continue;
         }
         if (sscanf(line, " %63s = \"%1023[^\"]\"", arg, value) == 2) {
diff --git a/util/qemu-option.c b/util/qemu-option.c
index efcb5dc..668e5d9 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -791,15 +791,6 @@ QemuOpts *qemu_opts_create(QemuOptsList *list, const char *id,
     return opts;
 }
 
-QemuOpts *qemu_opts_create_nofail(QemuOptsList *list)
-{
-    QemuOpts *opts;
-    Error *errp = NULL;
-    opts = qemu_opts_create(list, NULL, 0, &errp);
-    assert_no_error(errp);
-    return opts;
-}
-
 void qemu_opts_reset(QemuOptsList *list)
 {
     QemuOpts *opts, *next_opts;
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 6b97dc1..9d50f43 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -578,7 +578,7 @@ int inet_listen(const char *str, char *ostr, int olen,
 
     addr = inet_parse(str, errp);
     if (addr != NULL) {
-        opts = qemu_opts_create_nofail(&socket_optslist);
+        opts = qemu_opts_create(&socket_optslist, NULL, 1, &error_abort);
         inet_addr_to_opts(opts, addr);
         qapi_free_InetSocketAddress(addr);
         sock = inet_listen_opts(opts, port_offset, errp);
@@ -617,7 +617,7 @@ int inet_connect(const char *str, Error **errp)
 
     addr = inet_parse(str, errp);
     if (addr != NULL) {
-        opts = qemu_opts_create_nofail(&socket_optslist);
+        opts = qemu_opts_create(&socket_optslist, NULL, 1, &error_abort);
         inet_addr_to_opts(opts, addr);
         qapi_free_InetSocketAddress(addr);
         sock = inet_connect_opts(opts, errp, NULL, NULL);
@@ -651,7 +651,7 @@ int inet_nonblocking_connect(const char *str,
 
     addr = inet_parse(str, errp);
     if (addr != NULL) {
-        opts = qemu_opts_create_nofail(&socket_optslist);
+        opts = qemu_opts_create(&socket_optslist, NULL, 1, &error_abort);
         inet_addr_to_opts(opts, addr);
         qapi_free_InetSocketAddress(addr);
         sock = inet_connect_opts(opts, errp, callback, opaque);
@@ -794,7 +794,7 @@ int unix_listen(const char *str, char *ostr, int olen, Error **errp)
     char *path, *optstr;
     int sock, len;
 
-    opts = qemu_opts_create_nofail(&socket_optslist);
+    opts = qemu_opts_create(&socket_optslist, NULL, 1, &error_abort);
 
     optstr = strchr(str, ',');
     if (optstr) {
@@ -822,7 +822,7 @@ int unix_connect(const char *path, Error **errp)
     QemuOpts *opts;
     int sock;
 
-    opts = qemu_opts_create_nofail(&socket_optslist);
+    opts = qemu_opts_create(&socket_optslist, NULL, 1, &error_abort);
     qemu_opt_set(opts, "path", path);
     sock = unix_connect_opts(opts, errp, NULL, NULL);
     qemu_opts_del(opts);
@@ -839,7 +839,7 @@ int unix_nonblocking_connect(const char *path,
 
     g_assert(callback != NULL);
 
-    opts = qemu_opts_create_nofail(&socket_optslist);
+    opts = qemu_opts_create(&socket_optslist, NULL, 1, &error_abort);
     qemu_opt_set(opts, "path", path);
     sock = unix_connect_opts(opts, errp, callback, opaque);
     qemu_opts_del(opts);
@@ -889,7 +889,7 @@ int socket_connect(SocketAddress *addr, Error **errp,
     QemuOpts *opts;
     int fd;
 
-    opts = qemu_opts_create_nofail(&socket_optslist);
+    opts = qemu_opts_create(&socket_optslist, NULL, 1, &error_abort);
     switch (addr->kind) {
     case SOCKET_ADDRESS_KIND_INET:
         inet_addr_to_opts(opts, addr->inet);
@@ -921,7 +921,7 @@ int socket_listen(SocketAddress *addr, Error **errp)
     QemuOpts *opts;
     int fd;
 
-    opts = qemu_opts_create_nofail(&socket_optslist);
+    opts = qemu_opts_create(&socket_optslist, NULL, 1, &error_abort);
     switch (addr->kind) {
     case SOCKET_ADDRESS_KIND_INET:
         inet_addr_to_opts(opts, addr->inet);
@@ -949,7 +949,7 @@ int socket_dgram(SocketAddress *remote, SocketAddress *local, Error **errp)
     QemuOpts *opts;
     int fd;
 
-    opts = qemu_opts_create_nofail(&socket_optslist);
+    opts = qemu_opts_create(&socket_optslist, NULL, 1, &error_abort);
     switch (remote->kind) {
     case SOCKET_ADDRESS_KIND_INET:
         qemu_opt_set(opts, "host", remote->inet->host);
diff --git a/vl.c b/vl.c
index 8d5d874..4d5704b 100644
--- a/vl.c
+++ b/vl.c
@@ -543,7 +543,7 @@ QemuOpts *qemu_get_machine_opts(void)
     assert(list);
     opts = qemu_opts_find(list, NULL);
     if (!opts) {
-        opts = qemu_opts_create_nofail(list);
+        opts = qemu_opts_create(list, NULL, 1, &error_abort);
     }
     return opts;
 }
@@ -2252,7 +2252,8 @@ static int balloon_parse(const char *arg)
                 return  -1;
         } else {
             /* create empty opts */
-            opts = qemu_opts_create_nofail(qemu_find_opts("device"));
+            opts = qemu_opts_create(qemu_find_opts("device"), NULL, 1,
+                                    &error_abort);
         }
         qemu_opt_set(opts, "driver", "virtio-balloon");
         return 0;
@@ -2512,14 +2513,14 @@ static int virtcon_parse(const char *devname)
         exit(1);
     }
 
-    bus_opts = qemu_opts_create_nofail(device);
+    bus_opts = qemu_opts_create(device, NULL, 1, &error_abort);
     if (arch_type == QEMU_ARCH_S390X) {
         qemu_opt_set(bus_opts, "driver", "virtio-serial-s390");
     } else {
         qemu_opt_set(bus_opts, "driver", "virtio-serial-pci");
     }
 
-    dev_opts = qemu_opts_create_nofail(device);
+    dev_opts = qemu_opts_create(device, NULL, 1, &error_abort);
     qemu_opt_set(dev_opts, "driver", "virtconsole");
 
     snprintf(label, sizeof(label), "virtcon%d", index);
@@ -3376,8 +3377,9 @@ int main(int argc, char **argv, char **envp)
                 }
 
                 qemu_opt_set_bool(fsdev, "readonly",
-                                qemu_opt_get_bool(opts, "readonly", 0));
-                device = qemu_opts_create_nofail(qemu_find_opts("device"));
+                                  qemu_opt_get_bool(opts, "readonly", 0));
+                device = qemu_opts_create(qemu_find_opts("device"), NULL, 1,
+                                          &error_abort);
                 qemu_opt_set(device, "driver", "virtio-9p-pci");
                 qemu_opt_set(device, "fsdev",
                              qemu_opt_get(opts, "mount_tag"));
@@ -3397,7 +3399,8 @@ int main(int argc, char **argv, char **envp)
                 }
                 qemu_opt_set(fsdev, "fsdriver", "synth");
 
-                device = qemu_opts_create_nofail(qemu_find_opts("device"));
+                device = qemu_opts_create(qemu_find_opts("device"), NULL, 1,
+                                          &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");
-- 
1.8.4.4

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [RFC PATCH v1 4/5] qemu-option: Remove qemu_opts_create_nofail
  2013-12-03  5:51 ` [Qemu-devel] [RFC PATCH v1 4/5] qemu-option: Remove qemu_opts_create_nofail Peter Crosthwaite
@ 2013-12-03  9:42   ` Markus Armbruster
  2013-12-03 10:17     ` Peter Crosthwaite
  2013-12-04  6:45     ` Peter Crosthwaite
  0 siblings, 2 replies; 25+ messages in thread
From: Markus Armbruster @ 2013-12-03  9:42 UTC (permalink / raw)
  To: Peter Crosthwaite; +Cc: imammedo, pbonzini, qemu-devel, afaerber

Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes:

> This is a boiler-plate _nofail variant of qemu_opts_create. Remove and
> use error_abort in call sites.
>
> A null argument needs to be added for the id field in affected callsites
> due to inconsistency between the normal and no_fail variants.

Two arguments, actually, NULL id and zero fail_if_exists:

QemuOpts *qemu_opts_create_nofail(QemuOptsList *list)
{
    QemuOpts *opts;
    Error *errp = NULL;
    opts = qemu_opts_create(list, NULL, 0, &errp);
    assert_no_error(errp);
    return opts;
}

> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
>
>  block/blkdebug.c       |  2 +-
>  block/blkverify.c      |  2 +-
>  block/curl.c           |  2 +-
>  block/gluster.c        |  2 +-
>  block/iscsi.c          |  2 +-
>  block/nbd.c            |  2 +-
>  block/qcow2.c          |  2 +-
>  block/raw-posix.c      |  2 +-
>  block/raw-win32.c      |  4 ++--
>  block/rbd.c            |  2 +-
>  block/sheepdog.c       |  2 +-
>  block/vvfat.c          |  2 +-
>  blockdev.c             |  6 ++++--
>  hw/watchdog/watchdog.c |  3 ++-
>  include/qemu/option.h  |  1 -
>  qdev-monitor.c         |  2 +-
>  qemu-img.c             |  2 +-
>  util/qemu-config.c     |  2 +-
>  util/qemu-option.c     |  9 ---------
>  util/qemu-sockets.c    | 18 +++++++++---------
>  vl.c                   | 17 ++++++++++-------
>  21 files changed, 41 insertions(+), 45 deletions(-)
>
> diff --git a/block/blkdebug.c b/block/blkdebug.c
> index 16d2b91..6b49df8 100644
> --- a/block/blkdebug.c
> +++ b/block/blkdebug.c
> @@ -359,7 +359,7 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
>      const char *filename, *config;
>      int ret;
>  
> -    opts = qemu_opts_create_nofail(&runtime_opts);
> +    opts = qemu_opts_create(&runtime_opts, NULL, 1, &error_abort);
>      qemu_opts_absorb_qdict(opts, options, &local_err);
>      if (error_is_set(&local_err)) {
>          error_propagate(errp, local_err);

Err, doesn't this change fail_if_exists from zero to one?  Same
everywhere.

> diff --git a/block/blkverify.c b/block/blkverify.c
> index 3c63528..d901852 100644
> --- a/block/blkverify.c
> +++ b/block/blkverify.c
> @@ -125,7 +125,7 @@ static int blkverify_open(BlockDriverState *bs, QDict *options, int flags,
>      const char *filename, *raw;
>      int ret;
>  
> -    opts = qemu_opts_create_nofail(&runtime_opts);
> +    opts = qemu_opts_create(&runtime_opts, NULL, 1, &error_abort);
>      qemu_opts_absorb_qdict(opts, options, &local_err);
>      if (error_is_set(&local_err)) {
>          error_propagate(errp, local_err);
> diff --git a/block/curl.c b/block/curl.c
> index 5a46f97..25dd433 100644
> --- a/block/curl.c
> +++ b/block/curl.c
> @@ -413,7 +413,7 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
>          return -EROFS;
>      }
>  
> -    opts = qemu_opts_create_nofail(&runtime_opts);
> +    opts = qemu_opts_create(&runtime_opts, NULL, 1, &error_abort);
>      qemu_opts_absorb_qdict(opts, options, &local_err);
>      if (error_is_set(&local_err)) {
>          qerror_report_err(local_err);
> diff --git a/block/gluster.c b/block/gluster.c
> index 877686a..38c1a72 100644
> --- a/block/gluster.c
> +++ b/block/gluster.c
> @@ -298,7 +298,7 @@ static int qemu_gluster_open(BlockDriverState *bs,  QDict *options,
>      Error *local_err = NULL;
>      const char *filename;
>  
> -    opts = qemu_opts_create_nofail(&runtime_opts);
> +    opts = qemu_opts_create(&runtime_opts, NULL, 1, &error_abort);
>      qemu_opts_absorb_qdict(opts, options, &local_err);
>      if (error_is_set(&local_err)) {
>          qerror_report_err(local_err);
> diff --git a/block/iscsi.c b/block/iscsi.c
> index a2d578c..b5a8221 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -1241,7 +1241,7 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
>          return -EINVAL;
>      }
>  
> -    opts = qemu_opts_create_nofail(&runtime_opts);
> +    opts = qemu_opts_create(&runtime_opts, NULL, 1, &error_abort);
>      qemu_opts_absorb_qdict(opts, options, &local_err);
>      if (error_is_set(&local_err)) {
>          qerror_report_err(local_err);
> diff --git a/block/nbd.c b/block/nbd.c
> index c8deeee..b281b9c 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -234,7 +234,7 @@ static int nbd_config(BDRVNBDState *s, QDict *options)
>          return -EINVAL;
>      }
>  
> -    s->socket_opts = qemu_opts_create_nofail(&socket_optslist);
> +    s->socket_opts = qemu_opts_create(&socket_optslist, NULL, 1, &error_abort);

Long line, please wrap.

>  
>      qemu_opts_absorb_qdict(s->socket_opts, options, &local_err);
>      if (error_is_set(&local_err)) {
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 6e5d98d..ac63e06 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -669,7 +669,7 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
>      }
>  
>      /* Enable lazy_refcounts according to image and command line options */
> -    opts = qemu_opts_create_nofail(&qcow2_runtime_opts);
> +    opts = qemu_opts_create(&qcow2_runtime_opts, NULL, 1, &error_abort);
>      qemu_opts_absorb_qdict(opts, options, &local_err);
>      if (error_is_set(&local_err)) {
>          error_propagate(errp, local_err);
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index f836c8e..996ec34 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -284,7 +284,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
>      const char *filename;
>      int fd, ret;
>  
> -    opts = qemu_opts_create_nofail(&raw_runtime_opts);
> +    opts = qemu_opts_create(&raw_runtime_opts, NULL, 1, &error_abort);
>      qemu_opts_absorb_qdict(opts, options, &local_err);
>      if (error_is_set(&local_err)) {
>          error_propagate(errp, local_err);
> diff --git a/block/raw-win32.c b/block/raw-win32.c
> index 2bad5a3..5881836 100644
> --- a/block/raw-win32.c
> +++ b/block/raw-win32.c
> @@ -248,7 +248,7 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags,
>  
>      s->type = FTYPE_FILE;
>  
> -    opts = qemu_opts_create_nofail(&raw_runtime_opts);
> +    opts = qemu_opts_create(&raw_runtime_opts, NULL, 1, &error_abort);
>      qemu_opts_absorb_qdict(opts, options, &local_err);
>      if (error_is_set(&local_err)) {
>          error_propagate(errp, local_err);
> @@ -550,7 +550,7 @@ static int hdev_open(BlockDriverState *bs, QDict *options, int flags,
>      Error *local_err = NULL;
>      const char *filename;
>  
> -    QemuOpts *opts = qemu_opts_create_nofail(&raw_runtime_opts);
> +    QemuOpts *opts = qemu_opts_create(&raw_runtime_opts, NULL, 1, &error_abort);

Long line, please wrap.

>      qemu_opts_absorb_qdict(opts, options, &local_err);
>      if (error_is_set(&local_err)) {
>          error_propagate(errp, local_err);
> diff --git a/block/rbd.c b/block/rbd.c
> index 4a1ea5b..e7250dd 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -461,7 +461,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
>      const char *filename;
>      int r;
>  
> -    opts = qemu_opts_create_nofail(&runtime_opts);
> +    opts = qemu_opts_create(&runtime_opts, NULL, 1, &error_abort);
>      qemu_opts_absorb_qdict(opts, options, &local_err);
>      if (error_is_set(&local_err)) {
>          qerror_report_err(local_err);
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index ef387de..12f0b3f 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -1375,7 +1375,7 @@ static int sd_open(BlockDriverState *bs, QDict *options, int flags,
>  
>      s->bs = bs;
>  
> -    opts = qemu_opts_create_nofail(&runtime_opts);
> +    opts = qemu_opts_create(&runtime_opts, NULL, 1, &error_abort);
>      qemu_opts_absorb_qdict(opts, options, &local_err);
>      if (error_is_set(&local_err)) {
>          qerror_report_err(local_err);
> diff --git a/block/vvfat.c b/block/vvfat.c
> index 3ddaa0b..57bc0f9 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -1085,7 +1085,7 @@ DLOG(if (stderr == NULL) {
>      setbuf(stderr, NULL);
>  })
>  
> -    opts = qemu_opts_create_nofail(&runtime_opts);
> +    opts = qemu_opts_create(&runtime_opts, NULL, 1, &error_abort);
>      qemu_opts_absorb_qdict(opts, options, &local_err);
>      if (error_is_set(&local_err)) {
>          qerror_report_err(local_err);
> diff --git a/blockdev.c b/blockdev.c
> index 330aa4a..78e8455 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -682,7 +682,8 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
>      bs_opts = qdict_new();
>      qemu_opts_to_qdict(all_opts, bs_opts);
>  
> -    legacy_opts = qemu_opts_create_nofail(&qemu_legacy_drive_opts);
> +    legacy_opts = qemu_opts_create(&qemu_legacy_drive_opts, NULL, 1,
> +                                   &error_abort);
>      qemu_opts_absorb_qdict(legacy_opts, bs_opts, &local_err);
>      if (error_is_set(&local_err)) {
>          qerror_report_err(local_err);
> @@ -853,7 +854,8 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
>  
>      if (type == IF_VIRTIO) {
>          QemuOpts *devopts;
> -        devopts = qemu_opts_create_nofail(qemu_find_opts("device"));
> +        devopts = qemu_opts_create(qemu_find_opts("device"), NULL, 1,
> +                                   &error_abort);
>          if (arch_type == QEMU_ARCH_S390X) {
>              qemu_opt_set(devopts, "driver", "virtio-blk-s390");
>          } else {
> diff --git a/hw/watchdog/watchdog.c b/hw/watchdog/watchdog.c
> index 387962e..0ab2a0e 100644
> --- a/hw/watchdog/watchdog.c
> +++ b/hw/watchdog/watchdog.c
> @@ -66,7 +66,8 @@ int select_watchdog(const char *p)
>      QLIST_FOREACH(model, &watchdog_list, entry) {
>          if (strcasecmp(model->wdt_name, p) == 0) {
>              /* add the device */
> -            opts = qemu_opts_create_nofail(qemu_find_opts("device"));
> +            opts = qemu_opts_create(qemu_find_opts("device"), NULL, 1,
> +                                    &error_abort);
>              qemu_opt_set(opts, "driver", p);
>              return 0;
>          }
> diff --git a/include/qemu/option.h b/include/qemu/option.h
> index 5c0c6dd..3ea871a 100644
> --- a/include/qemu/option.h
> +++ b/include/qemu/option.h
> @@ -136,7 +136,6 @@ int qemu_opt_foreach(QemuOpts *opts, qemu_opt_loopfunc func, void *opaque,
>  QemuOpts *qemu_opts_find(QemuOptsList *list, const char *id);
>  QemuOpts *qemu_opts_create(QemuOptsList *list, const char *id,
>                             int fail_if_exists, Error **errp);
> -QemuOpts *qemu_opts_create_nofail(QemuOptsList *list);
>  void qemu_opts_reset(QemuOptsList *list);
>  void qemu_opts_loc_restore(QemuOpts *opts);
>  int qemu_opts_set(QemuOptsList *list, const char *id,
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index dc37a43..dd6e68c 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -730,7 +730,7 @@ int qemu_global_option(const char *str)
>          return -1;
>      }
>  
> -    opts = qemu_opts_create_nofail(&qemu_global_opts);
> +    opts = qemu_opts_create(&qemu_global_opts, NULL, 1, &error_abort);
>      qemu_opt_set(opts, "driver", driver);
>      qemu_opt_set(opts, "property", property);
>      qemu_opt_set(opts, "value", str+offset+1);
> diff --git a/qemu-img.c b/qemu-img.c
> index b6b5644..4f6b519 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -2477,7 +2477,7 @@ static int img_resize(int argc, char **argv)
>      }
>  
>      /* Parse size */
> -    param = qemu_opts_create_nofail(&resize_options);
> +    param = qemu_opts_create(&resize_options, NULL, 1, &error_abort);
>      if (qemu_opt_set(param, BLOCK_OPT_SIZE, size)) {
>          /* Error message already printed when size parsing fails */
>          ret = -1;
> diff --git a/util/qemu-config.c b/util/qemu-config.c
> index 04da942..a803922 100644
> --- a/util/qemu-config.c
> +++ b/util/qemu-config.c
> @@ -311,7 +311,7 @@ int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname)
>                  error_free(local_err);
>                  goto out;
>              }
> -            opts = qemu_opts_create_nofail(list);
> +            opts = qemu_opts_create(list, NULL, 1, &error_abort);
>              continue;
>          }
>          if (sscanf(line, " %63s = \"%1023[^\"]\"", arg, value) == 2) {
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index efcb5dc..668e5d9 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -791,15 +791,6 @@ QemuOpts *qemu_opts_create(QemuOptsList *list, const char *id,
>      return opts;
>  }
>  
> -QemuOpts *qemu_opts_create_nofail(QemuOptsList *list)
> -{
> -    QemuOpts *opts;
> -    Error *errp = NULL;
> -    opts = qemu_opts_create(list, NULL, 0, &errp);
> -    assert_no_error(errp);
> -    return opts;
> -}
> -
>  void qemu_opts_reset(QemuOptsList *list)
>  {
>      QemuOpts *opts, *next_opts;
> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index 6b97dc1..9d50f43 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -578,7 +578,7 @@ int inet_listen(const char *str, char *ostr, int olen,
>  
>      addr = inet_parse(str, errp);
>      if (addr != NULL) {
> -        opts = qemu_opts_create_nofail(&socket_optslist);
> +        opts = qemu_opts_create(&socket_optslist, NULL, 1, &error_abort);
>          inet_addr_to_opts(opts, addr);
>          qapi_free_InetSocketAddress(addr);
>          sock = inet_listen_opts(opts, port_offset, errp);
> @@ -617,7 +617,7 @@ int inet_connect(const char *str, Error **errp)
>  
>      addr = inet_parse(str, errp);
>      if (addr != NULL) {
> -        opts = qemu_opts_create_nofail(&socket_optslist);
> +        opts = qemu_opts_create(&socket_optslist, NULL, 1, &error_abort);
>          inet_addr_to_opts(opts, addr);
>          qapi_free_InetSocketAddress(addr);
>          sock = inet_connect_opts(opts, errp, NULL, NULL);
> @@ -651,7 +651,7 @@ int inet_nonblocking_connect(const char *str,
>  
>      addr = inet_parse(str, errp);
>      if (addr != NULL) {
> -        opts = qemu_opts_create_nofail(&socket_optslist);
> +        opts = qemu_opts_create(&socket_optslist, NULL, 1, &error_abort);
>          inet_addr_to_opts(opts, addr);
>          qapi_free_InetSocketAddress(addr);
>          sock = inet_connect_opts(opts, errp, callback, opaque);
> @@ -794,7 +794,7 @@ int unix_listen(const char *str, char *ostr, int olen, Error **errp)
>      char *path, *optstr;
>      int sock, len;
>  
> -    opts = qemu_opts_create_nofail(&socket_optslist);
> +    opts = qemu_opts_create(&socket_optslist, NULL, 1, &error_abort);
>  
>      optstr = strchr(str, ',');
>      if (optstr) {
> @@ -822,7 +822,7 @@ int unix_connect(const char *path, Error **errp)
>      QemuOpts *opts;
>      int sock;
>  
> -    opts = qemu_opts_create_nofail(&socket_optslist);
> +    opts = qemu_opts_create(&socket_optslist, NULL, 1, &error_abort);
>      qemu_opt_set(opts, "path", path);
>      sock = unix_connect_opts(opts, errp, NULL, NULL);
>      qemu_opts_del(opts);
> @@ -839,7 +839,7 @@ int unix_nonblocking_connect(const char *path,
>  
>      g_assert(callback != NULL);
>  
> -    opts = qemu_opts_create_nofail(&socket_optslist);
> +    opts = qemu_opts_create(&socket_optslist, NULL, 1, &error_abort);
>      qemu_opt_set(opts, "path", path);
>      sock = unix_connect_opts(opts, errp, callback, opaque);
>      qemu_opts_del(opts);
> @@ -889,7 +889,7 @@ int socket_connect(SocketAddress *addr, Error **errp,
>      QemuOpts *opts;
>      int fd;
>  
> -    opts = qemu_opts_create_nofail(&socket_optslist);
> +    opts = qemu_opts_create(&socket_optslist, NULL, 1, &error_abort);
>      switch (addr->kind) {
>      case SOCKET_ADDRESS_KIND_INET:
>          inet_addr_to_opts(opts, addr->inet);
> @@ -921,7 +921,7 @@ int socket_listen(SocketAddress *addr, Error **errp)
>      QemuOpts *opts;
>      int fd;
>  
> -    opts = qemu_opts_create_nofail(&socket_optslist);
> +    opts = qemu_opts_create(&socket_optslist, NULL, 1, &error_abort);
>      switch (addr->kind) {
>      case SOCKET_ADDRESS_KIND_INET:
>          inet_addr_to_opts(opts, addr->inet);
> @@ -949,7 +949,7 @@ int socket_dgram(SocketAddress *remote, SocketAddress *local, Error **errp)
>      QemuOpts *opts;
>      int fd;
>  
> -    opts = qemu_opts_create_nofail(&socket_optslist);
> +    opts = qemu_opts_create(&socket_optslist, NULL, 1, &error_abort);
>      switch (remote->kind) {
>      case SOCKET_ADDRESS_KIND_INET:
>          qemu_opt_set(opts, "host", remote->inet->host);
> diff --git a/vl.c b/vl.c
> index 8d5d874..4d5704b 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -543,7 +543,7 @@ QemuOpts *qemu_get_machine_opts(void)
>      assert(list);
>      opts = qemu_opts_find(list, NULL);
>      if (!opts) {
> -        opts = qemu_opts_create_nofail(list);
> +        opts = qemu_opts_create(list, NULL, 1, &error_abort);
>      }
>      return opts;
>  }
> @@ -2252,7 +2252,8 @@ static int balloon_parse(const char *arg)
>                  return  -1;
>          } else {
>              /* create empty opts */
> -            opts = qemu_opts_create_nofail(qemu_find_opts("device"));
> +            opts = qemu_opts_create(qemu_find_opts("device"), NULL, 1,
> +                                    &error_abort);
>          }
>          qemu_opt_set(opts, "driver", "virtio-balloon");
>          return 0;
> @@ -2512,14 +2513,14 @@ static int virtcon_parse(const char *devname)
>          exit(1);
>      }
>  
> -    bus_opts = qemu_opts_create_nofail(device);
> +    bus_opts = qemu_opts_create(device, NULL, 1, &error_abort);
>      if (arch_type == QEMU_ARCH_S390X) {
>          qemu_opt_set(bus_opts, "driver", "virtio-serial-s390");
>      } else {
>          qemu_opt_set(bus_opts, "driver", "virtio-serial-pci");
>      }
>  
> -    dev_opts = qemu_opts_create_nofail(device);
> +    dev_opts = qemu_opts_create(device, NULL, 1, &error_abort);
>      qemu_opt_set(dev_opts, "driver", "virtconsole");
>  
>      snprintf(label, sizeof(label), "virtcon%d", index);
> @@ -3376,8 +3377,9 @@ int main(int argc, char **argv, char **envp)
>                  }
>  
>                  qemu_opt_set_bool(fsdev, "readonly",
> -                                qemu_opt_get_bool(opts, "readonly", 0));
> -                device = qemu_opts_create_nofail(qemu_find_opts("device"));
> +                                  qemu_opt_get_bool(opts, "readonly", 0));

Spurious whitespace change, please fix.

> +                device = qemu_opts_create(qemu_find_opts("device"), NULL, 1,
> +                                          &error_abort);
>                  qemu_opt_set(device, "driver", "virtio-9p-pci");
>                  qemu_opt_set(device, "fsdev",
>                               qemu_opt_get(opts, "mount_tag"));
> @@ -3397,7 +3399,8 @@ int main(int argc, char **argv, char **envp)
>                  }
>                  qemu_opt_set(fsdev, "fsdriver", "synth");
>  
> -                device = qemu_opts_create_nofail(qemu_find_opts("device"));
> +                device = qemu_opts_create(qemu_find_opts("device"), NULL, 1,
> +                                          &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");

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [RFC PATCH v1 4/5] qemu-option: Remove qemu_opts_create_nofail
  2013-12-03  9:42   ` Markus Armbruster
@ 2013-12-03 10:17     ` Peter Crosthwaite
  2013-12-03 10:44       ` Markus Armbruster
  2013-12-04  6:45     ` Peter Crosthwaite
  1 sibling, 1 reply; 25+ messages in thread
From: Peter Crosthwaite @ 2013-12-03 10:17 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: Igor Mammedov, qemu-devel@nongnu.org Developers,
	Andreas Färber, Paolo Bonzini

On Tue, Dec 3, 2013 at 7:42 PM, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes:
>
>> This is a boiler-plate _nofail variant of qemu_opts_create. Remove and
>> use error_abort in call sites.
>>
>> A null argument needs to be added for the id field in affected callsites
>> due to inconsistency between the normal and no_fail variants.
>
> Two arguments, actually, NULL id and zero fail_if_exists:
>

Yep. I was assuming fail_if_exists was implicit as part of no-fail so
I wasn't considering that an inconsistency. But as _nofail doesn't
actually fail_if_exist's then its just as inconsistent. Will fix.

> QemuOpts *qemu_opts_create_nofail(QemuOptsList *list)
> {
>     QemuOpts *opts;
>     Error *errp = NULL;
>     opts = qemu_opts_create(list, NULL, 0, &errp);
>     assert_no_error(errp);
>     return opts;
> }
>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> ---
>>
>>  block/blkdebug.c       |  2 +-
>>  block/blkverify.c      |  2 +-
>>  block/curl.c           |  2 +-
>>  block/gluster.c        |  2 +-
>>  block/iscsi.c          |  2 +-
>>  block/nbd.c            |  2 +-
>>  block/qcow2.c          |  2 +-
>>  block/raw-posix.c      |  2 +-
>>  block/raw-win32.c      |  4 ++--
>>  block/rbd.c            |  2 +-
>>  block/sheepdog.c       |  2 +-
>>  block/vvfat.c          |  2 +-
>>  blockdev.c             |  6 ++++--
>>  hw/watchdog/watchdog.c |  3 ++-
>>  include/qemu/option.h  |  1 -
>>  qdev-monitor.c         |  2 +-
>>  qemu-img.c             |  2 +-
>>  util/qemu-config.c     |  2 +-
>>  util/qemu-option.c     |  9 ---------
>>  util/qemu-sockets.c    | 18 +++++++++---------
>>  vl.c                   | 17 ++++++++++-------
>>  21 files changed, 41 insertions(+), 45 deletions(-)
>>
>> diff --git a/block/blkdebug.c b/block/blkdebug.c
>> index 16d2b91..6b49df8 100644
>> --- a/block/blkdebug.c
>> +++ b/block/blkdebug.c
>> @@ -359,7 +359,7 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
>>      const char *filename, *config;
>>      int ret;
>>
>> -    opts = qemu_opts_create_nofail(&runtime_opts);
>> +    opts = qemu_opts_create(&runtime_opts, NULL, 1, &error_abort);
>>      qemu_opts_absorb_qdict(opts, options, &local_err);
>>      if (error_is_set(&local_err)) {
>>          error_propagate(errp, local_err);
>
> Err, doesn't this change fail_if_exists from zero to one?  Same
> everywhere.
>

Yes. Rash assumption my by me that the nofail version fails_on_exist. Will fix.

>> diff --git a/block/blkverify.c b/block/blkverify.c
>> index 3c63528..d901852 100644
>> --- a/block/blkverify.c
>> +++ b/block/blkverify.c
>> @@ -125,7 +125,7 @@ static int blkverify_open(BlockDriverState *bs, QDict *options, int flags,
>>      const char *filename, *raw;
>>      int ret;
>>
>> -    opts = qemu_opts_create_nofail(&runtime_opts);
>> +    opts = qemu_opts_create(&runtime_opts, NULL, 1, &error_abort);
>>      qemu_opts_absorb_qdict(opts, options, &local_err);
>>      if (error_is_set(&local_err)) {
>>          error_propagate(errp, local_err);
>> diff --git a/block/curl.c b/block/curl.c
>> index 5a46f97..25dd433 100644
>> --- a/block/curl.c
>> +++ b/block/curl.c
>> @@ -413,7 +413,7 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
>>          return -EROFS;
>>      }
>>
>> -    opts = qemu_opts_create_nofail(&runtime_opts);
>> +    opts = qemu_opts_create(&runtime_opts, NULL, 1, &error_abort);
>>      qemu_opts_absorb_qdict(opts, options, &local_err);
>>      if (error_is_set(&local_err)) {
>>          qerror_report_err(local_err);
>> diff --git a/block/gluster.c b/block/gluster.c
>> index 877686a..38c1a72 100644
>> --- a/block/gluster.c
>> +++ b/block/gluster.c
>> @@ -298,7 +298,7 @@ static int qemu_gluster_open(BlockDriverState *bs,  QDict *options,
>>      Error *local_err = NULL;
>>      const char *filename;
>>
>> -    opts = qemu_opts_create_nofail(&runtime_opts);
>> +    opts = qemu_opts_create(&runtime_opts, NULL, 1, &error_abort);
>>      qemu_opts_absorb_qdict(opts, options, &local_err);
>>      if (error_is_set(&local_err)) {
>>          qerror_report_err(local_err);
>> diff --git a/block/iscsi.c b/block/iscsi.c
>> index a2d578c..b5a8221 100644
>> --- a/block/iscsi.c
>> +++ b/block/iscsi.c
>> @@ -1241,7 +1241,7 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
>>          return -EINVAL;
>>      }
>>
>> -    opts = qemu_opts_create_nofail(&runtime_opts);
>> +    opts = qemu_opts_create(&runtime_opts, NULL, 1, &error_abort);
>>      qemu_opts_absorb_qdict(opts, options, &local_err);
>>      if (error_is_set(&local_err)) {
>>          qerror_report_err(local_err);
>> diff --git a/block/nbd.c b/block/nbd.c
>> index c8deeee..b281b9c 100644
>> --- a/block/nbd.c
>> +++ b/block/nbd.c
>> @@ -234,7 +234,7 @@ static int nbd_config(BDRVNBDState *s, QDict *options)
>>          return -EINVAL;
>>      }
>>
>> -    s->socket_opts = qemu_opts_create_nofail(&socket_optslist);
>> +    s->socket_opts = qemu_opts_create(&socket_optslist, NULL, 1, &error_abort);
>
> Long line, please wrap.
>
>>
>>      qemu_opts_absorb_qdict(s->socket_opts, options, &local_err);
>>      if (error_is_set(&local_err)) {
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index 6e5d98d..ac63e06 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -669,7 +669,7 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
>>      }
>>
>>      /* Enable lazy_refcounts according to image and command line options */
>> -    opts = qemu_opts_create_nofail(&qcow2_runtime_opts);
>> +    opts = qemu_opts_create(&qcow2_runtime_opts, NULL, 1, &error_abort);
>>      qemu_opts_absorb_qdict(opts, options, &local_err);
>>      if (error_is_set(&local_err)) {
>>          error_propagate(errp, local_err);
>> diff --git a/block/raw-posix.c b/block/raw-posix.c
>> index f836c8e..996ec34 100644
>> --- a/block/raw-posix.c
>> +++ b/block/raw-posix.c
>> @@ -284,7 +284,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
>>      const char *filename;
>>      int fd, ret;
>>
>> -    opts = qemu_opts_create_nofail(&raw_runtime_opts);
>> +    opts = qemu_opts_create(&raw_runtime_opts, NULL, 1, &error_abort);
>>      qemu_opts_absorb_qdict(opts, options, &local_err);
>>      if (error_is_set(&local_err)) {
>>          error_propagate(errp, local_err);
>> diff --git a/block/raw-win32.c b/block/raw-win32.c
>> index 2bad5a3..5881836 100644
>> --- a/block/raw-win32.c
>> +++ b/block/raw-win32.c
>> @@ -248,7 +248,7 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags,
>>
>>      s->type = FTYPE_FILE;
>>
>> -    opts = qemu_opts_create_nofail(&raw_runtime_opts);
>> +    opts = qemu_opts_create(&raw_runtime_opts, NULL, 1, &error_abort);
>>      qemu_opts_absorb_qdict(opts, options, &local_err);
>>      if (error_is_set(&local_err)) {
>>          error_propagate(errp, local_err);
>> @@ -550,7 +550,7 @@ static int hdev_open(BlockDriverState *bs, QDict *options, int flags,
>>      Error *local_err = NULL;
>>      const char *filename;
>>
>> -    QemuOpts *opts = qemu_opts_create_nofail(&raw_runtime_opts);
>> +    QemuOpts *opts = qemu_opts_create(&raw_runtime_opts, NULL, 1, &error_abort);
>
> Long line, please wrap.
>

hmm my checkpatch script must have issues. Sry.

>>      qemu_opts_absorb_qdict(opts, options, &local_err);
>>      if (error_is_set(&local_err)) {
>>          error_propagate(errp, local_err);
>> diff --git a/block/rbd.c b/block/rbd.c
>> index 4a1ea5b..e7250dd 100644
>> --- a/block/rbd.c
>> +++ b/block/rbd.c
>> @@ -461,7 +461,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
>>      const char *filename;
>>      int r;
>>
>> -    opts = qemu_opts_create_nofail(&runtime_opts);
>> +    opts = qemu_opts_create(&runtime_opts, NULL, 1, &error_abort);
>>      qemu_opts_absorb_qdict(opts, options, &local_err);
>>      if (error_is_set(&local_err)) {
>>          qerror_report_err(local_err);
>> diff --git a/block/sheepdog.c b/block/sheepdog.c
>> index ef387de..12f0b3f 100644
>> --- a/block/sheepdog.c
>> +++ b/block/sheepdog.c
>> @@ -1375,7 +1375,7 @@ static int sd_open(BlockDriverState *bs, QDict *options, int flags,
>>
>>      s->bs = bs;
>>
>> -    opts = qemu_opts_create_nofail(&runtime_opts);
>> +    opts = qemu_opts_create(&runtime_opts, NULL, 1, &error_abort);
>>      qemu_opts_absorb_qdict(opts, options, &local_err);
>>      if (error_is_set(&local_err)) {
>>          qerror_report_err(local_err);
>> diff --git a/block/vvfat.c b/block/vvfat.c
>> index 3ddaa0b..57bc0f9 100644
>> --- a/block/vvfat.c
>> +++ b/block/vvfat.c
>> @@ -1085,7 +1085,7 @@ DLOG(if (stderr == NULL) {
>>      setbuf(stderr, NULL);
>>  })
>>
>> -    opts = qemu_opts_create_nofail(&runtime_opts);
>> +    opts = qemu_opts_create(&runtime_opts, NULL, 1, &error_abort);
>>      qemu_opts_absorb_qdict(opts, options, &local_err);
>>      if (error_is_set(&local_err)) {
>>          qerror_report_err(local_err);
>> diff --git a/blockdev.c b/blockdev.c
>> index 330aa4a..78e8455 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -682,7 +682,8 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
>>      bs_opts = qdict_new();
>>      qemu_opts_to_qdict(all_opts, bs_opts);
>>
>> -    legacy_opts = qemu_opts_create_nofail(&qemu_legacy_drive_opts);
>> +    legacy_opts = qemu_opts_create(&qemu_legacy_drive_opts, NULL, 1,
>> +                                   &error_abort);
>>      qemu_opts_absorb_qdict(legacy_opts, bs_opts, &local_err);
>>      if (error_is_set(&local_err)) {
>>          qerror_report_err(local_err);
>> @@ -853,7 +854,8 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
>>
>>      if (type == IF_VIRTIO) {
>>          QemuOpts *devopts;
>> -        devopts = qemu_opts_create_nofail(qemu_find_opts("device"));
>> +        devopts = qemu_opts_create(qemu_find_opts("device"), NULL, 1,
>> +                                   &error_abort);
>>          if (arch_type == QEMU_ARCH_S390X) {
>>              qemu_opt_set(devopts, "driver", "virtio-blk-s390");
>>          } else {
>> diff --git a/hw/watchdog/watchdog.c b/hw/watchdog/watchdog.c
>> index 387962e..0ab2a0e 100644
>> --- a/hw/watchdog/watchdog.c
>> +++ b/hw/watchdog/watchdog.c
>> @@ -66,7 +66,8 @@ int select_watchdog(const char *p)
>>      QLIST_FOREACH(model, &watchdog_list, entry) {
>>          if (strcasecmp(model->wdt_name, p) == 0) {
>>              /* add the device */
>> -            opts = qemu_opts_create_nofail(qemu_find_opts("device"));
>> +            opts = qemu_opts_create(qemu_find_opts("device"), NULL, 1,
>> +                                    &error_abort);
>>              qemu_opt_set(opts, "driver", p);
>>              return 0;
>>          }
>> diff --git a/include/qemu/option.h b/include/qemu/option.h
>> index 5c0c6dd..3ea871a 100644
>> --- a/include/qemu/option.h
>> +++ b/include/qemu/option.h
>> @@ -136,7 +136,6 @@ int qemu_opt_foreach(QemuOpts *opts, qemu_opt_loopfunc func, void *opaque,
>>  QemuOpts *qemu_opts_find(QemuOptsList *list, const char *id);
>>  QemuOpts *qemu_opts_create(QemuOptsList *list, const char *id,
>>                             int fail_if_exists, Error **errp);
>> -QemuOpts *qemu_opts_create_nofail(QemuOptsList *list);
>>  void qemu_opts_reset(QemuOptsList *list);
>>  void qemu_opts_loc_restore(QemuOpts *opts);
>>  int qemu_opts_set(QemuOptsList *list, const char *id,
>> diff --git a/qdev-monitor.c b/qdev-monitor.c
>> index dc37a43..dd6e68c 100644
>> --- a/qdev-monitor.c
>> +++ b/qdev-monitor.c
>> @@ -730,7 +730,7 @@ int qemu_global_option(const char *str)
>>          return -1;
>>      }
>>
>> -    opts = qemu_opts_create_nofail(&qemu_global_opts);
>> +    opts = qemu_opts_create(&qemu_global_opts, NULL, 1, &error_abort);
>>      qemu_opt_set(opts, "driver", driver);
>>      qemu_opt_set(opts, "property", property);
>>      qemu_opt_set(opts, "value", str+offset+1);
>> diff --git a/qemu-img.c b/qemu-img.c
>> index b6b5644..4f6b519 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -2477,7 +2477,7 @@ static int img_resize(int argc, char **argv)
>>      }
>>
>>      /* Parse size */
>> -    param = qemu_opts_create_nofail(&resize_options);
>> +    param = qemu_opts_create(&resize_options, NULL, 1, &error_abort);
>>      if (qemu_opt_set(param, BLOCK_OPT_SIZE, size)) {
>>          /* Error message already printed when size parsing fails */
>>          ret = -1;
>> diff --git a/util/qemu-config.c b/util/qemu-config.c
>> index 04da942..a803922 100644
>> --- a/util/qemu-config.c
>> +++ b/util/qemu-config.c
>> @@ -311,7 +311,7 @@ int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname)
>>                  error_free(local_err);
>>                  goto out;
>>              }
>> -            opts = qemu_opts_create_nofail(list);
>> +            opts = qemu_opts_create(list, NULL, 1, &error_abort);
>>              continue;
>>          }
>>          if (sscanf(line, " %63s = \"%1023[^\"]\"", arg, value) == 2) {
>> diff --git a/util/qemu-option.c b/util/qemu-option.c
>> index efcb5dc..668e5d9 100644
>> --- a/util/qemu-option.c
>> +++ b/util/qemu-option.c
>> @@ -791,15 +791,6 @@ QemuOpts *qemu_opts_create(QemuOptsList *list, const char *id,
>>      return opts;
>>  }
>>
>> -QemuOpts *qemu_opts_create_nofail(QemuOptsList *list)
>> -{
>> -    QemuOpts *opts;
>> -    Error *errp = NULL;
>> -    opts = qemu_opts_create(list, NULL, 0, &errp);
>> -    assert_no_error(errp);
>> -    return opts;
>> -}
>> -
>>  void qemu_opts_reset(QemuOptsList *list)
>>  {
>>      QemuOpts *opts, *next_opts;
>> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
>> index 6b97dc1..9d50f43 100644
>> --- a/util/qemu-sockets.c
>> +++ b/util/qemu-sockets.c
>> @@ -578,7 +578,7 @@ int inet_listen(const char *str, char *ostr, int olen,
>>
>>      addr = inet_parse(str, errp);
>>      if (addr != NULL) {
>> -        opts = qemu_opts_create_nofail(&socket_optslist);
>> +        opts = qemu_opts_create(&socket_optslist, NULL, 1, &error_abort);
>>          inet_addr_to_opts(opts, addr);
>>          qapi_free_InetSocketAddress(addr);
>>          sock = inet_listen_opts(opts, port_offset, errp);
>> @@ -617,7 +617,7 @@ int inet_connect(const char *str, Error **errp)
>>
>>      addr = inet_parse(str, errp);
>>      if (addr != NULL) {
>> -        opts = qemu_opts_create_nofail(&socket_optslist);
>> +        opts = qemu_opts_create(&socket_optslist, NULL, 1, &error_abort);
>>          inet_addr_to_opts(opts, addr);
>>          qapi_free_InetSocketAddress(addr);
>>          sock = inet_connect_opts(opts, errp, NULL, NULL);
>> @@ -651,7 +651,7 @@ int inet_nonblocking_connect(const char *str,
>>
>>      addr = inet_parse(str, errp);
>>      if (addr != NULL) {
>> -        opts = qemu_opts_create_nofail(&socket_optslist);
>> +        opts = qemu_opts_create(&socket_optslist, NULL, 1, &error_abort);
>>          inet_addr_to_opts(opts, addr);
>>          qapi_free_InetSocketAddress(addr);
>>          sock = inet_connect_opts(opts, errp, callback, opaque);
>> @@ -794,7 +794,7 @@ int unix_listen(const char *str, char *ostr, int olen, Error **errp)
>>      char *path, *optstr;
>>      int sock, len;
>>
>> -    opts = qemu_opts_create_nofail(&socket_optslist);
>> +    opts = qemu_opts_create(&socket_optslist, NULL, 1, &error_abort);
>>
>>      optstr = strchr(str, ',');
>>      if (optstr) {
>> @@ -822,7 +822,7 @@ int unix_connect(const char *path, Error **errp)
>>      QemuOpts *opts;
>>      int sock;
>>
>> -    opts = qemu_opts_create_nofail(&socket_optslist);
>> +    opts = qemu_opts_create(&socket_optslist, NULL, 1, &error_abort);
>>      qemu_opt_set(opts, "path", path);
>>      sock = unix_connect_opts(opts, errp, NULL, NULL);
>>      qemu_opts_del(opts);
>> @@ -839,7 +839,7 @@ int unix_nonblocking_connect(const char *path,
>>
>>      g_assert(callback != NULL);
>>
>> -    opts = qemu_opts_create_nofail(&socket_optslist);
>> +    opts = qemu_opts_create(&socket_optslist, NULL, 1, &error_abort);
>>      qemu_opt_set(opts, "path", path);
>>      sock = unix_connect_opts(opts, errp, callback, opaque);
>>      qemu_opts_del(opts);
>> @@ -889,7 +889,7 @@ int socket_connect(SocketAddress *addr, Error **errp,
>>      QemuOpts *opts;
>>      int fd;
>>
>> -    opts = qemu_opts_create_nofail(&socket_optslist);
>> +    opts = qemu_opts_create(&socket_optslist, NULL, 1, &error_abort);
>>      switch (addr->kind) {
>>      case SOCKET_ADDRESS_KIND_INET:
>>          inet_addr_to_opts(opts, addr->inet);
>> @@ -921,7 +921,7 @@ int socket_listen(SocketAddress *addr, Error **errp)
>>      QemuOpts *opts;
>>      int fd;
>>
>> -    opts = qemu_opts_create_nofail(&socket_optslist);
>> +    opts = qemu_opts_create(&socket_optslist, NULL, 1, &error_abort);
>>      switch (addr->kind) {
>>      case SOCKET_ADDRESS_KIND_INET:
>>          inet_addr_to_opts(opts, addr->inet);
>> @@ -949,7 +949,7 @@ int socket_dgram(SocketAddress *remote, SocketAddress *local, Error **errp)
>>      QemuOpts *opts;
>>      int fd;
>>
>> -    opts = qemu_opts_create_nofail(&socket_optslist);
>> +    opts = qemu_opts_create(&socket_optslist, NULL, 1, &error_abort);
>>      switch (remote->kind) {
>>      case SOCKET_ADDRESS_KIND_INET:
>>          qemu_opt_set(opts, "host", remote->inet->host);
>> diff --git a/vl.c b/vl.c
>> index 8d5d874..4d5704b 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -543,7 +543,7 @@ QemuOpts *qemu_get_machine_opts(void)
>>      assert(list);
>>      opts = qemu_opts_find(list, NULL);
>>      if (!opts) {
>> -        opts = qemu_opts_create_nofail(list);
>> +        opts = qemu_opts_create(list, NULL, 1, &error_abort);
>>      }
>>      return opts;
>>  }
>> @@ -2252,7 +2252,8 @@ static int balloon_parse(const char *arg)
>>                  return  -1;
>>          } else {
>>              /* create empty opts */
>> -            opts = qemu_opts_create_nofail(qemu_find_opts("device"));
>> +            opts = qemu_opts_create(qemu_find_opts("device"), NULL, 1,
>> +                                    &error_abort);
>>          }
>>          qemu_opt_set(opts, "driver", "virtio-balloon");
>>          return 0;
>> @@ -2512,14 +2513,14 @@ static int virtcon_parse(const char *devname)
>>          exit(1);
>>      }
>>
>> -    bus_opts = qemu_opts_create_nofail(device);
>> +    bus_opts = qemu_opts_create(device, NULL, 1, &error_abort);
>>      if (arch_type == QEMU_ARCH_S390X) {
>>          qemu_opt_set(bus_opts, "driver", "virtio-serial-s390");
>>      } else {
>>          qemu_opt_set(bus_opts, "driver", "virtio-serial-pci");
>>      }
>>
>> -    dev_opts = qemu_opts_create_nofail(device);
>> +    dev_opts = qemu_opts_create(device, NULL, 1, &error_abort);
>>      qemu_opt_set(dev_opts, "driver", "virtconsole");
>>
>>      snprintf(label, sizeof(label), "virtcon%d", index);
>> @@ -3376,8 +3377,9 @@ int main(int argc, char **argv, char **envp)
>>                  }
>>
>>                  qemu_opt_set_bool(fsdev, "readonly",
>> -                                qemu_opt_get_bool(opts, "readonly", 0));
>> -                device = qemu_opts_create_nofail(qemu_find_opts("device"));
>> +                                  qemu_opt_get_bool(opts, "readonly", 0));
>
> Spurious whitespace change, please fix.
>

Is the change incorrect? I just did it as it was right next to my
change and corrects relative to the local formatting.

Regards,
Peter

>> +                device = qemu_opts_create(qemu_find_opts("device"), NULL, 1,
>> +                                          &error_abort);
>>                  qemu_opt_set(device, "driver", "virtio-9p-pci");
>>                  qemu_opt_set(device, "fsdev",
>>                               qemu_opt_get(opts, "mount_tag"));
>> @@ -3397,7 +3399,8 @@ int main(int argc, char **argv, char **envp)
>>                  }
>>                  qemu_opt_set(fsdev, "fsdriver", "synth");
>>
>> -                device = qemu_opts_create_nofail(qemu_find_opts("device"));
>> +                device = qemu_opts_create(qemu_find_opts("device"), NULL, 1,
>> +                                          &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");
>

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [RFC PATCH v1 4/5] qemu-option: Remove qemu_opts_create_nofail
  2013-12-03 10:17     ` Peter Crosthwaite
@ 2013-12-03 10:44       ` Markus Armbruster
  0 siblings, 0 replies; 25+ messages in thread
From: Markus Armbruster @ 2013-12-03 10:44 UTC (permalink / raw)
  To: Peter Crosthwaite
  Cc: Igor Mammedov, Paolo Bonzini, qemu-devel@nongnu.org Developers,
	Andreas Färber

Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes:

> On Tue, Dec 3, 2013 at 7:42 PM, Markus Armbruster <armbru@redhat.com> wrote:
>> Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes:
[...]
>>> @@ -3376,8 +3377,9 @@ int main(int argc, char **argv, char **envp)
>>>                  }
>>>
>>>                  qemu_opt_set_bool(fsdev, "readonly",
>>> -                                qemu_opt_get_bool(opts, "readonly", 0));
>>> -                device = qemu_opts_create_nofail(qemu_find_opts("device"));
>>> +                                  qemu_opt_get_bool(opts, "readonly", 0));
>>
>> Spurious whitespace change, please fix.
>>
>
> Is the change incorrect? I just did it as it was right next to my
> change and corrects relative to the local formatting.
>
> Regards,
> Peter

Ah, it was an intentional indentation fix, not an editing accident!
Your choice (I guess I'd refrain from it myself).

>>> +                device = qemu_opts_create(qemu_find_opts("device"), NULL, 1,
>>> +                                          &error_abort);
>>>                  qemu_opt_set(device, "driver", "virtio-9p-pci");
>>>                  qemu_opt_set(device, "fsdev",
>>>                               qemu_opt_get(opts, "mount_tag"));
[...]

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [RFC PATCH v1 4/5] qemu-option: Remove qemu_opts_create_nofail
  2013-12-03  9:42   ` Markus Armbruster
  2013-12-03 10:17     ` Peter Crosthwaite
@ 2013-12-04  6:45     ` Peter Crosthwaite
  1 sibling, 0 replies; 25+ messages in thread
From: Peter Crosthwaite @ 2013-12-04  6:45 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: Igor Mammedov, qemu-devel@nongnu.org Developers,
	Andreas Färber, Paolo Bonzini

On Tue, Dec 3, 2013 at 7:42 PM, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes:
>
>> This is a boiler-plate _nofail variant of qemu_opts_create. Remove and
>> use error_abort in call sites.
>>
>> A null argument needs to be added for the id field in affected callsites
>> due to inconsistency between the normal and no_fail variants.
>
> Two arguments, actually, NULL id and zero fail_if_exists:
>
> QemuOpts *qemu_opts_create_nofail(QemuOptsList *list)
> {
>     QemuOpts *opts;
>     Error *errp = NULL;
>     opts = qemu_opts_create(list, NULL, 0, &errp);
>     assert_no_error(errp);
>     return opts;
> }
>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> ---
>>
>>  block/blkdebug.c       |  2 +-
>>  block/blkverify.c      |  2 +-
>>  block/curl.c           |  2 +-
>>  block/gluster.c        |  2 +-
>>  block/iscsi.c          |  2 +-
>>  block/nbd.c            |  2 +-
>>  block/qcow2.c          |  2 +-
>>  block/raw-posix.c      |  2 +-
>>  block/raw-win32.c      |  4 ++--
>>  block/rbd.c            |  2 +-
>>  block/sheepdog.c       |  2 +-
>>  block/vvfat.c          |  2 +-
>>  blockdev.c             |  6 ++++--
>>  hw/watchdog/watchdog.c |  3 ++-
>>  include/qemu/option.h  |  1 -
>>  qdev-monitor.c         |  2 +-
>>  qemu-img.c             |  2 +-
>>  util/qemu-config.c     |  2 +-
>>  util/qemu-option.c     |  9 ---------
>>  util/qemu-sockets.c    | 18 +++++++++---------
>>  vl.c                   | 17 ++++++++++-------
>>  21 files changed, 41 insertions(+), 45 deletions(-)
>>
>> diff --git a/block/blkdebug.c b/block/blkdebug.c
>> index 16d2b91..6b49df8 100644
>> --- a/block/blkdebug.c
>> +++ b/block/blkdebug.c
>> @@ -359,7 +359,7 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
>>      const char *filename, *config;
>>      int ret;
>>
>> -    opts = qemu_opts_create_nofail(&runtime_opts);
>> +    opts = qemu_opts_create(&runtime_opts, NULL, 1, &error_abort);
>>      qemu_opts_absorb_qdict(opts, options, &local_err);
>>      if (error_is_set(&local_err)) {
>>          error_propagate(errp, local_err);
>
> Err, doesn't this change fail_if_exists from zero to one?  Same
> everywhere.
>
>> diff --git a/block/blkverify.c b/block/blkverify.c
>> index 3c63528..d901852 100644
>> --- a/block/blkverify.c
>> +++ b/block/blkverify.c
>> @@ -125,7 +125,7 @@ static int blkverify_open(BlockDriverState *bs, QDict *options, int flags,
>>      const char *filename, *raw;
>>      int ret;
>>
>> -    opts = qemu_opts_create_nofail(&runtime_opts);
>> +    opts = qemu_opts_create(&runtime_opts, NULL, 1, &error_abort);
>>      qemu_opts_absorb_qdict(opts, options, &local_err);
>>      if (error_is_set(&local_err)) {
>>          error_propagate(errp, local_err);
>> diff --git a/block/curl.c b/block/curl.c
>> index 5a46f97..25dd433 100644
>> --- a/block/curl.c
>> +++ b/block/curl.c
>> @@ -413,7 +413,7 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
>>          return -EROFS;
>>      }
>>
>> -    opts = qemu_opts_create_nofail(&runtime_opts);
>> +    opts = qemu_opts_create(&runtime_opts, NULL, 1, &error_abort);
>>      qemu_opts_absorb_qdict(opts, options, &local_err);
>>      if (error_is_set(&local_err)) {
>>          qerror_report_err(local_err);
>> diff --git a/block/gluster.c b/block/gluster.c
>> index 877686a..38c1a72 100644
>> --- a/block/gluster.c
>> +++ b/block/gluster.c
>> @@ -298,7 +298,7 @@ static int qemu_gluster_open(BlockDriverState *bs,  QDict *options,
>>      Error *local_err = NULL;
>>      const char *filename;
>>
>> -    opts = qemu_opts_create_nofail(&runtime_opts);
>> +    opts = qemu_opts_create(&runtime_opts, NULL, 1, &error_abort);
>>      qemu_opts_absorb_qdict(opts, options, &local_err);
>>      if (error_is_set(&local_err)) {
>>          qerror_report_err(local_err);
>> diff --git a/block/iscsi.c b/block/iscsi.c
>> index a2d578c..b5a8221 100644
>> --- a/block/iscsi.c
>> +++ b/block/iscsi.c
>> @@ -1241,7 +1241,7 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
>>          return -EINVAL;
>>      }
>>
>> -    opts = qemu_opts_create_nofail(&runtime_opts);
>> +    opts = qemu_opts_create(&runtime_opts, NULL, 1, &error_abort);
>>      qemu_opts_absorb_qdict(opts, options, &local_err);
>>      if (error_is_set(&local_err)) {
>>          qerror_report_err(local_err);
>> diff --git a/block/nbd.c b/block/nbd.c
>> index c8deeee..b281b9c 100644
>> --- a/block/nbd.c
>> +++ b/block/nbd.c
>> @@ -234,7 +234,7 @@ static int nbd_config(BDRVNBDState *s, QDict *options)
>>          return -EINVAL;
>>      }
>>
>> -    s->socket_opts = qemu_opts_create_nofail(&socket_optslist);
>> +    s->socket_opts = qemu_opts_create(&socket_optslist, NULL, 1, &error_abort);
>
> Long line, please wrap.
>

I have this as length 79 and it passes checkpatch. Is it just for the
sake of making some spare space?

Regards,
Peter

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [Qemu-devel] [RFC PATCH v1 5/5] qerror: Remove assert_no_error()
  2013-12-03  5:49 [Qemu-devel] [RFC PATCH v1 0/5] Add error_abort and associated cleanups Peter Crosthwaite
                   ` (3 preceding siblings ...)
  2013-12-03  5:51 ` [Qemu-devel] [RFC PATCH v1 4/5] qemu-option: Remove qemu_opts_create_nofail Peter Crosthwaite
@ 2013-12-03  5:52 ` Peter Crosthwaite
  2013-12-03  9:44 ` [Qemu-devel] [RFC PATCH v1 0/5] Add error_abort and associated cleanups Markus Armbruster
  5 siblings, 0 replies; 25+ messages in thread
From: Peter Crosthwaite @ 2013-12-03  5:52 UTC (permalink / raw)
  To: qemu-devel; +Cc: imammedo, afaerber, armbru, pbonzini

This is no longer needed, and is obsoleted by error_abort. Remove.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 include/qapi/qmp/qerror.h | 1 -
 qobject/qerror.c          | 8 --------
 2 files changed, 9 deletions(-)

diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
index c30c2f6..73c67b7 100644
--- a/include/qapi/qmp/qerror.h
+++ b/include/qapi/qmp/qerror.h
@@ -29,7 +29,6 @@ typedef struct QError {
 QString *qerror_human(const QError *qerror);
 void qerror_report(ErrorClass err_class, const char *fmt, ...) GCC_FMT_ATTR(2, 3);
 void qerror_report_err(Error *err);
-void assert_no_error(Error *err);
 
 /*
  * QError class list
diff --git a/qobject/qerror.c b/qobject/qerror.c
index 3aee1cf..1b56858 100644
--- a/qobject/qerror.c
+++ b/qobject/qerror.c
@@ -121,14 +121,6 @@ void qerror_report_err(Error *err)
     }
 }
 
-void assert_no_error(Error *err)
-{
-    if (err) {
-        qerror_report_err(err);
-        abort();
-    }
-}
-
 /**
  * qobject_to_qerror(): Convert a QObject into a QError
  */
-- 
1.8.4.4

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [RFC PATCH v1 0/5] Add error_abort and associated cleanups
  2013-12-03  5:49 [Qemu-devel] [RFC PATCH v1 0/5] Add error_abort and associated cleanups Peter Crosthwaite
                   ` (4 preceding siblings ...)
  2013-12-03  5:52 ` [Qemu-devel] [RFC PATCH v1 5/5] qerror: Remove assert_no_error() Peter Crosthwaite
@ 2013-12-03  9:44 ` Markus Armbruster
  2013-12-03 11:49   ` Igor Mammedov
                     ` (2 more replies)
  5 siblings, 3 replies; 25+ messages in thread
From: Markus Armbruster @ 2013-12-03  9:44 UTC (permalink / raw)
  To: Peter Crosthwaite; +Cc: imammedo, pbonzini, qemu-devel, afaerber

Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes:

> Following our discussion RE self asserting API calls, here is a spin of
> my proposal. This series obsoletes the need for _nofail variants for
> Error ** accepting APIs. Is also greately reduces the verbosity of calls
> sites that are currently asserting against errors.
>
> Patch 1 is the main event - addition of error_abort. The following
> patches then cleanup uses of _nofail and assert_no_error().
>
> To give it a smoke test, I introduce a (critical) bug into QOM:
[...]
>  32 files changed, 100 insertions(+), 143 deletions(-)

I like it.  Nice diffstat, too.

There are some _nofail functions left, but none of them can use
error_abort.

If anything assigns to error_abort, we're probably screwed.  No bright
idea how to prevent that at compile-time.

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [RFC PATCH v1 0/5] Add error_abort and associated cleanups
  2013-12-03  9:44 ` [Qemu-devel] [RFC PATCH v1 0/5] Add error_abort and associated cleanups Markus Armbruster
@ 2013-12-03 11:49   ` Igor Mammedov
  2013-12-03 11:57   ` Paolo Bonzini
  2013-12-03 12:58   ` Eric Blake
  2 siblings, 0 replies; 25+ messages in thread
From: Igor Mammedov @ 2013-12-03 11:49 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: pbonzini, Peter Crosthwaite, qemu-devel, afaerber

On Tue, 03 Dec 2013 10:44:57 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes:
> 
> > Following our discussion RE self asserting API calls, here is a spin of
> > my proposal. This series obsoletes the need for _nofail variants for
> > Error ** accepting APIs. Is also greately reduces the verbosity of calls
> > sites that are currently asserting against errors.
> >
> > Patch 1 is the main event - addition of error_abort. The following
> > patches then cleanup uses of _nofail and assert_no_error().
> >
> > To give it a smoke test, I introduce a (critical) bug into QOM:
> [...]
> >  32 files changed, 100 insertions(+), 143 deletions(-)
> 
> I like it.  Nice diffstat, too.
> 
> There are some _nofail functions left, but none of them can use
> error_abort.
> 
> If anything assigns to error_abort, we're probably screwed.  No bright
> idea how to prevent that at compile-time.

Maybe
 
Error * const error_abort; 

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [RFC PATCH v1 0/5] Add error_abort and associated cleanups
  2013-12-03  9:44 ` [Qemu-devel] [RFC PATCH v1 0/5] Add error_abort and associated cleanups Markus Armbruster
  2013-12-03 11:49   ` Igor Mammedov
@ 2013-12-03 11:57   ` Paolo Bonzini
  2013-12-03 12:03     ` Peter Crosthwaite
  2013-12-03 12:58   ` Eric Blake
  2 siblings, 1 reply; 25+ messages in thread
From: Paolo Bonzini @ 2013-12-03 11:57 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: imammedo, Peter Crosthwaite, qemu-devel, afaerber

Il 03/12/2013 10:44, Markus Armbruster ha scritto:
> 
> There are some _nofail functions left, but none of them can use
> error_abort.
> 
> If anything assigns to error_abort, we're probably screwed.  No bright
> idea how to prevent that at compile-time.

Isn't it always used by address?

Paolo

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [RFC PATCH v1 0/5] Add error_abort and associated cleanups
  2013-12-03 11:57   ` Paolo Bonzini
@ 2013-12-03 12:03     ` Peter Crosthwaite
  0 siblings, 0 replies; 25+ messages in thread
From: Peter Crosthwaite @ 2013-12-03 12:03 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Igor Mammedov, Markus Armbruster, Andreas Färber,
	qemu-devel@nongnu.org Developers

On Tue, Dec 3, 2013 at 9:57 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 03/12/2013 10:44, Markus Armbruster ha scritto:
>>
>> There are some _nofail functions left, but none of them can use
>> error_abort.
>>
>> If anything assigns to error_abort, we're probably screwed.  No bright
>> idea how to prevent that at compile-time.
>
> Isn't it always used by address?
>

Yes, but my implementation has reliance on it being null.

That can be fixed with a bit a statement re-ordering.

Regards,
Peter

> Paolo
>

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [RFC PATCH v1 0/5] Add error_abort and associated cleanups
  2013-12-03  9:44 ` [Qemu-devel] [RFC PATCH v1 0/5] Add error_abort and associated cleanups Markus Armbruster
  2013-12-03 11:49   ` Igor Mammedov
  2013-12-03 11:57   ` Paolo Bonzini
@ 2013-12-03 12:58   ` Eric Blake
  2013-12-03 13:53     ` Markus Armbruster
  2 siblings, 1 reply; 25+ messages in thread
From: Eric Blake @ 2013-12-03 12:58 UTC (permalink / raw)
  To: Markus Armbruster, Peter Crosthwaite
  Cc: imammedo, qemu-devel, afaerber, pbonzini

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

On 12/03/2013 02:44 AM, Markus Armbruster wrote:
> Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes:
> 
>> Following our discussion RE self asserting API calls, here is a spin of
>> my proposal. This series obsoletes the need for _nofail variants for
>> Error ** accepting APIs. Is also greately reduces the verbosity of calls
>> sites that are currently asserting against errors.
>>
>> Patch 1 is the main event - addition of error_abort. The following
>> patches then cleanup uses of _nofail and assert_no_error().
>>
>> To give it a smoke test, I introduce a (critical) bug into QOM:
> [...]
>>  32 files changed, 100 insertions(+), 143 deletions(-)
> 
> I like it.  Nice diffstat, too.
> 
> There are some _nofail functions left, but none of them can use
> error_abort.
> 

Also, is it worth adding asserts and/or compiler annotations to require
that the Error **err argument of functions be non-NULL, to ensure that
callers are always passing either a valid destination or one of the
special addresses?  But doing so would probably require adding a special
address for error_ignore for callers that intend to discard an error in
cases where the return type of the function lets them know to proceed
with a fallback implementation (that is, cases where ignoring an error
makes sense).

-- 
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: 621 bytes --]

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [RFC PATCH v1 0/5] Add error_abort and associated cleanups
  2013-12-03 12:58   ` Eric Blake
@ 2013-12-03 13:53     ` Markus Armbruster
  2013-12-03 20:33       ` Igor Mammedov
  0 siblings, 1 reply; 25+ messages in thread
From: Markus Armbruster @ 2013-12-03 13:53 UTC (permalink / raw)
  To: Eric Blake; +Cc: imammedo, Peter Crosthwaite, pbonzini, qemu-devel, afaerber

Eric Blake <eblake@redhat.com> writes:

> On 12/03/2013 02:44 AM, Markus Armbruster wrote:
>> Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes:
>> 
>>> Following our discussion RE self asserting API calls, here is a spin of
>>> my proposal. This series obsoletes the need for _nofail variants for
>>> Error ** accepting APIs. Is also greately reduces the verbosity of calls
>>> sites that are currently asserting against errors.
>>>
>>> Patch 1 is the main event - addition of error_abort. The following
>>> patches then cleanup uses of _nofail and assert_no_error().
>>>
>>> To give it a smoke test, I introduce a (critical) bug into QOM:
>> [...]
>>>  32 files changed, 100 insertions(+), 143 deletions(-)
>> 
>> I like it.  Nice diffstat, too.
>> 
>> There are some _nofail functions left, but none of them can use
>> error_abort.
>> 
>
> Also, is it worth adding asserts and/or compiler annotations to require
> that the Error **err argument of functions be non-NULL, to ensure that
> callers are always passing either a valid destination or one of the
> special addresses?  But doing so would probably require adding a special
> address for error_ignore for callers that intend to discard an error in
> cases where the return type of the function lets them know to proceed
> with a fallback implementation (that is, cases where ignoring an error
> makes sense).

Right now, we use NULL as "ignore errors" argument.

NULL gives us a chance to express "caller must not ignore errors" via
some non-null annotation that gets fed to a static analyzer.

I doubt that would be possible with a special error_ignore object.

Anyway, this series is about "abort on error".  Let's keep "ignore
errors" issues separate.

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [RFC PATCH v1 0/5] Add error_abort and associated cleanups
  2013-12-03 13:53     ` Markus Armbruster
@ 2013-12-03 20:33       ` Igor Mammedov
  2013-12-03 20:43         ` Eric Blake
  2013-12-05 10:37         ` Paolo Bonzini
  0 siblings, 2 replies; 25+ messages in thread
From: Igor Mammedov @ 2013-12-03 20:33 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: pbonzini, Peter Crosthwaite, qemu-devel, afaerber

On Tue, 03 Dec 2013 14:53:06 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Eric Blake <eblake@redhat.com> writes:
> 
> > On 12/03/2013 02:44 AM, Markus Armbruster wrote:
> >> Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes:
> >> 
> >>> Following our discussion RE self asserting API calls, here is a spin of
> >>> my proposal. This series obsoletes the need for _nofail variants for
> >>> Error ** accepting APIs. Is also greately reduces the verbosity of calls
> >>> sites that are currently asserting against errors.
> >>>
> >>> Patch 1 is the main event - addition of error_abort. The following
> >>> patches then cleanup uses of _nofail and assert_no_error().
> >>>
> >>> To give it a smoke test, I introduce a (critical) bug into QOM:
> >> [...]
> >>>  32 files changed, 100 insertions(+), 143 deletions(-)
> >> 
> >> I like it.  Nice diffstat, too.
> >> 
> >> There are some _nofail functions left, but none of them can use
> >> error_abort.
> >> 
> >
> > Also, is it worth adding asserts and/or compiler annotations to require
> > that the Error **err argument of functions be non-NULL, to ensure that
> > callers are always passing either a valid destination or one of the
> > special addresses?  But doing so would probably require adding a special
> > address for error_ignore for callers that intend to discard an error in
> > cases where the return type of the function lets them know to proceed
> > with a fallback implementation (that is, cases where ignoring an error
> > makes sense).
> 
> Right now, we use NULL as "ignore errors" argument.
> 
> NULL gives us a chance to express "caller must not ignore errors" via
> some non-null annotation that gets fed to a static analyzer.
> 
> I doubt that would be possible with a special error_ignore object.
> 
> Anyway, this series is about "abort on error".  Let's keep "ignore
> errors" issues separate.
I'm sorry for hijacking thread, but that actually an issue that started an
original discussion.
Where void returning QOM API functions are used with NULL, without any chance
to detect that error happened. So abusing NULL errp in this functions
might lead to hard to find runtime errors.
I think Eric's suggestion was to enforce passing non NULL errp and let caller
to deal with error gracefully so that above mentioned misuse was impossible.
Why is ignoring errors from "void foo(...)" like API considered acceptable?


-- 
Regards,
  Igor

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [RFC PATCH v1 0/5] Add error_abort and associated cleanups
  2013-12-03 20:33       ` Igor Mammedov
@ 2013-12-03 20:43         ` Eric Blake
  2013-12-04  9:11           ` Markus Armbruster
  2013-12-05 10:37         ` Paolo Bonzini
  1 sibling, 1 reply; 25+ messages in thread
From: Eric Blake @ 2013-12-03 20:43 UTC (permalink / raw)
  To: Igor Mammedov, Markus Armbruster
  Cc: pbonzini, Peter Crosthwaite, qemu-devel, afaerber

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

On 12/03/2013 01:33 PM, Igor Mammedov wrote:

>>> Also, is it worth adding asserts and/or compiler annotations to require
>>> that the Error **err argument of functions be non-NULL, to ensure that
>>> callers are always passing either a valid destination or one of the
>>> special addresses?  But doing so would probably require adding a special
>>> address for error_ignore for callers that intend to discard an error in
>>> cases where the return type of the function lets them know to proceed
>>> with a fallback implementation (that is, cases where ignoring an error
>>> makes sense).
>>
>> Right now, we use NULL as "ignore errors" argument.
>>
>> NULL gives us a chance to express "caller must not ignore errors" via
>> some non-null annotation that gets fed to a static analyzer.
>>
>> I doubt that would be possible with a special error_ignore object.
>>
>> Anyway, this series is about "abort on error".  Let's keep "ignore
>> errors" issues separate.
> I'm sorry for hijacking thread, but that actually an issue that started an
> original discussion.
> Where void returning QOM API functions are used with NULL, without any chance
> to detect that error happened. So abusing NULL errp in this functions
> might lead to hard to find runtime errors.
> I think Eric's suggestion was to enforce passing non NULL errp and let caller
> to deal with error gracefully so that above mentioned misuse was impossible.
> Why is ignoring errors from "void foo(...)" like API considered acceptable?

Okay, so it sounds like consensus is that using NULL as the means to
ignore errors is okay when there is an alternative way to detect error,
but that for any function that returns void, adding an assert(errp)
would be appropriate because the caller cannot safely ignore the
failure.  It's not worth inventing an error_ignore special address, but
for functions that have no way to report errors except via errp, then it
IS worth enforcing that the caller is either prepared to handle the
error or has passed &error_abort (or any other special addresses we add
later).

-- 
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: 621 bytes --]

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [RFC PATCH v1 0/5] Add error_abort and associated cleanups
  2013-12-03 20:43         ` Eric Blake
@ 2013-12-04  9:11           ` Markus Armbruster
  2013-12-04 14:46             ` Eric Blake
  0 siblings, 1 reply; 25+ messages in thread
From: Markus Armbruster @ 2013-12-04  9:11 UTC (permalink / raw)
  To: Eric Blake
  Cc: Igor Mammedov, Peter Crosthwaite, qemu-devel, afaerber, pbonzini

Eric Blake <eblake@redhat.com> writes:

> On 12/03/2013 01:33 PM, Igor Mammedov wrote:
>
>>>> Also, is it worth adding asserts and/or compiler annotations to require
>>>> that the Error **err argument of functions be non-NULL, to ensure that
>>>> callers are always passing either a valid destination or one of the
>>>> special addresses?  But doing so would probably require adding a special
>>>> address for error_ignore for callers that intend to discard an error in
>>>> cases where the return type of the function lets them know to proceed
>>>> with a fallback implementation (that is, cases where ignoring an error
>>>> makes sense).
>>>
>>> Right now, we use NULL as "ignore errors" argument.
>>>
>>> NULL gives us a chance to express "caller must not ignore errors" via
>>> some non-null annotation that gets fed to a static analyzer.
>>>
>>> I doubt that would be possible with a special error_ignore object.
>>>
>>> Anyway, this series is about "abort on error".  Let's keep "ignore
>>> errors" issues separate.
>> I'm sorry for hijacking thread, but that actually an issue that started an
>> original discussion.
>> Where void returning QOM API functions are used with NULL, without any chance
>> to detect that error happened. So abusing NULL errp in this functions
>> might lead to hard to find runtime errors.
>> I think Eric's suggestion was to enforce passing non NULL errp and let caller
>> to deal with error gracefully so that above mentioned misuse was impossible.
>> Why is ignoring errors from "void foo(...)" like API considered acceptable?
>
> Okay, so it sounds like consensus is that using NULL as the means to
> ignore errors is okay when there is an alternative way to detect error,
> but that for any function that returns void, adding an assert(errp)
> would be appropriate because the caller cannot safely ignore the
> failure.  It's not worth inventing an error_ignore special address, but
> for functions that have no way to report errors except via errp, then it
> IS worth enforcing that the caller is either prepared to handle the
> error or has passed &error_abort (or any other special addresses we add
> later).

No objection to asserting that the caller passed an error object when
the error object is the only way to signal failure.  You can't force
your callers to check for failure, but the assertion could help prevent
accidental misuse.

Assertions fire at run-time, though.

Asserting "argument not null" first thing in the function should enable
a sufficiently smart whole-program static checker to flag null
arguments.

But having such a static check right at compile-time would be much
better.  Could attribute nonnull do it?  If yes, do we still need the
assertion?

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [RFC PATCH v1 0/5] Add error_abort and associated cleanups
  2013-12-04  9:11           ` Markus Armbruster
@ 2013-12-04 14:46             ` Eric Blake
  0 siblings, 0 replies; 25+ messages in thread
From: Eric Blake @ 2013-12-04 14:46 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: Igor Mammedov, Peter Crosthwaite, qemu-devel, afaerber, pbonzini

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

On 12/04/2013 02:11 AM, Markus Armbruster wrote:

> No objection to asserting that the caller passed an error object when
> the error object is the only way to signal failure.  You can't force
> your callers to check for failure, but the assertion could help prevent
> accidental misuse.
> 
> Assertions fire at run-time, though.

Unfortunately true.

> 
> Asserting "argument not null" first thing in the function should enable
> a sufficiently smart whole-program static checker to flag null
> arguments.

Coverity is such a checker; I think clang can as well.

> 
> But having such a static check right at compile-time would be much
> better.  Could attribute nonnull do it?  If yes, do we still need the
> assertion?

gcc's implementation of attribute nonnull is complete trash.  And the
gcc developers know it.  The attribute is still useful for Coverity, but
at least in libvirt, we have taken to using the attribute ONLY when
compiling under a static checker and omitting it under gcc because gcc's
implementation of the attribute is so horribly botched.

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=17308

So even with attribute nonnull, you still need the assertion.

-- 
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: 621 bytes --]

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [RFC PATCH v1 0/5] Add error_abort and associated cleanups
  2013-12-03 20:33       ` Igor Mammedov
  2013-12-03 20:43         ` Eric Blake
@ 2013-12-05 10:37         ` Paolo Bonzini
  2013-12-05 15:32           ` Igor Mammedov
  1 sibling, 1 reply; 25+ messages in thread
From: Paolo Bonzini @ 2013-12-05 10:37 UTC (permalink / raw)
  To: Igor Mammedov; +Cc: Peter Crosthwaite, Markus Armbruster, afaerber, qemu-devel

Il 03/12/2013 21:33, Igor Mammedov ha scritto:
> I'm sorry for hijacking thread, but that actually an issue that started an
> original discussion.
> Where void returning QOM API functions are used with NULL, without any chance
> to detect that error happened. So abusing NULL errp in this functions
> might lead to hard to find runtime errors.
> I think Eric's suggestion was to enforce passing non NULL errp and let caller
> to deal with error gracefully so that above mentioned misuse was impossible.
> Why is ignoring errors from "void foo(...)" like API considered acceptable?

See http://permalink.gmane.org/gmane.comp.emulators.qemu/243779

> * Peter's alternative
>   + self-documenting
>   + consistent
>   + predictable

I'll add another small advantage which is fewer SLOC.

> * make Error* mandatory for all void functions
>   + consistent
>   + almost predictable (because in C you can ignore return values)
>   - not necessarily does the right thing (e.g. cleanup functions)
>   - requires manual effort to abide to the policy

Better wording of the last: a missing &error_abort is easier to spot
than a missing assert_no_error(errp).

Paolo

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [RFC PATCH v1 0/5] Add error_abort and associated cleanups
  2013-12-05 10:37         ` Paolo Bonzini
@ 2013-12-05 15:32           ` Igor Mammedov
  2013-12-05 15:59             ` Paolo Bonzini
  0 siblings, 1 reply; 25+ messages in thread
From: Igor Mammedov @ 2013-12-05 15:32 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: Peter Crosthwaite, Markus Armbruster, afaerber, qemu-devel

On Thu, 05 Dec 2013 11:37:27 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 03/12/2013 21:33, Igor Mammedov ha scritto:
> > I'm sorry for hijacking thread, but that actually an issue that started an
> > original discussion.
> > Where void returning QOM API functions are used with NULL, without any chance
> > to detect that error happened. So abusing NULL errp in this functions
> > might lead to hard to find runtime errors.
> > I think Eric's suggestion was to enforce passing non NULL errp and let caller
> > to deal with error gracefully so that above mentioned misuse was impossible.
> > Why is ignoring errors from "void foo(...)" like API considered acceptable?
> 
> See http://permalink.gmane.org/gmane.comp.emulators.qemu/243779
> 
> > * Peter's alternative
> >   + self-documenting
> >   + consistent
> >   + predictable
> 
> I'll add another small advantage which is fewer SLOC.
There is not argument against Peter's approach at all,
question is what do we do with NULL errp in void API functions?

> 
> > * make Error* mandatory for all void functions
one more advantage:
+ not need to pepper every property setter/getter with local_error + error_propagate(),
  i.e. reduced code duplication.

> >   + consistent
> >   + almost predictable (because in C you can ignore return values)
there is no return values from void functions.

> >   - not necessarily does the right thing (e.g. cleanup functions)
we can pass &error_abort instead of NULL there if we don't care. If there will
be error it would mean something went horribly wrong and perhaps code
should care if error happens there.

for special cases we could invent &ignore_error if there will be real need for it.

> >   - requires manual effort to abide to the policy
with assert inside API there is no manual effort. But as Marcus
noted these errors will be only runtime detectable :(

> 
> Better wording of the last: a missing &error_abort is easier to spot
> than a missing assert_no_error(errp).
> 
> Paolo

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [RFC PATCH v1 0/5] Add error_abort and associated cleanups
  2013-12-05 15:32           ` Igor Mammedov
@ 2013-12-05 15:59             ` Paolo Bonzini
  0 siblings, 0 replies; 25+ messages in thread
From: Paolo Bonzini @ 2013-12-05 15:59 UTC (permalink / raw)
  To: Igor Mammedov; +Cc: Peter Crosthwaite, Markus Armbruster, afaerber, qemu-devel

Il 05/12/2013 16:32, Igor Mammedov ha scritto:
>>> > > * make Error* mandatory for all void functions
> one more advantage:
> + not need to pepper every property setter/getter with local_error + error_propagate(),
>   i.e. reduced code duplication.

Note that for something like tail calls there is no need for
error_propagate.  See get_enum/set_enum (and a lot more examples) in
hw/core/qdev-properties.c.

With your proposal, this:

    visit_type_bool(v, &value, name, &local_err);
    if (local_err) {
        error_propagate(errp, local_err);
        return;
    }
    bit_prop_set(dev, prop, value);

could indeed become

    visit_type_bool(v, &value, name, errp);
    bit_prop_set(dev, prop, value);

because both caller and callee return void.  This is because both the
caller (prop_set_bit) and callee (visit_type_bool) are void.  But here
come the next point:

>>> > >   + consistent
>>> > >   + almost predictable (because in C you can ignore return values)
> there is no return values from void functions.

You can ignore return values from int-returning functions.  So if I see

   func(..., NULL);
   func(..., errp);

it's not clear if it aborts on error, or just eats the error.

> >   - requires manual effort to abide to the policy
> with assert inside API there is no manual effort. But as Marcus
> noted these errors will be only runtime detectable :(

I referred to manual effort of adding assertions in leaf functions.
Just one missing assertion can be very problematic if non-leafs start
relying on this behavior.

I think Error is hard to misuse if you don't mind being verbose.  This
proposal would cut the verbosity (not sure how much, but it definitely
could) but it would be easier to misuse.

In terms of Rusty's API design manifesto
(http://sweng.the-davies.net/Home/rustys-api-design-manifesto) your
proposal would move the API from 4 to 3 or 2:

    4. Follow common convention and you'll get it right.
    3. Read the documentation and you'll get it right.
    2. Read the implementation and you'll get it right.

Paolo

^ permalink raw reply	[flat|nested] 25+ messages in thread

end of thread, other threads:[~2013-12-05 15:59 UTC | newest]

Thread overview: 25+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-12-03  5:49 [Qemu-devel] [RFC PATCH v1 0/5] Add error_abort and associated cleanups Peter Crosthwaite
2013-12-03  5:49 ` [Qemu-devel] [RFC PATCH v1 1/5] error: Add error_abort Peter Crosthwaite
2013-12-03  5:50 ` [Qemu-devel] [RFC PATCH v1 2/5] hw: Remove assert_no_error usages Peter Crosthwaite
2013-12-03  9:35   ` Markus Armbruster
2013-12-03 10:04     ` Peter Crosthwaite
2013-12-03  5:51 ` [Qemu-devel] [RFC PATCH v1 3/5] target-i386: Remove assert_no_error usage Peter Crosthwaite
2013-12-03  5:51 ` [Qemu-devel] [RFC PATCH v1 4/5] qemu-option: Remove qemu_opts_create_nofail Peter Crosthwaite
2013-12-03  9:42   ` Markus Armbruster
2013-12-03 10:17     ` Peter Crosthwaite
2013-12-03 10:44       ` Markus Armbruster
2013-12-04  6:45     ` Peter Crosthwaite
2013-12-03  5:52 ` [Qemu-devel] [RFC PATCH v1 5/5] qerror: Remove assert_no_error() Peter Crosthwaite
2013-12-03  9:44 ` [Qemu-devel] [RFC PATCH v1 0/5] Add error_abort and associated cleanups Markus Armbruster
2013-12-03 11:49   ` Igor Mammedov
2013-12-03 11:57   ` Paolo Bonzini
2013-12-03 12:03     ` Peter Crosthwaite
2013-12-03 12:58   ` Eric Blake
2013-12-03 13:53     ` Markus Armbruster
2013-12-03 20:33       ` Igor Mammedov
2013-12-03 20:43         ` Eric Blake
2013-12-04  9:11           ` Markus Armbruster
2013-12-04 14:46             ` Eric Blake
2013-12-05 10:37         ` Paolo Bonzini
2013-12-05 15:32           ` Igor Mammedov
2013-12-05 15:59             ` Paolo Bonzini

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).