Re: [Qemu-devel] [PATCH 09/30] access BusState.allow_hotplug using wraper qbus_is_hotpluggable()

[Tang Chen]

On 09/24/2014 07:47 PM, Igor Mammedov wrote:
> it would allow transparently switch detection if Bus
> is hotpluggable from allow_hotplug field to hotplug_handler
> link and drop allow_hotplug field once all users are
> converted to hotplug handler API.
>
> Signed-off-by: Igor Mammedov <address@hidden>
> ---
>   hw/core/qdev.c           | 6 +++---
>   hw/i386/acpi-build.c     | 2 +-
>   hw/pci/pci-hotplug-old.c | 4 ++--
>   include/hw/qdev-core.h   | 5 +++++
>   qdev-monitor.c           | 2 +-
>   5 files changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index fcb1638..5e5b963 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -86,7 +86,7 @@ static void bus_add_child(BusState *bus, DeviceState *child)
>       BusChild *kid = g_malloc0(sizeof(*kid));
>   
>       if (qdev_hotplug) {
> -        assert(bus->allow_hotplug);
> +        assert(qbus_is_hotpluggable(bus));
>       }
>   
>       kid->index = bus->max_index++;
> @@ -213,7 +213,7 @@ void qdev_unplug(DeviceState *dev, Error **errp)
>   {
>       DeviceClass *dc = DEVICE_GET_CLASS(dev);
>   
> -    if (dev->parent_bus && !dev->parent_bus->allow_hotplug) {
> +    if (dev->parent_bus && !qbus_is_hotpluggable(dev->parent_bus)) {
>           error_set(errp, QERR_BUS_NO_HOTPLUG, dev->parent_bus->name);
>           return;
>       }
> @@ -925,7 +925,7 @@ static bool device_get_hotpluggable(Object *obj, Error 
> **errp)
>       DeviceState *dev = DEVICE(obj);
>   
>       return dc->hotpluggable && (dev->parent_bus == NULL ||
> -                                dev->parent_bus->allow_hotplug);
> +                                qbus_is_hotpluggable(dev->parent_bus));
>   }
>   
>   static bool device_get_hotplugged(Object *obj, Error **err)
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index a313321..00be4bb 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -774,7 +774,7 @@ static void *acpi_set_bsel(PCIBus *bus, void *opaque)
>       unsigned *bsel_alloc = opaque;
>       unsigned *bus_bsel;
>   
> -    if (bus->qbus.allow_hotplug) {
> +    if (qbus_is_hotpluggable(BUS(bus))) {
>           bus_bsel = g_malloc(sizeof *bus_bsel);
>   
>           *bus_bsel = (*bsel_alloc)++;
> diff --git a/hw/pci/pci-hotplug-old.c b/hw/pci/pci-hotplug-old.c
> index cf2caeb..f9b7398 100644
> --- a/hw/pci/pci-hotplug-old.c
> +++ b/hw/pci/pci-hotplug-old.c
> @@ -77,7 +77,7 @@ static PCIDevice *qemu_pci_hot_add_nic(Monitor *mon,
>           monitor_printf(mon, "Invalid PCI device address %s\n", devaddr);
>           return NULL;
>       }
> -    if (!((BusState*)bus)->allow_hotplug) {
> +    if (!qbus_is_hotpluggable(BUS(bus))) {
>           monitor_printf(mon, "PCI bus doesn't support hotplug\n");
>           return NULL;
>       }
> @@ -224,7 +224,7 @@ static PCIDevice *qemu_pci_hot_add_storage(Monitor *mon,
>           monitor_printf(mon, "Invalid PCI device address %s\n", devaddr);
>           return NULL;
>       }
> -    if (!((BusState*)bus)->allow_hotplug) {
> +    if (!qbus_is_hotpluggable(BUS(bus))) {
>           monitor_printf(mon, "PCI bus doesn't support hotplug\n");
>           return NULL;
>       }
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 178fee2..48a96d2 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -368,4 +368,9 @@ static inline void qbus_set_hotplug_handler(BusState *bus, 
> DeviceState *handler,
>                                QDEV_HOTPLUG_HANDLER_PROPERTY, errp);
>       bus->allow_hotplug = 1;
>   }
> +
> +static inline bool qbus_is_hotpluggable(BusState *bus)
> +{
> +   return bus->allow_hotplug || bus->hotplug_handler;

Why not synchronize these two members ?  What is the case that bus has
a hotplug_handler but allow_hotplug is false ?

BTW, I think there are other places that using bus->allow_hotplug 
directly, right ?

eg:
s390_virtio_bus_init()
{
    ......
    _bus->allow_hotplug = 1;
    ......
}

Thanks.

> +}
>   #endif
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index 5ec6606..f6db461 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -515,7 +515,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>               return NULL;
>           }
>       }
> -    if (qdev_hotplug && bus && !bus->allow_hotplug) {
> +    if (qdev_hotplug && bus && !qbus_is_hotpluggable(bus)) {
>           qerror_report(QERR_BUS_NO_HOTPLUG, bus->name);
>           return NULL;
>       }