Re: [Qemu-devel] [PATCH] qdev: Check for the availability of a hotplug controller before adding a device

[Igor Mammedov]

On Tue, 3 Oct 2017 15:49:46 -0300
Eduardo Habkost <address@hidden> wrote:

> On Tue, Oct 03, 2017 at 06:46:02PM +0200, Thomas Huth wrote:
> > The qdev_unplug() function contains a g_assert(hotplug_ctrl) statement,
> > so QEMU crashes when the user tries to device_add + device_del a device
> > that does not have a corresponding hotplug controller. This could be
> > provoked for a couple of devices in the past (see commit 4c93950659487c7ad
> > or 84ebd3e8c7d4fe955 for example). So devices clearly need a hotplug
> > controller when they are suitable for device_add.
> > The code in qdev_device_add() already checks whether the bus has a proper
> > hotplug controller, but for devices that do not have a corresponding bus,
> > there is no appropriate check available. In that case we should check
> > whether the machine itself provides a suitable hotplug controller and
> > refuse to plug the device if none is available.
> > 
> > Signed-off-by: Thomas Huth <address@hidden>
> > ---
> >  This is the follow-up patch from my earlier try "hw/core/qdev: Do not
> >  allow hot-plugging without hotplug controller" ... AFAICS the function
> >  qdev_device_add() is now the right spot to do the check.
> > 
> >  hw/core/qdev.c         | 28 ++++++++++++++++++++--------
> >  include/hw/qdev-core.h |  1 +
> >  qdev-monitor.c         |  9 +++++++++
> >  3 files changed, 30 insertions(+), 8 deletions(-)
> > 
> > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > index 606ab53..a953ec9 100644
> > --- a/hw/core/qdev.c
> > +++ b/hw/core/qdev.c
> > @@ -253,19 +253,31 @@ void qdev_set_legacy_instance_id(DeviceState *dev, 
> > int alias_id,
> >      dev->alias_required_for_version = required_for_version;
> >  }
> >  
> > +HotplugHandler *qdev_get_machine_hotplug_handler(DeviceState *dev)
> > +{
> > +    MachineState *machine;
> > +    MachineClass *mc;
> > +    Object *m_obj = qdev_get_machine();
> > +
> > +    if (object_dynamic_cast(m_obj, TYPE_MACHINE)) {
> > +        machine = MACHINE(m_obj);
> > +        mc = MACHINE_GET_CLASS(machine);
> > +        if (mc->get_hotplug_handler) {
> > +            return mc->get_hotplug_handler(machine, dev);
> > +        }
> > +    }
> > +
> > +    return NULL;
> > +}
> > +
> >  HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev)
> >  {
> > -    HotplugHandler *hotplug_ctrl = NULL;
> > +    HotplugHandler *hotplug_ctrl;
> >  
> >      if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
> >          hotplug_ctrl = dev->parent_bus->hotplug_handler;
> > -    } else if (object_dynamic_cast(qdev_get_machine(), TYPE_MACHINE)) {
> > -        MachineState *machine = MACHINE(qdev_get_machine());
> > -        MachineClass *mc = MACHINE_GET_CLASS(machine);
> > -
> > -        if (mc->get_hotplug_handler) {
> > -            hotplug_ctrl = mc->get_hotplug_handler(machine, dev);
> > -        }
> > +    } else {
> > +        hotplug_ctrl = qdev_get_machine_hotplug_handler(dev);
> >      }
> >      return hotplug_ctrl;
> >  }
> > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> > index 0891461..5aa536d 100644
> > --- a/include/hw/qdev-core.h
> > +++ b/include/hw/qdev-core.h
> > @@ -285,6 +285,7 @@ DeviceState *qdev_try_create(BusState *bus, const char 
> > *name);
> >  void qdev_init_nofail(DeviceState *dev);
> >  void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
> >                                   int required_for_version);
> > +HotplugHandler *qdev_get_machine_hotplug_handler(DeviceState *dev);
> >  HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev);
> >  void qdev_unplug(DeviceState *dev, Error **errp);
> >  void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev,
> > diff --git a/qdev-monitor.c b/qdev-monitor.c
> > index 8fd6df9..2891dde 100644
> > --- a/qdev-monitor.c
> > +++ b/qdev-monitor.c
> > @@ -626,6 +626,15 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error 
> > **errp)
> >          return NULL;
> >      }
> >  
> > +    /* In case we don't have a bus, there must be a machine hotplug 
> > handler */
> > +    if (qdev_hotplug && !bus && !qdev_get_machine_hotplug_handler(dev)) {
> > +        error_setg(errp, "Device '%s' can not be hotplugged on this 
> > machine",
> > +                   driver);
> > +        object_unparent(OBJECT(dev));  
> 
> Isn't it better to check qdev_get_machine_hotplug_handler()
> earlier (before the qdev_set_parent_bus() and qdev_set_id()
> lines), so object_unparent() isn't necessary?
> 
> (We probably don't need to call object_unparent() here, already,
> because bus is NULL.  But moving the check before the "if (bus)
> qdev_set_parent_bus()" statement would make this more obvious).
it might be bus or bus-less device, so making check before
qdev_set_parent_bus() should be simpler.


> I would prefer to eventually make
> MachineClass::get_hotplug_handler() get a typename or
> DeviceClass* argument instead of DeviceState*, so we don't even
> create the device object.  But I don't think it's a requirement
> for this bug fix.
choice of hotplug handler might theoretically depend on plugged
device instance (over-engineered? as far as I recall none does it so far)

> 
> 
> > +        object_unref(OBJECT(dev));
> > +        return NULL;
> > +    }
wrt error exit path, I'd rework error path in qdev_device_add() in separate 
patch first
to look like it is in device_set_realized() and then just jump to appropriate 
label
from here.

> > +
> >      dev->opts = opts;
> >      object_property_set_bool(OBJECT(dev), true, "realized", &err);
> >      if (err != NULL) {
> > -- 
> > 1.8.3.1
> >   
>