Re: [Qemu-devel] [PATCH v2 00/15] propagate Errors to do_device_add()

Re: [Qemu-devel] [PATCH v2 00/15] propagate Errors to do_device_add()

[Markus Armbruster]

No longer applies cleanly, but git-am -3 takes care of everything except
for a trivial #include conflict.  I didn't test the result, though :)

Re: [Qemu-devel] [PATCH v2 02/15] do_device_add(): look up "device" opts list with qemu_find_opts_err()

[Markus Armbruster]

Laszlo Ersek <lersek@redhat.com> writes:

> Conversion status (call chains covered or substituted by error propagation
> marked with square brackets):
>
> do_device_add -> [qemu_find_opts -> error_report]
> do_device_add -> qdev_device_add -> qerror_report
> do_device_add -> qdev_device_add -> qbus_find -> qbus_find_recursive
>   -> qerror_report
> do_device_add -> qdev_device_add -> qbus_find -> qerror_report
> do_device_add -> qdev_device_add -> set_property -> qdev_prop_parse
>   -> qerror_report_err
>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  hw/qdev-monitor.c |    7 ++++++-
>  1 files changed, 6 insertions(+), 1 deletions(-)
>
> diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
> index 56d66c3..bbdc90f 100644
> --- a/hw/qdev-monitor.c
> +++ b/hw/qdev-monitor.c
> @@ -590,15 +590,20 @@ void do_info_qdm(Monitor *mon, const QDict *qdict)
>  int do_device_add(Monitor *mon, const QDict *qdict, QObject **ret_data)
>  {
>      Error *local_err = NULL;
> +    QemuOptsList *list;
>      QemuOpts *opts;
>      DeviceState *dev;
>  
> -    opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict, &local_err);
> +    list = qemu_find_opts_err("device", &local_err);
> +    if (!error_is_set(&local_err)) {
> +        opts = qemu_opts_from_qdict(list, qdict, &local_err);
> +    }
>      if (error_is_set(&local_err)) {
>          qerror_report_err(local_err);
>          error_free(local_err);
>          return -1;
>      }
> +
>      if (!monitor_cur_is_qmp() && qdev_device_help(opts)) {
>          qemu_opts_del(opts);
>          return 0;
       }
       dev = qdev_device_add(opts, &local_err);

/work/armbru/qemu/qdev-monitor.c:667:9: warning: ‘opts’ may be used uninitialized in this function [-Wmaybe-uninitialized]

Not really, of course.  We get here only if local_err is clear, and then
we surely set opts.

Recommend "fix" this warning by dropping this patch.
qemu_find_opts("device") can't fail.  We already assume that elsewhere,
e.g. in drive_init(), balloon_parse(), virtcon_parse(), ...

