Re: [Qemu-devel] [PATCH] cow: make padding in the header explicit

[Stefan Hajnoczi]

On Thu, Sep 04, 2014 at 04:10:14PM +0200, Kevin Wolf wrote:
> Am 04.09.2014 um 15:51 hat Stefan Hajnoczi geschrieben:
> > On Thu, Sep 04, 2014 at 06:07:32AM -0600, Eric Blake wrote:
> > > On 09/04/2014 02:58 AM, Stefan Hajnoczi wrote:
> > > > On-disk structures should be marked packed so the compiler does not
> > > > insert padding for field alignment.  Padding should be explicit so
> > > > on-disk layout is obvious and we don't rely on the architecture-specific
> > > > ABI for alignment rules.
> > > > 
> > > > The pahole(1) diff shows that the padding is now explicit and offsets
> > > > are unchanged:
> > > > 
> > > >         char                       backing_file[1024];   /*     8  1024 
> > > > */
> > > >         /* --- cacheline 16 boundary (1024 bytes) was 8 bytes ago --- */
> > > >         int32_t                    mtime;                /*  1032     4 
> > > > */
> > > > -
> > > > -       /* XXX 4 bytes hole, try to pack */
> > > > -
> > > > +       uint32_t                   padding;              /*  1036     4 
> > > > */
> > > >         uint64_t                   size;                 /*  1040     8 
> > > > */
> > > 
> > > Was a 32-bit build also inserting this padding, or do we have historical
> > > differences where 32-bit and 64-bit cow files are actually different,
> > > and we may need to be prepared to parse files from both sources?
> > 
> > Good point.  Let's not merge this patch since it breaks 32-bit hosts.
> > 
> > The fact that no one hit problems when exchanging files between 32-bit
> > and 64-bit machines shows that the cow format is rarely used.
> > 
> > At this point we have 2 different formats: one without padding
> > (i386-style) and one with padding (x86_64-style).  The chance of more
> > variants is small but who knows, maybe some other host architecture ABI
> > has yet another alignment rule for uint64_t.
> > 
> > I'd like to git rm block/cow.c but I suppose the backwards-compatible
> > thing to do is to introduce subformats to support both variants.
> > Opinions?
> 
> Can we safely detect which of the subformats we have? But I'm not sure
> if it's even worth fixing.

I think it would default to the subformat depending on the host
architecture but allow overriding with -o subformat=i386|x86_64.

I'm also not sure if it's worth fixing.  The cow file format is so
rarely used I wonder if we'd be better off without it.

Stefan

pgpEUV_lpOsJN.pgp
Description: PGP signature