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

[Marcel Apfelbaum]

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 <address@hidden>


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!