Re: [Qemu-devel] [PATCH v2 05/15] qdev_prop_parse(): change return type to void

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Thu, 7 Feb 2013 15:12:22 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
>
>> On Tue, Feb 05, 2013 at 09:39:18PM +0100, Laszlo Ersek wrote:
>> > Problems are now reported via Error only.
>> > 
>> > Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> > ---
>> >  hw/qdev-properties.h |    4 ++--
>> >  hw/qdev-monitor.c    |    3 ++-
>> >  hw/qdev-properties.c |   14 ++++++--------
>> >  3 files changed, 10 insertions(+), 11 deletions(-)
>> > 
>> > diff --git a/hw/qdev-properties.h b/hw/qdev-properties.h
>> > index 0fe4030..43fd11a 100644
>> > --- a/hw/qdev-properties.h
>> > +++ b/hw/qdev-properties.h
>> > @@ -100,8 +100,8 @@ extern PropertyInfo qdev_prop_pci_host_devaddr;
>> >  
>> >  /* Set properties between creation and init.  */
>> >  void *qdev_get_prop_ptr(DeviceState *dev, Property *prop);
>> > -int qdev_prop_parse(DeviceState *dev, const char *name, const char *value,
>> > -                    Error **errp);
>> > +void qdev_prop_parse(DeviceState *dev, const char *name, const char *value,
>> > +                     Error **errp);
>> >  void qdev_prop_set_bit(DeviceState *dev, const char *name, bool value);
>> >  void qdev_prop_set_uint8(DeviceState *dev, const char *name, uint8_t value);
>> >  void qdev_prop_set_uint16(DeviceState *dev, const char *name, uint16_t value);
>> > diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
>> > index 5eb1c8c..cf96046 100644
>> > --- a/hw/qdev-monitor.c
>> > +++ b/hw/qdev-monitor.c
>> > @@ -110,7 +110,8 @@ static int set_property(const char *name, const char *value, void *opaque)
>> >      if (strcmp(name, "bus") == 0)
>> >          return 0;
>> >  
>> > -    if (qdev_prop_parse(dev, name, value, &err) == -1) {
>> > +    qdev_prop_parse(dev, name, value, &err);
>> > +    if (error_is_set(&err)) {
>> 
>> You can use "if (err)" instead. I believe it is acceptable, as there's
>> lots of (recently introduced) QEMU code using this style.
>
> Yes, people tend to use if (err) in new code. For me it honestly doesn't
> matter much, although I wonder if we should have a more strict rule for this.

I prefer plain "if (err)".

>> >          qerror_report_err(err);
>> >          error_free(err);
>> >          return -1;
>> > diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
>> > index 8e3d014..d94909e 100644
>> > --- a/hw/qdev-properties.c
>> > +++ b/hw/qdev-properties.c
>> > @@ -835,8 +835,8 @@ void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev,
>> >      }
>> >  }
>> >  
>> > -int qdev_prop_parse(DeviceState *dev, const char *name, const char *value,
>> > -                    Error **errp)
>> > +void qdev_prop_parse(DeviceState *dev, const char *name, const char *value,
>> > +                     Error **errp)
>> >  {
>> >      char *legacy_name;
>> >      Error *err = NULL;
>> > @@ -849,11 +849,7 @@ int qdev_prop_parse(DeviceState *dev, const char *name, const char *value,
>> >      }
>> >      g_free(legacy_name);
>> >  
>> > -    if (err) {
>> > -        error_propagate(errp, err);
>> > -        return -1;
>> > -    }
>> > -    return 0;
>> > +    error_propagate(errp, err);
>> 
>> I didn't expect it to be valid to call error_propagate() if error is
>> _not_ set. All cases of error_propagate() usage I have seen before first
>> checked if error was set. But by looking at the implementation, it seems
>> to be OK.

Yes, it does the right thing.  Weaker preconditions are good :)

>> Maybe we should extend the error_propagate() documentation to mention it
>> is OK to call error_propagate() even if error is unset?

Makes sense.

[...]

