Add error_abort and associated cleanups
Subsystems (2)
cover Add error_abort and associated cleanups
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:
qom/object.c +0 -0
@@ -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(-)
Signature · 1 line
1.8.4.4
RFC PATCH 1/5 error: Add error_abort
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.
---
include/qapi/error.h | 6 ++++++ util/error.c | 22 +++++++++++++++++++++- 2 files changed, 27 insertions(+), 1 deletion(-)
include/qapi/error.h +6 -0
@@ -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
util/error.c +21 -1
@@ -23,6 +23,8 @@ struct ErrorErrorClass 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);
Signature · 1 line
1.8.4.4
RFC PATCH 2/5 hw: Remove assert_no_error usages
In reply to Peter CrosthwaiteReplace 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.
---
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(-)
hw/core/qdev-properties-system.c +2 -6
@@ -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)
hw/core/qdev-properties.c +10 -30
@@ -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 ; }
hw/core/qdev.c +7 -21
@@ -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. */
hw/dma/xilinx_axidma.c +4 -9
@@ -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);
hw/net/xilinx_axienet.c +4 -9
@@ -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);
include/hw/xilinx.h +4 -10
@@ -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 voidxilinx_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);
Signature · 1 line
1.8.4.4
Re: [Qemu-devel] [RFC PATCH v1 2/5] hw: Remove assert_no_error usages
Quoted 5 lines · Peter Crosthwaite
Replace
assert_no_error()usages with theerror_abortsystem.&error_abortis 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 <[email protected]>
[...]
Quoted 4 lines
hw/core/qdev.c +0
-0
[...]
Quoted 8 lines
hw/core/qdev.c +0
-4@@ -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);
hw/core/qdev.c static void device_initfn(Object *obj) · line 753
Quoted 10 lines
hw/core/qdev.c +2
-5+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?
Quoted 23 lines
hw/core/qdev.c +2
-7object_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. */ hw/dma/xilinx_axidma.c +0
-0
hw/dma/xilinx_axidma.c static void xilinx_axidma_init(Object *obj) · line 574
Quoted 9 lines
hw/dma/xilinx_axidma.c +1
-3@@ -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.
[...]
Re: [Qemu-devel] [RFC PATCH v1 2/5] hw: Remove assert_no_error usages
Quoted 28 lines · Markus Armbruster
Quoted 5 lines · Peter Crosthwaite
Replace
assert_no_error()usages with theerror_abortsystem.&error_abortis 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 <[email protected]>[...]
Quoted 4 lines
hw/core/qdev.c +0
-0[...]
Quoted 15 lines
hw/core/qdev.c +2
-6@@ -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);
Quoted 9 lines
hw/core/qdev.c static void device_initfn(Object *obj) · line 753
Quoted 3 lines
hw/core/qdev.c +0
-3-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 anassert_no_error(), therefore err must still be null here. Correct?
Correct, it's dead code.
Quoted 28 lines
Quoted 28 lines
hw/core/qdev.c +2
-7object_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. */ hw/dma/xilinx_axidma.c +0
-1@@ -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 ;
Quoted 9 lines
hw/dma/xilinx_axidma.c static void xilinx_axidma_init(Object *obj) · line 574
Quoted 4 lines
hw/dma/xilinx_axidma.c +1
-2object_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
Quoted 2 lines
[...]
RFC PATCH 3/5 target-i386: Remove assert_no_error usage
In reply to Peter CrosthwaiteReplace an assert_no_error() usage with the error_abort system.
---
target-i386/cpu.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
target-i386/cpu.c +1 -3
@@ -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 ; }
Signature · 1 line
1.8.4.4
RFC PATCH 4/5 qemu-option: Remove qemu_opts_create_nofail
In reply to Peter CrosthwaiteThis 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.
---
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(-)
block/blkdebug.c +1 -1
@@ -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);
block/blkverify.c +1 -1
@@ -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);
block/curl.c +1 -1
@@ -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);
block/gluster.c +1 -1
@@ -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);
block/iscsi.c +1 -1
@@ -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);
block/nbd.c +1 -1
@@ -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)) {
block/qcow2.c +1 -1
@@ -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);
block/raw-posix.c +1 -1
@@ -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);
block/raw-win32.c +2 -2
@@ -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);
block/rbd.c +1 -1
@@ -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);
block/sheepdog.c +1 -1
@@ -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);
block/vvfat.c +1 -1
@@ -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);
blockdev.c +4 -2
@@ -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 {
hw/watchdog/watchdog.c +2 -1
@@ -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 ; }
include/qemu/option.h +0 -1
@@ -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,
qdev-monitor.c +1 -1
@@ -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 );
qemu-img.c +1 -1
@@ -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 ;
util/qemu-config.c +1 -1
@@ -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 ) {
util/qemu-option.c +0 -9
@@ -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;
util/qemu-sockets.c +9 -9
@@ -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);
vl.c +10 -7
@@ -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" );
Signature · 1 line
1.8.4.4
Re: [Qemu-devel] [RFC PATCH v1 4/5] qemu-option: Remove qemu_opts_create_nofail
Quoted 5 lines · Peter Crosthwaite
This is a boiler-plate
_nofailvariant ofqemu_opts_create. Remove and useerror_abortin call sites.A null argument needs to be added for the id field in affected callsites due to inconsistency between the normal and
no_failvariants.
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;
}Quoted 31 lines
Signed-off-by Peter Crosthwaite <[email protected]>---
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(-)block/blkdebug.c +0
-0
block/blkdebug.c static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags, · line 365
Quoted 8 lines
block/blkdebug.c +1
-1@@ -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.
Quoted 57 lines
block/blkverify.c +1
-1@@ -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);block/curl.c +1
-1@@ -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);block/gluster.c +1
-1@@ -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);block/iscsi.c +1
-1@@ -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);block/nbd.c +0
-0
block/nbd.c static int nbd_config(BDRVNBDState *s, QDict *options) · line 237
Quoted 5 lines
block/nbd.c +1
-1@@ -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.
Quoted 43 lines
block/nbd.c +0
-0qemu_opts_absorb_qdict (s->socket_opts, options, &local_err);if (error_is_set (&local_err)) {block/qcow2.c +1
-1@@ -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);block/raw-posix.c +1
-1@@ -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);block/raw-win32.c +1
-1@@ -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);
block/raw-win32.c static int hdev_open(BlockDriverState *bs, QDict *options, int flags, · line 553
Quoted 5 lines
block/raw-win32.c +1
-1@@ -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.
Quoted 277 lines
block/raw-win32.c +0
-0qemu_opts_absorb_qdict (opts, options, &local_err);if (error_is_set (&local_err)) {error_propagate (errp, local_err);block/rbd.c +1
-1@@ -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);block/sheepdog.c +1
-1@@ -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);block/vvfat.c +1
-1@@ -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);blockdev.c +4
-2@@ -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 {hw/watchdog/watchdog.c +2
-1@@ -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 ; }include/qemu/option.h +0
-1@@ -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,qdev-monitor.c +1
-1@@ -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 );qemu-img.c +1
-1@@ -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 ;util/qemu-config.c +1
-1@@ -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 ) {util/qemu-option.c +0
-9@@ -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;util/qemu-sockets.c +9
-9@@ -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);vl.c +5
-4@@ -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);
vl.c int main(int argc, char **argv, char **envp) · line 3380
Quoted 6 lines
vl.c +1
-2@@ -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.
Quoted 15 lines
vl.c +4
-1+ 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" );
Re: [Qemu-devel] [RFC PATCH v1 4/5] qemu-option: Remove qemu_opts_create_nofail
Quoted 10 lines · Markus Armbruster
Quoted 5 lines · Peter Crosthwaite
This is a boiler-plate
_nofailvariant ofqemu_opts_create. Remove and useerror_abortin call sites.A null argument needs to be added for the id field in affected callsites due to inconsistency between the normal and
no_failvariants.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.
Quoted 43 lines
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; }Quoted 34 lines
Signed-off-by Peter Crosthwaite <[email protected]>---
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(-)block/blkdebug.c +0
-0@@ -359,7 +359,7 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,const char *filename, *config;int ret;
Quoted 9 lines
block/blkdebug.c static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags, · line 365
Quoted 5 lines
block/blkdebug.c +1
-1- 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_existsfrom zero to one? Same everywhere.
Yes. Rash assumption my by me that the nofail version fails_on_exist. Will fix.
Quoted 111 lines
Quoted 57 lines
block/blkverify.c +1
-1@@ -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);block/curl.c +1
-1@@ -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);block/gluster.c +1
-1@@ -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);block/iscsi.c +1
-1@@ -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);block/nbd.c +0
-0block/nbd.c static int nbd_config(BDRVNBDState *s, QDict *options) · line 237
Quoted 5 lines
block/nbd.c +1
-1@@ -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.
Quoted 46 lines
block/nbd.c +0
-0qemu_opts_absorb_qdict (s->socket_opts, options, &local_err);if (error_is_set (&local_err)) {block/qcow2.c +1
-1@@ -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);block/raw-posix.c +1
-1@@ -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);block/raw-win32.c +1
-1@@ -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;
Quoted 5 lines
block/raw-win32.c static int hdev_open(BlockDriverState *bs, QDict *options, int flags, · line 553
Quoted 2 lines
block/raw-win32.c +1
-1-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.
Quoted 279 lines
Quoted 279 lines
block/raw-win32.c +0
-0qemu_opts_absorb_qdict (opts, options, &local_err);if (error_is_set (&local_err)) {error_propagate (errp, local_err);block/rbd.c +1
-1@@ -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);block/sheepdog.c +1
-1@@ -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);block/vvfat.c +1
-1@@ -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);blockdev.c +4
-2@@ -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 {hw/watchdog/watchdog.c +2
-1@@ -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 ; }include/qemu/option.h +0
-1@@ -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,qdev-monitor.c +1
-1@@ -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 );qemu-img.c +1
-1@@ -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 ;util/qemu-config.c +1
-1@@ -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 ) {util/qemu-option.c +0
-9@@ -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;util/qemu-sockets.c +9
-9@@ -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);vl.c +5
-4@@ -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) }
Quoted 7 lines
vl.c int main(int argc, char **argv, char **envp) · line 3380
Quoted 4 lines
vl.c +1
-2qemu_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
Quoted 16 lines
Quoted 15 lines
vl.c +4
-1+ 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" );
Re: [Qemu-devel] [RFC PATCH v1 4/5] qemu-option: Remove qemu_opts_create_nofail
Quoted 2 lines · Peter Crosthwaite
Quoted 1 line · Markus Armbruster
Peter Crosthwaite <[email protected]> writes:
[...]
Quoted 11 lines
Quoted 10 lines
vl.c int main(int argc, char **argv, char **envp) · line 3380
Quoted 7 lines
vl.c +1
-2@@ -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.
Quoted 5 lines
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).
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
vl.c +2
-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" ));
[...]
Re: [Qemu-devel] [RFC PATCH v1 4/5] qemu-option: Remove qemu_opts_create_nofail
In reply to Markus ArmbrusterQuoted 122 lines · Markus Armbruster
Quoted 5 lines · Peter Crosthwaite
This is a boiler-plate
_nofailvariant ofqemu_opts_create. Remove and useerror_abortin call sites.A null argument needs to be added for the id field in affected callsites due to inconsistency between the normal and
no_failvariants.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; }Quoted 31 lines
Signed-off-by Peter Crosthwaite <[email protected]>---
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(-)block/blkdebug.c +0
-0block/blkdebug.c static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags, · line 365
Quoted 8 lines
block/blkdebug.c +1
-1@@ -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_existsfrom zero to one? Same everywhere.Quoted 60 lines
block/blkverify.c +1
-1@@ -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);block/curl.c +1
-1@@ -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);block/gluster.c +1
-1@@ -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);block/iscsi.c +1
-1@@ -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);block/nbd.c +0
-0@@ -234,7 +234,7 @@ static int nbd_config(BDRVNBDState *s, QDict *options)return -EINVAL ; }
Quoted 5 lines
block/nbd.c static int nbd_config(BDRVNBDState *s, QDict *options) · line 237
Quoted 2 lines
block/nbd.c +1
-1- 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
RFC PATCH 5/5 qerror: Remove assert_no_error()
In reply to Peter CrosthwaiteThis is no longer needed, and is obsoleted by error_abort. Remove.
---
include/qapi/qmp/qerror.h | 1 - qobject/qerror.c | 8 -------- 2 files changed, 9 deletions(-)
include/qapi/qmp/qerror.h +0 -1
@@ -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
qobject/qerror.c +0 -8
@@ -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 */
Signature · 1 line
1.8.4.4
Quoted 9 lines · Peter Crosthwaite
Following our discussion RE self asserting API calls, here is a spin of my proposal. This series obsoletes the need for
_nofailvariants 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_nofailandassert_no_error().To give it a smoke test, I introduce a (critical) bug into QOM:
[...]
Quoted 1 line
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.
Quoted 14 lines · On Tue, 03 Dec 2013 10:44:57 +0100 Markus Armbruster <[email protected]> wrote:
Quoted 9 lines · Peter Crosthwaite
Following our discussion RE self asserting API calls, here is a spin of my proposal. This series obsoletes the need for
_nofailvariants 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_nofailandassert_no_error().To give it a smoke test, I introduce a (critical) bug into QOM:
[...]
Quoted 1 line
32 files changed, 100 insertions(+), 143 deletions(-)
Quoted 7 lines
I like it. Nice diffstat, too.
There are some
_nofailfunctions left, but none of them can useerror_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;
Il 03/12/2013 10:44, Markus Armbruster ha scritto:
Quoted 6 lines
There are some
_nofailfunctions left, but none of them can useerror_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
Quoted 10 lines · Paolo Bonzini
Il 03/12/2013 10:44, Markus Armbruster ha scritto:
Quoted 6 lines
There are some
_nofailfunctions left, but none of them can useerror_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
Quoted 2 lines
Paolo
Quoted 10 lines · Markus Armbruster
Quoted 8 lines · Peter Crosthwaite
Following our discussion RE self asserting API calls, here is a spin of my proposal. This series obsoletes the need for
_nofailvariants 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_nofailandassert_no_error().
Quoted 9 lines
Quoted 1 line
To give it a smoke test, I introduce a (critical) bug into QOM:
[...]
Quoted 1 line
32 files changed, 100 insertions(+), 143 deletions(-)I like it. Nice diffstat, too.
There are some
_nofailfunctions left, but none of them can useerror_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).
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Quoted 21 lines · Eric Blake
Quoted 10 lines · Markus Armbruster
Quoted 8 lines · Peter Crosthwaite
Following our discussion RE self asserting API calls, here is a spin of my proposal. This series obsoletes the need for
_nofailvariants 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_nofailandassert_no_error().Quoted 9 lines
Quoted 1 line
To give it a smoke test, I introduce a (critical) bug into QOM:
[...]
Quoted 1 line
32 files changed, 100 insertions(+), 143 deletions(-)I like it. Nice diffstat, too.
There are some
_nofailfunctions left, but none of them can useerror_abort.
Quoted 8 lines
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_ignorefor 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.
Quoted 34 lines · On Tue, 03 Dec 2013 14:53:06 +0100 Markus Armbruster <[email protected]> wrote:
Quoted 21 lines · Eric Blake
Quoted 10 lines · Markus Armbruster
Quoted 8 lines · Peter Crosthwaite
Following our discussion RE self asserting API calls, here is a spin of my proposal. This series obsoletes the need for
_nofailvariants 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_nofailandassert_no_error().Quoted 9 lines
Quoted 1 line
To give it a smoke test, I introduce a (critical) bug into QOM:
[...]
Quoted 1 line
32 files changed, 100 insertions(+), 143 deletions(-)I like it. Nice diffstat, too.
There are some
_nofailfunctions left, but none of them can useerror_abort.Quoted 8 lines
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_ignorefor 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.
Quoted 7 lines
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_ignoreobject.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?
Signature · 2 lines
Regards, Igor
Quoted 18 lines · Igor Mammedov
Quoted 11 lines
Quoted 8 lines
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_ignorefor 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.
Quoted 7 lines
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_ignoreobject.Anyway, this series is about "abort on error". Let's keep "ignore errors" issues separate.
Quoted 8 lines
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).
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Quoted 29 lines · Eric Blake
Quoted 18 lines · Igor Mammedov
Quoted 11 lines
Quoted 8 lines
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_ignorefor 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.
Quoted 7 lines
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_ignoreobject.Anyway, this series is about "abort on error". Let's keep "ignore errors" issues separate.
Quoted 8 lines
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?
Quoted 9 lines
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_ignorespecial 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?
Quoted 6 lines · Markus Armbruster
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.
Quoted 4 lines
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.
Quoted 4 lines
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.
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Il 03/12/2013 21:33, Igor Mammedov ha scritto:
Quoted 8 lines
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
Quoted 4 lines
* Peter's alternative
+ self-documenting
+ consistent
+ predictable
I'll add another small advantage which is fewer SLOC.
Quoted 5 lines
* 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
Quoted 10 lines · On Thu, 05 Dec 2013 11:37:27 +0100 Paolo Bonzini <[email protected]> wrote:
Il 03/12/2013 21:33, Igor Mammedov ha scritto:
Quoted 8 lines
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?
Quoted 8 lines
See http://permalink.gmane.org/gmane.comp.emulators.qemu/243779
Quoted 4 lines
* Peter's alternative
+ self-documenting
+ consistent
+ predictableI'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?
Quoted 2 lines
Quoted 2 lines
* 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.
Quoted 2 lines
Quoted 2 lines
+ consistent
+ almost predictable (because in C you can ignore return values)
there is no return values from void functions.
Quoted 1 line
Quoted 1 line
- 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.
Quoted 1 line
Quoted 1 line
- 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 :(
Quoted 5 lines
Better wording of the last: a missing
&error_abortis easier to spot than a missingassert_no_error(errp).Paolo
Il 05/12/2013 16:32, Igor Mammedov ha scritto:
Quoted 4 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
* make Error* mandatory for all void functions
one more advantage:
+ not need to pepper every property setter/getter withlocal_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:
Quoted 3 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
+ 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.
Quoted 3 lines
Quoted 1 line
- 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