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
