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 <address@hidden>
> Reviewed-by: Eric Blake <address@hidden>
> ---
>  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)