| Message ID | 1499788408-10096-3-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | New |
| Headers | show |
Hi ----- Original Message ----- > 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 > 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. > > Suggested-by: Markus Armbruster <armbru@redhat.com> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> minor comment below > --- > Since the previous patch fixed the only case that was > using a default value but not explicitly setting .defval.[ui], > this patch is a bit easier to review: .set_default = true > is added in exactly the places that initialize .defval.[ui]. > --- > include/hw/qdev-core.h | 1 + > include/hw/qdev-properties.h | 20 ++++++++++++++++++++ > hw/core/qdev.c | 2 +- > 3 files changed, 22 insertions(+), 1 deletion(-) > > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h > index 9d7c1c0..d110163 100644 > --- 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; > 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, \ > } > > +#define DEFINE_PROP_SIGNED_NODEFAULT(_name, _state, _field, _prop, _type) { > \ > + .name = (_name), \ > + .info = &(_prop), \ > + .offset = offsetof(_state, _field) \ > + + type_check(_type, typeof_field(_state, _field)), \ > + } > + > #define DEFINE_PROP_BIT(_name, _state, _field, _bit, _defval) { \ > .name = (_name), \ > .info = &(qdev_prop_bit), \ > .bitnr = (_bit), \ > .offset = offsetof(_state, _field) \ > + type_check(uint32_t,typeof_field(_state, _field)), \ > + .set_default = true, \ > .defval.u = (bool)_defval, \ > } > > @@ -60,15 +69,24 @@ extern PropertyInfo qdev_prop_arraylen; > .info = &(_prop), \ > .offset = offsetof(_state, _field) \ > + type_check(_type, typeof_field(_state, _field)), \ > + .set_default = true, \ > .defval.u = (_type)_defval, \ > } > > +#define DEFINE_PROP_UNSIGNED_NODEFAULT(_name, _state, _field, _prop, _type) > { \ > + .name = (_name), \ > + .info = &(_prop), \ > + .offset = offsetof(_state, _field) \ > + + type_check(_type, typeof_field(_state, _field)), \ > + } > + > #define DEFINE_PROP_BIT64(_name, _state, _field, _bit, _defval) { \ > .name = (_name), \ > .info = &(qdev_prop_bit64), \ > .bitnr = (_bit), \ > .offset = offsetof(_state, _field) \ > + type_check(uint64_t, typeof_field(_state, _field)), \ > + .set_default = true, \ > .defval.u = (bool)_defval, \ > } > > @@ -77,6 +95,7 @@ extern PropertyInfo qdev_prop_arraylen; > .info = &(qdev_prop_bool), \ > .offset = offsetof(_state, _field) \ > + type_check(bool, typeof_field(_state, _field)), \ > + .set_default = true, \ > .defval.u = (bool)_defval, \ > } > > @@ -110,6 +129,7 @@ extern PropertyInfo qdev_prop_arraylen; > _arrayfield, _arrayprop, _arraytype) { \ > .name = (PROP_ARRAY_LEN_PREFIX _name), \ > .info = &(qdev_prop_arraylen), \ > + .set_default = true, \ > .defval.u = 0, \ > .offset = offsetof(_state, _field) \ > + type_check(uint32_t, typeof_field(_state, _field)), \ > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > index 849952a..96965a7 100644 > --- a/hw/core/qdev.c > +++ b/hw/core/qdev.c > @@ -793,7 +793,7 @@ void qdev_property_add_static(DeviceState *dev, Property > *prop, > prop->info->description, > &error_abort); > > - if (prop->info->set_default_value) { > + if (prop->set_default) { Not sure if it's worth to have an assert(prop->info->set_default_value), probably not necessary. > prop->info->set_default_value(obj, prop); > } > } > -- > 2.7.4 > >
On 11 July 2017 at 18:15, Marc-André Lureau <marcandre.lureau@redhat.com> wrote: >> @@ -793,7 +793,7 @@ void qdev_property_add_static(DeviceState *dev, Property >> *prop, >> prop->info->description, >> &error_abort); >> >> - if (prop->info->set_default_value) { >> + if (prop->set_default) { > > Not sure if it's worth to have an assert(prop->info->set_default_value), probably not necessary. > >> prop->info->set_default_value(obj, prop); Yes, we'll just segfault on the NULL pointer immediately anyway. I tend to the view that assertions are for turning obscure and delayed failures into clearer and more immediate failures, and in this case the failure is already pretty clear and immediate. thanks -- PMM
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? > Suggested-by: Markus Armbruster <armbru@redhat.com> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > Since the previous patch fixed the only case that was > using a default value but not explicitly setting .defval.[ui], > this patch is a bit easier to review: .set_default = true > is added in exactly the places that initialize .defval.[ui]. > --- > include/hw/qdev-core.h | 1 + > include/hw/qdev-properties.h | 20 ++++++++++++++++++++ > hw/core/qdev.c | 2 +- > 3 files changed, 22 insertions(+), 1 deletion(-) > > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h > index 9d7c1c0..d110163 100644 > --- 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. > 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. > > +#define DEFINE_PROP_SIGNED_NODEFAULT(_name, _state, _field, _prop, _type) { \ > + .name = (_name), \ > + .info = &(_prop), \ > + .offset = offsetof(_state, _field) \ > + + type_check(_type, typeof_field(_state, _field)), \ > + } > + > #define DEFINE_PROP_BIT(_name, _state, _field, _bit, _defval) { \ > .name = (_name), \ > .info = &(qdev_prop_bit), \ > .bitnr = (_bit), \ > .offset = offsetof(_state, _field) \ > + type_check(uint32_t,typeof_field(_state, _field)), \ > + .set_default = true, \ > .defval.u = (bool)_defval, \ > } > > @@ -60,15 +69,24 @@ extern PropertyInfo qdev_prop_arraylen; > .info = &(_prop), \ > .offset = offsetof(_state, _field) \ > + type_check(_type, typeof_field(_state, _field)), \ > + .set_default = true, \ > .defval.u = (_type)_defval, \ > } > > +#define DEFINE_PROP_UNSIGNED_NODEFAULT(_name, _state, _field, _prop, _type) { \ > + .name = (_name), \ > + .info = &(_prop), \ > + .offset = offsetof(_state, _field) \ > + + type_check(_type, typeof_field(_state, _field)), \ > + } > + > #define DEFINE_PROP_BIT64(_name, _state, _field, _bit, _defval) { \ > .name = (_name), \ > .info = &(qdev_prop_bit64), \ > .bitnr = (_bit), \ > .offset = offsetof(_state, _field) \ > + type_check(uint64_t, typeof_field(_state, _field)), \ > + .set_default = true, \ > .defval.u = (bool)_defval, \ > } > > @@ -77,6 +95,7 @@ extern PropertyInfo qdev_prop_arraylen; > .info = &(qdev_prop_bool), \ > .offset = offsetof(_state, _field) \ > + type_check(bool, typeof_field(_state, _field)), \ > + .set_default = true, \ > .defval.u = (bool)_defval, \ > } > > @@ -110,6 +129,7 @@ extern PropertyInfo qdev_prop_arraylen; > _arrayfield, _arrayprop, _arraytype) { \ > .name = (PROP_ARRAY_LEN_PREFIX _name), \ > .info = &(qdev_prop_arraylen), \ > + .set_default = true, \ > .defval.u = 0, \ > .offset = offsetof(_state, _field) \ > + type_check(uint32_t, typeof_field(_state, _field)), \ > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > index 849952a..96965a7 100644 > --- a/hw/core/qdev.c > +++ b/hw/core/qdev.c > @@ -793,7 +793,7 @@ void qdev_property_add_static(DeviceState *dev, Property *prop, > prop->info->description, > &error_abort); > > - if (prop->info->set_default_value) { > + if (prop->set_default) { > prop->info->set_default_value(obj, prop); > } > } Preferably with a suitable comment on member set_default and an improved commit message: Reviewed-by: Markus Armbruster <armbru@redhat.com>
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? 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 <armbru@redhat.com> thanks -- PMM
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 * 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. */ ? thanks -- PMM
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
On 13 July 2017 at 18:10, Markus Armbruster <armbru@redhat.com> wrote: > 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 ..." Yes. thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On 13 July 2017 at 18:10, Markus Armbruster <armbru@redhat.com> wrote: >> 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 ..." > > Yes. Update your comment and commit message accordingly, and you may add Reviewed-by: Markus Armbruster <armbru@redhat.com>
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h index 9d7c1c0..d110163 100644 --- 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; 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, \ } +#define DEFINE_PROP_SIGNED_NODEFAULT(_name, _state, _field, _prop, _type) { \ + .name = (_name), \ + .info = &(_prop), \ + .offset = offsetof(_state, _field) \ + + type_check(_type, typeof_field(_state, _field)), \ + } + #define DEFINE_PROP_BIT(_name, _state, _field, _bit, _defval) { \ .name = (_name), \ .info = &(qdev_prop_bit), \ .bitnr = (_bit), \ .offset = offsetof(_state, _field) \ + type_check(uint32_t,typeof_field(_state, _field)), \ + .set_default = true, \ .defval.u = (bool)_defval, \ } @@ -60,15 +69,24 @@ extern PropertyInfo qdev_prop_arraylen; .info = &(_prop), \ .offset = offsetof(_state, _field) \ + type_check(_type, typeof_field(_state, _field)), \ + .set_default = true, \ .defval.u = (_type)_defval, \ } +#define DEFINE_PROP_UNSIGNED_NODEFAULT(_name, _state, _field, _prop, _type) { \ + .name = (_name), \ + .info = &(_prop), \ + .offset = offsetof(_state, _field) \ + + type_check(_type, typeof_field(_state, _field)), \ + } + #define DEFINE_PROP_BIT64(_name, _state, _field, _bit, _defval) { \ .name = (_name), \ .info = &(qdev_prop_bit64), \ .bitnr = (_bit), \ .offset = offsetof(_state, _field) \ + type_check(uint64_t, typeof_field(_state, _field)), \ + .set_default = true, \ .defval.u = (bool)_defval, \ } @@ -77,6 +95,7 @@ extern PropertyInfo qdev_prop_arraylen; .info = &(qdev_prop_bool), \ .offset = offsetof(_state, _field) \ + type_check(bool, typeof_field(_state, _field)), \ + .set_default = true, \ .defval.u = (bool)_defval, \ } @@ -110,6 +129,7 @@ extern PropertyInfo qdev_prop_arraylen; _arrayfield, _arrayprop, _arraytype) { \ .name = (PROP_ARRAY_LEN_PREFIX _name), \ .info = &(qdev_prop_arraylen), \ + .set_default = true, \ .defval.u = 0, \ .offset = offsetof(_state, _field) \ + type_check(uint32_t, typeof_field(_state, _field)), \ diff --git a/hw/core/qdev.c b/hw/core/qdev.c index 849952a..96965a7 100644 --- a/hw/core/qdev.c +++ b/hw/core/qdev.c @@ -793,7 +793,7 @@ void qdev_property_add_static(DeviceState *dev, Property *prop, prop->info->description, &error_abort); - if (prop->info->set_default_value) { + if (prop->set_default) { prop->info->set_default_value(obj, prop); } }
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 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. Suggested-by: Markus Armbruster <armbru@redhat.com> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- Since the previous patch fixed the only case that was using a default value but not explicitly setting .defval.[ui], this patch is a bit easier to review: .set_default = true is added in exactly the places that initialize .defval.[ui]. --- include/hw/qdev-core.h | 1 + include/hw/qdev-properties.h | 20 ++++++++++++++++++++ hw/core/qdev.c | 2 +- 3 files changed, 22 insertions(+), 1 deletion(-)