[PATCH] domain_capabilities: Assert enums fit into unsigned int bitmask

[Michal Privoznik]

On 11/19/20 12:47 PM, Erik Skultety wrote:
> On Thu, Nov 19, 2020 at 12:22:07PM +0100, Michal Privoznik wrote:
>> On 11/19/20 9:48 AM, Erik Skultety wrote:
>>
>>>> where VIR_DECLARE_MEMBER() would assert() that 1 <<
>>>> VIR_DOMAIN_VIDEO_TYPE_LAST fits into 'unsigned int' (or whatever type we
>>>> will use), and also declare 'modelType' member for the struct. I'm not sure
>>>> a tags generator would be capable of seeing that.
>>>
>>> Why do you have to do both at the same time? Why not putting the assert right
>>> after the virDomainVideoType enum typedef? I think it's much more obvious if
>>> the guard comes right after what it actually guards.
>>
>> You mean to put the assert into domain_conf.h? I'm not sure it would make
>> sense. Because the assert actually guides virDomainCapsEnum struct and the
>> way we store info there and not virDomainVideoType per se. For instance for
>> enums not (yet) exposed on domCaps we don't care about their size.
>>
> 
> I was also not opting to guard all of them.

I know, but my point was, if I opened domain_conf.h and saw asserts only 
for some enums it would keep me wondering what is going on and why the 
rest doesn't have asserts.

> 
>> Moreover, if I leave those asserts where they are now, then during
>> compilation the compiler reports error right where the problem is and not at
>> a place where it leaves you wondering why the assert is there.
> 
> I'd say the confusion rate is about the same as when you're guarding several
> virDomainCapsEnum variables, none of which has the corresponding type mentioned
> in a commentary right next to it.

The thing is, you can't insert G_STATIC_ASSERT into a struct definition, 
it doesn't compile:

struct _virDomainCapsDeviceVideo {
     virTristateBool supported;
     virDomainCapsEnum modelType;   /* virDomainVideoType */
     STATIC_ASSERT_ENUM(VIR_DOMAIN_VIDEO_TYPE_LAST);
};


/usr/include/glib-2.0/glib/gmacros.h:745:31: error: expected 
specifier-qualifier-list before ‘typedef’
   745 | #define G_STATIC_ASSERT(expr) typedef char G_PASTE 
(_GStaticAssertCompileTimeAssertion_, __COUNTER__)[(expr) ? 1 : -1] 
G_GNUC_UNUSED
       |                               ^~~~~~~
../src/conf/domain_capabilities.h:40:5: note: in expansion of macro 
‘G_STATIC_ASSERT’
    40 |     G_STATIC_ASSERT(last <= sizeof(unsigned int) * CHAR_BIT)
       |     ^~~~~~~~~~~~~~~
../src/conf/domain_capabilities.h:96:5: note: in expansion of macro 
‘STATIC_ASSERT_ENUM’
    96 |     STATIC_ASSERT_ENUM(VIR_DOMAIN_VIDEO_TYPE_LAST);
       |     ^~~~~~~~~~~~~~~~~~

That leaves us with only two options: before of after the struct.

> On the other hand, I can see the value and convenience in copy-paste - when
> one needs to add a new domain capability, they see that all the other struct
> definitions are guarded, so they do the same, hard to argue against that...
> compared to when you need not to forget to guard the enum that gets propagated
> into the capability.
> 
> Fair enough, but make sure you leave out the assert for VIR_TRISTATE_BOOL, I
> mean we can guarantee that this one will never magically acquire another 29
> values.

In this case I'd prefer consistency. Surely, verifying one assert() at 
build time is taking no time.

> 
> Reviewed-by: Erik Skultety <eskultet at redhat.com>
> 

Michal