From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([208.118.235.92]:53692)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <kwolf@redhat.com>) id 1TBN1j-0008Dr-UX
	for qemu-devel@nongnu.org; Tue, 11 Sep 2012 05:45:09 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <kwolf@redhat.com>) id 1TBN1i-0008S7-NH
	for qemu-devel@nongnu.org; Tue, 11 Sep 2012 05:45:03 -0400
Received: from mx1.redhat.com ([209.132.183.28]:58094)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <kwolf@redhat.com>) id 1TBN1i-0008Q2-FB
	for qemu-devel@nongnu.org; Tue, 11 Sep 2012 05:45:02 -0400
Message-ID: <504F0819.2030603@redhat.com>
Date: Tue, 11 Sep 2012 11:44:57 +0200
From: Kevin Wolf <kwolf@redhat.com>
MIME-Version: 1.0
References: <1344613185-12308-1-git-send-email-wdongxu@linux.vnet.ibm.com>
	<1344613185-12308-6-git-send-email-wdongxu@linux.vnet.ibm.com>
	<20120906201953.GE522@illuin>
	<CAGrFBsgULUcg-S0T02KKRn5FeABQQ13d3r9NJk9vS4cDz8Ppxw@mail.gmail.com>
In-Reply-To: <CAGrFBsgULUcg-S0T02KKRn5FeABQQ13d3r9NJk9vS4cDz8Ppxw@mail.gmail.com>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH V12 5/6] add-cow file format
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
Cc: Michael Roth <mdroth@linux.vnet.ibm.com>, qemu-devel@nongnu.org

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