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

[Kevin Wolf]

Am 04.09.2014 um 17:43 hat Stefan Hajnoczi geschrieben:
> 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.

Hm, okay. If we can't do it automatically, that's an option, too.

> 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.

It has never been used as a native qemu image format. As I understand
it, its use case is compatibility with existing images, and one of the
great things about qemu-img is that it can open more or less any random
image that you get from somewhere. The exotic formats are part of this.

If we wanted to simplify our code, we could probably make it read-only
(at the cost of losing qemu-iotests support), but then it's so simple
that leaving r/w support around won't hurt us.


Hm, actually, looking at the kernel (arch/um/drivers/cow_user.c), it
doesn't look as if we were compatible at all for v2, at least in the
current kernel version, we use a different backing file name length...
And they have the struct packed today, so we should probably follow
their example.

On the other hand, out of three versions, we only support v2, and we
don't use the same format as the kernel. Yeah, I guess we might just
drop the support then, it's not helpful for compatibility. The only
other way would be to update it to handle all three versions the same as
the kernel code.

Kevin

pgpjoEsDEXRNq.pgp
Description: PGP signature