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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 07/05/14 14:07, Ian Campbell wrote:
> On Wed, 2014-05-07 at 13:14 +0100, Andrew Cooper wrote:
>>>> @@ -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.

But it changes the value in the record length header even if it doesn't
change the content of the record in the stream.  The issue here is
between implicit trailing 0s for alignment purposes (which are not
counted in the record length), and an explicit trailing _resvd field
(which is counted in the record length).

Talking a literal interpretation of the spec, there is a _resvd field,
meaning that the record length should indeed be 4 bytes longer than the
current implementation.

If we are dropping the __packed attributes, I would want some
XC_BUILD_BUG_ON()s to prevent any further surprises (guess how I
discovered this surprise to start with ;p), but where to stick those is
not obvious.

Might it be acceptable to have a single .c file containing stuff like
this which is compiled but not linked?

~Andrew

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