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

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

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?


-- 
Best regards,
Vladimir