From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35819) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VOuLT-0002YY-Pe for qemu-devel@nongnu.org; Wed, 25 Sep 2013 15:02:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VOuLO-0000xB-So for qemu-devel@nongnu.org; Wed, 25 Sep 2013 15:01:55 -0400 Received: from mx1.redhat.com ([209.132.183.28]:39379) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VOuLO-0000wz-Lw for qemu-devel@nongnu.org; Wed, 25 Sep 2013 15:01:50 -0400 Date: Wed, 25 Sep 2013 15:01:47 -0400 From: Jeff Cody Message-ID: <20130925190100.GD5035@localhost.localdomain> References: <61a5c47913cce201bbea67a973544805f7b13947.1379615569.git.jcody@redhat.com> <523B4A04.80109@twiddle.net> <20130920041812.GJ15106@localhost.localdomain> <87vc1wyps5.fsf@blackfin.pond.sub.org> <20130925151236.GC5035@localhost.localdomain> <20130925172520.GH2898@dhcp-200-207.str.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20130925172520.GH2898@dhcp-200-207.str.redhat.com> Subject: Re: [Qemu-devel] [PATCH 4/5] block: cow - used QEMU_PACKED for on-disk structures List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: Richard Henderson , Markus Armbruster , stefanha@redhat.com, qemu-devel@nongnu.org On Wed, Sep 25, 2013 at 07:25:20PM +0200, Kevin Wolf wrote: > Am 25.09.2013 um 17:12 hat Jeff Cody geschrieben: > > On Fri, Sep 20, 2013 at 08:23:54AM +0200, Markus Armbruster wrote: > > > Jeff Cody writes: > > > > > > > On Thu, Sep 19, 2013 at 12:01:24PM -0700, Richard Henderson wrote: > > > >> On 09/19/2013 11:43 AM, Jeff Cody wrote: > > > >> > cow_header_v2 is read and written directly from the image file > > > >> > with bdrv_pread()/bdrv_pwrite(), and as such should be packed to > > > >> > avoid unintentional padding. > > > >> > > > > >> > Also change struct cow_header_v2 to a typedef, and some minor > > > >> > code style changes to keep checkpatch.pl happy. > > > >> > > > > >> > Signed-off-by: Jeff Cody > > > >> > --- > > > >> > block/cow.c | 21 +++++++++++---------- > > > >> > 1 file changed, 11 insertions(+), 10 deletions(-) > > > >> > > > > >> > diff --git a/block/cow.c b/block/cow.c > > > >> > index 909c3e7..9c15afb 100644 > > > >> > --- a/block/cow.c > > > >> > +++ b/block/cow.c > > > >> > @@ -32,14 +32,14 @@ > > > >> > #define COW_MAGIC 0x4f4f4f4d /* MOOO */ > > > >> > #define COW_VERSION 2 > > > >> > > > > >> > -struct cow_header_v2 { > > > >> > +typedef struct QEMU_PACKED cow_header_v2 { > > > >> > uint32_t magic; > > > >> > uint32_t version; > > > >> > char backing_file[1024]; > > > >> > int32_t mtime; > > > >> > uint64_t size; > > > >> > uint32_t sectorsize; > > > >> > -}; > > > >> > +} COWHeaderV2; > > > >> > > > >> This changes the layout of this struct. In particular, there's padding > > > >> (depending on the host) between mtime and size. > > > >> > > > > > > > > You are right, and that poses a problem for this patch. > > > > > > > >> I don't know what the right solution is: COWHeaderV3 with the bug fix, leaving > > > >> V2 alone; adding an int32_t dummy there where the padding was; nothing, > > > >> considering the padding to be gone a good thing. > > > >> > > > > > > > > I'm not sure either. I don't think the right thing is to take the > > > > patch as-is, because that will likely break a lot of existing COW > > > > images (I just checked, and on x86_64, it is 1056 bytes unpacked, or > > > > 1048 bytes packed). > > > > > > > > Unfortunately, this means that theoretically, image files with this > > > > format may not be portable, depending on the hosts' compiler and > > > > alignment. In reality, it likely is not a problem. > > > > > > > > I'll drop this one for v2. > > > > > > Possible solutions: > > > > > > * Declare format "cow" non-portable. To move a cow to another system, > > > you have to convert to a portable format. > > > > > > > I favor this approach, especially since "cow" is not likely to be used > > in new environments. > > But COW isn't a native qemu format. We should do whatever the format > really requires, so that new qemu versions handle it correctly - and if > old qemu versions produced corrupted files, bad luck. > It is from UML, right? Is there an official spec that is still around (most of the links I have found suffer from link rot)? The closest I could find to a spec were old UML patches for x86_64 that cleaned up some data types, so that the following was defined: struct cow_header_v2 { __u32 magic; __u32 version; char backing_file[PATH_LEN_V2]; time_t mtime; __u64 size; int sectorsize; }; That remains ambiguous, although given the era I suppose it could be argued that 32-bit architecture and alignment is assumed. But if this is the original spec, then it seems like a non-portable one.