Re: [Qemu-devel] [PATCH 2/3] pcdimm: add 'type' field to PCDIMMDeviceInfo

[Markus Armbruster]

Eric Blake <address@hidden> 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: <address@hidden>), 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.