| Message ID | 1453963872-13549-3-git-send-email-vsementsov@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes: > The field is needed to distinguish pc-dimm and nvdimm. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > Signed-off-by: Denis V. Lunev <den@openvz.org> > CC: Stefan Hajnoczi <stefanha@redhat.com> > CC: Xiao Guangrong <guangrong.xiao@linux.intel.com> > CC: "Michael S. Tsirkin" <mst@redhat.com> > CC: Igor Mammedov <imammedo@redhat.com> > CC: Eric Blake <eblake@redhat.com> > CC: Markus Armbruster <armbru@redhat.com> > --- > hw/mem/pc-dimm.c | 1 + > qapi-schema.json | 5 ++++- > 2 files changed, 5 insertions(+), 1 deletion(-) > > diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c > index 4f30950..7469bd4 100644 > --- a/hw/mem/pc-dimm.c > +++ b/hw/mem/pc-dimm.c > @@ -178,6 +178,7 @@ int qmp_pc_dimm_device_list(Object *obj, void *opaque) if (object_dynamic_cast(obj, TYPE_PC_DIMM)) { DeviceState *dev = DEVICE(obj); if (dev->realized) { [...] > di->size = object_property_get_int(OBJECT(dimm), PC_DIMM_SIZE_PROP, > NULL); > di->memdev = object_get_canonical_path(OBJECT(dimm->hostmem)); > + di->type = g_strdup(object_get_typename(obj)); @type is the type name of a subtype of TYPE_PC_DIMM. Current subtypes are TYPE_PC_DIMM itself and TYPE_NVDIMM, i.e. "pc-dimm" and "nvdimm". > > info->u.dimm = di; > elem->value = info; > diff --git a/qapi-schema.json b/qapi-schema.json > index 8d04897..3bcc957 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -3924,6 +3924,8 @@ > # > # @hotpluggable: true if device if could be added/removed while machine is running > # > +# @type: device type: 'pc-dimm' or 'nvdimm' (since 2.6) > +# Not wrong, but brittle: it breaks whenever someone adds another subtype and doesn't remember to update this spot. Remembering it seems unlikely, because it's somewhere else entirely. Let's document it to be the type name of a subtype of pc-dimm, and that you can query the possible values with QMP command { "execute": "qom-list-types", "arguments": { "implements": "pc-dimm", "abstract": false } } > # Since: 2.1 > ## > { 'struct': 'PCDIMMDeviceInfo', > @@ -3934,7 +3936,8 @@ > 'node': 'int', > 'memdev': 'str', > 'hotplugged': 'bool', > - 'hotpluggable': 'bool' > + 'hotpluggable': 'bool', > + 'type': 'str' > } > }
On 01/27/2016 11:51 PM, Vladimir Sementsov-Ogievskiy wrote: > The field is needed to distinguish pc-dimm and nvdimm. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > Signed-off-by: Denis V. Lunev <den@openvz.org> > CC: Stefan Hajnoczi <stefanha@redhat.com> > CC: Xiao Guangrong <guangrong.xiao@linux.intel.com> > CC: "Michael S. Tsirkin" <mst@redhat.com> > CC: Igor Mammedov <imammedo@redhat.com> > CC: Eric Blake <eblake@redhat.com> > CC: Markus Armbruster <armbru@redhat.com> > --- > +++ b/qapi-schema.json > @@ -3924,6 +3924,8 @@ > # > # @hotpluggable: true if device if could be added/removed while machine is running > # > +# @type: device type: 'pc-dimm' or 'nvdimm' (since 2.6) > +# > # Since: 2.1 > ## > { 'struct': 'PCDIMMDeviceInfo', > @@ -3934,7 +3936,8 @@ > 'node': 'int', > 'memdev': 'str', > 'hotplugged': 'bool', > - 'hotpluggable': 'bool' > + 'hotpluggable': 'bool', > + 'type': 'str' No. Since it is a finite set of values (just two possible), you should be using an enum here rather than open-coded 'str'. Something like: { 'enum': 'DIMMType', 'data': [ 'pc-dimm', 'nvdimm' ] }
On 03.02.2016 01:12, Eric Blake wrote: > On 01/27/2016 11:51 PM, Vladimir Sementsov-Ogievskiy wrote: >> The field is needed to distinguish pc-dimm and nvdimm. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> Signed-off-by: Denis V. Lunev <den@openvz.org> >> CC: Stefan Hajnoczi <stefanha@redhat.com> >> CC: Xiao Guangrong <guangrong.xiao@linux.intel.com> >> CC: "Michael S. Tsirkin" <mst@redhat.com> >> CC: Igor Mammedov <imammedo@redhat.com> >> CC: Eric Blake <eblake@redhat.com> >> CC: Markus Armbruster <armbru@redhat.com> >> --- >> +++ b/qapi-schema.json >> @@ -3924,6 +3924,8 @@ >> # >> # @hotpluggable: true if device if could be added/removed while machine is running >> # >> +# @type: device type: 'pc-dimm' or 'nvdimm' (since 2.6) >> +# >> # Since: 2.1 >> ## >> { 'struct': 'PCDIMMDeviceInfo', >> @@ -3934,7 +3936,8 @@ >> 'node': 'int', >> 'memdev': 'str', >> 'hotplugged': 'bool', >> - 'hotpluggable': 'bool' >> + 'hotpluggable': 'bool', >> + 'type': 'str' > No. Since it is a finite set of values (just two possible), you should > be using an enum here rather than open-coded 'str'. Something like: > > { 'enum': 'DIMMType', 'data': [ 'pc-dimm', 'nvdimm' ] } > Are you sure? This is only output Info, so user will never "set" this field. Also, qemu type system (as I understand) is based on string names. object_dynamic_cast and other functions uses "const char *typename". This enum will be out of qemu type system and we will have to sync it.. Is there already some practice of translating string typenames to enum values?
On 02/03/2016 05:00 AM, Vladimir Sementsov-Ogievskiy wrote: >>> +# @type: device type: 'pc-dimm' or 'nvdimm' (since 2.6) >>> +# >>> # Since: 2.1 >>> ## >>> { 'struct': 'PCDIMMDeviceInfo', >>> @@ -3934,7 +3936,8 @@ >>> 'node': 'int', >>> 'memdev': 'str', >>> 'hotplugged': 'bool', >>> - 'hotpluggable': 'bool' >>> + 'hotpluggable': 'bool', >>> + 'type': 'str' >> No. Since it is a finite set of values (just two possible), you should >> be using an enum here rather than open-coded 'str'. Something like: >> >> { 'enum': 'DIMMType', 'data': [ 'pc-dimm', 'nvdimm' ] } >> > > Are you sure? This is only output Info, so user will never "set" this > field. Also, qemu type system (as I understand) is based on string > names. object_dynamic_cast and other functions uses "const char > *typename". This enum will be out of qemu type system and we will have > to sync it.. Is there already some practice of translating string > typenames to enum values? Yes, exposing a finite set of strings as an enum is ideal for the user interface, even if we carry string values instead of enum values in other places in the code. QAPI already includes convenience methods for translating between strings and enum values (EnumName_lookup[] to go from int to string, and qapi_enum_parse() to reverse from string back to enum).
Eric Blake <eblake@redhat.com> writes: > On 02/03/2016 05:00 AM, Vladimir Sementsov-Ogievskiy wrote: > >>>> +# @type: device type: 'pc-dimm' or 'nvdimm' (since 2.6) >>>> +# >>>> # Since: 2.1 >>>> ## >>>> { 'struct': 'PCDIMMDeviceInfo', >>>> @@ -3934,7 +3936,8 @@ >>>> 'node': 'int', >>>> 'memdev': 'str', >>>> 'hotplugged': 'bool', >>>> - 'hotpluggable': 'bool' >>>> + 'hotpluggable': 'bool', >>>> + 'type': 'str' >>> No. Since it is a finite set of values (just two possible), you should >>> be using an enum here rather than open-coded 'str'. Something like: >>> >>> { 'enum': 'DIMMType', 'data': [ 'pc-dimm', 'nvdimm' ] } >>> >> >> Are you sure? This is only output Info, so user will never "set" this >> field. Also, qemu type system (as I understand) is based on string >> names. object_dynamic_cast and other functions uses "const char >> *typename". This enum will be out of qemu type system and we will have >> to sync it.. Is there already some practice of translating string >> typenames to enum values? > > Yes, exposing a finite set of strings as an enum is ideal for the user > interface, even if we carry string values instead of enum values in > other places in the code. QAPI already includes convenience methods for > translating between strings and enum values (EnumName_lookup[] to go > from int to string, and qapi_enum_parse() to reverse from string back to > enum). Well, it's not an arbitrary set of strings, it the set of names of concrete subtypes of "pc-dimm". See also my reply to this patch (Message-ID: <8737tbjfp9.fsf@blackfin.pond.sub.org>), which also shows how to introspect this set. Doesn't mean we must not create an enum for this set. If we do, we get to map from type name to enum value in qmp_pc_dimm_device_list(). Begs the question what to do when we run into a type name we don't know to map. The deeper question is whether we want to duplicate sets of QOM type names as QAPI enums in general. I'm leaning towards "we don't", but I'm happy to hear arguments for doing it.
diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c index 4f30950..7469bd4 100644 --- a/hw/mem/pc-dimm.c +++ b/hw/mem/pc-dimm.c @@ -178,6 +178,7 @@ int qmp_pc_dimm_device_list(Object *obj, void *opaque) di->size = object_property_get_int(OBJECT(dimm), PC_DIMM_SIZE_PROP, NULL); di->memdev = object_get_canonical_path(OBJECT(dimm->hostmem)); + di->type = g_strdup(object_get_typename(obj)); info->u.dimm = di; elem->value = info; diff --git a/qapi-schema.json b/qapi-schema.json index 8d04897..3bcc957 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -3924,6 +3924,8 @@ # # @hotpluggable: true if device if could be added/removed while machine is running # +# @type: device type: 'pc-dimm' or 'nvdimm' (since 2.6) +# # Since: 2.1 ## { 'struct': 'PCDIMMDeviceInfo', @@ -3934,7 +3936,8 @@ 'node': 'int', 'memdev': 'str', 'hotplugged': 'bool', - 'hotpluggable': 'bool' + 'hotpluggable': 'bool', + 'type': 'str' } }