Re: [Xen-devel] [PATCH v4 3/9] tools/libxc: Stream specification and some common code

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2014-05-07 at 13:14 +0100, Andrew Cooper wrote:
> On 07/05/14 12:57, Ian Campbell wrote:
> > On Wed, 2014-04-30 at 19:36 +0100, Andrew Cooper wrote:
> >> diff --git a/tools/libxc/saverestore/stream_format.h 
> >> b/tools/libxc/saverestore/stream_format.h
> >> new file mode 100644
> >> index 0000000..efcca60
> >> --- /dev/null
> >> +++ b/tools/libxc/saverestore/stream_format.h
> > Reference to the spec (which doesn't appear to be in this series
> > anywhere BTW)
> 
> The spec pdf is referenced in the header.  I suppose it should move into
> docs/.

Please.

> 
> >
> >> @@ -0,0 +1,158 @@
> >> +#ifndef __STREAM_FORMAT__H
> >> +#define __STREAM_FORMAT__H
> >> +
> >> +#include <inttypes.h>
> >> +
> >> +/* TODO - find somewhere more appropriate. rec_tsc_info needs it for 
> >> 64bit */
> > You seem to use it on all of them though. Can we not use explicit
> > padding, as you seem to do for many of the other structs?
> 
> The issue with rec_tsc_info is its trailing alignment.  i.e.
> sizeof(rec_tsc_info) is different between 32 and 64 bit builds, yet all
> fields are at the same offset from the beginning.  Putting explicit
> trailing padding would invalidate the current code writing
> sizeof(rec_tsc_info) bytes into the stream.

Putting it inside the struct, like you do elsewhere, would not have this
problem I think.

> This particular bit is to match the spec, but I see what you mean.  I
> shall make it consistently upper case.
> 
> I hadn't actually considered an enum.  It might be ok as each type field
> is passed as a uint32_t, although changing it would be faff for no
> appreciable gain.

Yes, if you can't change the variables which store it there isn't much
gain. If you can change the variables then it lets you use switch
statements to make sure you handle all cases, but that probably doesn't
justify the churn.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel