[Qemu-devel] [PATCH v3 0/7] qdev: Mostly wean off QError

[Qemu-devel] [PATCH v3 0/7] qdev: Mostly wean off QError

[Markus Armbruster]

Only the calls in do_device_add() remain, because QMP's command
handler interface requires them.  They'll go away when I wean QMP off
QError.

Bonus: a few error reporting improvements.

Casualty: some explanatory messages, see PATCH 5.

v3:
* Trivially rebased, R-bys retained
* PATCH 1: Add rationale to commit message [Andreas]
* PATCH 2: Don't drop the comment outright, update it [Andreas]
* PATCH 3: Comment and commit message typos [Andreas]
v2:
* Trivially rebased
* PATCH 5+7: Error and commit message polish [Eric]

Markus Armbruster (7):
  qdev: Deprecated qdev_init() is finally unused, drop
  qdev: Un-deprecate qdev_init_nofail()
  qdev-monitor: Stop error avalanche in qbus_find_recursive()
  qdev-monitor: Fix check for full bus
  qdev-monitor: Convert qbus_find() to Error
  qdev-monitor: Propagate errors through set_property()
  qdev-monitor: Propagate errors through qdev_device_add()

 hw/core/qdev.c            |  47 ++++++---------
 include/hw/qdev-core.h    |   5 +-
 include/monitor/qdev.h    |   2 +-
 include/qapi/qmp/qerror.h |   3 -
 qdev-monitor.c            | 143 ++++++++++++++++++++++++++--------------------
 vl.c                      |   7 ++-
 6 files changed, 106 insertions(+), 101 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH v3 1/7] qdev: Deprecated qdev_init() is finally unused, drop

[Markus Armbruster]

qdev_init() is a wrapper around setting property "realized" to true,
plus error handling that passes errors to qerror_report_err().
qerror_report_err() is a transitional interface to help with
converting existing monitor commands to QMP.  It should not be used
elsewhere.

All code has been modernized to avoid qdev_init() and its
inappropriate error handling.  We can finally drop it.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Andreas Färber <afaerber@suse.de>
---
 hw/core/qdev.c         | 47 ++++++++++++++++-------------------------------
 include/hw/qdev-core.h |  3 +--
 2 files changed, 17 insertions(+), 33 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index b0f0f84..d433675 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -126,9 +126,9 @@ void qbus_set_bus_hotplug_handler(BusState *bus, Error **errp)
     qbus_set_hotplug_handler_internal(bus, OBJECT(bus), errp);
 }
 
-/* Create a new device.  This only initializes the device state structure
-   and allows properties to be set.  qdev_init should be called to
-   initialize the actual device emulation.  */
+/* Create a new device.  This only initializes the device state
+   structure and allows properties to be set.  The device still needs
+   to be realized.  See qdev-core.h.  */
 DeviceState *qdev_create(BusState *bus, const char *name)
 {
     DeviceState *dev;
@@ -168,27 +168,6 @@ DeviceState *qdev_try_create(BusState *bus, const char *type)
     return dev;
 }
 
-/* Initialize a device.  Device properties should be set before calling
-   this function.  IRQs and MMIO regions should be connected/mapped after
-   calling this function.
-   On failure, destroy the device and return negative value.
-   Return 0 on success.  */
-int qdev_init(DeviceState *dev)
-{
-    Error *local_err = NULL;
-
-    assert(!dev->realized);
-
-    object_property_set_bool(OBJECT(dev), true, "realized", &local_err);
-    if (local_err != NULL) {
-        qerror_report_err(local_err);
-        error_free(local_err);
-        object_unparent(OBJECT(dev));
-        return -1;
-    }
-    return 0;
-}
-
 static QTAILQ_HEAD(device_listeners, DeviceListener) device_listeners
     = QTAILQ_HEAD_INITIALIZER(device_listeners);
 
@@ -364,13 +343,19 @@ void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev,
     object_unparent(OBJECT(dev));
 }
 
