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

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

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            | 141 ++++++++++++++++++++++++++--------------------
 vl.c                      |   7 ++-
 6 files changed, 103 insertions(+), 100 deletions(-)

-- 
1.9.3

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

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@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 1/7] qdev: Deprecated qdev_init() is finally unused, drop

[Eric Blake]

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

On 06/08/2015 12:57 PM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/core/qdev.c         | 47 ++++++++++++++++-------------------------------
>  include/hw/qdev-core.h |  3 +--
>  2 files changed, 17 insertions(+), 33 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH 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>
---
 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 2/7] qdev: Un-deprecate qdev_init_nofail()

[Eric Blake]

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

On 06/08/2015 12:57 PM, Markus Armbruster wrote:
> It's a perfectly sensible helper function.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  include/hw/qdev-core.h | 2 --
>  1 file changed, 2 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH 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>
---
 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 3/7] qdev-monitor: Stop error avalance in qbus_find_recursive()

[Eric Blake]

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

On 06/08/2015 12:57 PM, Markus Armbruster wrote:
> 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>
> ---
>  qdev-monitor.c | 62 ++++++++++++++++++++++++++++++++++++----------------------
>  1 file changed, 39 insertions(+), 23 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

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

[Eric Blake]

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

On 06/08/2015 12:57 PM, Markus Armbruster wrote:
> 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>
> ---
>  qdev-monitor.c | 31 +++++++++++++++++--------------
>  1 file changed, 17 insertions(+), 14 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/qapi/qmp/qerror.h |  3 ---
 qdev-monitor.c            | 30 +++++++++++++++++++-----------
 2 files changed, 19 insertions(+), 14 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..f8876ef 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -288,6 +288,7 @@ 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;
@@ -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 busses",
+                           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

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

[Eric Blake]

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

On 06/08/2015 12:57 PM, Markus Armbruster wrote:
> 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.

Could we add some sort of error_append_hmp_hint() that adds additional
messages to an existing error object, for use when the error will be
printed via HMP but is a no-op for QMP?  (and make it callable more than
once, since qbus_list_dev() uses error_printf() in that role more than once)

But that can be a later patch, this one is fine as-is for following
existing practice.

> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  include/qapi/qmp/qerror.h |  3 ---
>  qdev-monitor.c            | 30 +++++++++++++++++++-----------
>  2 files changed, 19 insertions(+), 14 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"

Might want to mention one less baroque macro in qerror.h in the commit
message as an intentional part of the conversion.

> @@ -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 busses",

Stupid spell-check on my mailer is flagging 'busses' as a typo, even
though dictionary.com says both spellings are acceptable.  Other sources
prefer 'buses' and say 'busses' is out of favor:
http://grammarist.com/spelling/buses-busses/

You could always skirt the confusion by creative wording like "has
multiple bus children".  But it is a pre-existing issue [if an issue at
all], so I don't care enough to make it hold up this patch.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 06/08/2015 12:57 PM, Markus Armbruster wrote:
>> 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.
>
> Could we add some sort of error_append_hmp_hint() that adds additional
> messages to an existing error object, for use when the error will be
> printed via HMP but is a no-op for QMP?  (and make it callable more than
> once, since qbus_list_dev() uses error_printf() in that role more than once)

Should work.  Should get its own series, of course.  Volunteer welcome
:)

> But that can be a later patch, this one is fine as-is for following
> existing practice.

Agree.

>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  include/qapi/qmp/qerror.h |  3 ---
>>  qdev-monitor.c            | 30 +++++++++++++++++++-----------
>>  2 files changed, 19 insertions(+), 14 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"
>
> Might want to mention one less baroque macro in qerror.h in the commit
> message as an intentional part of the conversion.

Could append:

    While there, eliminate QERR_BUS_NOT_FOUND.

>> @@ -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 busses",
>
> Stupid spell-check on my mailer is flagging 'busses' as a typo, even
> though dictionary.com says both spellings are acceptable.  Other sources
> prefer 'buses' and say 'busses' is out of favor:
> http://grammarist.com/spelling/buses-busses/

If I have to respin anyway, I can fold in s/busses/buses/, and amend

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

> You could always skirt the confusion by creative wording like "has
> multiple bus children".

Nah, we just spell like the dictionary says we should.

>                          But it is a pre-existing issue [if an issue at
> all], so I don't care enough to make it hold up this patch.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

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

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qdev-monitor.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/qdev-monitor.c b/qdev-monitor.c
index f8876ef..01ea919 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 6/7] qdev-monitor: Propagate errors through set_property()

[Eric Blake]

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

On 06/08/2015 12:57 PM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  qdev-monitor.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@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 01ea919..fa56295 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 a %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 7d3718a..0839e2d 100644
--- a/vl.c
+++ b/vl.c
@@ -2134,11 +2134,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 7/7] qdev-monitor: Propagate errors through qdev_device_add()

[Eric Blake]

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

On 06/08/2015 12:57 PM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  include/monitor/qdev.h |  2 +-
>  qdev-monitor.c         | 36 +++++++++++++++---------------------
>  vl.c                   |  7 +++++--
>  3 files changed, 21 insertions(+), 24 deletions(-)
> 

>          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 a %s bus",
> +                       driver, object_get_typename(OBJECT(bus)));

Pre-existing - "a '%s' bus" or "an %s bus" might look a bit nicer than
"a %s bus" for some bus names (depends on the initial sound during
pronunciation).  Certainly a nightmare for translation rules to other
languages that don't follow English rules on indefinite article usage.
Maybe some word-smithing can avoid the indefinite article altogether?
But not something I care about enough to hold up the series.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 06/08/2015 12:57 PM, Markus Armbruster wrote:
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  include/monitor/qdev.h |  2 +-
>>  qdev-monitor.c         | 36 +++++++++++++++---------------------
>>  vl.c                   |  7 +++++--
>>  3 files changed, 21 insertions(+), 24 deletions(-)
>> 
>
>>          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 a %s bus",
>> +                       driver, object_get_typename(OBJECT(bus)));
>
> Pre-existing - "a '%s' bus" or "an %s bus" might look a bit nicer than
> "a %s bus" for some bus names (depends on the initial sound during
> pronunciation).  Certainly a nightmare for translation rules to other
> languages that don't follow English rules on indefinite article usage.

For a real nightmare, try following every other language's rules[*].
Not following English rules will feel like a lullaby after that.

> Maybe some word-smithing can avoid the indefinite article altogether?

Do you think "Device '%s' can't go on %s bus" would be an improvement?

> But not something I care about enough to hold up the series.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!


[*] http://search.cpan.org/dist/Locale-Maketext/lib/Locale/Maketext/TPJ13.pod?#A_Localization_Horror_Story:_It_Could_Happen_To_You

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

[Eric Blake]

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

On 06/09/2015 12:12 AM, Markus Armbruster wrote:

>>
>>>          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 a %s bus",
>>> +                       driver, object_get_typename(OBJECT(bus)));
>>
>> Pre-existing - "a '%s' bus" or "an %s bus" might look a bit nicer than
>> "a %s bus" for some bus names (depends on the initial sound during
>> pronunciation).  Certainly a nightmare for translation rules to other
>> languages that don't follow English rules on indefinite article usage.
> 
> For a real nightmare, try following every other language's rules[*].
> Not following English rules will feel like a lullaby after that.
> 
>> Maybe some word-smithing can avoid the indefinite article altogether?
> 
> Do you think "Device '%s' can't go on %s bus" would be an improvement?

Yes, I think that works.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]