9:59 a.m.
There are few cases where virReportEnumRangeError() is passed to a
mismatching enum. With patch 4/4 we can check for this at compile time
(assuming -Wenum-compare is available).
For instance with clang I get:
FAILED: src/conf/libvirt_conf.a.p/domain_validate.c.o
clang ...
../src/conf/domain_validate.c:184:9: error: comparison of different enumeration types
('virDomainVideoBackendType' and 'virDomainInputType')
[-Werror,-Wenum-compare]
virReportEnumRangeError(virDomainInputType, video->backend);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../src/util/virerror.h:175:47: note: expanded from macro
'virReportEnumRangeError'
(__typeof__(value))1 == (typname)1 && sizeof((typname)1)
!= 0 ? #typname : #typname)
~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~
Whereas with gcc I get more obscure error:
FAILED: src/conf/libvirt_conf.a.p/domain_validate.c.o
cc ...
In file included from ../src/util/virthread.h:25,
from ../src/util/virobject.h:25,
from ../src/conf/capabilities.h:28,
from ../src/conf/domain_conf.h:31,
from ../src/conf/domain_validate.h:25,
from ../src/conf/domain_validate.c:23:
../src/conf/domain_validate.c: In function ‘virDomainVideoDefValidate’:
../src/util/virerror.h:175:47: error: comparison between ‘enum <anonymous>’ and
‘enum <anonymous>’ [-Werror=enum-compare]
175 | (__typeof__(value))1 == (typname)1 &&
sizeof((typname)1) != 0 ? #typname : #typname)
| ^~
../src/conf/domain_validate.c:184:9: note: in expansion of macro
‘virReportEnumRangeError’
184 | virReportEnumRangeError(virDomainInputType, video->backend);
| ^~~~~~~~~~~~~~~~~~~~~~~
But I guess this is also okay-ish - it reports there is a problem with
code.
NB this only works with variables that are of a certain enum types. It
does not work with those generic 'int var /* someEnum */' declarations,
understandably. But as we use virXMLPropEnum() more, we can declare
variables of their true type (enum) and get more and more calls to
virReportEnumRangeError() checked.
Michal Prívozník (4):
virnetdevvportprofile: Turn virNetDevVPortProfileLinkOp enum into a
proper typedef
virNetDevVPortProfileOp8021Qbh: Use proper type in
virReportEnumRangeError()
virDomainVideoDefValidate: Use proper type in
virReportEnumRangeError()
virerror: Make virReportEnumRangeError() check for type mismatch
src/conf/domain_validate.c | 2 +-
src/util/virerror.h | 14 ++++++++------
src/util/virnetdevvportprofile.c | 10 +++++-----
3 files changed, 14 insertions(+), 12 deletions(-)
--
2.41.0
9:59 a.m.
New subject: [PATCH 1/4] virnetdevvportprofile: Turn virNetDevVPortProfileLinkOp enum into a proper typedef
This allows us to declare variables without using 'enum
virNetDev....' and will become more useful in the near future
(when virReportEnumRangeError() is fixed).
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/virnetdevvportprofile.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/util/virnetdevvportprofile.c b/src/util/virnetdevvportprofile.c
index 33bcd350c4..6d5bfbe870 100644
--- a/src/util/virnetdevvportprofile.c
+++ b/src/util/virnetdevvportprofile.c
@@ -69,12 +69,12 @@ VIR_LOG_INIT("util.netdevvportprofile");
# define LLDPAD_PID_FILE "/var/run/lldpad.pid"
-enum virNetDevVPortProfileLinkOp {
+typedef enum {
VIR_NETDEV_VPORT_PROFILE_LINK_OP_ASSOCIATE = 0x1,
VIR_NETDEV_VPORT_PROFILE_LINK_OP_DISASSOCIATE = 0x2,
VIR_NETDEV_VPORT_PROFILE_LINK_OP_PREASSOCIATE = 0x3,
VIR_NETDEV_VPORT_PROFILE_LINK_OP_PREASSOCIATE_RR = 0x4,
-};
+} virNetDevVPortProfileLinkOp;
#endif
@@ -1024,7 +1024,7 @@ virNetDevVPortProfileOp8021Qbg(const char *ifname,
const virMacAddr *macaddr,
int vf,
const virNetDevVPortProfile *virtPort,
- enum virNetDevVPortProfileLinkOp virtPortOp,
+ virNetDevVPortProfileLinkOp virtPortOp,
bool setlink_only)
{
int op = PORT_REQUEST_ASSOCIATE;
@@ -1093,7 +1093,7 @@ virNetDevVPortProfileOp8021Qbh(const char *ifname,
int32_t vf,
const virNetDevVPortProfile *virtPort,
const unsigned char *vm_uuid,
- enum virNetDevVPortProfileLinkOp virtPortOp)
+ virNetDevVPortProfileLinkOp virtPortOp)
{
int rc = 0;
char *physfndev = NULL;
--
2.41.0
9:59 a.m.
New subject: [PATCH 2/4] virNetDevVPortProfileOp8021Qbh: Use proper type in virReportEnumRangeError()
The @virtPortOp variable inside of virNetDevVPortProfileOp8021Qbh
is of type virNetDevVPortProfileLinkOp. Pass the proper type to
virReportEnumRangeError().
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/virnetdevvportprofile.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/virnetdevvportprofile.c b/src/util/virnetdevvportprofile.c
index 6d5bfbe870..c755fa79ec 100644
--- a/src/util/virnetdevvportprofile.c
+++ b/src/util/virnetdevvportprofile.c
@@ -1182,7 +1182,7 @@ virNetDevVPortProfileOp8021Qbh(const char *ifname,
rc = -1;
break;
default:
- virReportEnumRangeError(virNetDevVPortProfileType, virtPortOp);
+ virReportEnumRangeError(virNetDevVPortProfileLinkOp, virtPortOp);
rc = -1;
break;
}
--
2.41.0
9:59 a.m.
New subject: [PATCH 3/4] virDomainVideoDefValidate: Use proper type in virReportEnumRangeError()
The @backend member of _virDomainVideoDef struct is of type
virDomainVideoBackendType. Pass the proper type to
virReportEnumRangeError().
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/domain_validate.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
index e70841c1d6..ac32fa1477 100644
--- a/src/conf/domain_validate.c
+++ b/src/conf/domain_validate.c
@@ -181,7 +181,7 @@ virDomainVideoDefValidate(const virDomainVideoDef *video,
break;
case VIR_DOMAIN_VIDEO_BACKEND_TYPE_LAST:
default:
- virReportEnumRangeError(virDomainInputType, video->backend);
+ virReportEnumRangeError(virDomainVideoBackendType, video->backend);
return -1;
}
--
2.41.0
10 a.m.
New subject: [PATCH 4/4] virerror: Make virReportEnumRangeError() check for type mismatch
As can be seen from previous commits, it's fairly easy to pass a
different type to virReportEnumRangeError() than the actual
variable is of. So far, we have a sizeof() hack to check if some
nonsensical types are not passed, e.g. it catches cases where a
function name is passed instead of an enum. Extend the hack to
check whether proper enum was passed.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/virerror.h | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/src/util/virerror.h b/src/util/virerror.h
index ee85247433..d4b2679a09 100644
--- a/src/util/virerror.h
+++ b/src/util/virerror.h
@@ -161,16 +161,18 @@ void virReportSystemErrorFull(int domcode,
#define virReportRestrictedError(...) \
virReportErrorHelper(VIR_FROM_THIS, VIR_ERR_OPERATION_DENIED, \
__FILE__, __FUNCTION__, __LINE__, __VA_ARGS__)
-/* The sizeof(...) comparison here is a hack to catch typos
- * in the name of the enum by triggering a compile error, as well
- * as detecting if you passed a typename that refers to a function
- * or struct type, instead of an enum. It should get optimized away
- * since sizeof() is known at compile time */
+/* The ternary operator here is a hack to catch typos in the name of
+ * the enum and mismatching enum by triggering a compile error, as
+ * well as detecting if you passed a typename that refers to a
+ * function or struct type, instead of an enum. It should get
+ * optimized away since the value is constant and thus is known at
+ * compile time. */
#define virReportEnumRangeError(typname, value) \
virReportErrorHelper(VIR_FROM_THIS, VIR_ERR_INTERNAL_ERROR, \
__FILE__, __FUNCTION__, __LINE__, \
"Unexpected enum value %d for %s", \
- value, sizeof((typname)1) != 0 ? #typname : #typname);
+ value, \
+ (__typeof__(value))1 == (typname)1 && sizeof((typname)1)
!= 0 ? #typname : #typname)
#define virReportError(code, ...) \
virReportErrorHelper(VIR_FROM_THIS, code, __FILE__, \
--
2.41.0
2:27 p.m.
On a Tuesday in 2023, Michal Privoznik wrote:
There are few cases where virReportEnumRangeError() is passed to a
mismatching enum. With patch 4/4 we can check for this at compile time
(assuming -Wenum-compare is available).
For instance with clang I get:
FAILED: src/conf/libvirt_conf.a.p/domain_validate.c.o
clang ...
../src/conf/domain_validate.c:184:9: error: comparison of different enumeration types
('virDomainVideoBackendType' and 'virDomainInputType')
[-Werror,-Wenum-compare]
virReportEnumRangeError(virDomainInputType, video->backend);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../src/util/virerror.h:175:47: note: expanded from macro
'virReportEnumRangeError'
(__typeof__(value))1 == (typname)1 && sizeof((typname)1)
!= 0 ? #typname : #typname)
~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~
Whereas with gcc I get more obscure error:
FAILED: src/conf/libvirt_conf.a.p/domain_validate.c.o
cc ...
In file included from ../src/util/virthread.h:25,
from ../src/util/virobject.h:25,
from ../src/conf/capabilities.h:28,
from ../src/conf/domain_conf.h:31,
from ../src/conf/domain_validate.h:25,
from ../src/conf/domain_validate.c:23:
../src/conf/domain_validate.c: In function ‘virDomainVideoDefValidate’:
../src/util/virerror.h:175:47: error: comparison between ‘enum <anonymous>’ and
‘enum <anonymous>’ [-Werror=enum-compare]
175 | (__typeof__(value))1 == (typname)1 &&
sizeof((typname)1) != 0 ? #typname : #typname)
| ^~
../src/conf/domain_validate.c:184:9: note: in expansion of macro
‘virReportEnumRangeError’
184 | virReportEnumRangeError(virDomainInputType, video->backend);
| ^~~~~~~~~~~~~~~~~~~~~~~
But I guess this is also okay-ish - it reports there is a problem with
code.
NB this only works with variables that are of a certain enum types. It
does not work with those generic 'int var /* someEnum */' declarations,
understandably. But as we use virXMLPropEnum() more, we can declare
variables of their true type (enum) and get more and more calls to
virReportEnumRangeError() checked.
Michal Prívozník (4):
virnetdevvportprofile: Turn virNetDevVPortProfileLinkOp enum into a
proper typedef
virNetDevVPortProfileOp8021Qbh: Use proper type in
virReportEnumRangeError()
virDomainVideoDefValidate: Use proper type in
virReportEnumRangeError()
virerror: Make virReportEnumRangeError() check for type mismatch
src/conf/domain_validate.c | 2 +-
src/util/virerror.h | 14 ++++++++------
src/util/virnetdevvportprofile.c | 10 +++++-----
3 files changed, 14 insertions(+), 12 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
450
days inactive
451
days old
5 comments
2 participants
participants (2)
-
Ján Tomko -
Michal Privoznik