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

[Markus Armbruster]

Laszlo Ersek <address@hidden> 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 <address@hidden>
> ---
>  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;
    }