* [Socratic RFC PATCH] include: attempt to document device_class_set_props
@ 2023-03-27 13:15 Alex Bennée
2023-03-27 15:30 ` Mark Cave-Ayland
0 siblings, 1 reply; 8+ messages in thread
From: Alex Bennée @ 2023-03-27 13:15 UTC (permalink / raw)
To: qemu-devel
Cc: Alex Bennée, Paolo Bonzini, Daniel P. Berrangé,
Eduardo Habkost
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);
/**
--
2.39.2
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Socratic RFC PATCH] include: attempt to document device_class_set_props
2023-03-27 13:15 [Socratic RFC PATCH] include: attempt to document device_class_set_props Alex Bennée
@ 2023-03-27 15:30 ` Mark Cave-Ayland
2023-03-27 16:12 ` Alex Bennée
0 siblings, 1 reply; 8+ messages in thread
From: Mark Cave-Ayland @ 2023-03-27 15:30 UTC (permalink / raw)
To: Alex Bennée, qemu-devel
Cc: Paolo Bonzini, Daniel P. Berrangé, Eduardo Habkost
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Socratic RFC PATCH] include: attempt to document device_class_set_props
2023-03-27 15:30 ` Mark Cave-Ayland
@ 2023-03-27 16:12 ` Alex Bennée
2023-03-27 19:49 ` Mark Cave-Ayland
0 siblings, 1 reply; 8+ messages in thread
From: Alex Bennée @ 2023-03-27 16:12 UTC (permalink / raw)
To: Mark Cave-Ayland
Cc: qemu-devel, Paolo Bonzini, Daniel P. Berrangé, Eduardo Habkost
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)
?
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Socratic RFC PATCH] include: attempt to document device_class_set_props
2023-03-27 16:12 ` Alex Bennée
@ 2023-03-27 19:49 ` Mark Cave-Ayland
2023-03-27 22:09 ` Alex Bennée
0 siblings, 1 reply; 8+ messages in thread
From: Mark Cave-Ayland @ 2023-03-27 19:49 UTC (permalink / raw)
To: Alex Bennée
Cc: qemu-devel, Paolo Bonzini, Daniel P. Berrangé, Eduardo Habkost
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Socratic RFC PATCH] include: attempt to document device_class_set_props
2023-03-27 19:49 ` Mark Cave-Ayland
@ 2023-03-27 22:09 ` Alex Bennée
2023-03-28 9:41 ` Peter Maydell
0 siblings, 1 reply; 8+ messages in thread
From: Alex Bennée @ 2023-03-27 22:09 UTC (permalink / raw)
To: Mark Cave-Ayland
Cc: qemu-devel, Paolo Bonzini, Daniel P. Berrangé, Eduardo Habkost
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.
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Socratic RFC PATCH] include: attempt to document device_class_set_props
2023-03-27 22:09 ` Alex Bennée
@ 2023-03-28 9:41 ` Peter Maydell
2023-03-28 11:05 ` Alex Bennée
0 siblings, 1 reply; 8+ messages in thread
From: Peter Maydell @ 2023-03-28 9:41 UTC (permalink / raw)
To: Alex Bennée
Cc: Mark Cave-Ayland, qemu-devel, Paolo Bonzini,
Daniel P. Berrangé,
Eduardo Habkost
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Socratic RFC PATCH] include: attempt to document device_class_set_props
2023-03-28 9:41 ` Peter Maydell
@ 2023-03-28 11:05 ` Alex Bennée
2023-04-14 15:50 ` Alex Bennée
0 siblings, 1 reply; 8+ messages in thread
From: Alex Bennée @ 2023-03-28 11:05 UTC (permalink / raw)
To: Peter Maydell
Cc: Mark Cave-Ayland, qemu-devel, Paolo Bonzini,
Daniel P. Berrangé,
Eduardo Habkost
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
Virtualisation Tech Lead @ Linaro
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Socratic RFC PATCH] include: attempt to document device_class_set_props
2023-03-28 11:05 ` Alex Bennée
@ 2023-04-14 15:50 ` Alex Bennée
0 siblings, 0 replies; 8+ messages in thread
From: Alex Bennée @ 2023-04-14 15:50 UTC (permalink / raw)
To: Alex Bennée
Cc: Peter Maydell, Mark Cave-Ayland, qemu-devel, Paolo Bonzini,
Daniel P. Berrangé,
Eduardo Habkost
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.
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-04-14 15:56 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-27 13:15 [Socratic RFC PATCH] include: attempt to document device_class_set_props Alex Bennée
2023-03-27 15:30 ` Mark Cave-Ayland
2023-03-27 16:12 ` Alex Bennée
2023-03-27 19:49 ` Mark Cave-Ayland
2023-03-27 22:09 ` Alex Bennée
2023-03-28 9:41 ` Peter Maydell
2023-03-28 11:05 ` Alex Bennée
2023-04-14 15:50 ` Alex Bennée
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).