Am 10.09.2012 04:25, schrieb Dong Xu Wang:
> On Fri, Sep 7, 2012 at 4:19 AM, Michael Roth <mdr...@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
