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

[Jeff Cody]

On Fri, Sep 20, 2013 at 08:23:54AM +0200, Markus Armbruster wrote:
> Jeff Cody <address@hidden> 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 <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.
> 
> 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.

> * Keep using the non-portable header.  When opening an existing image,
>   guess which of the two header variants it got: the padding should be
>   zero, size and sectorsize sane.  Perhaps provide an option to overrule
>   the guess.
> 
> Who's still using format "cow"?
>