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

Michal Privoznik mprivozn at redhat.com
Wed Nov 18 17:31:33 UTC 2020


On 11/18/20 2:52 PM, Erik Skultety wrote:
> On Wed, Nov 18, 2020 at 01:06:07PM +0100, Michal Privoznik wrote:
>> The way our domain capabilities work currently, is that we have
>> virDomainCapsEnum struct which contains 'unsigned int values'
>> member which serves as a bitmask. More complicated structs are
>> composed from this struct, giving us whole virDomainCaps
>> eventually.
>>
>> Whenever we want to report that a certain value is supported, the
>> '1 << value' bit is set in the corresponding unsigned int member.
>> This works as long as the resulting value after bitshift does not
>> overflow unsigned int. There is a check inside
>> virDomainCapsEnumSet() which ensures exactly this, but no caller
>> really checks whether virDomainCapsEnumSet() succeeded. Also,
>> checking at runtime is a bit too late.
>>
>> Fortunately, we know the largest value we want to store in each
>> member, because each enum of ours ends with _LAST member.
>> Therefore, we can check at build time whether an overflow can
>> occur.
> 
> I'm wondering how does this build failure knowledge actually help us? Are we
> going to start re-evaluating our approach to enums, splitting them somehow? I
> don't think so. The standard only mandates the enum to be large enough so that
> the constants fit into an int, but it's been a while since then and 32bit is
> simply not going to cut it. The almighty internet also claims that compilers
> (GCC specifically) automatically re-size the enum given the declaration, so if
> you do 1 << 32, I would expect that the compiler should allocate a 64bit data
> type, once we're past 64, well, no static assert is going to help us anyway, so
> we need to start thinking about this more intensively.

I think you might have misunderstood. This is not about our enums, but 
the way we store individual values in domCaps. If you have an enum, say 
with video models, then in to express supported models in domCaps is to 
set (1 << val) bit for each val that we find supported (in qemuCaps). 
For instance:

1 << VIR_DOMAIN_VIDEO_TYPE_NONE
1 << VIR_DOMAIN_VIDEO_TYPE_VGA /* if qemuCaps have QEMU_CAPS_DEVICE_VGA */
1 << VIR_DOMAIN_VIDEO_TYPE_CIRRUS /* QEMU_CAPS_DEVICE_CIRRUS_VGA */

and so on. This bitmask is saved into 'unsigned int' currently. And what 
this patch tries to ensure is that 'unsigned int' is enough for all 
possible models known to libvirt currently. If it doesn't fit then we 
will need to switch to a bigger type or use virBitmap. The reason why it 
is not virBitmap currently is that virDomainCaps is complicated 
structure and freeing all bitmaps through each member (which on itself 
is another structure) is just too much code. Especially if 'unsigned 
int' serves us good for now.

> 
> Additionally, since we're using compiler extension quite a bit already, I say
> we make use of those if type expansion is not automatic (I think you have to
> use it explicitly if you want to force a smaller type, like a short int).
> 
> In case I'm misunderstanding something, then I still think that the macro
> definition should go to something like internal.h and be named in a consistent
> fashion like VIR_ENUM_STATIC_ASSERT.
> Moreover, GLib claims the macro can be used anywhere where the typedef is in
> scope, thus I believe the right place to put these asserts is right after the
> actual typedef rather than before a specific struct - doing this also avoid
> duplication if several structures make us of e.g. virTristateBool.

I thought about this but figured it would harm tags generation, wouldn't 
it? For instance consider following:

struct _virDomainCapsDeviceVideo {
     virTristateBool supported;
     VIR_DECLARE_MEMBER(modelType, VIR_DOMAIN_VIDEO_TYPE_LAST);
};

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.

Michal




More information about the libvir-list mailing list