-/* Like qdev_init(), but terminate program via error_report() instead of
-   returning an error value.  This is okay during machine creation.
-   Don't use for hotplug, because there callers need to recover from
-   failure.  Exception: if you know the device's init() callback can't
-   fail, then qdev_init_nofail() can't fail either, and is therefore
-   usable even then.  But relying on the device implementation that
-   way is somewhat unclean, and best avoided.  */
+/*
+ * Realize @dev.
+ * Device properties should be set before calling this function.  IRQs
+ * and MMIO regions should be connected/mapped after calling this
+ * function.
+ * On failure, report an error with error_report() and terminate the
+ * program.  This is okay during machine creation.  Don't use for
+ * hotplug, because there callers need to recover from failure.
+ * Exception: if you know the device's init() callback can't fail,
+ * then qdev_init_nofail() can't fail either, and is therefore usable
+ * even then.  But relying on the device implementation that way is
+ * somewhat unclean, and best avoided.
+ */
 void qdev_init_nofail(DeviceState *dev)
 {
     Error *err = NULL;
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index d4be92f..5789b91 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -66,7 +66,7 @@ struct VMStateDescription;
  * After successful realization, setting static properties will fail.
  *
  * As an interim step, the #DeviceState:realized property is set by deprecated
- * functions qdev_init() and qdev_init_nofail().
+ * function qdev_init_nofail().
  * In the future, devices will propagate this state change to their children
  * and along busses they expose.
  * The point in time will be deferred to machine creation, so that values
@@ -262,7 +262,6 @@ typedef struct GlobalProperty {
 
 DeviceState *qdev_create(BusState *bus, const char *name);
 DeviceState *qdev_try_create(BusState *bus, const char *name);
-int qdev_init(DeviceState *dev) QEMU_WARN_UNUSED_RESULT;
 void qdev_init_nofail(DeviceState *dev);
 void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
                                  int required_for_version);
-- 
1.9.3

[Qemu-devel] [PATCH v3 2/7] qdev: Un-deprecate qdev_init_nofail()

[Markus Armbruster]

It's a perfectly sensible helper function.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 include/hw/qdev-core.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 5789b91..b736b03 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -65,8 +65,8 @@ struct VMStateDescription;
  * Operations depending on @props static properties should go into @realize.
  * After successful realization, setting static properties will fail.
  *
- * As an interim step, the #DeviceState:realized property is set by deprecated
- * function qdev_init_nofail().
+ * As an interim step, the #DeviceState:realized property can also be
+ * set with qdev_init_nofail().
  * In the future, devices will propagate this state change to their children
  * and along busses they expose.
  * The point in time will be deferred to machine creation, so that values
-- 
1.9.3

[Qemu-devel] [PATCH v3 3/7] qdev-monitor: Stop error avalanche in qbus_find_recursive()

[Markus Armbruster]

Reproducer:

    $ qemu-system-x86_64 -nodefaults -device virtio-rng-pci -device virtio-rng-pci -device virtio-rng-device,bus=virtio-bus
    qemu-system-x86_64: -device virtio-rng-device,bus=virtio-bus: Bus 'virtio-bus' is full
    qemu-system-x86_64: -device virtio-rng-device,bus=virtio-bus: Bus 'virtio-bus' is full
    qemu-system-x86_64: -device virtio-rng-device,bus=virtio-bus: Bus 'virtio-bus' not found

qbus_find_recursive() reports the "is full" error itself, and leaves
reporting "not found" to its caller.  The result is confusion.  Write
it a function contract that permits leaving all error reporting to the
caller, and implement it.  Update callers to detect and report "is
full".

Screwed up when commit 1395af6 added the max_dev limit and the "is
full" error condition to enforce it.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 qdev-monitor.c | 62 ++++++++++++++++++++++++++++++++++++----------------------
 1 file changed, 39 insertions(+), 23 deletions(-)

diff --git a/qdev-monitor.c b/qdev-monitor.c
index d71d1ee..2c4d4c8 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -364,43 +364,55 @@ static DeviceState *qbus_find_dev(BusState *bus, char *elem)
     return NULL;
 }
 
