Re: [Qemu-devel] [RFC PATCH 2/4] block: vhdx header for the QEMU support of VHDX images

[Jeff Cody]

On Tue, Feb 19, 2013 at 10:02:51AM +0100, Stefan Hajnoczi wrote:
> On Mon, Feb 18, 2013 at 06:03:30PM -0500, Jeff Cody wrote:
> > +/* the guid is a 16 byte unique ID - the definition for this used by
> > + * Microsoft is not just 16 bytes though - it is a structure that is 
> > defined,
> > + * so we need to follow it here so that endianness does not trip us up */
> > +
> > +typedef struct ms_guid {
> > +    uint32_t    data1;
> > +    uint16_t    data2;
> > +    uint16_t    data3;
> > +    uint8_t     data4[8];
> > +} ms_guid;
> > +
> > +#define guid_cmp(a, b) \
> > +    (memcmp(&(a), &(b), sizeof(ms_guid)) == 0)
> 
> Inline memcmp() is simple enough, no need for a macro.  "cmp" is a bad
> name since cmp functions usually return -1, 0, 1 for sort(3) usage.
> "eq" would be a better name, but simply using memcmp() is clearest IMO.
> 

Thanks for the reviews, Stefan, I appreciate it.

Yeah, I went back and forth on that in my head, and thought guid_cmp
(or guid_eq) was more readable.  But I am fine just using memcmp
instead, that may be better.

> > +/* Individual region table entry.  There may be a maximum of 2047 of these
> > + *
> > + *  There are two known region table properties.  Both are required.
> > + *  BAT (block allocation table):  2DC27766F62342009D64115E9BFD4A08
> > + *  Metadata:                      8B7CA20647904B9AB8FE575F050F886E
> > + */
> > +typedef struct vhdx_region_table_entry {
> > +    ms_guid     guid;                   /* 128-bit unique identifier */
> > +    uint64_t    file_offset;            /* offset of the object in the 
> > file.
> > +                                           Must be multiple of 1MB */
> > +    uint32_t    length;                 /* length, in bytes, of the object 
> > */
> > +    union vhdx_rt_bitfield {
> > +        struct {
> > +        uint32_t    required:1;        /* 1 if this region must be 
> > recognized
> > +                                          in order to load the file */
> > +        uint32_t    reserved:31;
> > +        } bits;
> > +        uint32_t data;
> > +    } bitfield;
> 
> Bitfield in a file format structure, yikes.  Endianness, layout, etc.
> Better to use uint32_t flags with a VHDX_RT_FLAG_REQUIRED bitmask
> constant?

Yeah, pretty ugly - it is also how it is present in the VHDX spec,
which is why I left the structure definition the same.  The endianness
of it has to be dealt with either way during the parsing and writing,
so I didn't see any compelling reason to abstract the struct away from
a bitfield.

> 
> > +/* This is a packed struct that generally should not have alignment issues,
> > + * as it is just uint64_t at heart */
> > +typedef struct QEMU_PACKED vhdx_bat_entry {
> > +    union vhdx_bat_bitfield {
> > +        struct {
> > +            uint64_t    state:3;           /* state of the block (see 
> > above) */
> > +            uint64_t    reserved:17;
> > +            uint64_t    file_offset_mb:44; /* offset within file in 1MB 
> > units */
> > +        } bits;
> > +        uint64_t data;
> > +    } bitfield;
> 
> More bitfields...

And uint64_t ones, at that!  This one makes more sense, spec-wise, in
that the BAT is likely over a MB as it is.  This is also the one
struct that I did use packed, since it is essentially a uint64_t; I
wasn't too worried about it.