On Thu, 25 Sep 2014 10:00:05 +0800
Tang Chen <tangc...@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 <imamm...@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;
> > }
>
