On 12 July 2017 at 12:22, Markus Armbruster <arm...@redhat.com> wrote:
> Peter Maydell <peter.mayd...@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?
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 <arm...@redhat.com>
thanks
-- PMM
