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

[Qemu-devel] [PATCH v2 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.

Based on "[PULL 0/9] Error reporting patches".

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 avalance 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    |   3 -
 include/monitor/qdev.h    |   2 +-
 include/qapi/qmp/qerror.h |   3 -
 qdev-monitor.c            | 143 ++++++++++++++++++++++++++--------------------
 vl.c                      |   7 ++-
 6 files changed, 104 insertions(+), 101 deletions(-)

-- 
1.9.3

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

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 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

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

[Andreas Färber]

Hi Markus,

Could you please add a verbose rationale here like "qdev_init() does not
propagate the Error* and should be replaced by ..."?

Am 13.06.2015 um 13:18 schrieb Markus Armbruster:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  hw/core/qdev.c         | 47 ++++++++++++++++-------------------------------
>  include/hw/qdev-core.h |  3 +--
>  2 files changed, 17 insertions(+), 33 deletions(-)

Otherwise,

Reviewed-by: Andreas Färber <afaerber@suse.de>

Thanks,
Andreas

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

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

[Markus Armbruster]

Andreas Färber <afaerber@suse.de> writes:

> Hi Markus,
>
> Could you please add a verbose rationale here like "qdev_init() does not
> propagate the Error* and should be replaced by ..."?

What about:

    qdev_init() is a wrapper around setting property "realized" to true
    plus error handling, which 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.

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

Thanks!

[Qemu-devel] [PATCH v2 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 | 2 --
 1 file changed, 2 deletions(-)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 5789b91..fbfc741 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -65,8 +65,6 @@ 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().
  * 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

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

[Andreas Färber]

Am 13.06.2015 um 13:18 schrieb Markus Armbruster:
> It's a perfectly sensible helper function.

But only in the current state. Once/if we just set realized=true on
/machine level, then no other helper functions will need to set it, as
pointed out in the following paragraph.

> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  include/hw/qdev-core.h | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 5789b91..fbfc741 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -65,8 +65,6 @@ 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().
>   * 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

Nack to the patch as is. I would be much more open to it if you just
removed the word "deprecated" rather than the full paragraph explaining
where it is currently set.

Regards,
Andreas

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

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

[Markus Armbruster]

Andreas Färber <afaerber@suse.de> writes:

> Am 13.06.2015 um 13:18 schrieb Markus Armbruster:
>> It's a perfectly sensible helper function.
>
> But only in the current state. Once/if we just set realized=true on
> /machine level, then no other helper functions will need to set it, as
> pointed out in the following paragraph.
>
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> ---
>>  include/hw/qdev-core.h | 2 --
>>  1 file changed, 2 deletions(-)
>> 
>> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
>> index 5789b91..fbfc741 100644
>> --- a/include/hw/qdev-core.h
>> +++ b/include/hw/qdev-core.h
>> @@ -65,8 +65,6 @@ 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().
>>   * 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
>
> Nack to the patch as is. I would be much more open to it if you just
> removed the word "deprecated" rather than the full paragraph explaining
> where it is currently set.

I can certainly do that.  May I add your R-by then?

[Qemu-devel] [PATCH v2 3/7] qdev-monitor: Stop error avalance 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 7dd62dd..1408c86 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

Re: [Qemu-devel] [PATCH v2 3/7] qdev-monitor: Stop error avalance in qbus_find_recursive()

[Andreas Färber]

Did you mean avalanche?

Am 13.06.2015 um 13:18 schrieb 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 7dd62dd..1408c86 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;
> +}
> +
> +/**

This should probably just be "/*".

I'll trust you on getting the functional logic right, can't do a full
review right now.

Andreas

> + * 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);
> 


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

Re: [Qemu-devel] [PATCH v2 3/7] qdev-monitor: Stop error avalance in qbus_find_recursive()

[Markus Armbruster]

Andreas Färber <afaerber@suse.de> writes:

> Did you mean avalanche?

Yes, fixing...

> Am 13.06.2015 um 13:18 schrieb 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 7dd62dd..1408c86 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;
>> +}
>> +
>> +/**
>
> This should probably just be "/*".

Fixing...

> I'll trust you on getting the functional logic right, can't do a full
> review right now.

Thank you all the same!

[Qemu-devel] [PATCH v2 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 1408c86..a54c368 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 v2 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 a54c368..b5b9a88 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 v2 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>
---
 qdev-monitor.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/qdev-monitor.c b/qdev-monitor.c
index b5b9a88..e7e9a50 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

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

[Andreas Färber]

Am 13.06.2015 um 13:18 schrieb Markus Armbruster:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  qdev-monitor.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Andreas Färber <afaerber@suse.de>

Andreas

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

[Qemu-devel] [PATCH v2 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>
---
 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 e7e9a50..686c9df 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 9542095..f0dcb6b 100644
--- a/vl.c
+++ b/vl.c
@@ -2185,11 +2185,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 v2 7/7] qdev-monitor: Propagate errors through qdev_device_add()

[Andreas Färber]

Am 13.06.2015 um 13:18 schrieb 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>
> ---
>  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 e7e9a50..686c9df 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) {

Here ...

>          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);

... and here you've elegantly eliminated assumptions about return value
and errp relations. Before, we were accessing err without checking that
it is non-NULL - error_free() shouldn't care but qerror_report_err()
might and just out of design principle.

>              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) {

Would seem safer to change this to "if (local_err) {" now that it is
dealing with local_err.

> +        qerror_report_err(local_err);
> +        error_free(local_err);
>          qemu_opts_del(opts);
>          return -1;
>      }
> diff --git a/vl.c b/vl.c
> index 9542095..f0dcb6b 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2185,11 +2185,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;
>  }

Anyway,

Reviewed-by: Andreas Färber <afaerber@suse.de>

Regards,
Andreas

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

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

[Markus Armbruster]

Andreas Färber <afaerber@suse.de> writes:

> Am 13.06.2015 um 13:18 schrieb 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>
>> ---
>>  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 e7e9a50..686c9df 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) {
>
> Here ...
>
>>          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);
>
> ... and here you've elegantly eliminated assumptions about return value
> and errp relations. Before, we were accessing err without checking that
> it is non-NULL - error_free() shouldn't care but qerror_report_err()
> might and just out of design principle.
>
>>              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) {
>
> Would seem safer to change this to "if (local_err) {" now that it is
> dealing with local_err.
>
>> +        qerror_report_err(local_err);
>> +        error_free(local_err);
>>          qemu_opts_del(opts);
>>          return -1;
>>      }

After my next patch series, this will be

    dev = qdev_device_add(opts, &local_err);
    if (!dev) {
        error_propagate(errp, local_err);
        qemu_opts_del(opts);
        return;
    }

We got similar logic all over the tree.

Whether to check the returned pointer or the error variable is a matter
of taste.  I prefer to check the pointer, not least because it's less
likely to confuse Coverity in my experience.

>> diff --git a/vl.c b/vl.c
>> index 9542095..f0dcb6b 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -2185,11 +2185,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;
>>  }
>
> Anyway,
>
> Reviewed-by: Andreas Färber <afaerber@suse.de>

Thanks!