qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v3 2/2] qdev: Check for the availability of a ho


From: Thomas Huth
Subject: Re: [Qemu-devel] [PATCH v3 2/2] qdev: Check for the availability of a hotplug controller before adding a device
Date: Mon, 13 Nov 2017 15:12:31 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0

On 13.11.2017 15:00, Markus Armbruster wrote:
> Thomas Huth <address@hidden> writes:
> 
>> 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), and can currently for example also be
>> triggered like this:
>>
>> $ s390x-softmmu/qemu-system-s390x -M none -nographic 
>> QEMU 2.10.50 monitor - type 'help' for more information
>> (qemu) device_add qemu-s390x-cpu,id=x
>> (qemu) device_del x
>> **
>> ERROR:qemu/qdev-monitor.c:872:qdev_unplug: assertion failed: (hotplug_ctrl)
>> Aborted (core dumped)
>>
>> So devices clearly need a hotplug controller when they should be usable
>> with device_add.
>> The code in qdev_device_add() already checks whether the bus has a proper
>> hotplug controller,
> 
> Where?  Hmm, I guess it's this one:
> 
>     if (qdev_hotplug && bus && !qbus_is_hotpluggable(bus)) {
>         error_setg(errp, QERR_BUS_NO_HOTPLUG, bus->name);
>         return NULL;
>     }

Right.

>>                     but for devices that do not have a corresponding bus,
>> there is no appropriate check available yet. 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.
[...]
>> diff --git a/qdev-monitor.c b/qdev-monitor.c
>> index 9188d20..38c0fc2 100644
>> --- a/qdev-monitor.c
>> +++ b/qdev-monitor.c
>> @@ -614,6 +614,11 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error 
>> **errp)
>>  
>        if (qdev_hotplug && bus && !qbus_is_hotpluggable(bus)) {
>            error_setg(errp, QERR_BUS_NO_HOTPLUG, bus->name);
>            return NULL;
>        }
> 
>        if (!migration_is_idle()) {
>            error_setg(errp, "device_add not allowed while migrating");
>            return NULL;
>        }
> 
>        /* create device */
>        dev = DEVICE(object_new(driver));
> 
>>      if (bus) {
>>          qdev_set_parent_bus(dev, bus);
>> +    } else if (qdev_hotplug && !qdev_get_machine_hotplug_handler(dev)) {
>> +        /* No bus, no machine hotplug handler --> device is not 
>> hotpluggable */
> 
> Long line.

Less than 80 columns, so that's fine.

>> +        error_setg(&err, "Device '%s' can not be hotplugged on this 
>> machine",
>> +                   driver);
>> +        goto err_del_dev;
>>      }
>>  
>>      qdev_set_id(dev, qemu_opts_id(opts));
> 
> Hmm.  We need to check "can hotplug" in two separate ways, with bus and
> without bus.  Can we keep the two ways on one place?  Something like
> 
>        if (qdev_hotplug) {
>            if (bus && !qbus_is_hotpluggable(bus)) {
>                error_setg(errp, QERR_BUS_NO_HOTPLUG, bus->name);
>                return NULL;
>            }
>            if (!bus && !qdev_get_machine_hotplug_handler(dev)) {
>                error_setg(&err,
>                           "Machine doesn't support hot-plugging device '%s'"
>                           driver);
>                return NULL;
>            }
>       }

We likely could ... but I don't like that change since in that case
you've got to always do "dev = DEVICE(object_new(driver))" (and the code
needs to be at the end of the function, so it would need "goto
err_del_dev" instead of "return NULL"). If we keep the checks separate,
the first check can bail out without creating the device first, so I'd
really prefer to keep my patch that way.

 Thomas



reply via email to

[Prev in Thread] Current Thread [Next in Thread]