Re: [Qemu-devel] [PATCH 4/5] block: cow - used QEMU_PACKED for on-disk structures

[Jeff Cody]

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 <address@hidden>
> > ---
> >  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.