+static inline bool qbus_is_full(BusState *bus)
+{
+    BusClass *bus_class = BUS_GET_CLASS(bus);
+    return bus_class->max_dev && bus->max_index >= bus_class->max_dev;
+}
+
+/*
+ * Search the tree rooted at @bus for a bus.
+ * If @name, search for a bus with that name.  Note that bus names
+ * need not be unique.  Yes, that's screwed up.
+ * Else search for a bus that is a subtype of @bus_typename.
+ * If more than one exists, prefer one that can take another device.
+ * Return the bus if found, else %NULL.
+ */
 static BusState *qbus_find_recursive(BusState *bus, const char *name,
                                      const char *bus_typename)
 {
-    BusClass *bus_class = BUS_GET_CLASS(bus);
     BusChild *kid;
-    BusState *child, *ret;
-    int match = 1;
+    BusState *pick, *child, *ret;
+    bool match;
 
-    if (name && (strcmp(bus->name, name) != 0)) {
-        match = 0;
-    } else if (bus_typename && !object_dynamic_cast(OBJECT(bus), bus_typename)) {
-        match = 0;
-    } else if ((bus_class->max_dev != 0) && (bus_class->max_dev <= bus->max_index)) {
-        if (name != NULL) {
-            /* bus was explicitly specified: return an error. */
-            qerror_report(ERROR_CLASS_GENERIC_ERROR, "Bus '%s' is full",
-                          bus->name);
-            return NULL;
-        } else {
-            /* bus was not specified: try to find another one. */
-            match = 0;
-        }
+    assert(name || bus_typename);
+    if (name) {
+        match = !strcmp(bus->name, name);
+    } else {
+        match = !!object_dynamic_cast(OBJECT(bus), bus_typename);
     }
-    if (match) {
-        return bus;
+
+    if (match && !qbus_is_full(bus)) {
+        return bus;             /* root matches and isn't full */
     }
 
+    pick = match ? bus : NULL;
+
     QTAILQ_FOREACH(kid, &bus->children, sibling) {
         DeviceState *dev = kid->child;
         QLIST_FOREACH(child, &dev->child_bus, sibling) {
             ret = qbus_find_recursive(child, name, bus_typename);
-            if (ret) {
-                return ret;
+            if (ret && !qbus_is_full(ret)) {
+                return ret;     /* a descendant matches and isn't full */
+            }
+            if (ret && !pick) {
+                pick = ret;
             }
         }
     }
-    return NULL;
+
+    /* root or a descendant matches, but is full */
+    return pick;
 }
 
 static BusState *qbus_find(const char *path)
@@ -423,6 +435,10 @@ static BusState *qbus_find(const char *path)
         if (!bus) {
             qerror_report(QERR_BUS_NOT_FOUND, elem);
             return NULL;
+        } else if (qbus_is_full(bus)) {
+            qerror_report(ERROR_CLASS_GENERIC_ERROR, "Bus '%s' is full",
+                          elem);
+            return NULL;
         }
         pos = len;
     }
@@ -529,7 +545,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
         }
     } else if (dc->bus_type != NULL) {
         bus = qbus_find_recursive(sysbus_get_default(), NULL, dc->bus_type);
-        if (!bus) {
+        if (!bus || qbus_is_full(bus)) {
             qerror_report(ERROR_CLASS_GENERIC_ERROR,
                           "No '%s' bus found for device '%s'",
                           dc->bus_type, driver);
-- 
1.9.3

[Qemu-devel] [PATCH v3 4/7] qdev-monitor: Fix check for full bus

[Markus Armbruster]

Property bus has always been too screwed up to be really usable for
values other than plain bus IDs.  This just fixes a bug that crept in
in commit 1395af6 "qdev: add a maximum device allowed field for the
bus."

It doesn't always fail when it should:

    $ qemu-system-x86_64 -nodefaults -device virtio-serial-pci -device virtio-rng-device,bus=pci.0/virtio-serial-pci/virtio-bus

Happily plugs the virtio-rng-device into the virtio-bus provided by
virtio-serial-pci, even though its only slot is already occupied by a
virtio-serial-device.

And sometimes fails when it shouldn't:

    $ qemu-system-x86_64 -nodefaults -device virtio-serial-pci -device virtserialport,bus=virtio-bus/virtio-serial-device

Yes, the virtio-bus is full, but the virtio-serial-bus provided by
virtio-serial-device isn't, and that's the one we're trying to use.

Root cause: we check "bus full" when we resolve the first element of
the path.  That's the correct one only when it's also the last one.

Fix by moving the "bus full" check to right before we return a bus.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 qdev-monitor.c | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/qdev-monitor.c b/qdev-monitor.c
index 2c4d4c8..afc0395 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -435,10 +435,6 @@ static BusState *qbus_find(const char *path)
         if (!bus) {
             qerror_report(QERR_BUS_NOT_FOUND, elem);
             return NULL;
-        } else if (qbus_is_full(bus)) {
-            qerror_report(ERROR_CLASS_GENERIC_ERROR, "Bus '%s' is full",
-                          elem);
-            return NULL;
         }
         pos = len;
     }
@@ -449,7 +445,7 @@ static BusState *qbus_find(const char *path)
             pos++;
         }
         if (path[pos] == '\0') {
-            return bus;
+            break;
         }
 
         /* find device */
@@ -474,21 +470,21 @@ static BusState *qbus_find(const char *path)
         if (path[pos] == '\0') {
             /* last specified element is a device.  If it has exactly
              * one child bus accept it nevertheless */
-            switch (dev->num_child_bus) {
-            case 0:
-                qerror_report(ERROR_CLASS_GENERIC_ERROR,
-                              "Device '%s' has no child bus", elem);
-                return NULL;
-            case 1:
-                return QLIST_FIRST(&dev->child_bus);
-            default:
+            if (dev->num_child_bus == 1) {
+                bus = QLIST_FIRST(&dev->child_bus);
+                break;
+            }
+            if (dev->num_child_bus) {
                 qerror_report(ERROR_CLASS_GENERIC_ERROR,
                               "Device '%s' has multiple child busses", elem);
                 if (!monitor_cur_is_qmp()) {
                     qbus_list_bus(dev);
                 }
-                return NULL;
+            } else {
+                qerror_report(ERROR_CLASS_GENERIC_ERROR,
+                              "Device '%s' has no child bus", elem);
             }
+            return NULL;
         }
 
         /* find bus */
@@ -506,6 +502,13 @@ static BusState *qbus_find(const char *path)
             return NULL;
         }
     }
