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

[Erik Skultety]

On Wed, Nov 18, 2020 at 06:31:33PM +0100, Michal Privoznik wrote:
> 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 */
> 

Yep, that's what happens when I try to look at something from one too many
angles simultaneously and then combine all the thoughts into a blob such as
^that one.

> 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.

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.

Erik