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 <thuth@redhat.com>
---
 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));
+        object_unref(OBJECT(dev));
+        return NULL;
+    }
+
     dev->opts = opts;
     object_property_set_bool(OBJECT(dev), true, "realized", &err);
     if (err != NULL) {
-- 
1.8.3.1

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 <thuth@redhat.com>
> ---
>  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).

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.


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

-- 
Eduardo

On Tue, 3 Oct 2017 15:49:46 -0300
Eduardo Habkost <ehabkost@redhat.com> 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 <thuth@redhat.com>
> > ---
> >  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
> >   
> 

On 05.10.2017 10:36, Igor Mammedov wrote:
> On Tue, 3 Oct 2017 15:49:46 -0300
> Eduardo Habkost <ehabkost@redhat.com> 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 <thuth@redhat.com>
>>> ---
>>>  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.

The check for devices that have a bus is already done earlier in this
function ("if (qdev_hotplug && bus && !qbus_is_hotpluggable(bus))") ...
but yes, I'll move the new check for bus-less devices a little bit
earlier so that the unparent is not necessary anymore.

>> 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)

Yes, currently we might be able to do it with the typename only ... but
that seems to be a rather big rework right now, and we might indeed need
a real device instance later again, so I'd prefer to rather not do that
rework right now.

>>> +        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.

Ok, I can have a try whether that looks better.

 Thomas

On Thu, 5 Oct 2017 11:00:29 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 05.10.2017 10:36, Igor Mammedov wrote:
> > On Tue, 3 Oct 2017 15:49:46 -0300
> > Eduardo Habkost <ehabkost@redhat.com> 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 <thuth@redhat.com>
> >>> ---
> >>>  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.  
> 
> The check for devices that have a bus is already done earlier in this
> function ("if (qdev_hotplug && bus && !qbus_is_hotpluggable(bus))") ...
> but yes, I'll move the new check for bus-less devices a little bit
> earlier so that the unparent is not necessary anymore.
> 
> >> 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)  
> 
> Yes, currently we might be able to do it with the typename only ... but
> that seems to be a rather big rework right now, and we might indeed need
> a real device instance later again, so I'd prefer to rather not do that
> rework right now.
it was just remark why it uses 'dev' and a call for an action.

> 
> >>> +        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.  
> 
> Ok, I can have a try whether that looks better.
the function already has couple error exit point that
duplicate cleanup steps, so it probably would read better
with cleanup

> 
>  Thomas
> 

On Thu, 5 Oct 2017 11:12:41 +0200
Igor Mammedov <imammedo@redhat.com> wrote:

> On Thu, 5 Oct 2017 11:00:29 +0200
> Thomas Huth <thuth@redhat.com> wrote:
[...] 
> > Yes, currently we might be able to do it with the typename only ... but
> > that seems to be a rather big rework right now, and we might indeed need
> > a real device instance later again, so I'd prefer to rather not do that
> > rework right now.  
> it was just remark why it uses 'dev' and a call for an action.
forgot the most important word

s/a call/NOT a call/

[...]

On Tue,  3 Oct 2017 18:46:02 +0200
Thomas Huth <thuth@redhat.com> 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 <thuth@redhat.com>
> ---
>  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)) {
current machine hotplug handler serves both cold and hot-plug so in reality it's
just  'plug' handler.

Is there a point -device/device_add devices on board that doesn't have 'hotplug'
handler that would wire device up properly?

Do we have such devices?



> +        error_setg(errp, "Device '%s' can not be hotplugged on this machine",
> +                   driver);
> +        object_unparent(OBJECT(dev));
> +        object_unref(OBJECT(dev));
> +        return NULL;
> +    }
> +
>      dev->opts = opts;
>      object_property_set_bool(OBJECT(dev), true, "realized", &err);
>      if (err != NULL) {

On 04.10.2017 13:36, Igor Mammedov wrote:
> On Tue,  3 Oct 2017 18:46:02 +0200
> Thomas Huth <thuth@redhat.com> 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 <thuth@redhat.com>
>> ---
>>  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)) {
> current machine hotplug handler serves both cold and hot-plug so in reality it's
> just  'plug' handler.
> 
> Is there a point -device/device_add devices on board that doesn't have 'hotplug'
> handler that would wire device up properly?

Sorry, I did not get that question ... do you mean whether there's any
code that uses qdev_device_add() to add a device without hotplug
controller? I don't think so. It's currently only used by
qmp_device_add() for the QMP/HMP command, in vl.c for -device and in the
USB code for xen-usb host device. So this function currently really only
makes sense for devices that have a hotplug controller.

 Thomas

On Wed, Oct 04, 2017 at 09:29:54PM +0200, Thomas Huth wrote:
> On 04.10.2017 13:36, Igor Mammedov wrote:
> > On Tue,  3 Oct 2017 18:46:02 +0200
> > Thomas Huth <thuth@redhat.com> 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 <thuth@redhat.com>
> >> ---
> >>  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)) {
> > current machine hotplug handler serves both cold and hot-plug so in reality it's
> > just  'plug' handler.
> > 
> > Is there a point -device/device_add devices on board that doesn't have 'hotplug'
> > handler that would wire device up properly?
> 
> Sorry, I did not get that question ... do you mean whether there's any
> code that uses qdev_device_add() to add a device without hotplug
> controller? I don't think so. It's currently only used by
> qmp_device_add() for the QMP/HMP command, in vl.c for -device and in the
> USB code for xen-usb host device. So this function currently really only
> makes sense for devices that have a hotplug controller.

I assume you are talking only about hotpluggable devices.  With
-device, qdev_device_add() is also used for devices that don't
have a hotplug controller, but this is supposed to be true only
for non-hotpluggable devices.

-- 
Eduardo

On 04.10.2017 23:21, Eduardo Habkost wrote:
> On Wed, Oct 04, 2017 at 09:29:54PM +0200, Thomas Huth wrote:
>> On 04.10.2017 13:36, Igor Mammedov wrote:
>>> On Tue,  3 Oct 2017 18:46:02 +0200
>>> Thomas Huth <thuth@redhat.com> 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 <thuth@redhat.com>
>>>> ---
>>>>  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)) {
>>> current machine hotplug handler serves both cold and hot-plug so in reality it's
>>> just  'plug' handler.
>>>
>>> Is there a point -device/device_add devices on board that doesn't have 'hotplug'
>>> handler that would wire device up properly?
>>
>> Sorry, I did not get that question ... do you mean whether there's any
>> code that uses qdev_device_add() to add a device without hotplug
>> controller? I don't think so. It's currently only used by
>> qmp_device_add() for the QMP/HMP command, in vl.c for -device and in the
>> USB code for xen-usb host device. So this function currently really only
>> makes sense for devices that have a hotplug controller.
> 
> I assume you are talking only about hotpluggable devices.  With
> -device, qdev_device_add() is also used for devices that don't
> have a hotplug controller, but this is supposed to be true only
> for non-hotpluggable devices.

Right, the cold-pluggable devices should be fine because of the
"qdev_hotplug" check, forgot to mention that, sorry.

 Thomas