+
+    if (qbus_is_full(bus)) {
+        qerror_report(ERROR_CLASS_GENERIC_ERROR, "Bus '%s' is full",
+                      path);
+        return NULL;
+    }
+    return bus;
 }
 
 DeviceState *qdev_device_add(QemuOpts *opts)
-- 
1.9.3

[Qemu-devel] [PATCH v3 5/7] qdev-monitor: Convert qbus_find() to Error

[Markus Armbruster]

As usual, the conversion breaks printing explanatory messages after
the error: actual printing of the error gets delayed, so the
explanations precede rather than follow it.

Pity.  Disable them for now.  See also commit 7216ae3.

While there, eliminate QERR_BUS_NOT_FOUND, and clean up unusual
spelling in the error message.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 include/qapi/qmp/qerror.h |  3 ---
 qdev-monitor.c            | 32 ++++++++++++++++++++------------
 2 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
index e567339..6468e40 100644
--- a/include/qapi/qmp/qerror.h
+++ b/include/qapi/qmp/qerror.h
@@ -43,9 +43,6 @@ void qerror_report_err(Error *err);
 #define QERR_BUS_NO_HOTPLUG \
     ERROR_CLASS_GENERIC_ERROR, "Bus '%s' does not support hotplugging"
 
-#define QERR_BUS_NOT_FOUND \
-    ERROR_CLASS_GENERIC_ERROR, "Bus '%s' not found"
-
 #define QERR_DEVICE_HAS_NO_MEDIUM \
     ERROR_CLASS_GENERIC_ERROR, "Device '%s' has no medium"
 
diff --git a/qdev-monitor.c b/qdev-monitor.c
index afc0395..12d8f6b 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -288,12 +288,13 @@ static Object *qdev_get_peripheral_anon(void)
     return dev;
 }
 
