Re: [Qemu-devel] [PATCH] qdev: Fix device_add bus assumptions

[Andreas Färber]

Am 18.04.2013 10:41, schrieb Igor Mammedov:
> On Tue, 16 Apr 2013 03:50:21 +0200
> Andreas Färber <address@hidden> wrote:
> 
>> Drop an unreachable fallback bus assignment to SysBus.
>>
>> If no ,bus= is specified, only search busses recursively for bus type if
>> the DeviceClass has a bus_type specified. Handle resulting NULL cases.
>>
>> Signed-off-by: Andreas Färber <address@hidden>
>> ---
>>  qdev-monitor.c | 16 ++++++++--------
>>  1 file changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/qdev-monitor.c b/qdev-monitor.c
>> index 9a78ccf..73d7946 100644
>> --- a/qdev-monitor.c
>> +++ b/qdev-monitor.c
>> @@ -18,6 +18,7 @@
>>   */
>>  
>>  #include "hw/qdev.h"
>> +#include "hw/sysbus.h"
>>  #include "monitor/monitor.h"
>>  #include "monitor/qdev.h"
>>  #include "qmp-commands.h"
>> @@ -415,7 +416,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>>      DeviceClass *k;
>>      const char *driver, *path, *id;
>>      DeviceState *qdev;
>> -    BusState *bus;
>> +    BusState *bus = NULL;
>>  
>>      driver = qemu_opt_get(opts, "driver");
>>      if (!driver) {
>> @@ -453,7 +454,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>>                            driver, object_get_typename(OBJECT(bus)));
>>              return NULL;
>>          }
>> -    } else {
>> +    } else if (k->bus_type != NULL) {
>>          bus = qbus_find_recursive(sysbus_get_default(), NULL, k->bus_type);
>>          if (!bus) {
>>              qerror_report(QERR_NO_BUS_FOR_DEVICE,
>> @@ -461,18 +462,17 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>>              return NULL;
>>          }
>>      }
>> -    if (qdev_hotplug && !bus->allow_hotplug) {
>> +    if (qdev_hotplug && bus && !bus->allow_hotplug) {
>>          qerror_report(QERR_BUS_NO_HOTPLUG, bus->name);
>>          return NULL;
>>      }
>>  
>> -    if (!bus) {
>> -        bus = sysbus_get_default();
>> -    }
>> -
> I've checked all direct childs of TYPE_DEVICE and they all set k->bus_type,
> with only one exception of TYPE_CPU.

> So it should be safe to remove fallback
> from qdev_device_add POV.

Sorry, don't understand what exactly you are suggesting?

> However  TYPE_CPU breaks assumption that device always has parent_bus set to 
> not
> NULL in qdev_unplug() and qdev_print()
> 
> It would be better to add something like this:
> // untested
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 4eb0134..45009ba 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -207,7 +207,7 @@ void qdev_unplug(DeviceState *dev, Error **errp)
>  {
>      DeviceClass *dc = DEVICE_GET_CLASS(dev);
>  
> -    if (!dev->parent_bus->allow_hotplug) {
> +    if (dev->parent_bus && !dev->parent_bus->allow_hotplug) {
>          error_set(errp, QERR_BUS_NO_HOTPLUG, dev->parent_bus->name);
>          return;
>      }

This part looks good, can you resend as a proper patch? Even if we can't
test the no-parent_bus unplug path ATM, it shouldn't hurt to fix
assumptions about it.

Andreas

> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index 9a78ccf..2476e4e 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -557,7 +557,9 @@ static void qdev_print(Monitor *mon, DeviceState *dev, 
> int indent)
>          qdev_print_props(mon, dev, DEVICE_CLASS(class)->props, indent);
>          class = object_class_get_parent(class);
>      } while (class != object_class_by_name(TYPE_DEVICE));
> -    bus_print_dev(dev->parent_bus, mon, dev, indent);
> +    if (dev->parent_bus) {
> +        bus_print_dev(dev->parent_bus, mon, dev, indent);
> +    }
>      QLIST_FOREACH(child, &dev->child_bus, sibling) {
>          qbus_print(mon, child, indent);
>      }
> 
>>      /* create device, set properties */
>>      qdev = DEVICE(object_new(driver));
>> -    qdev_set_parent_bus(qdev, bus);
>> +
>> +    if (bus) {
>> +        qdev_set_parent_bus(qdev, bus);
>> +    }
>>  
>>      id = qemu_opts_id(opts);
>>      if (id) {
> 
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg