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

[Peter Maydell]

On 12 July 2017 at 12:22, Markus Armbruster <address@hidden> wrote:
> Peter Maydell <address@hidden> 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?

Yes, instance_init is the obvious place (though in fact pretty
much anything that runs before the user of the device sets
properties will do).

>>      union {
>>          int64_t i;
>>          uint64_t u;
>> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
>> index 36d040c..66816a5 100644
>> --- a/include/hw/qdev-properties.h
>> +++ b/include/hw/qdev-properties.h
>> @@ -43,15 +43,24 @@ extern PropertyInfo qdev_prop_arraylen;
>>          .info      = &(_prop),                                          \
>>          .offset    = offsetof(_state, _field)                           \
>>              + type_check(_type,typeof_field(_state, _field)),           \
>> +        .set_default = true,                                            \
>>          .defval.i  = (_type)_defval,                                    \
>>          }
>
> If you flip the sense of the flag, you don't need to mess with the
> PropertyInfo, and don't need the note referring reviewers to the
> previous patch.  However, the case "use .defval" already requires an
> initializer, so adding one for .set_default seems fair.  Okay.

Having the flag this way round was your idea :-)
Putting it the other way round might have been a smaller change
in some sense (you'd have had to retain the check on
set_default_value being NULL), but I do think it's nicer
in the end...

> Preferably with a suitable comment on member set_default and an improved
> commit message:
> Reviewed-by: Markus Armbruster <address@hidden>

thanks
-- PMM