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

[Paolo Bonzini <pbonzini@redhat.com>]

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.

> Maybe I should rename them to __NOTSET.

Perhaps "_DEFAULT" is even bette?

Paolo

>>>
>>> +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.

Paolo