Re: [Qemu-devel] [PATCH v2 06/15] set_property(): extend signature with Error

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Thu, 7 Feb 2013 15:18:41 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
>
>> On Tue, Feb 05, 2013 at 09:39:19PM +0100, Laszlo Ersek wrote:
>> > 
>> > Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> > ---
>> >  hw/qdev-monitor.c |   21 ++++++++++++++++-----
>> >  1 files changed, 16 insertions(+), 5 deletions(-)
>> > 
>> > diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
>> > index cf96046..545e66c 100644
>> > --- a/hw/qdev-monitor.c
>> > +++ b/hw/qdev-monitor.c
>> > @@ -100,9 +100,14 @@ static void qdev_print_devinfo(ObjectClass *klass, void *opaque)
>> >      error_printf("\n");
>> >  }
>> >  
>> > +typedef struct {
>> > +    DeviceState *dev;
>> > +    Error **errp;
>> > +} PropertyContext;
>> > +
>> >  static int set_property(const char *name, const char *value, void *opaque)
>> >  {
>> > -    DeviceState *dev = opaque;
>> > +    PropertyContext *ctx = opaque;
>> >      Error *err = NULL;
>> >  
>> >      if (strcmp(name, "driver") == 0)
>> > @@ -110,7 +115,7 @@ static int set_property(const char *name, const char *value, void *opaque)
>> >      if (strcmp(name, "bus") == 0)
>> >          return 0;
>> >  
>> > -    qdev_prop_parse(dev, name, value, &err);
>> > +    qdev_prop_parse(ctx->dev, name, value, &err);
>> >      if (error_is_set(&err)) {
>> >          qerror_report_err(err);
>> >          error_free(err);
>> > @@ -481,10 +486,16 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>> >      if (id) {
>> >          qdev->id = id;
>> >      }
>> > -    if (qemu_opt_foreach(opts, set_property, qdev, 1) != 0) {
>> > -        qdev_free(qdev);
>> > -        return NULL;
>> > +
>> > +    {
>> > +        PropertyContext ctx = { .dev = qdev, .errp = NULL };
>> > +
>> > +        if (qemu_opt_foreach(opts, set_property, &ctx, 1) != 0) {
>> 
>> What about redefining qemu_opt_loopfunc to accept an Error parameter?
>> 
>> qemu_opt_foreach() is used in only 4 places in the whole QEMU code, and
>> it already has code to abort on error (but using the function return
>> value instead of an Error** paramater).
>
> He would have to convert the users too, which would be very welcome,
> but it's out of the scope of this series IMO (but would be fine as
> a first series to this work).

I'm fine with this patch as is.

I wouldn't add Error support to qemu_opt_foreach(), I'd replace it by a
more C-ish qemu_opt_next():

    for (opt = qemu_opt_next(NULL); opt; opt = qemu_opt_next(opt)) {
        set_property(qdev, qemu_opt_name(opt), qemu_opt_value(opt), &err);
        if (err) {
            ...
            break;
        }
    }

Higher-order functions are great in Lisp, but awkward in C.

>
>> 
>> 
>> 
>> > +            qdev_free(qdev);
>> > +            return NULL;
>> > +        }
>> >      }
>> > +
>> >      if (qdev->id) {
>> >          object_property_add_child(qdev_get_peripheral(), qdev->id,
>> >                                    OBJECT(qdev), NULL);
>> > -- 
>> > 1.7.1
>> > 
>> > 
>> 

Re: [Qemu-devel] [PATCH v2 07/15] set_property(): push error handling to callers

[Markus Armbruster]

Laszlo Ersek <lersek@redhat.com> writes:

> The return type can't be changed to void, because "set_property" must have
> type "qemu_opt_loopfunc".

Further evidence of qemu_opt_foreach()'s awkwardness.

[...]

Re: [Qemu-devel] [PATCH v2 08/15] qbus_find_recursive(): reorganize

[Markus Armbruster]

Laszlo Ersek <lersek@redhat.com> writes:

> Eliminate the "match" variable, and move the remaining locals to the
> narrowest possible scope.

Also rename parameter bus_typename to type.

>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  hw/qdev-monitor.c |   35 ++++++++++++++++-------------------
>  1 files changed, 16 insertions(+), 19 deletions(-)
>
> diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
> index da32763..64359ee 100644
> --- a/hw/qdev-monitor.c
> +++ b/hw/qdev-monitor.c
> @@ -289,38 +289,35 @@ static DeviceState *qbus_find_dev(BusState *bus, char *elem)
>  }
>  
>  static BusState *qbus_find_recursive(BusState *bus, const char *name,
> -                                     const char *bus_typename)
> +                                     const char *type)
>  {
> -    BusClass *bus_class = BUS_GET_CLASS(bus);
>      BusChild *kid;
> -    BusState *child, *ret;
> -    int match = 1;
>  
> -    if (name && (strcmp(bus->name, name) != 0)) {
> -        match = 0;
> -    }
> -    if (bus_typename && !object_dynamic_cast(OBJECT(bus), bus_typename)) {
> -        match = 0;
> -    }
> -    if ((bus_class->max_dev != 0) && (bus_class->max_dev <= bus->max_index)) {
> +    if ((name == NULL || strcmp(bus->name, name) == 0) &&
> +        (type == NULL || object_dynamic_cast(OBJECT(bus), type) != NULL)) {
> +        BusClass *bus_class = BUS_GET_CLASS(bus);
> +
> +        /* name (if any) and type (if any) match; check free slots */
> +        if (bus_class->max_dev == 0 || bus->max_index < bus_class->max_dev) {
> +            return bus;
> +        }
> +
> +        /* bus is full -- fatal error for search by name */
>          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;
>          }
>      }
> -    if (match) {
> -        return bus;
> -    }

I'm not sure this is an improvement.  Hairy before, hairy after.  Maybe
it becomes more of an improvement later in the series.

>  
>      QTAILQ_FOREACH(kid, &bus->children, sibling) {
>          DeviceState *dev = kid->child;
> +        BusState *child;
> +
>          QLIST_FOREACH(child, &dev->child_bus, sibling) {
> -            ret = qbus_find_recursive(child, name, bus_typename);
> +            BusState *ret;
> +
> +            ret = qbus_find_recursive(child, name, type);
>              if (ret) {
>                  return ret;
>              }

I doubt moving locals to the narrowest scope is worth the churn.

Wait a second...  code before this patch:

    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;

        if (name && (strcmp(bus->name, name) != 0)) {
            match = 0;
        }
        if (bus_typename && !object_dynamic_cast(OBJECT(bus), bus_typename)) {
            match = 0;
        }
        if ((bus_class->max_dev != 0) && (bus_class->max_dev <= bus->max_index)) {

We get here when bus if full.

            if (name != NULL) {

bus is full, and we're looking for a named bus.

                /* bus was explicitly specified: return an error. */
                qerror_report(ERROR_CLASS_GENERIC_ERROR, "Bus '%s' is full",
                              bus->name);
                return NULL;

Error out on full bus, as long as we're looking for a named bus (name !=
NULL), whether the full bus matches name and type (match != 0) or not
(match == 0).

            } else {
                /* bus was not specified: try to find another one. */
                match = 0;
            }
        }
        if (match) {

bus matches name and type, and is not full.

            return bus;
        }

        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;
                }
            }
        }
        return NULL;
    }