+#if 0 /* conversion from qerror_report() to error_set() broke their use */
 static void qbus_list_bus(DeviceState *dev)
 {
     BusState *child;
     const char *sep = " ";
 
-    error_printf("child busses at \"%s\":",
+    error_printf("child buses at \"%s\":",
                  dev->id ? dev->id : object_get_typename(OBJECT(dev)));
     QLIST_FOREACH(child, &dev->child_bus, sibling) {
         error_printf("%s\"%s\"", sep, child->name);
@@ -317,6 +318,7 @@ static void qbus_list_dev(BusState *bus)
     }
     error_printf("\n");
 }
+#endif
 
 static BusState *qbus_find_bus(DeviceState *dev, char *elem)
 {
@@ -415,7 +417,7 @@ static BusState *qbus_find_recursive(BusState *bus, const char *name,
     return pick;
 }
 
-static BusState *qbus_find(const char *path)
+static BusState *qbus_find(const char *path, Error **errp)
 {
     DeviceState *dev;
     BusState *bus;
@@ -433,7 +435,7 @@ static BusState *qbus_find(const char *path)
         }
         bus = qbus_find_recursive(sysbus_get_default(), elem, NULL);
         if (!bus) {
-            qerror_report(QERR_BUS_NOT_FOUND, elem);
+            error_setg(errp, "Bus '%s' not found", elem);
             return NULL;
         }
         pos = len;
@@ -456,10 +458,12 @@ static BusState *qbus_find(const char *path)
         pos += len;
         dev = qbus_find_dev(bus, elem);
         if (!dev) {
-            qerror_report(QERR_DEVICE_NOT_FOUND, elem);
+            error_set(errp, QERR_DEVICE_NOT_FOUND, elem);
+#if 0 /* conversion from qerror_report() to error_set() broke this: */
             if (!monitor_cur_is_qmp()) {
                 qbus_list_dev(bus);
             }
+#endif
             return NULL;
         }
 
@@ -475,14 +479,15 @@ static BusState *qbus_find(const char *path)
                 break;
             }
             if (dev->num_child_bus) {
-                qerror_report(ERROR_CLASS_GENERIC_ERROR,
-                              "Device '%s' has multiple child busses", elem);
+                error_setg(errp, "Device '%s' has multiple child buses",
+                           elem);
+#if 0 /* conversion from qerror_report() to error_set() broke this: */
                 if (!monitor_cur_is_qmp()) {
                     qbus_list_bus(dev);
                 }
+#endif
             } else {
-                qerror_report(ERROR_CLASS_GENERIC_ERROR,
-                              "Device '%s' has no child bus", elem);
+                error_setg(errp, "Device '%s' has no child bus", elem);
             }
             return NULL;
         }
@@ -495,17 +500,18 @@ static BusState *qbus_find(const char *path)
         pos += len;
         bus = qbus_find_bus(dev, elem);
         if (!bus) {
-            qerror_report(QERR_BUS_NOT_FOUND, elem);
+            error_setg(errp, "Bus '%s' not found", elem);
+#if 0 /* conversion from qerror_report() to error_set() broke this: */
             if (!monitor_cur_is_qmp()) {
                 qbus_list_bus(dev);
             }
+#endif
             return NULL;
         }
     }
 
     if (qbus_is_full(bus)) {
-        qerror_report(ERROR_CLASS_GENERIC_ERROR, "Bus '%s' is full",
-                      path);
+        error_setg(errp, "Bus '%s' is full", path);
         return NULL;
     }
     return bus;
