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'}]
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,
};
---
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
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'}] > > 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, > }; 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. > > > --- > 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 > > Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
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 ...
© 2016 - 2024 Red Hat, Inc.