Re: [Qemu-devel] [PATCH v2 01/21] qdev: Replace cannot_instantiate_with_device_add_yet with !user_creatable

[Marcel Apfelbaum <marcel@redhat.com>]

On 04/05/2017 10:42 PM, Eduardo Habkost wrote:
> On Wed, Apr 05, 2017 at 10:01:29PM +0300, Marcel Apfelbaum wrote:
>> On 04/04/2017 11:24 PM, Eduardo Habkost wrote:
>>> cannot_instantiate_with_device_add_yet was introduced by commit
>>> 837d37167dc446af8a91189108b363c04609e296 to replace no_user. It
>>> was supposed to be a temporary measure.
>>>
>>
>>
>> Hi Eduardo,
>>
>>> When it was introduced, we had 54
>>> cannot_instantiate_with_device_add_yet=true lines in the code.
>>> Today (3 years later) this number has not shrinked: we now have
>>> 57 cannot_instantiate_with_device_add_yet=true lines. I think it
>>> is safe to say it is not a temporary measure, and we won't see
>>> the flag go away soon.
>>>
>>> Instead of a long field name that misleads people to believe it
>>> is temporary, replace it a shorter and less misleading field:
>>> user_creatable.
>>
>> I completely agree with you. I never liked "negation" fields
>> and especially the ones ending with "_yet".
>>
>> I also don't understand why "user_creatable" can't be used for white-listing *Q35*.
>> I do understand that on some archs we need a white-list per board,
>> but I don't think x86 needs it.
>>
>>>
>>> Except for code comments, changes were generated using the
>>> following Coccinelle patch:
>>>
>>>   @@
>>>   expression DC;
>>>   @@
>>>   (
>>>   -DC->cannot_instantiate_with_device_add_yet = false;
>>>   +DC->user_creatable = true;
>>
>> Is it necessary? Isn't "creatable = true" the default?
>
> cannot_instantiate_with_device_add_yet is also false by default,
> but if there's code explicitly setting it to false somewhere, it
> is probably necessary for some reason. The script doesn't care
> what's the reason, because this is a 100% mechanical translation
> from the old logic to equivalent logic using the new field.
>

I understand.

> If you are still curious about specific cases: explicit
> cannot_instantiate_with_device_add_yet=false/user_creatable=true
> is necessary at x86_cpu_common_class_init(), because TYPE_CPU has
> cannot_instantiate_with_device_add_yet=true (most CPU types don't
> support device_add), but TYPE_X86_CPU sets it back to false
> (because x86 supports CPU hotplug).
>
>>
>>>   |
>>>   -DC->cannot_instantiate_with_device_add_yet = true;
>>>   +DC->user_creatable = false;
>>>   )
>>>
>>>   @@
>>>   typedef ObjectClass;
>>>   expression dc;
>>>   identifier class, data;
>>>   @@
>>>    static void device_class_init(ObjectClass *class, void *data)
>>>    {
>>>    ...
>>>    dc->hotpluggable = true;
>>>   +dc->user_creatable = true;
>>
>> OK... it seems risky :). Is true that "hotpluggable" => creatable,
>> but the prev rule should work, right?
>
> Note that this part is only changing device_class_init(), and no
> other function. dc->hotpluggable=true appears here by pure
> chance: it's just because this is the spot where the new
> dc->user_creatable=true line is being added.
>
> This explicit line addition is required because there's no
> explicit dc->user_creatable=false line in device_class_init
> because QOM objects are always zeroed on creation)
>

Thanks for the answers.

Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>


Thanks,
Marcel

>>
>>>    ...
>>>    }
>>>
>>>   @@
>>>   @@
>>>    struct DeviceClass {
>>>    ...
>>>   -bool cannot_instantiate_with_device_add_yet;
>>>   +bool user_creatable;
>>>    ...
>>>   }
>>>
>>>   @@
>>>   expression DC;
>>>   @@
>>>   (
>>>   -!DC->cannot_instantiate_with_device_add_yet
>>>   +DC->user_creatable
>>>   |
>>>   -DC->cannot_instantiate_with_device_add_yet
>>>   +!DC->user_creatable
>>>   )
>>>
>>
>> Nice script!
>
> Thanks!
>