Re: [Qemu-devel] [RFC PATCH qemu] qdev: Use "string" for QOM string properties

[Markus Armbruster <armbru@redhat.com>]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Tue, Jun 05, 2018 at 07:15:08PM +1000, Alexey Kardashevskiy wrote:
>> For QOM properties QEMU uses "string", for example:
>> 
>> {"execute": "qom-list", "arguments": {"path": "/machine"}}
>> [{'return': [{'type': 'string', 'name': 'append'},  {'type': 'string', 'name': 'kernel'}]
>> 
>> However for the device properties copied from DEVICE_CLASS(class)->props,
>> "str" is used for a type (vnet0 is "-device virtio-net-pci,id=vnet0"):
>> 
>> {"execute": "qom-list", "arguments": {"path": "/machine/peripheral/vnet0"}}
>> ret=[{'return': [{'name': 'tx', 'type': 'str'}]

Sad.

>> This changes string type when adding them to QOM property list so
>> we get one string type in QMP.
>> 
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>> 
>> Not a huge improvement and might actually break something but since
>> I got that far I decided to post this anyway :)
>> 
>> The alternatives are: 1) do nothing 2) do this:
>> 
>>  const PropertyInfo qdev_prop_string = {
>> -    .name  = "str",
>> +    .name  = "string",
>>      .release = release_string,
>>      .get   = get_string,
>>      .set   = set_string,
>>  };

To gauge the effect of this change, we need to track down uses of
.name.  See below.

>
> As further points of reference:
>
>  - object_property_add_str & object_class_property_add_str
>    both use "string".
>    
>  - QAPI schema uses "str"
>
> So the inconsistency is even bigger than just qdev. I'm inclined
> to say "QAPI schema" is the canonical source of truth, and so
> every use of "string" should change to "str".
>
> CC'd Markus for 2nd opinion.

I agree with the notion to give QAPI precedence on interface
introspection questions, simply because its become our base for
introspectable interfaces.

However, this case isn't as simple as "QAPI uses 'str', and that's
that.'

QAPI's 'str' started out as name of a built-in type, not exposed at any
external interface.

With QMP schema introspection, the names of built-in types got exposed,
but only as a reading aid, with an explicit admonition not to rely on
them (docs/devel/qapi-code-gen.txt section Client JSON Protocol
introspection):

    Command and event names are part of the wire ABI, but type names are
    not.  Therefore, the SchemaInfo for types have auto-generated
    meaningless names.
    [...]
    The SchemaInfo for a built-in type has the same name as the type in
    the QAPI schema (see section Built-in Types) [...].  It has variant
    member "json-type" that shows how values of this type are encoded on
    the wire.
    [...]
    As explained above, type names are not part of the wire ABI.  Not even
    the names of built-in types.  Clients should examine member
    "json-type" instead of hard-coding names of built-in types.

This gives us license to change the type name 'str' in QMP schema
introspection.  Clients that rely on 'str' are simply broken.

Doesn't mean we *want* to change it.  We might not want to turn latent
client bugs into actively biting ones.  Or we might like 'str' better.

>> ---
>>  hw/core/qdev.c | 9 +++++++--
>>  1 file changed, 7 insertions(+), 2 deletions(-)
>> 
>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>> index f6f9247..2895693 100644
>> --- a/hw/core/qdev.c
>> +++ b/hw/core/qdev.c
>> @@ -687,7 +687,7 @@ static void qdev_property_add_legacy(DeviceState *dev, Property *prop,
>>      }
>>  
>>      name = g_strdup_printf("legacy-%s", prop->name);
>> -    object_property_add(OBJECT(dev), name, "str",
>> +    object_property_add(OBJECT(dev), name, "string",
>>                          prop->info->print ? qdev_get_legacy_property : prop->info->get,
>>                          NULL,
>>                          NULL,
>> @@ -711,6 +711,7 @@ void qdev_property_add_static(DeviceState *dev, Property *prop,
>>  {
>>      Error *local_err = NULL;
>>      Object *obj = OBJECT(dev);
>> +    const char *proptype;
>>  
>>      if (prop->info->create) {
>>          prop->info->create(obj, prop, &local_err);
>> @@ -723,7 +724,11 @@ void qdev_property_add_static(DeviceState *dev, Property *prop,
>>          if (!prop->info->get && !prop->info->set) {
>>              return;
>>          }
>> -        object_property_add(obj, prop->name, prop->info->name,
>> +        proptype = prop->info->name;
>> +        if (0 == strncmp(proptype, "str", 3)) {
>> +            proptype = "string";
>> +        }
>> +        object_property_add(obj, prop->name, proptype,
>>                              prop->info->get, prop->info->set,
>>                              prop->info->release,
>>                              prop, &local_err);
>> -- 
>> 2.11.0

Can you convince us that you got all the sources of "str" in qom-list?

Taking a step back: the type names are a merry "pass the first string
that crosses your mind" game for developers to play.  Are there more
inconsistencies?  How can we fix them?  How can we avoid having them
creep back?

I promised to track down uses of PropertyInfo member @name for you:

(1) make_device_property_info() uses it to initialize an
    ObjectPropertyInfo's type name.  Visible as result of qom-list,
    device-list-properties, qom-list-properties.  Its specification in
    misc.json reads:

# @type: the type of the property.  This will typically come in one of four
#        forms:
#
#        1) A primitive type such as 'u8', 'u16', 'bool', 'str', or 'double'.
#           These types are mapped to the appropriate JSON type.
#
#        2) A child type in the form 'child<subtype>' where subtype is a qdev
#           device type name.  Child properties create the composition tree.
#
#        3) A link type in the form 'link<subtype>' where subtype is a qdev
#           device type name.  Link properties form the device model graph.

  This makes (ill-specified!) primitive type names ABI, unlike QMP
  schema introspection.

  Note the documentation mentions 'str', but not 'string'.

(2) set_prop_arraylen(), qdev_property_add_static() pass it to
    object_property_add() as type name, which puts it into
    ObjectProperty member @type.  I lack the time to figure out how
    that's used right now.

Due to (1), changing qdev_prop_string.name would be an ABI break.  It's
a problematic corner of the ABI (to put it politely), but it's what we
have.  Before we discuss whether we can break it anyway, we'd have to
figure out whether and how (2) impacts ABI.

Actually, any fix making qom-list use either 'str' or 'string'
consistently is an ABI break of sorts.  We could declare it a bug fix,
though.

What a steaming pile of ...