Re: [Qemu-devel] [PATCH V4 2/5] libqblock type defines

[Wenchao Xia <xiawenc@linux.vnet.ibm.com>]

于 2012-9-27 18:16, Paolo Bonzini 写道:
> Il 27/09/2012 11:52, Wenchao Xia ha scritto:
>>>
>>> Please use QBO_ instead of QB_ throughout.  Also write COMPAT instead of
>>> CPT, and remove CPT_NONE since 0.10 is the default:
>>>
>>    __NONE is used to indicate whether this property is set or get, so
>> it is actually have 3 status than just 2: Not set, V010, V110. It
>> is the same reason that I use __NONE in bool values, especially __NONE
>> could represent it is not got in information retrieving.
>
> Yes, that I guessed.  I thought in many cases we can anticipate that the
> default is not going to change, but perhaps it's better to be safe
> (which is what you did).
>
> Please do change FALSE/TRUE to OFF/METADATA for preallocation enums, and
> please remove MONOLITHIC from QBO_FMT_VMDK_SUBFMT_MONOLITHIC_NONE.
>
   OK, these should be changed.

>> Maybe I should rename them to __NOTSET.
>
> Perhaps "_DEFAULT" is even bette?
>
> Paolo
>
   Yep, _DEFAULT seems good to me.

>>>>
>>>> +typedef struct QBlockStaticInfoAddr {
>>>> +    uint64_t *virt_size;
>>>> +    QBlockProtInfo *backing_loc;
>>>> +    bool *encrypt;
>>>> +} QBlockStaticInfoAddr;
>>>
>>> Why the indirection?
>>>
>>    It helps user to get these important members, otherwise
>> user will need
>> Switch (fmt) {
>>    case RAW:
>>      ....
>>    case QCOW2:
>>      ...
>> }
>> for every attribute. The indirection address will let user directly
>> access the members.
>
> Ah, ok, now I understand.  Interesting.  An alternative could be to add
> generic accessors like these:
>
> uint64_t qblock_get_virt_size(QBlockFmtInfo *fmt);
> QBlockProtInfo *qblock_get_backing_loc(QBlockFmtInfo *fmt);
> bool qblock_get_encrypt(QBlockFmtInfo *fmt);
>
> etc. that include the switch statement.
>
   I think it is reasonable use generic accessors instead of embbed
structure, maybe an additional helper function should be added as:

QBlockStaticInfoAddr *qblock_get_static_info_addr(QBlockStaticInfo
  *info)

   In this way only one switch statement is needed in source code,
and only one new API is added.

> Paolo
>


-- 
Best Regards

Wenchao Xia