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

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.

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.