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