Re: [Qemu-devel] [PATCH 1/2] virtio-gpu/2d: add hardware spec include file

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2176 bytes --]

On 09/12/2014 04:44 AM, Gerd Hoffmann wrote:

>>> +enum virtgpu_ctrl_type {
>>> +        VIRTGPU_UNDEFINED = 0,
>>> +
>>> +        /* 2d commands */
>>> +        VIRTGPU_CMD_GET_DISPLAY_INFO = 0x0100,
>>
>> Please consider also adding:
>>
>> #define VIRTGPU_CMD_GET_DISPLAY_INFO VIRTGPU_CMD_GET_DISPLAY_INFO
>>
>> and friends.  It makes it MUCH nicer for application software to probe
>> for later extensions if every member of the enum is also associated with
>> a preprocessor macro.
> 
> I don't think this will ever be shipped as library header for external
> users ...

Fair enough. I wasn't sure, so it didn't hurt to ask.

> 
>>> +struct virtgpu_ctrl_hdr {
>>> +        uint32_t type;
>>> +        uint32_t flags;
>>> +        uint64_t fence_id;
>>> +        uint32_t ctx_id;
>>> +        uint32_t padding;
>>> +};
>>> +
>>
>> Is the padding to ensure that this is aligned regardless of 32-bit or
>> 64-bit hosts?
> 
> Yes.
> 
>> Is it worth adding a compile-time assertion about the
>> size of the struct to ensure the compiler doesn't add any additional
>> padding?
> 
> Makes sense.  What is the usual trick to do that?

Among others,

struct dummy {
  int size_check : (sizeof(virtgpu_ctrl_hdr) == 24 ? 1 : -1);
};

Since bitfields cannot have a negative size, the compiler will
forcefully fail compilation if the struct size ever changes.  Similar
tricks include setting up array bounds that would be negative on
failure, or (inside a function body) declaring a switch that has
colliding case statements on failure.  But depending on the set of
compiler warning options in effect, declaring an otherwise unused struct
may be too noisy.  So if you need more ideas and a good read, check out
the comments in how gnulib does it (the link mentions GPLv3+, but that
file is also shipped as LGPLv2+ so it is compatible with qemu):

git.savannah.gnu.org/cgit/gnulib.git/tree/lib/verify.h

with an end result that a user just writes:

verify(sizeof(virtgpu_ctrl_hdr) == 24);

and calls it a day.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]