Re: [PATCH v2 00/44] Less clumsy error checking

[Markus Armbruster <armbru@redhat.com>]

diff -w between v1 rebased and v2, with:

diff --git a/include/qapi/error.h b/include/qapi/error.h
index c3d84d610a..5ceb3ace06 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -145,10 +145,10 @@
  * Likewise, do *not*
  *     Error *err = NULL;
  *     if (cond1) {
- *         error_setg(err, ...);
+ *         error_setg(&err, ...);
  *     }
  *     if (cond2) {
- *         error_setg(err, ...); // WRONG!
+ *         error_setg(&err, ...); // WRONG!
  *     }
  * because this may pass a non-null err to error_setg().
  */
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 7d1b8d579c..ebc19ede7f 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -284,7 +284,7 @@ void visit_free(Visitor *v);
  * On failure, set *@obj to NULL and store an error through @errp.
  * Can happen only when @v is an input visitor.
  *
- * Return true on succes, false on failure.
+ * Return true on success, false on failure.
  *
  * After visit_start_struct() succeeds, the caller may visit its
  * members one after the other, passing the member's name and address
@@ -304,7 +304,7 @@ bool visit_start_struct(Visitor *v, const char *name, void **obj,
  * On failure, store an error through @errp.  Can happen only when @v
  * is an input visitor.
  *
- * Return true on succes, false on failure.
+ * Return true on success, false on failure.
  *
  * Should be called prior to visit_end_struct() if all other
  * intermediate visit steps were successful, to allow the visitor one
@@ -343,7 +343,7 @@ void visit_end_struct(Visitor *v, void **obj);
  * On failure, set *@list to NULL and store an error through @errp.
  * Can happen only when @v is an input visitor.
  *
- * Return true on succes, false on failure.
+ * Return true on success, false on failure.
  *
  * After visit_start_list() succeeds, the caller may visit its members
  * one after the other.  A real visit (where @list is non-NULL) uses
@@ -380,7 +380,7 @@ GenericList *visit_next_list(Visitor *v, GenericList *tail, size_t size);
  * On failure, store an error through @errp.  Can happen only when @v
  * is an input visitor.
  *
- * Return true on succes, false on failure.
+ * Return true on success, false on failure.
  *
  * Should be called prior to visit_end_list() if all other
  * intermediate visit steps were successful, to allow the visitor one
@@ -418,7 +418,7 @@ void visit_end_list(Visitor *v, void **list);
  * On failure, set *@obj to NULL and store an error through @errp.
  * Can happen only when @v is an input visitor.
  *
- * Return true on succes, false on failure.
+ * Return true on success, false on failure.
  *
  * If successful, this must be paired with visit_end_alternate() with
  * the same @obj to clean up, even if visiting the contents of the
@@ -476,7 +476,7 @@ bool visit_optional(Visitor *v, const char *name, bool *present);
  * On failure, store an error through @errp.  Can happen only when @v
  * is an input visitor.
  *
- * Return true on succes, false on failure.
+ * Return true on success, false on failure.
  *
  * May call visit_type_str() under the hood, and the enum visit may
  * fail even if the corresponding string visit succeeded; this implies
@@ -510,7 +510,7 @@ bool visit_is_dealloc(Visitor *v);
  * On failure, store an error through @errp.  Can happen only when @v
  * is an input visitor.
  *
- * Return true on succes, false on failure.
+ * Return true on success, false on failure.
  */
 bool visit_type_int(Visitor *v, const char *name, int64_t *obj, Error **errp);
 
@@ -591,7 +591,7 @@ bool visit_type_size(Visitor *v, const char *name, uint64_t *obj,
  * On failure, store an error through @errp.  Can happen only when @v
  * is an input visitor.
  *
- * Return true on succes, false on failure.
+ * Return true on success, false on failure.
  */
 bool visit_type_bool(Visitor *v, const char *name, bool *obj, Error **errp);
 
@@ -612,7 +612,7 @@ bool visit_type_bool(Visitor *v, const char *name, bool *obj, Error **errp);
  * On failure, set *@obj to NULL and store an error through @errp.
  * Can happen only when @v is an input visitor.
  *
- * Return true on succes, false on failure.
+ * Return true on success, false on failure.
  *
  * FIXME: Callers that try to output NULL *obj should not be allowed.
  */
@@ -631,7 +631,7 @@ bool visit_type_str(Visitor *v, const char *name, char **obj, Error **errp);
  * On failure, store an error through @errp.  Can happen only when @v
  * is an input visitor.
  *
- * Return true on succes, false on failure.
+ * Return true on success, false on failure.
  */
 bool visit_type_number(Visitor *v, const char *name, double *obj,
                        Error **errp);
@@ -649,7 +649,7 @@ bool visit_type_number(Visitor *v, const char *name, double *obj,
  * On failure, set *@obj to NULL and store an error through @errp.
  * Can happen only when @v is an input visitor.
  *
- * Return true on succes, false on failure.
+ * Return true on success, false on failure.
  *
  * Note that some kinds of input can't express arbitrary QObject.
  * E.g. the visitor returned by qobject_input_visitor_new_keyval()
@@ -669,7 +669,7 @@ bool visit_type_any(Visitor *v, const char *name, QObject **obj, Error **errp);
  * On failure, set *@obj to NULL and store an error through @errp.
  * Can happen only when @v is an input visitor.
  *
- * Return true on succes, false on failure.
+ * Return true on success, false on failure.
  */
 bool visit_type_null(Visitor *v, const char *name, QNull **obj,
                      Error **errp);
diff --git a/block/vxhs.c b/block/vxhs.c
index ef2848fb60..dc0e254730 100644
--- a/block/vxhs.c
+++ b/block/vxhs.c
@@ -300,6 +300,7 @@ static int vxhs_open(BlockDriverState *bs, QDict *options,
     QemuOpts *opts = NULL;
     QemuOpts *tcp_opts = NULL;
     char *of_vsa_addr = NULL;
+    Error *local_err = NULL;
     const char *vdisk_id_opt;
     const char *server_host_opt;
     int ret = 0;
diff --git a/bootdevice.c b/bootdevice.c
index 8185402a5a..add4e3d2d1 100644
--- a/bootdevice.c
+++ b/bootdevice.c
@@ -303,15 +303,13 @@ static void device_set_bootindex(Object *obj, Visitor *v, const char *name,
     /* check whether bootindex is present in fw_boot_order list  */
     check_boot_index(boot_index, &local_err);
     if (local_err) {
-        goto out;
+        error_propagate(errp, local_err);
+        return;
     }
     /* change bootindex to a new one */
     *prop->bootindex = boot_index;
 
     add_boot_device_path(*prop->bootindex, prop->dev, prop->suffix);
-
-out:
-    error_propagate(errp, local_err);
 }
 
 static void property_release_bootindex(Object *obj, const char *name,
diff --git a/hw/core/platform-bus.c b/hw/core/platform-bus.c
index b700ff45fe..5037ca265e 100644
--- a/hw/core/platform-bus.c
+++ b/hw/core/platform-bus.c
@@ -66,7 +66,6 @@ hwaddr platform_bus_get_mmio_addr(PlatformBusDevice *pbus, SysBusDevice *sbdev,
 
     parent_mr = object_property_get_link(OBJECT(sbdev_mr), "container",
                                          &error_abort);
-    assert(parent_mr);
     if (parent_mr != pbus_mr_obj) {
         /* MMIO region is not mapped on platform bus */
         return -1;
diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index 81903d2711..67bee1bcb8 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -118,17 +118,15 @@ uint32_t x86_cpu_apic_id_from_index(X86MachineState *x86ms,
 
 void x86_cpu_new(X86MachineState *x86ms, int64_t apic_id, Error **errp)
 {
-    Error *local_err = NULL;
     Object *cpu = object_new(MACHINE(x86ms)->cpu_type);
 
-    if (!object_property_set_uint(cpu, "apic-id", apic_id, &local_err)) {
+    if (!object_property_set_uint(cpu, "apic-id", apic_id, errp)) {
         goto out;
     }
     qdev_realize(DEVICE(cpu), NULL, errp);
 
 out:
     object_unref(cpu);
-    error_propagate(errp, local_err);
 }
 
 void x86_cpus_init(X86MachineState *x86ms, int default_cpu_version)
diff --git a/hw/intc/arm_gicv3_its_kvm.c b/hw/intc/arm_gicv3_its_kvm.c
index 87bc4aeca1..46835ed085 100644
--- a/hw/intc/arm_gicv3_its_kvm.c
+++ b/hw/intc/arm_gicv3_its_kvm.c
@@ -91,7 +91,6 @@ static void vm_change_state_handler(void *opaque, int running,
 static void kvm_arm_its_realize(DeviceState *dev, Error **errp)
 {
     GICv3ITSState *s = ARM_GICV3_ITS_COMMON(dev);
-    Error *local_err = NULL;
 
     s->dev_fd = kvm_create_device(kvm_state, KVM_DEV_TYPE_ARM_VGIC_ITS, false);
     if (s->dev_fd < 0) {
diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
index 6705220380..68bb1914b9 100644
--- a/hw/intc/xics_kvm.c
+++ b/hw/intc/xics_kvm.c
@@ -309,7 +309,6 @@ int ics_set_kvm_state(ICSState *ics, Error **errp)
     }
 
     for (i = 0; i < ics->nr_irqs; i++) {
-        Error *local_err = NULL;
         int ret;
 
         if (ics_irq_free(ics, i)) {
diff --git a/hw/ppc/spapr_pci_nvlink2.c b/hw/ppc/spapr_pci_nvlink2.c
index e4e09a93e6..dd8cd6db96 100644
--- a/hw/ppc/spapr_pci_nvlink2.c
+++ b/hw/ppc/spapr_pci_nvlink2.c
@@ -141,10 +141,9 @@ static void spapr_phb_pci_collect_nvgpu(PCIBus *bus, PCIDevice *pdev,
     if (tgt) {
         Error *local_err = NULL;
         SpaprPhbPciNvGpuConfig *nvgpus = opaque;
-        Object *mr_gpu = object_property_get_link(po, "nvlink2-mr[0]",
-                                                  &error_abort);
+        Object *mr_gpu = object_property_get_link(po, "nvlink2-mr[0]", NULL);
         Object *mr_npu = object_property_get_link(po, "nvlink2-atsd-mr[0]",
-                                                  &error_abort);
+                                                  NULL);
 
         g_assert(mr_gpu || mr_npu);
         if (mr_gpu) {
diff --git a/net/tap.c b/net/tap.c
index ca48f2a285..f9dcc2ef51 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -790,9 +790,8 @@ int net_init_tap(const Netdev *netdev, const char *name,
             return -1;
         }
 
-        fd = monitor_fd_param(cur_mon, tap->fd, &err);
+        fd = monitor_fd_param(cur_mon, tap->fd, errp);
         if (fd == -1) {
-            error_propagate(errp, err);
             return -1;
         }
 
@@ -837,9 +836,8 @@ int net_init_tap(const Netdev *netdev, const char *name,
         }
 
         for (i = 0; i < nfds; i++) {
-            fd = monitor_fd_param(cur_mon, fds[i], &err);
+            fd = monitor_fd_param(cur_mon, fds[i], errp);
             if (fd == -1) {
-                error_propagate(errp, err);
                 ret = -1;
                 goto free_fail;
             }
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index b186ddb4aa..aaa71f147b 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -278,8 +278,6 @@ out:
 static void execute_async(DWORD WINAPI (*func)(LPVOID), LPVOID opaque,
                           Error **errp)
 {
-    Error *local_err = NULL;
-
     HANDLE thread = CreateThread(NULL, 0, func, opaque, 0, NULL);
     if (!thread) {
         error_setg(errp, QERR_QGA_COMMAND_FAILED,
@@ -1269,7 +1267,6 @@ typedef enum {
 static void check_suspend_mode(GuestSuspendMode mode, Error **errp)
 {
     SYSTEM_POWER_CAPABILITIES sys_pwr_caps;
-    Error *local_err = NULL;
 
     ZeroMemory(&sys_pwr_caps, sizeof(sys_pwr_caps));
     if (!GetPwrCapabilities(&sys_pwr_caps)) {
diff --git a/qom/object.c b/qom/object.c
index 9b479621e4..8d698abf4d 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -528,7 +528,8 @@ void object_initialize(void *data, size_t size, const char *typename)
 
 bool object_initialize_child_with_props(Object *parentobj,
                                         const char *propname,
-                             void *childobj, size_t size, const char *type,
+                                        void *childobj, size_t size,
+                                        const char *type,
                                         Error **errp, ...)
 {
     va_list vargs;
@@ -544,7 +545,8 @@ bool object_initialize_child_with_props(Object *parentobj,
 
 bool object_initialize_child_with_propsv(Object *parentobj,
                                          const char *propname,
-                              void *childobj, size_t size, const char *type,
+                                         void *childobj, size_t size,
+                                         const char *type,
                                          Error **errp, va_list vargs)
 {
     bool ok = false;
diff --git a/util/qemu-option.c b/util/qemu-option.c
index da74a239e9..c5a9d49b46 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -281,7 +281,6 @@ static void qemu_opt_del_all(QemuOpts *opts, const char *name)
 const char *qemu_opt_get(QemuOpts *opts, const char *name)
 {
     QemuOpt *opt;
-    const char *def_val;
 
     if (opts == NULL) {
         return NULL;
@@ -289,9 +288,9 @@ const char *qemu_opt_get(QemuOpts *opts, const char *name)
 
     opt = qemu_opt_find(opts, name);
     if (!opt) {
-        def_val = find_default_by_name(opts, name);
-        return def_val;
+        return find_default_by_name(opts, name);
     }
+
     return opt->str;
 }
 
@@ -321,7 +320,6 @@ const char *qemu_opt_iter_next(QemuOptsIter *iter)
 char *qemu_opt_get_del(QemuOpts *opts, const char *name)
 {
     QemuOpt *opt;
-    const char *def_val;
     char *str;
 
     if (opts == NULL) {
@@ -330,8 +328,7 @@ char *qemu_opt_get_del(QemuOpts *opts, const char *name)
 
     opt = qemu_opt_find(opts, name);
     if (!opt) {
-        def_val = find_default_by_name(opts, name);
-        return g_strdup(def_val);
+        return g_strdup(find_default_by_name(opts, name));
     }
     str = opt->str;
     opt->str = NULL;
@@ -523,13 +520,13 @@ static QemuOpt *opt_create(QemuOpts *opts, const char *name, char *value,
     return opt;
 }
 
-static bool opt_set(QemuOpts *opts, QemuOpt *opt, bool *help_wanted,
+static bool opt_validate(QemuOpt *opt, bool *help_wanted,
                          Error **errp)
 {
     const QemuOptDesc *desc;
 
-    desc = find_desc_by_name(opts->list->desc, opt->name);
-    if (!desc && !opts_accepts_any(opts)) {
+    desc = find_desc_by_name(opt->opts->list->desc, opt->name);
+    if (!desc && !opts_accepts_any(opt->opts)) {
         error_setg(errp, QERR_INVALID_PARAMETER, opt->name);
         if (help_wanted && is_help_option(opt->name)) {
             *help_wanted = true;
@@ -550,7 +547,7 @@ bool qemu_opt_set(QemuOpts *opts, const char *name, const char *value,
 {
     QemuOpt *opt = opt_create(opts, name, g_strdup(value), false);
 
-    if (!opt_set(opts, opt, NULL, errp)) {
+    if (!opt_validate(opt, NULL, errp)) {
         qemu_opt_del(opt);
         return false;
     }
@@ -844,7 +841,7 @@ static bool opts_do_parse(QemuOpts *opts, const char *params,
 
         opt = opt_create(opts, option, value, prepend);
         g_free(option);
-        if (!opt_set(opts, opt, help_wanted, errp)) {
+        if (!opt_validate(opt, help_wanted, errp)) {
             qemu_opt_del(opt);
             return false;
         }