Code after this patch:

    static BusState *qbus_find_recursive(BusState *bus, const char *name,
                                         const char *type)
    {
        BusChild *kid;

        if ((name == NULL || strcmp(bus->name, name) == 0) &&
            (type == NULL || object_dynamic_cast(OBJECT(bus), type) != NULL)) {

We get here when bus matches name and type.

            BusClass *bus_class = BUS_GET_CLASS(bus);

            /* name (if any) and type (if any) match; check free slots */
            if (bus_class->max_dev == 0 || bus->max_index < bus_class->max_dev) {

bus matches name and type, and is not full.  Same as before.

                return bus;
            }

            /* bus is full -- fatal error for search by name */
            if (name != NULL) {

bus matches name and type, is full, and we're looking for a named bus.

                qerror_report(ERROR_CLASS_GENERIC_ERROR, "Bus '%s' is full",
                              bus->name);
                return NULL;

Error out on full bus, as long as we're looking for a named bus (name !=
NULL), and the full bus matches name and type.  Silent change!
Intentional?

            }
        }

        QTAILQ_FOREACH(kid, &bus->children, sibling) {
            DeviceState *dev = kid->child;
            BusState *child;

            QLIST_FOREACH(child, &dev->child_bus, sibling) {
                BusState *ret;

                ret = qbus_find_recursive(child, name, type);
                if (ret) {
                    return ret;
                }
            }
        }
        return NULL;
    }

Re: [Qemu-devel] [PATCH v2 10/15] qbus_find_recursive(): push Error and make it terminate a recursive search

[Markus Armbruster]

Laszlo Ersek <lersek@redhat.com> writes:

