Re: [PATCH v2] qdev: Report an error for machine without HotplugHandler

[Markus Armbruster]

Zhao Liu <zhao1.liu@intel.com> writes:

> Hi Markus,
>
> On Wed, Dec 20, 2023 at 08:53:21AM +0100, Markus Armbruster wrote:
>> Date: Wed, 20 Dec 2023 08:53:21 +0100
>> From: Markus Armbruster <armbru@redhat.com>
>> Subject: Re: [PATCH v2] qdev: Report an error for machine without
>>  HotplugHandler
>> 
>> Having hotpluggable = true when the device cannot be hot-plugged is
>> *wrong*.  You might be able to paper over the wrongness so the code
>> works anyway, but nothing good can come out of lying to developers
>> trying to understand how the code works.
>> 
>> Three ideas to avoid the lying:
>> 
>> 1. default hotpluggable to bus_type != NULL.
>> 
>> 2. assert(dc->bus_type || !dc->hotpluggable) in a suitable spot.
>> 
>> 3. Change the meaning of hotpluggable, and rename it to reflect its new
>> meaning.  Requires a careful reading of its uses.  I wouldn't go there.
>> 
>
> What about 4 (or maybe 3.1) - droping this hotpluggable flag and just use a
> helper (like qbus) to check if device is hotpluggable?
>
> This removes the confusion of that flag and also reduces the number of
> configuration items for DeviceState that require developer attention.
> A simple helper is as follows:
>
> static inline bool qdev_is_hotpluggable(DeviceState *dev)
> {
>     /*
>      * Many Machines don't implement qdev_hotplug_allowed().
>      *
>      * TODO: Once all of them complete missing qdev_hotplug_allowed(),
>      *       use qdev_hotplug_allowed() here.
>      */
>     bool hotpluggable = !!qdev_get_machine_hotplug_handler(dev);
>
>     if (!hotpluggable && dev->parent_bus) {
>         hotpluggable = qbus_is_hotpluggable(dev->parent_bus);
>     }
>
>     return hotpluggable;
> }

Worth exploring, I think.