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