Zhao Liu <zhao1....@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 <arm...@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.
