Re: [Qemu-devel] [PATCH V2 2/6] libqblock type and structure defines

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

于 2012-9-11 5:27, Eric Blake 写道:
> On 09/10/2012 02:26 AM, Wenchao Xia wrote:
>>    This patch contains type and defines used in APIs, one file for public usage
>> by user, one for libqblock internal usage.
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>>   libqblock/libqblock-internal.h |   50 ++++++++
>>   libqblock/libqblock-types.h    |  251 ++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 301 insertions(+), 0 deletions(-)
>>   create mode 100644 libqblock/libqblock-internal.h
>>   create mode 100644 libqblock/libqblock-types.h
>>
>
> As mentioned in 1/6, this should come earlier in the series, as it lays
> the fundamentals used by other new files.
>
   OK.

>> diff --git a/libqblock/libqblock-internal.h b/libqblock/libqblock-internal.h
>> new file mode 100644
>> index 0000000..fa27ed4
>> --- /dev/null
>> +++ b/libqblock/libqblock-internal.h
>> @@ -0,0 +1,50 @@
>
>> +
>> +#define QB_ERR_STRING_SIZE (1024)
>> +struct QBroker {
>> +    /* last error */
>> +    char err_msg[QB_ERR_STRING_SIZE];
>
> Is this fixed-width struct going to bite us in the future?  Suppose I
> pass in a file name that is already 1000 bytes long; it seems like I
> might be able to get you to overflow this buffer if your error message
> includes the name of my offending file.
>
   Yes it will, thanks for mention me. The resource will always have a
limit, I guess I can just increase the size to 4k to solve the issue.

>> +++ b/libqblock/libqblock-types.h
>> +/* this library is designed around this core struct. */
>> +struct QBlockState;
>> +
>> +/* every thread would have a broker. */
>
> s/would/should/
>
  OK.

>> +
>> +/**
>> + * QBlockStaticInfo: information about the block image.
>> + *
>> + * @loc: location info.
>> + * @fmt_type: format type.
>> + * @virt_size: virtual size in bytes.
>> + * @backing_loc: backing file location, its type is QB_PROT_NONE if not exist.
>> + * @encrypt: encrypt flag.
>> + * @sector_size: how many bytes in a sector, it is 512 usually.
>> + */
>> +struct QBlockStaticInfo {
>> +    struct QBlockProtInfo loc;
>> +    enum QBlockFmtType fmt_type;
>> +    uint64_t virt_size;
>> +    /* advance info */
>> +    struct QBlockProtInfo backing_loc;
>> +    bool encrypt;
>> +    int sector_size;
>> +};
>
> No reserved space for potential growth in this structure?
>
   It could have or not. If new members are added in the tail, then
reserved bytes is not needed, for that this structure's instance
will be allocated in libqblock API.


-- 
Best Regards

Wenchao Xia