| Message ID | 20230327131543.2857052-1-alex.bennee@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [Socratic,RFC] include: attempt to document device_class_set_props | expand |
On 27/03/2023 14:15, Alex Bennée wrote: > I'm still not sure how I achieve by use case of the parent class > defining the following properties: > > static Property vud_properties[] = { > DEFINE_PROP_CHR("chardev", VHostUserDevice, chardev), > DEFINE_PROP_UINT16("id", VHostUserDevice, id, 0), > DEFINE_PROP_UINT32("num_vqs", VHostUserDevice, num_vqs, 1), > DEFINE_PROP_END_OF_LIST(), > }; > > But for the specialisation of the class I want the id to default to > the actual device id, e.g.: > > static Property vu_rng_properties[] = { > DEFINE_PROP_UINT16("id", VHostUserDevice, id, VIRTIO_ID_RNG), > DEFINE_PROP_UINT32("num_vqs", VHostUserDevice, num_vqs, 1), > DEFINE_PROP_END_OF_LIST(), > }; > > And so far the API for doing that isn't super clear. > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > --- > include/hw/qdev-core.h | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h > index bd50ad5ee1..d4bbc30c92 100644 > --- a/include/hw/qdev-core.h > +++ b/include/hw/qdev-core.h > @@ -776,6 +776,15 @@ BusState *sysbus_get_default(void); > char *qdev_get_fw_dev_path(DeviceState *dev); > char *qdev_get_own_fw_dev_path_from_handler(BusState *bus, DeviceState *dev); > > +/** > + * device_class_set_props(): add a set of properties to an device > + * @dc: the parent DeviceClass all devices inherit > + * @props: an array of properties, terminate by DEFINE_PROP_END_OF_LIST() > + * > + * This will add a set of properties to the object. It will fault if > + * you attempt to add an existing property defined by a parent class. > + * To modify an inherited property you need to use???? > + */ > void device_class_set_props(DeviceClass *dc, Property *props); > > /** Hmmm that's an interesting one. Looking at the source in hw/core/qdev-properties.c you could possibly get away with something like this in vu_rng_class_init(): ObjectProperty *op = object_class_property_find(klass, "id"); object_property_set_default_uint(op, VIRTIO_ID_RNG); Of course this is all completely untested :) ATB, Mark.
Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> writes: > On 27/03/2023 14:15, Alex Bennée wrote: > >> I'm still not sure how I achieve by use case of the parent class >> defining the following properties: >> static Property vud_properties[] = { >> DEFINE_PROP_CHR("chardev", VHostUserDevice, chardev), >> DEFINE_PROP_UINT16("id", VHostUserDevice, id, 0), >> DEFINE_PROP_UINT32("num_vqs", VHostUserDevice, num_vqs, 1), >> DEFINE_PROP_END_OF_LIST(), >> }; >> But for the specialisation of the class I want the id to default to >> the actual device id, e.g.: >> static Property vu_rng_properties[] = { >> DEFINE_PROP_UINT16("id", VHostUserDevice, id, VIRTIO_ID_RNG), >> DEFINE_PROP_UINT32("num_vqs", VHostUserDevice, num_vqs, 1), >> DEFINE_PROP_END_OF_LIST(), >> }; >> And so far the API for doing that isn't super clear. >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >> --- >> include/hw/qdev-core.h | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h >> index bd50ad5ee1..d4bbc30c92 100644 >> --- a/include/hw/qdev-core.h >> +++ b/include/hw/qdev-core.h >> @@ -776,6 +776,15 @@ BusState *sysbus_get_default(void); >> char *qdev_get_fw_dev_path(DeviceState *dev); >> char *qdev_get_own_fw_dev_path_from_handler(BusState *bus, DeviceState *dev); >> +/** >> + * device_class_set_props(): add a set of properties to an device >> + * @dc: the parent DeviceClass all devices inherit >> + * @props: an array of properties, terminate by DEFINE_PROP_END_OF_LIST() >> + * >> + * This will add a set of properties to the object. It will fault if >> + * you attempt to add an existing property defined by a parent class. >> + * To modify an inherited property you need to use???? >> + */ >> void device_class_set_props(DeviceClass *dc, Property *props); >> /** > > Hmmm that's an interesting one. Looking at the source in > hw/core/qdev-properties.c you could possibly get away with something > like this in vu_rng_class_init(): > > ObjectProperty *op = object_class_property_find(klass, "id"); > object_property_set_default_uint(op, VIRTIO_ID_RNG); > > Of course this is all completely untested :) Sadly we assert on the existing prop->defval: static void object_property_set_default(ObjectProperty *prop, QObject *defval) { assert(!prop->defval); assert(!prop->init); prop->defval = defval; prop->init = object_property_init_defval; } Maybe the assert is too aggressive or we need a different helper, maybe a: void object_property_update_default_uint(ObjectProperty *prop, uint64_t value) ?
On 27/03/2023 17:12, Alex Bennée wrote: > Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> writes: > >> On 27/03/2023 14:15, Alex Bennée wrote: >> >>> I'm still not sure how I achieve by use case of the parent class >>> defining the following properties: >>> static Property vud_properties[] = { >>> DEFINE_PROP_CHR("chardev", VHostUserDevice, chardev), >>> DEFINE_PROP_UINT16("id", VHostUserDevice, id, 0), >>> DEFINE_PROP_UINT32("num_vqs", VHostUserDevice, num_vqs, 1), >>> DEFINE_PROP_END_OF_LIST(), >>> }; >>> But for the specialisation of the class I want the id to default to >>> the actual device id, e.g.: >>> static Property vu_rng_properties[] = { >>> DEFINE_PROP_UINT16("id", VHostUserDevice, id, VIRTIO_ID_RNG), >>> DEFINE_PROP_UINT32("num_vqs", VHostUserDevice, num_vqs, 1), >>> DEFINE_PROP_END_OF_LIST(), >>> }; >>> And so far the API for doing that isn't super clear. >>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >>> --- >>> include/hw/qdev-core.h | 9 +++++++++ >>> 1 file changed, 9 insertions(+) >>> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h >>> index bd50ad5ee1..d4bbc30c92 100644 >>> --- a/include/hw/qdev-core.h >>> +++ b/include/hw/qdev-core.h >>> @@ -776,6 +776,15 @@ BusState *sysbus_get_default(void); >>> char *qdev_get_fw_dev_path(DeviceState *dev); >>> char *qdev_get_own_fw_dev_path_from_handler(BusState *bus, DeviceState *dev); >>> +/** >>> + * device_class_set_props(): add a set of properties to an device >>> + * @dc: the parent DeviceClass all devices inherit >>> + * @props: an array of properties, terminate by DEFINE_PROP_END_OF_LIST() >>> + * >>> + * This will add a set of properties to the object. It will fault if >>> + * you attempt to add an existing property defined by a parent class. >>> + * To modify an inherited property you need to use???? >>> + */ >>> void device_class_set_props(DeviceClass *dc, Property *props); >>> /** >> >> Hmmm that's an interesting one. Looking at the source in >> hw/core/qdev-properties.c you could possibly get away with something >> like this in vu_rng_class_init(): >> >> ObjectProperty *op = object_class_property_find(klass, "id"); >> object_property_set_default_uint(op, VIRTIO_ID_RNG); >> >> Of course this is all completely untested :) > > Sadly we assert on the existing prop->defval: > > static void object_property_set_default(ObjectProperty *prop, QObject *defval) > { > assert(!prop->defval); > assert(!prop->init); > > prop->defval = defval; > prop->init = object_property_init_defval; > } > > Maybe the assert is too aggressive or we need a different helper, maybe > a: > > void object_property_update_default_uint(ObjectProperty *prop, uint64_t value) > > ? It seems in that case once the default has been set, it is impossible to change. The only other immediate option I can think of is to define a specific DEFINE_VHOST_PROPERTIES macro in a similar way to DEFINE_AUDIO_PROPERTIES which you can use to set the common properties for all VHostUserDevice devices, including providing the default ID. ATB, Mark.
Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> writes: > On 27/03/2023 17:12, Alex Bennée wrote: > >> Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> writes: >> >>> On 27/03/2023 14:15, Alex Bennée wrote: >>> >>>> I'm still not sure how I achieve by use case of the parent class >>>> defining the following properties: >>>> static Property vud_properties[] = { >>>> DEFINE_PROP_CHR("chardev", VHostUserDevice, chardev), >>>> DEFINE_PROP_UINT16("id", VHostUserDevice, id, 0), >>>> DEFINE_PROP_UINT32("num_vqs", VHostUserDevice, num_vqs, 1), >>>> DEFINE_PROP_END_OF_LIST(), >>>> }; >>>> But for the specialisation of the class I want the id to default to >>>> the actual device id, e.g.: >>>> static Property vu_rng_properties[] = { >>>> DEFINE_PROP_UINT16("id", VHostUserDevice, id, VIRTIO_ID_RNG), >>>> DEFINE_PROP_UINT32("num_vqs", VHostUserDevice, num_vqs, 1), >>>> DEFINE_PROP_END_OF_LIST(), >>>> }; >>>> And so far the API for doing that isn't super clear. >>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >>>> --- >>>> include/hw/qdev-core.h | 9 +++++++++ >>>> 1 file changed, 9 insertions(+) >>>> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h >>>> index bd50ad5ee1..d4bbc30c92 100644 >>>> --- a/include/hw/qdev-core.h >>>> +++ b/include/hw/qdev-core.h >>>> @@ -776,6 +776,15 @@ BusState *sysbus_get_default(void); >>>> char *qdev_get_fw_dev_path(DeviceState *dev); >>>> char *qdev_get_own_fw_dev_path_from_handler(BusState *bus, DeviceState *dev); >>>> +/** >>>> + * device_class_set_props(): add a set of properties to an device >>>> + * @dc: the parent DeviceClass all devices inherit >>>> + * @props: an array of properties, terminate by DEFINE_PROP_END_OF_LIST() >>>> + * >>>> + * This will add a set of properties to the object. It will fault if >>>> + * you attempt to add an existing property defined by a parent class. >>>> + * To modify an inherited property you need to use???? >>>> + */ >>>> void device_class_set_props(DeviceClass *dc, Property *props); >>>> /** >>> >>> Hmmm that's an interesting one. Looking at the source in >>> hw/core/qdev-properties.c you could possibly get away with something >>> like this in vu_rng_class_init(): >>> >>> ObjectProperty *op = object_class_property_find(klass, "id"); >>> object_property_set_default_uint(op, VIRTIO_ID_RNG); >>> >>> Of course this is all completely untested :) >> Sadly we assert on the existing prop->defval: >> static void object_property_set_default(ObjectProperty *prop, >> QObject *defval) >> { >> assert(!prop->defval); >> assert(!prop->init); >> prop->defval = defval; >> prop->init = object_property_init_defval; >> } >> Maybe the assert is too aggressive or we need a different helper, >> maybe >> a: >> void object_property_update_default_uint(ObjectProperty *prop, >> uint64_t value) >> ? > > It seems in that case once the default has been set, it is impossible > to change. The only other immediate option I can think of is to define > a specific DEFINE_VHOST_PROPERTIES macro in a similar way to > DEFINE_AUDIO_PROPERTIES which you can use to set the common properties > for all VHostUserDevice devices, including providing the default ID. I tried this: allow the default to change modified qom/object.c @@ -1557,11 +1557,16 @@ static void object_property_init_defval(Object *obj, ObjectProperty *prop) static void object_property_set_default(ObjectProperty *prop, QObject *defval) { - assert(!prop->defval); - assert(!prop->init); + if (prop->init == object_property_init_defval) { + fprintf(stderr, "%s: updating existing defval\n", __func__); + prop->defval = defval; + } else { + assert(!prop->defval); + assert(!prop->init); - prop->defval = defval; - prop->init = object_property_init_defval; + prop->defval = defval; + prop->init = object_property_init_defval; + } } > > > ATB, > > Mark.
On Mon, 27 Mar 2023 at 23:10, Alex Bennée <alex.bennee@linaro.org> wrote: > > > Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> writes: > > > On 27/03/2023 17:12, Alex Bennée wrote: > > > >> Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> writes: > >> > >>> On 27/03/2023 14:15, Alex Bennée wrote: > >>> > >>>> I'm still not sure how I achieve by use case of the parent class > >>>> defining the following properties: > >>>> static Property vud_properties[] = { > >>>> DEFINE_PROP_CHR("chardev", VHostUserDevice, chardev), > >>>> DEFINE_PROP_UINT16("id", VHostUserDevice, id, 0), > >>>> DEFINE_PROP_UINT32("num_vqs", VHostUserDevice, num_vqs, 1), > >>>> DEFINE_PROP_END_OF_LIST(), > >>>> }; > >>>> But for the specialisation of the class I want the id to default to > >>>> the actual device id, e.g.: > >>>> static Property vu_rng_properties[] = { > >>>> DEFINE_PROP_UINT16("id", VHostUserDevice, id, VIRTIO_ID_RNG), > >>>> DEFINE_PROP_UINT32("num_vqs", VHostUserDevice, num_vqs, 1), > >>>> DEFINE_PROP_END_OF_LIST(), > >>>> }; > >>>> And so far the API for doing that isn't super clear. > >>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > >>>> --- > >>>> include/hw/qdev-core.h | 9 +++++++++ > >>>> 1 file changed, 9 insertions(+) > >>>> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h > >>>> index bd50ad5ee1..d4bbc30c92 100644 > >>>> --- a/include/hw/qdev-core.h > >>>> +++ b/include/hw/qdev-core.h > >>>> @@ -776,6 +776,15 @@ BusState *sysbus_get_default(void); > >>>> char *qdev_get_fw_dev_path(DeviceState *dev); > >>>> char *qdev_get_own_fw_dev_path_from_handler(BusState *bus, DeviceState *dev); > >>>> +/** > >>>> + * device_class_set_props(): add a set of properties to an device > >>>> + * @dc: the parent DeviceClass all devices inherit > >>>> + * @props: an array of properties, terminate by DEFINE_PROP_END_OF_LIST() > >>>> + * > >>>> + * This will add a set of properties to the object. It will fault if > >>>> + * you attempt to add an existing property defined by a parent class. > >>>> + * To modify an inherited property you need to use???? > >>>> + */ > >>>> void device_class_set_props(DeviceClass *dc, Property *props); > >>>> /** > >>> > >>> Hmmm that's an interesting one. Looking at the source in > >>> hw/core/qdev-properties.c you could possibly get away with something > >>> like this in vu_rng_class_init(): > >>> > >>> ObjectProperty *op = object_class_property_find(klass, "id"); > >>> object_property_set_default_uint(op, VIRTIO_ID_RNG); > >>> > >>> Of course this is all completely untested :) > >> Sadly we assert on the existing prop->defval: > >> static void object_property_set_default(ObjectProperty *prop, > >> QObject *defval) > >> { > >> assert(!prop->defval); > >> assert(!prop->init); > >> prop->defval = defval; > >> prop->init = object_property_init_defval; > >> } > >> Maybe the assert is too aggressive or we need a different helper, > >> maybe > >> a: > >> void object_property_update_default_uint(ObjectProperty *prop, > >> uint64_t value) > >> ? > > > > It seems in that case once the default has been set, it is impossible > > to change. The only other immediate option I can think of is to define > > a specific DEFINE_VHOST_PROPERTIES macro in a similar way to > > DEFINE_AUDIO_PROPERTIES which you can use to set the common properties > > for all VHostUserDevice devices, including providing the default ID. > > I tried this: allow the default to change > > modified qom/object.c > @@ -1557,11 +1557,16 @@ static void object_property_init_defval(Object *obj, ObjectProperty *prop) > > static void object_property_set_default(ObjectProperty *prop, QObject *defval) > { > - assert(!prop->defval); > - assert(!prop->init); > + if (prop->init == object_property_init_defval) { > + fprintf(stderr, "%s: updating existing defval\n", __func__); > + prop->defval = defval; > + } else { > + assert(!prop->defval); > + assert(!prop->init); > > - prop->defval = defval; > - prop->init = object_property_init_defval; > + prop->defval = defval; > + prop->init = object_property_init_defval; > + } > } I think this leaves the door open to bugs where you create the property, somebody looks at it, and then you update the default value afterwards... -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On Mon, 27 Mar 2023 at 23:10, Alex Bennée <alex.bennee@linaro.org> wrote: >> >> >> Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> writes: >> >> > On 27/03/2023 17:12, Alex Bennée wrote: >> > >> >> Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> writes: >> >> >> >>> On 27/03/2023 14:15, Alex Bennée wrote: >> >>> >> >>>> I'm still not sure how I achieve by use case of the parent class >> >>>> defining the following properties: >> >>>> static Property vud_properties[] = { >> >>>> DEFINE_PROP_CHR("chardev", VHostUserDevice, chardev), >> >>>> DEFINE_PROP_UINT16("id", VHostUserDevice, id, 0), >> >>>> DEFINE_PROP_UINT32("num_vqs", VHostUserDevice, num_vqs, 1), >> >>>> DEFINE_PROP_END_OF_LIST(), >> >>>> }; >> >>>> But for the specialisation of the class I want the id to default to >> >>>> the actual device id, e.g.: >> >>>> static Property vu_rng_properties[] = { >> >>>> DEFINE_PROP_UINT16("id", VHostUserDevice, id, VIRTIO_ID_RNG), >> >>>> DEFINE_PROP_UINT32("num_vqs", VHostUserDevice, num_vqs, 1), >> >>>> DEFINE_PROP_END_OF_LIST(), >> >>>> }; >> >>>> And so far the API for doing that isn't super clear. >> >>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >> >>>> --- >> >>>> include/hw/qdev-core.h | 9 +++++++++ >> >>>> 1 file changed, 9 insertions(+) >> >>>> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h >> >>>> index bd50ad5ee1..d4bbc30c92 100644 >> >>>> --- a/include/hw/qdev-core.h >> >>>> +++ b/include/hw/qdev-core.h >> >>>> @@ -776,6 +776,15 @@ BusState *sysbus_get_default(void); >> >>>> char *qdev_get_fw_dev_path(DeviceState *dev); >> >>>> char *qdev_get_own_fw_dev_path_from_handler(BusState *bus, DeviceState *dev); >> >>>> +/** >> >>>> + * device_class_set_props(): add a set of properties to an device >> >>>> + * @dc: the parent DeviceClass all devices inherit >> >>>> + * @props: an array of properties, terminate by DEFINE_PROP_END_OF_LIST() >> >>>> + * >> >>>> + * This will add a set of properties to the object. It will fault if >> >>>> + * you attempt to add an existing property defined by a parent class. >> >>>> + * To modify an inherited property you need to use???? >> >>>> + */ >> >>>> void device_class_set_props(DeviceClass *dc, Property *props); >> >>>> /** >> >>> >> >>> Hmmm that's an interesting one. Looking at the source in >> >>> hw/core/qdev-properties.c you could possibly get away with something >> >>> like this in vu_rng_class_init(): >> >>> >> >>> ObjectProperty *op = object_class_property_find(klass, "id"); >> >>> object_property_set_default_uint(op, VIRTIO_ID_RNG); >> >>> >> >>> Of course this is all completely untested :) >> >> Sadly we assert on the existing prop->defval: >> >> static void object_property_set_default(ObjectProperty *prop, >> >> QObject *defval) >> >> { >> >> assert(!prop->defval); >> >> assert(!prop->init); >> >> prop->defval = defval; >> >> prop->init = object_property_init_defval; >> >> } >> >> Maybe the assert is too aggressive or we need a different helper, >> >> maybe >> >> a: >> >> void object_property_update_default_uint(ObjectProperty *prop, >> >> uint64_t value) >> >> ? >> > >> > It seems in that case once the default has been set, it is impossible >> > to change. The only other immediate option I can think of is to define >> > a specific DEFINE_VHOST_PROPERTIES macro in a similar way to >> > DEFINE_AUDIO_PROPERTIES which you can use to set the common properties >> > for all VHostUserDevice devices, including providing the default ID. >> >> I tried this: allow the default to change >> >> modified qom/object.c >> @@ -1557,11 +1557,16 @@ static void object_property_init_defval(Object *obj, ObjectProperty *prop) >> >> static void object_property_set_default(ObjectProperty *prop, QObject *defval) >> { >> - assert(!prop->defval); >> - assert(!prop->init); >> + if (prop->init == object_property_init_defval) { >> + fprintf(stderr, "%s: updating existing defval\n", __func__); >> + prop->defval = defval; >> + } else { >> + assert(!prop->defval); >> + assert(!prop->init); >> >> - prop->defval = defval; >> - prop->init = object_property_init_defval; >> + prop->defval = defval; >> + prop->init = object_property_init_defval; >> + } >> } > > I think this leaves the door open to bugs where you create > the property, somebody looks at it, and then you update > the default value afterwards... Really the pattern I have is: vhost-user-device has the property and is configurable vhost-user-rng-device specialises vhost-user-device and fixes the value I'm not sure how best to represent that. This should all be happening at class_init time.
Alex Bennée <alex.bennee@linaro.org> writes: > Peter Maydell <peter.maydell@linaro.org> writes: > >> On Mon, 27 Mar 2023 at 23:10, Alex Bennée <alex.bennee@linaro.org> wrote: >>> >>> >>> Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> writes: >>> >>> > On 27/03/2023 17:12, Alex Bennée wrote: >>> > >>> >> Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> writes: >>> >> >>> >>> On 27/03/2023 14:15, Alex Bennée wrote: >>> >>> >>> >>>> I'm still not sure how I achieve by use case of the parent class >>> >>>> defining the following properties: >>> >>>> static Property vud_properties[] = { >>> >>>> DEFINE_PROP_CHR("chardev", VHostUserDevice, chardev), >>> >>>> DEFINE_PROP_UINT16("id", VHostUserDevice, id, 0), >>> >>>> DEFINE_PROP_UINT32("num_vqs", VHostUserDevice, num_vqs, 1), >>> >>>> DEFINE_PROP_END_OF_LIST(), >>> >>>> }; >>> >>>> But for the specialisation of the class I want the id to default to >>> >>>> the actual device id, e.g.: >>> >>>> static Property vu_rng_properties[] = { >>> >>>> DEFINE_PROP_UINT16("id", VHostUserDevice, id, VIRTIO_ID_RNG), >>> >>>> DEFINE_PROP_UINT32("num_vqs", VHostUserDevice, num_vqs, 1), >>> >>>> DEFINE_PROP_END_OF_LIST(), >>> >>>> }; >>> >>>> And so far the API for doing that isn't super clear. >>> >>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >>> >>>> --- >>> >>>> include/hw/qdev-core.h | 9 +++++++++ >>> >>>> 1 file changed, 9 insertions(+) >>> >>>> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h >>> >>>> index bd50ad5ee1..d4bbc30c92 100644 >>> >>>> --- a/include/hw/qdev-core.h >>> >>>> +++ b/include/hw/qdev-core.h >>> >>>> @@ -776,6 +776,15 @@ BusState *sysbus_get_default(void); >>> >>>> char *qdev_get_fw_dev_path(DeviceState *dev); >>> >>>> char *qdev_get_own_fw_dev_path_from_handler(BusState *bus, DeviceState *dev); >>> >>>> +/** >>> >>>> + * device_class_set_props(): add a set of properties to an device >>> >>>> + * @dc: the parent DeviceClass all devices inherit >>> >>>> + * @props: an array of properties, terminate by DEFINE_PROP_END_OF_LIST() >>> >>>> + * >>> >>>> + * This will add a set of properties to the object. It will fault if >>> >>>> + * you attempt to add an existing property defined by a parent class. >>> >>>> + * To modify an inherited property you need to use???? >>> >>>> + */ >>> >>>> void device_class_set_props(DeviceClass *dc, Property *props); >>> >>>> /** >>> >>> >>> >>> Hmmm that's an interesting one. Looking at the source in >>> >>> hw/core/qdev-properties.c you could possibly get away with something >>> >>> like this in vu_rng_class_init(): >>> >>> >>> >>> ObjectProperty *op = object_class_property_find(klass, "id"); >>> >>> object_property_set_default_uint(op, VIRTIO_ID_RNG); >>> >>> >>> >>> Of course this is all completely untested :) >>> >> Sadly we assert on the existing prop->defval: >>> >> static void object_property_set_default(ObjectProperty *prop, >>> >> QObject *defval) >>> >> { >>> >> assert(!prop->defval); >>> >> assert(!prop->init); >>> >> prop->defval = defval; >>> >> prop->init = object_property_init_defval; >>> >> } >>> >> Maybe the assert is too aggressive or we need a different helper, >>> >> maybe >>> >> a: >>> >> void object_property_update_default_uint(ObjectProperty *prop, >>> >> uint64_t value) >>> >> ? >>> > >>> > It seems in that case once the default has been set, it is impossible >>> > to change. The only other immediate option I can think of is to define >>> > a specific DEFINE_VHOST_PROPERTIES macro in a similar way to >>> > DEFINE_AUDIO_PROPERTIES which you can use to set the common properties >>> > for all VHostUserDevice devices, including providing the default ID. >>> >>> I tried this: allow the default to change >>> >>> modified qom/object.c >>> @@ -1557,11 +1557,16 @@ static void object_property_init_defval(Object *obj, ObjectProperty *prop) >>> >>> static void object_property_set_default(ObjectProperty *prop, QObject *defval) >>> { >>> - assert(!prop->defval); >>> - assert(!prop->init); >>> + if (prop->init == object_property_init_defval) { >>> + fprintf(stderr, "%s: updating existing defval\n", __func__); >>> + prop->defval = defval; >>> + } else { >>> + assert(!prop->defval); >>> + assert(!prop->init); >>> >>> - prop->defval = defval; >>> - prop->init = object_property_init_defval; >>> + prop->defval = defval; >>> + prop->init = object_property_init_defval; >>> + } >>> } >> >> I think this leaves the door open to bugs where you create >> the property, somebody looks at it, and then you update >> the default value afterwards... > > Really the pattern I have is: > > vhost-user-device has the property and is configurable > vhost-user-rng-device specialises vhost-user-device and fixes the value > > I'm not sure how best to represent that. This should all be happening at > class_init time. Of course enabling my second derived class I ran into: ERROR:../../qom/object.c:1590:object_property_fix_default: assertion failed: (!prop->fixed) Bail out! ERROR:../../qom/object.c:1590:object_property_fix_default: assertion failed: (!prop->fixed) [New Thread 2088694.2088695] Thread 1 received signal SIGABRT, Aborted. __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50 50 ../sysdeps/unix/sysv/linux/raise.c: No such file or directory. (rr) bt #0 __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50 #1 0x00007f50194bd537 in __GI_abort () at abort.c:79 #2 0x00007f501b03fdcc in g_assertion_message (domain=<optimized out>, file=0x557b85d1ef1b "../../qom/object.c", line=<optimized out>, func=<optimized out>, message=<optimized out>) at ../../../glib/gtestutils.c:2937 #3 0x00007f501b09d2fb in g_assertion_message_expr (domain=0x0, file=0x557b85d1ef1b "../../qom/object.c", line=1590, func=0x557b85d1f8b0 <__func__.10> "object_property_fix_default", expr=<optimized out>) at ../../../glib/gtestutils.c:2963 #4 0x0000557b8589260e in object_property_fix_default (prop=0x557b88402f30, defval=0x557b885bda80) at ../../qom/object.c:1590 #5 0x0000557b8589266e in object_property_fix_default_uint (prop=0x557b88402f30, value=41) at ../../qom/object.c:1598 #6 0x0000557b857ccc28 in vu_gpio_class_init (klass=0x557b885bd360, data=0x0) at ../../hw/virtio/vhost-user-gpio.c:32 #7 0x0000557b8588fad5 in type_initialize (ti=0x557b883aec50) at ../../qom/object.c:366 #8 0x0000557b85891038 in object_class_foreach_tramp (key=0x557b883aedd0, value=0x557b883aec50, opaque=0x7ffe065f4290) at ../../qom/object.c:1081 #9 0x00007f501b062fa0 in g_hash_table_foreach (hash_table=0x557b882a7240 = {...}, func=0x557b85891008 <object_class_foreach_tramp>, user_data=0x7ffe065f4290) at ../../../glib/ghash.c:2067 #10 0x0000557b85891117 in object_class_foreach (fn=0x557b85891274 <object_class_get_list_tramp>, implements_type=0x557b85c579f5 "machine", include_abstract=false, opaque=0x7ffe065f42e0) at ../../qom/object.c:1103 #11 0x0000557b858912ef in object_class_get_list (implements_type=0x557b85c579f5 "machine", include_abstract=false) at ../../qom/object.c:1160 #12 0x0000557b85395077 in select_machine (qdict=0x557b883c0e80, errp=0x557b86657e40 <error_fatal>) at ../../softmmu/vl.c:1580 #13 0x0000557b85396068 in qemu_create_machine (qdict=0x557b883c0e80) at ../../softmmu/vl.c:2013 #14 0x0000557b85399c5d in qemu_init (argc=33, argv=0x7ffe065f4628) at ../../softmmu/vl.c:3544 #15 0x0000557b84fdda9f in main (argc=33, argv=0x7ffe065f4628) at ../../softmmu/main.c:47 (rr) f 4 #4 0x0000557b8589260e in object_property_fix_default (prop=0x557b88402f30, defval=0x557b885bda80) at ../../qom/object.c:1590 1590 g_assert(!prop->fixed); (rr) p &prop->fixed $1 = (_Bool *) 0x557b88402f80 (rr) watch *(_Bool *) 0x557b88402f80 Hardware watchpoint 1: *(_Bool *) 0x557b88402f80 (rr) rc Continuing. Thread 1 hit Hardware watchpoint 1: *(_Bool *) 0x557b88402f80 Old value = true New value = false 0x0000557b8589261e in object_property_fix_default (prop=0x557b88402f30, defval=0x557b88403780) at ../../qom/object.c:1593 1593 prop->fixed = true; (rr) bt #0 0x0000557b8589261e in object_property_fix_default (prop=0x557b88402f30, defval=0x557b88403780) at ../../qom/object.c:1593 #1 0x0000557b8589266e in object_property_fix_default_uint (prop=0x557b88402f30, value=4) at ../../qom/object.c:1598 #2 0x0000557b857ccad5 in vu_rng_class_init (klass=0x557b884015c0, data=0x0) at ../../hw/virtio/vhost-user-rng.c:33 #3 0x0000557b8588fad5 in type_initialize (ti=0x557b883aea90) at ../../qom/object.c:366 #4 0x0000557b85891038 in object_class_foreach_tramp (key=0x557b883aec10, value=0x557b883aea90, opaque=0x7ffe065f4290) at ../../qom/object.c:1081 #5 0x00007f501b062fa0 in g_hash_table_foreach (hash_table=0x557b882a7240 = {...}, func=0x557b85891008 <object_class_foreach_tramp>, user_data=0x7ffe065f4290) at ../../../glib/ghash.c:2067 #6 0x0000557b85891117 in object_class_foreach (fn=0x557b85891274 <object_class_get_list_tramp>, implements_type=0x557b85c579f5 "machine", include_abstract=false, opaque=0x7ffe065f42e0) at ../../qom/object.c:1103 #7 0x0000557b858912ef in object_class_get_list (implements_type=0x557b85c579f5 "machine", include_abstract=false) at ../../qom/object.c:1160 #8 0x0000557b85395077 in select_machine (qdict=0x557b883c0e80, errp=0x557b86657e40 <error_fatal>) at ../../softmmu/vl.c:1580 #9 0x0000557b85396068 in qemu_create_machine (qdict=0x557b883c0e80) at ../../softmmu/vl.c:2013 #10 0x0000557b85399c5d in qemu_init (argc=33, argv=0x7ffe065f4628) at ../../softmmu/vl.c:3544 #11 0x0000557b84fdda9f in main (argc=33, argv=0x7ffe065f4628) at ../../softmmu/main.c:47 So I guess duplicating the options as per Mark is going to be the next approach to try although it doesn't quite achieve what I want.
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h index bd50ad5ee1..d4bbc30c92 100644 --- a/include/hw/qdev-core.h +++ b/include/hw/qdev-core.h @@ -776,6 +776,15 @@ BusState *sysbus_get_default(void); char *qdev_get_fw_dev_path(DeviceState *dev); char *qdev_get_own_fw_dev_path_from_handler(BusState *bus, DeviceState *dev); +/** + * device_class_set_props(): add a set of properties to an device + * @dc: the parent DeviceClass all devices inherit + * @props: an array of properties, terminate by DEFINE_PROP_END_OF_LIST() + * + * This will add a set of properties to the object. It will fault if + * you attempt to add an existing property defined by a parent class. + * To modify an inherited property you need to use???? + */ void device_class_set_props(DeviceClass *dc, Property *props); /**
I'm still not sure how I achieve by use case of the parent class defining the following properties: static Property vud_properties[] = { DEFINE_PROP_CHR("chardev", VHostUserDevice, chardev), DEFINE_PROP_UINT16("id", VHostUserDevice, id, 0), DEFINE_PROP_UINT32("num_vqs", VHostUserDevice, num_vqs, 1), DEFINE_PROP_END_OF_LIST(), }; But for the specialisation of the class I want the id to default to the actual device id, e.g.: static Property vu_rng_properties[] = { DEFINE_PROP_UINT16("id", VHostUserDevice, id, VIRTIO_ID_RNG), DEFINE_PROP_UINT32("num_vqs", VHostUserDevice, num_vqs, 1), DEFINE_PROP_END_OF_LIST(), }; And so far the API for doing that isn't super clear. Signed-off-by: Alex Bennée <alex.bennee@linaro.org> --- include/hw/qdev-core.h | 9 +++++++++ 1 file changed, 9 insertions(+)