Re: [Qemu-devel] [PATCH] hw/core/qdev: Do not allow hot-plugging without hotplug controller

[Thomas Huth <thuth@redhat.com>]

On 11.09.2017 14:53, Igor Mammedov wrote:
> On Thu,  7 Sep 2017 11:22:42 +0200
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> qdev_unplug() bails out with an assertion if the user tries to device_del
>> a hot-plugged device that does not have a hotplug controller. Unfortunately,
>> our devices are all marked with hotpluggable = true by default (see the
>> device_class_init() function in qdev.c), so it currently can happen that
>> the user runs into this situation and QEMU gets terminated unexpectedly:
>>
>> $ qemu-system-aarch64 -M virt -nographic -nodefaults -monitor stdio -S
>> QEMU 2.10.50 monitor - type 'help' for more information
>> (qemu) device_add aux-to-i2c-bridge,id=x
>> (qemu) device_del x
>> **
>> ERROR:qdev-monitor.c:872:qdev_unplug: assertion failed: (hotplug_ctrl)
>> Aborted (core dumped)
>> Hotplugging devices without a hotplug controller does not make much sense,
>> so we should disallow this during the device_add process already!
>>
>> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  hw/core/qdev.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>> index 606ab53..d9ccce6 100644
>> --- a/hw/core/qdev.c
>> +++ b/hw/core/qdev.c
>> @@ -908,6 +908,11 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
>>              if (local_err != NULL) {
>>                  goto fail;
>>              }
>> +        } else if (dev->hotplugged) {
>> +            /* Hot-plugged device without hotplug controller? No way! */
>> +            error_setg(&local_err, QERR_DEVICE_NO_HOTPLUG,
>> +                       object_get_typename(obj));
>> +            goto fail;
>>          }
>>  
>>          if (dc->realize) {
> 
> maybe it should be other way around, i.e, fix device so that following would work
> 
>   device_set_realized()
>     if (dev->hotplugged && !dc->hotpluggable) {                                  
>         error_setg(errp, QERR_DEVICE_NO_HOTPLUG, object_get_typename(obj));      
>         return;                                                                  
>     }
> 
> instead of leaving device broken, like in yours
>  84ebd3e watchdog/wdt_diag288: Mark diag288 watchdog as non-hotpluggable

No, that apparently does not work right for new devices since people
keep forgetting to set hotpluggable = false there. Both, Paolo and Peter
suggested that we should not allow hot-plugging if there's no hot plug
controller - it indeed does not make sense, so we should not allow it.

 Thomas