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

[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.
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;
     }
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) {