Re: [Qemu-devel] [PATCH 16/24] qdev hotplug: infrastructure and monitor commands.

[Markus Armbruster <armbru@redhat.com>]

Gerd Hoffmann <kraxel@redhat.com> writes:

> From: Gerd Hoffmann <kraxel@redhat.com>
> Subject: [Qemu-devel] [PATCH 16/24] qdev hotplug: infrastructure and monitor
> 	commands.
> To: qemu-devel@nongnu.org
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Date: Fri, 25 Sep 2009 21:42:41 +0200
>
> Adds device_add and device_del commands.  device_add accepts accepts
> the same syntax like the -device command line switch.  device_del
> expects a device id.  So you should tag your devices with ids if you
> want to remove them later on, like this:
>
>   device_add pci-ohci,id=ohci
>   device_del ohci
>
> Unplugging via pci_del or usb_del works too.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/qdev.c       |   79 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/qdev.h       |   12 +++++++-
>  qemu-monitor.hx |   16 +++++++++++
>  vl.c            |    2 +
>  4 files changed, 107 insertions(+), 2 deletions(-)
>
> diff --git a/hw/qdev.c b/hw/qdev.c
> index a25245a..00cb849 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -31,6 +31,8 @@
>  #include "monitor.h"
>  
>  /* This is a nasty hack to allow passing a NULL bus to qdev_create.  */
> +static int qdev_hotplug = 0;
> +

I see the "nasty hack" part, but I don't see how qdev_create() accepts a
null bus now:

>  static BusState *main_system_bus;
>  
>  static DeviceInfo *device_info_list;
> @@ -102,6 +104,10 @@ DeviceState *qdev_create(BusState *bus, const char *name)
>      qdev_prop_set_defaults(dev, dev->parent_bus->info->props);
>      qdev_prop_set_compat(dev);
>      QLIST_INSERT_HEAD(&bus->children, dev, sibling);

It still derefences bus.

> +    if (qdev_hotplug) {
> +        assert(bus->allow_hotplug);
> +        dev->hotplugged = 1;
> +    }
>      dev->state = DEV_STATE_CREATED;
>      return dev;
>  }
> @@ -192,6 +198,11 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>                     path ? path : info->bus_info->name, info->name);
>          return NULL;
>      }
> +    if (qdev_hotplug && !bus->allow_hotplug) {
> +        qemu_error("Bus %s does not support hotplugging\n",
> +                   bus->name);
> +        return NULL;
> +    }
>  
>      /* create device, set properties */
>      qdev = qdev_create(bus, driver);
[...]

As far as I can see, all qdev_hotplug does is telling qdev_device_add()
and qdev_create() that this is a hotplug.

What about something like:

DeviceState *qdev_device_add(QemuOpts *opts, int hotplug)
{
[...]
    if (hotplug && !bus->allow_hotplug) {
        qemu_error("Bus %s does not support hotplugging\n",
                   bus->name);
        return NULL;
    }

    /* create device, set properties */
    qdev = qdev_create(bus, driver);
    if (hotplug) {
        dev->hotplugged = 1;
[...]
}