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