[Top][All Lists]

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

Re: [Qemu-devel] [PATCH v2 01/21] qdev: Replace cannot_instantiate_with_

From: Marcel Apfelbaum
Subject: Re: [Qemu-devel] [PATCH v2 01/21] qdev: Replace cannot_instantiate_with_device_add_yet with !user_creatable
Date: Thu, 6 Apr 2017 12:31:51 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1

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:

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 
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
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>



   struct DeviceClass {
  -bool cannot_instantiate_with_device_add_yet;
  +bool user_creatable;

  expression DC;

Nice script!


reply via email to

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