[v2,10/15] qbus_find_recursive(): push Error and make it terminate a recursive search

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

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.