[PATCH 0/4] virerror: Make virReportEnumRangeError() check for type mismatch
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
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@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
The @virtPortOp variable inside of virNetDevVPortProfileOp8021Qbh is of type virNetDevVPortProfileLinkOp. Pass the proper type to virReportEnumRangeError(). Signed-off-by: Michal Privoznik <mprivozn@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
The @backend member of _virDomainVideoDef struct is of type virDomainVideoBackendType. Pass the proper type to virReportEnumRangeError(). Signed-off-by: Michal Privoznik <mprivozn@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
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@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
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@redhat.com> Jano
participants (2)
-
Ján Tomko -
Michal Privoznik