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

[Igor Mammedov <imammedo@redhat.com>]

On Thu, 25 Sep 2014 10:00:05 +0800
Tang Chen <tangchen@cn.fujitsu.com> wrote:

> 
> 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 <imammedo@redhat.com>
> > ---
> >   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 ?
There is no point in synchronizing them since by the end of this
series 'allow_hotplug' field will be removed and only 'hotplug_handler'
will be left. allow_hotplug is kept till the 29/30 patch for maintaining
series bisect-ability and for the sake of splitting patches to small clean
ones.

> 
> 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;
> >       }
>