Re: [Qemu-arm] [Qemu-devel] [PATCH 2/3] qdev: support properties which don't set a default value

[Markus Armbruster <armbru@redhat.com>]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 12 July 2017 at 12:22, Markus Armbruster <armbru@redhat.com> wrote:
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>>> In some situations it's useful to have a qdev property which doesn't
>>> automatically set its default value when qdev_property_add_static is
>>> called (for instance when the default value is not constant).
>>>
>>> Support this by adding a flag to the Property struct indicating
>>> whether to set the default value.  This replaces the existing test
>>> for whether the PorpertyInfo set_default_value function pointer is
>>
>> PropertyInfo
>>
>>> NULL, and we set the .set_default field to true for all those cases
>>> of struct Property which use a PropertyInfo with a non-NULL
>>> set_default_value, so behaviour remains the same as before.
>>>
>>> We define two new macros DEFINE_PROP_SIGNED_NODEFAULT and
>>> DEFINE_PROP_UNSIGNED_NODEFAULT, to cover the most plausible use cases
>>> of wanting to set an integer property with no default value.
>>
>> Your text makes me wonder what is supposed to set the default value
>> then.  Glancing ahead to PATCH 3, it looks like method instance_init()
>> is.  Can you think of a scenario where something else sets it?
>
> OK, proposed extra paragraph for commit message:
>
> This gives us the semantics of:
>  * if .set_default is true and .info->set_default_value is not NULL,
>    then .defval is used as the the default value of the property

If I read your patch correctly, then this should be "if .set_default is
true, then .info->set_default_value() must not be null, and .defval is
used ..."

>  * otherwise, the property system does not set any default, and
>    the field will retain whatever initial value it was given by
>    the device's .instance_init method
>
> (to go just before the "We define two new macros..." para.)
>
>>> --- a/include/hw/qdev-core.h
>>> +++ b/include/hw/qdev-core.h
>>> @@ -226,6 +226,7 @@ struct Property {
>>>      PropertyInfo *info;
>>>      ptrdiff_t    offset;
>>>      uint8_t      bitnr;
>>> +    bool         set_default;
>>
>> Your chance to add the very first comment to struct Property (its
>> existing undocumentedness is an embarrassment, but not this patch's
>> problem).  Let's write down that this flag governs where (integer)
>> default values come from, either from member devfal or from method
>> instance_init() or whatever.
>
> OK, how about
> /**
>  * Property:
>  * @set_default: true if the default value should be set from @defval
>  *    (if false then no default value is set by the property system
>  *     and the field retains whatever value it was given by instance_init).
>  * @defval: default value for the property. This is used only if @set_default
>  *     is true and @info->set_default_value is not NULL.
>  */

Again, I believe ->set_default_value() must not be null when
->set_default is true.

>
> ?
>
> thanks
> -- PMM