Re: [Qemu-devel] [PATCH v2 01/10] qdev: Replace no_user by cannot_instantiate_with_device_add_yet

[Markus Armbruster]

Marcel Apfelbaum <address@hidden> writes:

> On Tue, 2013-10-29 at 17:08 +0100, address@hidden wrote:
>> From: Markus Armbruster <address@hidden>
>> 
>> In an ideal world, machines can be built by wiring devices together
>> with configuration, not code.  Unfortunately, that's not the world we
>> live in right now.  We still have quite a few devices that need to be
>> wired up by code.  If you try to device_add such a device, it'll fail
>> in sometimes mysterious ways.  If you're lucky, you get an
>> unmysterious immediate crash.
>> 
>> To protect users from such badness, DeviceClass member no_user used to
>> make device models unavailable with -device / device_add, but that
>> regressed in commit 18b6dad.  The device model is still omitted from
>> help, but is available anyway.
>> 
>> Attempts to fix the regression have been rejected with the argument
>> that the purpose of no_user isn't clear, and it's prone to misuse.
>> 
>> This commit clarifies no_user's purpose.  Anthony suggested to rename
>> it cannot_instantiate_with_device_add_yet_due_to_internal_bugs, which
>> I shorten somewhat to keep checkpatch happy.  While there, make it
>> bool.
>> 
>> Every use of cannot_instantiate_with_device_add_yet gets a FIXME
>> comment asking for rationale.  The next few commits will clean them
>> all up, either by providing a rationale, or by getting rid of the use.
>> 
>> With that done, the regression fix is hopefully acceptable.
>> 
>> Signed-off-by: Markus Armbruster <address@hidden>
[...]
>> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
>> index e191ca0..2b571d7 100644
>> --- a/include/hw/qdev-core.h
>> +++ b/include/hw/qdev-core.h
>> @@ -97,7 +97,18 @@ typedef struct DeviceClass {
>>      const char *fw_name;
>>      const char *desc;
>>      Property *props;
>> -    int no_user;
>> +
>> +    /*
>> +     * Shall we hide this device model from -device / device_add?
>> +     * All devices should support instantiation with device_add, and
>> +     * this flag should not exist.  But we're not there, yet.  Some
>> +     * devices fail to instantiate with cryptic error messages.
>> +     * Others instantiate, but don't work.  Exposing users to such
>> +     * behavior would be cruel; this flag serves to protect them.  It
>> +     * should never be set without a comment explaining why it is set.
>> +     * TODO remove once we're there
>> +     */
>> +    bool cannot_instantiate_with_device_add_yet;
>>  
>>      /* callbacks */
>>      void (*reset)(DeviceState *dev);
>> diff --git a/qdev-monitor.c b/qdev-monitor.c
>> index a02c925..36f6f09 100644
>> --- a/qdev-monitor.c
>> +++ b/qdev-monitor.c
>> @@ -87,7 +87,7 @@ static void qdev_print_devinfo(DeviceClass *dc)
>>      if (dc->desc) {
>>          error_printf(", desc \"%s\"", dc->desc);
>>      }
>> -    if (dc->no_user) {
>> +    if (dc->cannot_instantiate_with_device_add_yet) {
>>          error_printf(", no-user");
> Maybe also the message can be changed here?

I'd rather not change output of "info qdm" without good reason.

>>      }
>>      error_printf("\n");
>> @@ -127,7 +127,8 @@ static void qdev_print_devinfos(bool show_no_user)
> Same question about show_no_user parameter, maybe give it a "better"
> name?

My excuse for keeping show_no_user is it controls whether "no-user" is
printed.

Regardless, I'm willing to rename if folks think it's useful.

> Seems OK to me.
>
> Reviewed-by: Marcel Apfelbaum <address@hidden>

Thanks!

[...]