[libvirt] locking down struct size/layout in remote-protocol.x

[Jim Meyering]

Jim Meyering wrote:
> Eric Blake wrote:
>
>> On 05/06/2010 12:35 PM, Jim Meyering wrote:
>>> This week we noticed that a change to struct remote_error
>>> was causing trouble (new member addition changed the size of
>>> the object being decoded -- bad for the protocol).
>>>
>>> In order to ensure that no such changes sneak in again,
>>> I'm planning to do the following.
>>>
>>>     pdwtags src/libvirt_driver_remote_la-remote_protocol.o
>>>
>>> prints, among other things, detailed type info for every struct:
>>>
>>>     /* 89 */
>>>     struct remote_nonnull_domain {
>>>             remote_nonnull_string      name;                 /*     0     8 */
>>>             remote_uuid                uuid;                 /*     8    16 */
>>>             int                        id;                   /*    24     4 */
>>>
>>>             /* size: 32, cachelines: 1, members: 3 */
>>>             /* last cacheline: 32 bytes */
>>>
>>>             /* BRAIN FART ALERT! 32 != 28 + 0(holes), diff = 4 */
>>>
>>>     }; /* size: 32 */
>>
>> Ouch.  Architecture sizing plays a role.  On a 32-bit machine, the first
>> struct is:
>
> There's a way out.
>
>> /* 86 */
>> struct remote_nonnull_domain {
>>         remote_nonnull_string      name;                 /*     0     4 */
>>         remote_uuid                uuid;                 /*     4    16 */
>>         int                        id;                   /*    20     4 */
>>
>>         /* size: 24, cachelines: 1, members: 3 */
>>         /* last cacheline: 24 bytes */
>> }; /* size: 24 */
>>
>> Are we sure migration between 32-bit and 64-bit hypervisors works?  And
>> if it does, then these structs don't quite match what is actually sent
>> over the wire.  At any rate,
>
> Not a problem.  We don't send pointers over the wire.
> The types are designed to be used portably.
>
>>> And here's a sample of its output:
>>>
>>>     verify (sizeof (struct remote_nonnull_domain) == 32);
>>
>> we'd have to make this output conditional on sizeof(void*) to be
>> portable to both 32- and 64- bit machines.
>
> Right.  There are a couple options:
>
> - maintain two sets of sizeof-verifying "verify" directives,
>   one for 32-bit, the other for 64-bit.  With this, the verification
>   is performed at compile time and requires no special tools, in general.
>   However, in the unusual event that we update remote_protocol.x and
>   need to regenerate the directives, we'll have to run pdwtags on both
>   i686 and x86_64.
>
> - maintain a textual representation of these structs and their members
>   (including type names, but not sizes) and make pdwtags a requirement
>   for running "make syntax-check", which would perform the verification.
>   I.e., keep a copy of the output of pdwtags, but without the comments.

FYI, here's code to generate the latter:

    pdwtags src/libvirt_driver_remote_la-remote_protocol.o \
      |perl -0777 -n \
        -e 'foreach my $p (split m!\n\n/\* \d+ \*/\n!)' \
        -e '  { if ($p =~ /^struct remote_/) {' \
        -e '      $p =~ s!\t*/\*.*?\*/!!sg;' \
        -e '      $p =~ s!\s+\n!\n!sg;' \
        -e '      print "$p\n" } }' \
      > remote-protocol-structs

IMHO, it would be sufficient to enable a check comparing
this output to a version-controlled reference file, and
making it part of "make check".

Of course, this would add a dependency on pdwtags/dwarves,
but it would be fine/easy to skip the check on non-Linux systems.

Any objection to requiring the dwarves package for development on Linux?