@@ -536,8 +542,10 @@ DeviceState *qdev_device_add(QemuOpts *opts)
     /* find bus */
     path = qemu_opt_get(opts, "bus");
     if (path != NULL) {
-        bus = qbus_find(path);
+        bus = qbus_find(path, &err);
         if (!bus) {
+            qerror_report_err(err);
+            error_free(err);
             return NULL;
         }
         if (!object_dynamic_cast(OBJECT(bus), dc->bus_type)) {
-- 
1.9.3

[Qemu-devel] [PATCH v3 6/7] qdev-monitor: Propagate errors through set_property()

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Andreas Färber <afaerber@suse.de>
---
 qdev-monitor.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/qdev-monitor.c b/qdev-monitor.c
index 12d8f6b..c7b00e0 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -156,8 +156,7 @@ static int set_property(void *opaque, const char *name, const char *value,
 
     object_property_parse(obj, value, name, &err);
     if (err != NULL) {
-        qerror_report_err(err);
-        error_free(err);
+        error_propagate(errp, err);
         return -1;
     }
     return 0;
@@ -592,7 +591,8 @@ DeviceState *qdev_device_add(QemuOpts *opts)
     }
 
     /* set properties */
-    if (qemu_opt_foreach(opts, set_property, dev, NULL)) {
+    if (qemu_opt_foreach(opts, set_property, dev, &err)) {
+        qerror_report_err(err);
         object_unparent(OBJECT(dev));
         object_unref(OBJECT(dev));
         return NULL;
-- 
1.9.3

[Qemu-devel] [PATCH v3 7/7] qdev-monitor: Propagate errors through qdev_device_add()

[Markus Armbruster]

Also polish an error message while I'm touching the line anyway,

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Andreas Färber <afaerber@suse.de>
---
 include/monitor/qdev.h |  2 +-
 qdev-monitor.c         | 36 +++++++++++++++---------------------
 vl.c                   |  7 +++++--
 3 files changed, 21 insertions(+), 24 deletions(-)

diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h
index 7190752..2ce8578 100644
--- a/include/monitor/qdev.h
+++ b/include/monitor/qdev.h
@@ -11,6 +11,6 @@ void hmp_info_qdm(Monitor *mon, const QDict *qdict);
 void hmp_info_qom_tree(Monitor *mon, const QDict *dict);
 int do_device_add(Monitor *mon, const QDict *qdict, QObject **ret_data);
 int qdev_device_help(QemuOpts *opts);
-DeviceState *qdev_device_add(QemuOpts *opts);
+DeviceState *qdev_device_add(QemuOpts *opts, Error **errp);
 
 #endif
diff --git a/qdev-monitor.c b/qdev-monitor.c
index c7b00e0..f6ceea7 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -516,7 +516,7 @@ static BusState *qbus_find(const char *path, Error **errp)
     return bus;
 }
 
-DeviceState *qdev_device_add(QemuOpts *opts)
+DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
 {
     DeviceClass *dc;
     const char *driver, *path, *id;
@@ -526,44 +526,38 @@ DeviceState *qdev_device_add(QemuOpts *opts)
 
     driver = qemu_opt_get(opts, "driver");
     if (!driver) {
-        qerror_report(QERR_MISSING_PARAMETER, "driver");
+        error_set(errp, QERR_MISSING_PARAMETER, "driver");
         return NULL;
     }
 
     /* find driver */
-    dc = qdev_get_device_class(&driver, &err);
-    if (err) {
-        qerror_report_err(err);
-        error_free(err);
+    dc = qdev_get_device_class(&driver, errp);
+    if (!dc) {
         return NULL;
     }
 
     /* find bus */
     path = qemu_opt_get(opts, "bus");
     if (path != NULL) {
-        bus = qbus_find(path, &err);
+        bus = qbus_find(path, errp);
         if (!bus) {
-            qerror_report_err(err);
-            error_free(err);
             return NULL;
         }
         if (!object_dynamic_cast(OBJECT(bus), dc->bus_type)) {
-            qerror_report(ERROR_CLASS_GENERIC_ERROR,
-                          "Device '%s' can't go on a %s bus",
-                          driver, object_get_typename(OBJECT(bus)));
+            error_setg(errp, "Device '%s' can't go on %s bus",
+                       driver, object_get_typename(OBJECT(bus)));
             return NULL;
         }
     } else if (dc->bus_type != NULL) {
         bus = qbus_find_recursive(sysbus_get_default(), NULL, dc->bus_type);
         if (!bus || qbus_is_full(bus)) {
-            qerror_report(ERROR_CLASS_GENERIC_ERROR,
-                          "No '%s' bus found for device '%s'",
-                          dc->bus_type, driver);
+            error_setg(errp, "No '%s' bus found for device '%s'",
+                       dc->bus_type, driver);
             return NULL;
         }
     }
     if (qdev_hotplug && bus && !qbus_is_hotpluggable(bus)) {
-        qerror_report(QERR_BUS_NO_HOTPLUG, bus->name);
+        error_set(errp, QERR_BUS_NO_HOTPLUG, bus->name);
         return NULL;
     }
 
@@ -592,7 +586,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
 
     /* set properties */
     if (qemu_opt_foreach(opts, set_property, dev, &err)) {
-        qerror_report_err(err);
+        error_propagate(errp, err);
         object_unparent(OBJECT(dev));
         object_unref(OBJECT(dev));
         return NULL;
@@ -601,12 +595,10 @@ DeviceState *qdev_device_add(QemuOpts *opts)
     dev->opts = opts;
     object_property_set_bool(OBJECT(dev), true, "realized", &err);
     if (err != NULL) {
-        qerror_report_err(err);
-        error_free(err);
+        error_propagate(errp, err);
         dev->opts = NULL;
         object_unparent(OBJECT(dev));
         object_unref(OBJECT(dev));
-        qerror_report(QERR_DEVICE_INIT_FAILED, driver);
         return NULL;
     }
     return dev;
@@ -779,8 +771,10 @@ int do_device_add(Monitor *mon, const QDict *qdict, QObject **ret_data)
         qemu_opts_del(opts);
         return 0;
     }
-    dev = qdev_device_add(opts);
+    dev = qdev_device_add(opts, &local_err);
     if (!dev) {
+        qerror_report_err(local_err);
+        error_free(local_err);
         qemu_opts_del(opts);
         return -1;
     }
diff --git a/vl.c b/vl.c
index 2201e27..12c204c 100644
--- a/vl.c
+++ b/vl.c
@@ -2198,11 +2198,14 @@ static int device_help_func(void *opaque, QemuOpts *opts, Error **errp)
 
 static int device_init_func(void *opaque, QemuOpts *opts, Error **errp)
 {
+    Error *err = NULL;
     DeviceState *dev;
 
-    dev = qdev_device_add(opts);
-    if (!dev)
+    dev = qdev_device_add(opts, &err);
+    if (!dev) {
+        error_report_err(err);
         return -1;
+    }
     object_unref(OBJECT(dev));
     return 0;
 }
-- 
1.9.3

Re: [Qemu-devel] [PATCH v3 0/7] qdev: Mostly wean off QError

[Andreas Färber]

Am 19.06.2015 um 16:17 schrieb Markus Armbruster:
> Only the calls in do_device_add() remain, because QMP's command
> handler interface requires them.  They'll go away when I wean QMP off
> QError.
> 
> Bonus: a few error reporting improvements.
> 
> Casualty: some explanatory messages, see PATCH 5.
> 
> v3:
> * Trivially rebased, R-bys retained
> * PATCH 1: Add rationale to commit message [Andreas]
> * PATCH 2: Don't drop the comment outright, update it [Andreas]
> * PATCH 3: Comment and commit message typos [Andreas]
> v2:
> * Trivially rebased
> * PATCH 5+7: Error and commit message polish [Eric]
> 
> Markus Armbruster (7):
>   qdev: Deprecated qdev_init() is finally unused, drop
>   qdev: Un-deprecate qdev_init_nofail()

Thanks, I applied these two to qom-next:
https://github.com/afaerber/qemu-cpu/commits/qom-next

>   qdev-monitor: Stop error avalanche in qbus_find_recursive()
>   qdev-monitor: Fix check for full bus
>   qdev-monitor: Convert qbus_find() to Error
>   qdev-monitor: Propagate errors through set_property()
>   qdev-monitor: Propagate errors through qdev_device_add()

Only 7/7 touches qdev.h again and should apply independently, so should
be fine to go through your monitor queue.

Thanks,
Andreas

> 
>  hw/core/qdev.c            |  47 ++++++---------
>  include/hw/qdev-core.h    |   5 +-
>  include/monitor/qdev.h    |   2 +-
>  include/qapi/qmp/qerror.h |   3 -
>  qdev-monitor.c            | 143 ++++++++++++++++++++++++++--------------------
>  vl.c                      |   7 ++-
>  6 files changed, 106 insertions(+), 101 deletions(-)

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
21284 (AG Nürnberg)