Re: [Qemu-devel] [PATCH 5/7] qbus_find_recursive(): terminate search by name in case of fatal error

[Luiz Capitulino]

On Fri,  1 Feb 2013 18:38:17 +0100
Laszlo Ersek <address@hidden> wrote:

> Use an Error to communicate the "stop the search" message.
> 
> Signed-off-by: Laszlo Ersek <address@hidden>
> ---
>  hw/qdev-monitor.c |   24 ++++++++++++++++--------
>  1 files changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
> index 284dafa..83540fc 100644
> --- a/hw/qdev-monitor.c
> +++ b/hw/qdev-monitor.c
> @@ -288,7 +288,7 @@ static DeviceState *qbus_find_dev(BusState *bus, char 
> *elem)
>  }
>  
>  static BusState *qbus_find_recursive(BusState *bus, const char *name,
> -                                     const char *type)
> +                                     const char *type, Error **errp)
>  {
>      BusChild *kid;
>  
> @@ -303,8 +303,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;
>          }
>      }
> @@ -316,8 +315,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);
> -            if (ret) {
> +            ret = qbus_find_recursive(child, name, type, errp);
> +            if (ret || error_is_set(errp)) {

Does this mean ret is returned unconditionally? We should have only
one way to check for error.

>                  return ret;
>              }
>          }
> @@ -337,13 +336,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);
> +        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);
> +            }

I'll take it that this last qerror_report() call is an error generated
by qbus_find(). That is, qbus_find_recursive() _can_ return bus=NULL
on success.

>              return NULL;
>          }
>          pos = len;
> @@ -458,8 +464,10 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>              return NULL;
>          }
>      } else {
> -        bus = qbus_find_recursive(sysbus_get_default(), NULL, k->bus_type);
> +        bus = qbus_find_recursive(sysbus_get_default(), NULL, k->bus_type,
> +                                  &local_err);
>          if (!bus) {
> +            assert(!error_is_set(&local_err));
>              qerror_report(QERR_NO_BUS_FOR_DEVICE,
>                            k->bus_type, driver);
>              return NULL;