[Socratic RFC PATCH] include: attempt to document device_class_set_props

[Socratic RFC PATCH] include: attempt to document device_class_set_props

[Alex Bennée]

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

Re: [Socratic RFC PATCH] include: attempt to document device_class_set_props

[Mark Cave-Ayland]

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.

Re: [Socratic RFC PATCH] include: attempt to document device_class_set_props

[Alex Bennée]

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

Re: [Socratic RFC PATCH] include: attempt to document device_class_set_props

[Mark Cave-Ayland]

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.

Re: [Socratic RFC PATCH] include: attempt to document device_class_set_props

[Alex Bennée]

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

Re: [Socratic RFC PATCH] include: attempt to document device_class_set_props

[Peter Maydell]

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

Re: [Socratic RFC PATCH] include: attempt to document device_class_set_props

[Alex Bennée]

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

Re: [Socratic RFC PATCH] include: attempt to document device_class_set_props

[Alex Bennée]

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