> Push error handling to callers.
>
> When a bus is found by name and it has no free slots, that's a fatal error
> for the recursive search. Clarify the interface contract, and use error
> propagation to unwind the recursion quickly.
>
> Conversion status (call chains covered or substituted by error propagation
> marked with square brackets):
>
> do_device_add -> [qemu_find_opts -> error_report]
> do_device_add -> qdev_device_add -> qerror_report
> do_device_add -> qdev_device_add -> qbus_find -> [qbus_find_recursive
>   -> qerror_report]
> do_device_add -> qdev_device_add -> qbus_find -> qerror_report
> do_device_add -> qdev_device_add -> [set_property -> qdev_prop_parse
>   -> qerror_report_err]
>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  hw/qdev-monitor.c |   55 ++++++++++++++++++++++++++++++++++++++++++++++------
>  1 files changed, 48 insertions(+), 7 deletions(-)
>
> diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
> index 0a0bced..4fa64c9 100644
> --- a/hw/qdev-monitor.c
> +++ b/hw/qdev-monitor.c
> @@ -288,6 +288,40 @@ static DeviceState *qbus_find_dev(BusState *bus, char *elem)
>      return NULL;
>  }
>  
> +/*
> + * qbus_find_recursive() -- locate a bus by name and/or type in a subtree
> + *
> + * Search the bus subtree rooted in @bus for a bus matching the optionally
> + * specified @name and the optionally specified @type attributes. Matching
> + * candidates are examined for a free bus slot.
> + *
> + * The following return configurations are possible:
> + *
> + * - @retval != NULL   Matching bus with a free slot found.
> + *
> + * - @retval == NULL   No matching bus with a free slot was found.
> + *                     For (@name != NULL) calls, a further distinction is
> + *                     made for this case:
> + *
> + *   - *@errp == NULL  Bus not found by requested name. For recursive calls,
> + *                     the search should continue with siblings of @bus.
> + *
> + *   - *@errp != NULL  Bus found by name, but it has no free slot. For
> + *                     recursive calls, this is an abort condition; names are
> + *                     unique.
> + *
> + *  Parameters:
> + *
> + *  - @bus   root of subtree to search
> + *
> + *  - @name  if non-NULL, restrict search by this bus name
> + *
> + *  - @type  if non-NULL, restrict search by this bus type
> + *
> + *  - @errp  Returns the error if bus is found by name but it has no free slot.
> + *           (@errp != NULL && *@errp == NULL) must hold on input for all
> + *           calls.
> + */

Thanks for going the extra mile and documenting the function's
non-trivial contract.

