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.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/conf/domain_capabilities.h | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/src/conf/domain_capabilities.h b/src/conf/domain_capabilities.h
index f177af1744..b22d40abb2 100644
--- a/src/conf/domain_capabilities.h
+++ b/src/conf/domain_capabilities.h
@@ -36,6 +36,9 @@ struct _virDomainCapsEnum {
     unsigned int values; /* Bitmask of values supported in the corresponding enum */
 };
 
+#define STATIC_ASSERT_ENUM(last) \
+    G_STATIC_ASSERT(last <= sizeof(unsigned int) * CHAR_BIT)
+
 typedef struct _virDomainCapsStringValues virDomainCapsStringValues;
 typedef virDomainCapsStringValues *virDomainCapsStringValuesPtr;
 struct _virDomainCapsStringValues {
@@ -43,6 +46,8 @@ struct _virDomainCapsStringValues {
     size_t nvalues; /* number of strings */
 };
 
+STATIC_ASSERT_ENUM(VIR_DOMAIN_LOADER_TYPE_LAST);
+STATIC_ASSERT_ENUM(VIR_TRISTATE_BOOL_LAST);
 typedef struct _virDomainCapsLoader virDomainCapsLoader;
 typedef virDomainCapsLoader *virDomainCapsLoaderPtr;
 struct _virDomainCapsLoader {
@@ -53,6 +58,7 @@ struct _virDomainCapsLoader {
     virDomainCapsEnum secure;   /* Info about secure:virTristateBool */
 };
 
+STATIC_ASSERT_ENUM(VIR_DOMAIN_OS_DEF_FIRMWARE_LAST);
 typedef struct _virDomainCapsOS virDomainCapsOS;
 typedef virDomainCapsOS *virDomainCapsOSPtr;
 struct _virDomainCapsOS {
@@ -61,6 +67,9 @@ struct _virDomainCapsOS {
     virDomainCapsLoader loader;     /* Info about virDomainLoaderDef */
 };
 
+STATIC_ASSERT_ENUM(VIR_DOMAIN_DISK_DEVICE_LAST);
+STATIC_ASSERT_ENUM(VIR_DOMAIN_DISK_BUS_LAST);
+STATIC_ASSERT_ENUM(VIR_DOMAIN_DISK_MODEL_LAST);
 typedef struct _virDomainCapsDeviceDisk virDomainCapsDeviceDisk;
 typedef virDomainCapsDeviceDisk *virDomainCapsDeviceDiskPtr;
 struct _virDomainCapsDeviceDisk {
@@ -71,6 +80,7 @@ struct _virDomainCapsDeviceDisk {
     /* add new fields here */
 };
 
+STATIC_ASSERT_ENUM(VIR_DOMAIN_GRAPHICS_TYPE_LAST);
 typedef struct _virDomainCapsDeviceGraphics virDomainCapsDeviceGraphics;
 typedef virDomainCapsDeviceGraphics *virDomainCapsDeviceGraphicsPtr;
 struct _virDomainCapsDeviceGraphics {
@@ -78,6 +88,7 @@ struct _virDomainCapsDeviceGraphics {
     virDomainCapsEnum type;   /* virDomainGraphicsType */
 };
 
+STATIC_ASSERT_ENUM(VIR_DOMAIN_VIDEO_TYPE_LAST);
 typedef struct _virDomainCapsDeviceVideo virDomainCapsDeviceVideo;
 typedef virDomainCapsDeviceVideo *virDomainCapsDeviceVideoPtr;
 struct _virDomainCapsDeviceVideo {
@@ -85,6 +96,11 @@ struct _virDomainCapsDeviceVideo {
     virDomainCapsEnum modelType;   /* virDomainVideoType */
 };
 
+STATIC_ASSERT_ENUM(VIR_DOMAIN_HOSTDEV_MODE_LAST);
+STATIC_ASSERT_ENUM(VIR_DOMAIN_STARTUP_POLICY_LAST);
+STATIC_ASSERT_ENUM(VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST);
+STATIC_ASSERT_ENUM(VIR_DOMAIN_HOSTDEV_CAPS_TYPE_LAST);
+STATIC_ASSERT_ENUM(VIR_DOMAIN_HOSTDEV_PCI_BACKEND_TYPE_LAST);
 typedef struct _virDomainCapsDeviceHostdev virDomainCapsDeviceHostdev;
 typedef virDomainCapsDeviceHostdev *virDomainCapsDeviceHostdevPtr;
 struct _virDomainCapsDeviceHostdev {
@@ -97,6 +113,8 @@ struct _virDomainCapsDeviceHostdev {
     /* add new fields here */
 };
 
+STATIC_ASSERT_ENUM(VIR_DOMAIN_RNG_MODEL_LAST);
+STATIC_ASSERT_ENUM(VIR_DOMAIN_RNG_BACKEND_LAST);
 typedef struct _virDomainCapsDeviceRNG virDomainCapsDeviceRNG;
 typedef virDomainCapsDeviceRNG *virDomainCapsDeviceRNGPtr;
 struct _virDomainCapsDeviceRNG {
@@ -105,6 +123,7 @@ struct _virDomainCapsDeviceRNG {
     virDomainCapsEnum backendModel;   /* virDomainRNGBackend */
 };
 
+STATIC_ASSERT_ENUM(VIR_GIC_VERSION_LAST);
 typedef struct _virDomainCapsFeatureGIC virDomainCapsFeatureGIC;
 typedef virDomainCapsFeatureGIC *virDomainCapsFeatureGICPtr;
 struct _virDomainCapsFeatureGIC {
-- 
2.26.2

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

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

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

On 11/19/20 9:48 AM, Erik Skultety wrote:

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

You mean to put the assert into domain_conf.h? I'm not sure it would 
make sense. Because the assert actually guides virDomainCapsEnum struct 
and the way we store info there and not virDomainVideoType per se. For 
instance for enums not (yet) exposed on domCaps we don't care about 
their size.

Moreover, if I leave those asserts where they are now, then during 
compilation the compiler reports error right where the problem is and 
not at a place where it leaves you wondering why the assert is there.

Michal

On Thu, Nov 19, 2020 at 12:22:07PM +0100, Michal Privoznik wrote:
> On 11/19/20 9:48 AM, Erik Skultety wrote:
> 
> > > 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.
> 
> You mean to put the assert into domain_conf.h? I'm not sure it would make
> sense. Because the assert actually guides virDomainCapsEnum struct and the
> way we store info there and not virDomainVideoType per se. For instance for
> enums not (yet) exposed on domCaps we don't care about their size.
> 

I was also not opting to guard all of them.

> Moreover, if I leave those asserts where they are now, then during
> compilation the compiler reports error right where the problem is and not at
> a place where it leaves you wondering why the assert is there.

I'd say the confusion rate is about the same as when you're guarding several
virDomainCapsEnum variables, none of which has the corresponding type mentioned
in a commentary right next to it.
On the other hand, I can see the value and convenience in copy-paste - when
one needs to add a new domain capability, they see that all the other struct
definitions are guarded, so they do the same, hard to argue against that...
compared to when you need not to forget to guard the enum that gets propagated
into the capability.

Fair enough, but make sure you leave out the assert for VIR_TRISTATE_BOOL, I
mean we can guarantee that this one will never magically acquire another 29
values.

Reviewed-by: Erik Skultety <eskultet@redhat.com>

On 11/19/20 12:47 PM, Erik Skultety wrote:
> On Thu, Nov 19, 2020 at 12:22:07PM +0100, Michal Privoznik wrote:
>> On 11/19/20 9:48 AM, Erik Skultety wrote:
>>
>>>> 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.
>>
>> You mean to put the assert into domain_conf.h? I'm not sure it would make
>> sense. Because the assert actually guides virDomainCapsEnum struct and the
>> way we store info there and not virDomainVideoType per se. For instance for
>> enums not (yet) exposed on domCaps we don't care about their size.
>>
> 
> I was also not opting to guard all of them.

I know, but my point was, if I opened domain_conf.h and saw asserts only 
for some enums it would keep me wondering what is going on and why the 
rest doesn't have asserts.

> 
>> Moreover, if I leave those asserts where they are now, then during
>> compilation the compiler reports error right where the problem is and not at
>> a place where it leaves you wondering why the assert is there.
> 
> I'd say the confusion rate is about the same as when you're guarding several
> virDomainCapsEnum variables, none of which has the corresponding type mentioned
> in a commentary right next to it.

The thing is, you can't insert G_STATIC_ASSERT into a struct definition, 
it doesn't compile:

struct _virDomainCapsDeviceVideo {
     virTristateBool supported;
     virDomainCapsEnum modelType;   /* virDomainVideoType */
     STATIC_ASSERT_ENUM(VIR_DOMAIN_VIDEO_TYPE_LAST);
};


/usr/include/glib-2.0/glib/gmacros.h:745:31: error: expected 
specifier-qualifier-list before ‘typedef’
   745 | #define G_STATIC_ASSERT(expr) typedef char G_PASTE 
(_GStaticAssertCompileTimeAssertion_, __COUNTER__)[(expr) ? 1 : -1] 
G_GNUC_UNUSED
       |                               ^~~~~~~
../src/conf/domain_capabilities.h:40:5: note: in expansion of macro 
‘G_STATIC_ASSERT’
    40 |     G_STATIC_ASSERT(last <= sizeof(unsigned int) * CHAR_BIT)
       |     ^~~~~~~~~~~~~~~
../src/conf/domain_capabilities.h:96:5: note: in expansion of macro 
‘STATIC_ASSERT_ENUM’
    96 |     STATIC_ASSERT_ENUM(VIR_DOMAIN_VIDEO_TYPE_LAST);
       |     ^~~~~~~~~~~~~~~~~~

That leaves us with only two options: before of after the struct.

> On the other hand, I can see the value and convenience in copy-paste - when
> one needs to add a new domain capability, they see that all the other struct
> definitions are guarded, so they do the same, hard to argue against that...
> compared to when you need not to forget to guard the enum that gets propagated
> into the capability.
> 
> Fair enough, but make sure you leave out the assert for VIR_TRISTATE_BOOL, I
> mean we can guarantee that this one will never magically acquire another 29
> values.

In this case I'd prefer consistency. Surely, verifying one assert() at 
build time is taking no time.

> 
> Reviewed-by: Erik Skultety <eskultet@redhat.com>
> 

Michal