Re: [Qemu-devel] [PATCH 13/30] qdev: hotplug: set handler only if HOTPLUG_HANDLER interface is supported

[Igor Mammedov]

On Wed, 24 Sep 2014 14:19:06 +0200
Paolo Bonzini <address@hidden> wrote:

> Il 24/09/2014 13:48, Igor Mammedov ha scritto:
> > Check if 'handler' implements HOTPLUG_HANDLER interface
> > before setting it, if it's not then do nothing and leave
> > bus not hotpluggable.
> > 
> > That would allow to reuse the same code for creating bus
> > for example 'scsi_bus_new()' for both hotpluggable and not
> > hotpluggable controllers.
> > 
> > Signed-off-by: Igor Mammedov <address@hidden>
> > ---
> > PS:
> > Keep allow_hotplug = 1 before check to preserve legacy
> > behaviour for incremental conversion without breaking
> > bisect-ability. I will be removed later in series.
> > ---
> >  include/hw/qdev-core.h | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> > index ba812c5..653ff4a 100644
> > --- a/include/hw/qdev-core.h
> > +++ b/include/hw/qdev-core.h
> > @@ -366,9 +366,14 @@ char *qdev_get_dev_path(DeviceState *dev);
> >  static inline void qbus_set_hotplug_handler(BusState *bus, DeviceState 
> > *handler,
> >                                              Error **errp)
> >  {
> > +    bus->allow_hotplug = 1;
> > +
> > +    if (!object_dynamic_cast(OBJECT(handler), TYPE_HOTPLUG_HANDLER)) {
> > +        return;
> > +    }
> > +
> >      object_property_set_link(OBJECT(bus), OBJECT(handler),
> >                               QDEV_HOTPLUG_HANDLER_PROPERTY, errp);
> > -    bus->allow_hotplug = 1;
> >  }
> >  
> >  static inline bool qbus_is_hotpluggable(BusState *bus)
> > 
> 
> Ok, so scsi won't error_abort, but you are actually breaking
> hotplug/unplug on HBAs other than virtio-scsi and pvscsi.  On those HBAs
> you can actually do surprise removal and hotplug, but you have to
> manually rescan the bus in the virtual machine.
I was hesitant what do with other HBAs without explicit hotplug notification.
I'll add dropped patches that convert them as well.

> 
> Reviewed-by: Paolo Bonzini <address@hidden>
>