>  static BusState *qbus_find_recursive(BusState *bus, const char *name,
>                                       const char *type, Error **errp)
>  {
> @@ -304,8 +338,7 @@ static BusState *qbus_find_recursive(BusState *bus, const char *name,
>  
>          /* bus is full -- fatal error for search by name */
>          if (name != NULL) {
> -            qerror_report(ERROR_CLASS_GENERIC_ERROR, "Bus '%s' is full",
> -                          bus->name);
> +            error_setg(errp, "Bus '%s' is full", bus->name);
>              return NULL;
>          }
>      }
> @@ -317,8 +350,8 @@ static BusState *qbus_find_recursive(BusState *bus, const char *name,
>          QLIST_FOREACH(child, &dev->child_bus, sibling) {
>              BusState *ret;
>  
> -            ret = qbus_find_recursive(child, name, type, NULL);
> -            if (ret) {
> +            ret = qbus_find_recursive(child, name, type, errp);
> +            if (ret || error_is_set(errp)) {
>                  return ret;
>              }
>          }
> @@ -338,13 +371,20 @@ static BusState *qbus_find(const char *path)
>          bus = sysbus_get_default();
>          pos = 0;
>      } else {
> +        Error *err = NULL;
> +
>          if (sscanf(path, "%127[^/]%n", elem, &len) != 1) {
>              assert(!path[0]);
>              elem[0] = len = 0;
>          }
> -        bus = qbus_find_recursive(sysbus_get_default(), elem, NULL, NULL);
> +        bus = qbus_find_recursive(sysbus_get_default(), elem, NULL, &err);
>          if (!bus) {
> -            qerror_report(QERR_BUS_NOT_FOUND, elem);
> +            if (error_is_set(&err)) {
> +                qerror_report_err(err);
> +                error_free(err);
> +            } else {
> +                qerror_report(QERR_BUS_NOT_FOUND, elem);
> +            }
>              return NULL;
>          }
>          pos = len;
> @@ -460,8 +500,9 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>          }
>      } else {
>          bus = qbus_find_recursive(sysbus_get_default(), NULL, k->bus_type,
> -                                  NULL);
> +                                  &local_err);
>          if (!bus) {
> +            assert(!error_is_set(&local_err));
>              qerror_report(QERR_NO_BUS_FOR_DEVICE,
>                            k->bus_type, driver);
>              return NULL;

Straightforward way to lift the error reporting.

I didn't like the function before, and now I like it even less, as
lifting the error reporting complicates its contract further.

Note how many words you need to explain how to interpret
qbus_find_recursive()'s results.  I believe that's because it tries to
do two similar, but different things: find a bus of a certain type with
a free slot, or find look up a bus by name, which must have a free slot
(else error).  Three outcomes:

* Not found (both)

* Found, but no free slot (by name only)

* Found, has free slot (both)

I'd try to split the pie differently.  Have a function to walk the qbus
tree, taking parameters bool (*test)(BusState *, void *opaque), void
*opaque.  For each qbus, call test(qbus, opaque).  If it returns true,
stop the search and return qbus.  Else continue walk.  If walk ends
without test() returning true, return NULL.  No Error * anywhere.

Lookup by name should be trivial using this walker.  If the walker
returns NULL, error bus not found.  Else, if the returned bus has no
free slots, error bus full.  Else it's good.

Searching for a bus of a certain type with space should be just as easy.

Re: [Qemu-devel] [PATCH v2 12/15] qbus_find(): propagate error handling / consumption to callers

[Markus Armbruster]

Laszlo Ersek <lersek@redhat.com> writes:

> Conversion status (call chains covered or substituted by error propagation
> marked with square brackets):
>
> do_device_add -> [qemu_find_opts -> error_report]
> do_device_add -> qdev_device_add -> qerror_report
> do_device_add -> qdev_device_add -> [qbus_find -> qbus_find_recursive
>   -> qerror_report]
> do_device_add -> qdev_device_add -> [qbus_find -> qerror_report]
> do_device_add -> qdev_device_add -> [set_property -> qdev_prop_parse
>   -> qerror_report_err]
>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  hw/qdev-monitor.c |   30 ++++++++++++++++--------------
>  1 files changed, 16 insertions(+), 14 deletions(-)
>
> diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
> index 728aa24..80fc9f8 100644
> --- a/hw/qdev-monitor.c
> +++ b/hw/qdev-monitor.c
> @@ -371,20 +371,20 @@ static BusState *qbus_find(const char *path, Error **errp)
>          bus = sysbus_get_default();
>          pos = 0;
>      } else {
> -        Error *err = NULL;
> +        Error *named_bus_full = NULL;
>  
>          if (sscanf(path, "%127[^/]%n", elem, &len) != 1) {
>              assert(!path[0]);
>              elem[0] = len = 0;
>          }
> -        bus = qbus_find_recursive(sysbus_get_default(), elem, NULL, &err);
> +        bus = qbus_find_recursive(sysbus_get_default(), elem, NULL,
> +                                  &named_bus_full);
>          if (!bus) {
> -            if (error_is_set(&err)) {
> -                qerror_report_err(err);
> -                error_free(err);
> -            } else {
> -                qerror_report(QERR_BUS_NOT_FOUND, elem);
> -            }
> +            Error *not_found = NULL;
> +
> +            error_set(&not_found, QERR_BUS_NOT_FOUND, elem);
> +            error_propagate(errp, named_bus_full);
> +            error_propagate(errp, not_found);
>              return NULL;
>          }
>          pos = len;

Cute trick!

Would look even cuter if we had a function returning a newly minted
error object.

[...]

Re: [Qemu-devel] [PATCH v2 00/15] propagate Errors to do_device_add()

[Markus Armbruster]

I think you're on the right track.  I like your systematic approach.
Sorry for taking waaaaay too long to review.  You deserve better!