Re: [Qemu-devel] [PATCH V12 5/6] add-cow file format

[Kevin Wolf <kwolf@redhat.com>]

Am 10.09.2012 04:25, schrieb Dong Xu Wang:
> On Fri, Sep 7, 2012 at 4:19 AM, Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
>> On Fri, Aug 10, 2012 at 11:39:44PM +0800, Dong Xu Wang wrote:
>>> +typedef struct AddCowHeader {
>>> +    uint64_t        magic;
>>> +    uint32_t        version;
>>> +
>>> +    uint32_t        backing_filename_offset;
>>> +    uint32_t        backing_filename_size;
>>> +
>>> +    uint32_t        image_filename_offset;
>>> +    uint32_t        image_filename_size;
>>> +
>>> +    uint64_t        features;
>>> +    uint64_t        optional_features;
>>> +    uint32_t        header_pages_size;
>>> +} QEMU_PACKED AddCowHeader;
>>
>> You should avoid using packed structures for image format headers.
>> Instead, I would either:
>>
>> a) re-order the fields so that 32/64-bit fields, respectively, fall on
>> 32/64-bit boundaries (in your case, for instance, moving header_pages_size
>> above features) like qed/qcow2 do, or
>>
>> b) read/write the fields individually rather than reading/writing directly
>> into/from the header struct.
>>
>> The safest route is b). Adds a few lines of code, but you won't have to
>> re-work things (or worry about introducing bugs) later if you were to add,
>> say, a 32-bit value, and then a 64-bit value later.
> 
> While, Kevin's suggestion is using PACKED, so ..

Yes, I think QEMU_PACKED is fine, and it's the safest version.

It would be nice to additionally do Michael's option a) if you like, but
I don't think the header is accessed too often, so the optimisation
isn't that important.

Kevin