On Tue, Feb 19, 2013 at 08:11:48AM -0500, Jeff Cody wrote:
> 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.
Even the order within a byte is implementation defined, so unless you're
programming for a very specific compiler, they are completely useless
for on-disk structures. Let's remove them.
Kevin
