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

[Stefan Hajnoczi]

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.

> +/* 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?

> +/* 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...