1. Patchew
  2. Libvirt
  3. View series

[libvirt] [PATCH] Don't use enums in TPM struct fields

Daniel P. Berrangé posted 1 patch 6 years, 11 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20180606164700.7176-1-berrange@redhat.com
Test syntax-check passed
src/conf/domain_conf.c | 4 ++--
src/conf/domain_conf.h | 6 +++---
2 files changed, 5 insertions(+), 5 deletions(-)
[libvirt] [PATCH] Don't use enums in TPM struct fields
Posted by Daniel P. Berrangé 6 years, 11 months ago 
When using an enum in a struct field, the compiler is free to decide to
make it an unsigned type if it desires. This in turn leads to bugs when
code does

    if ((def->foo = virDomainFooTypeFromString(str)) < 0)
       ...

because 'def->foo' can't technically have an unsigned value from the
compiler's POV. While it is possible to add (int) casts in the code
example above, this is not desirable because it is easy to miss out
such casts. eg the code fixed here caused an error with clang builds

../../src/conf/domain_conf.c:12838:73: error: comparison of unsigned enum expression < 0 is always false [-Werror,-Wtautological-compare]
        if ((def->version = virDomainTPMVersionTypeFromString(version)) < 0) {
            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 src/conf/domain_conf.c | 4 ++--
 src/conf/domain_conf.h | 6 +++---
 2 files changed, 5 insertions(+), 5 deletions(-)

Pushed as a FreeBSD build fix

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index c1f2583c29..5be773cda4 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -12795,7 +12795,7 @@ virDomainTPMDefParseXML(virDomainXMLOptionPtr xmlopt,
 
     model = virXMLPropString(node, "model");
     if (model != NULL &&
-        (int)(def->model = virDomainTPMModelTypeFromString(model)) < 0) {
+        (def->model = virDomainTPMModelTypeFromString(model)) < 0) {
         virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
                        _("Unknown TPM frontend model '%s'"), model);
         goto error;
@@ -12824,7 +12824,7 @@ virDomainTPMDefParseXML(virDomainXMLOptionPtr xmlopt,
         goto error;
     }
 
-    if ((int)(def->type = virDomainTPMBackendTypeFromString(backend)) < 0) {
+    if ((def->type = virDomainTPMBackendTypeFromString(backend)) < 0) {
         virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
                        _("Unknown TPM backend type '%s'"),
                        backend);
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 5f8960d90b..8a8121bf83 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1307,10 +1307,10 @@ typedef enum {
 # define VIR_DOMAIN_TPM_DEFAULT_DEVICE "/dev/tpm0"
 
 struct _virDomainTPMDef {
-    virDomainTPMBackendType type;
+    int type; /* virDomainTPMBackendType */
     virDomainDeviceInfo info;
-    virDomainTPMModel model;
-    virDomainTPMVersion version;
+    int model; /* virDomainTPMModel */
+    int version; /* virDomainTPMVersion */
     union {
         struct {
             virDomainChrSourceDef source;
-- 
2.17.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Don't use enums in TPM struct fields
Posted by Ján Tomko 6 years, 11 months ago 
On Wed, Jun 06, 2018 at 05:47:00PM +0100, Daniel P. Berrangé wrote:
>When using an enum in a struct field,

or anywhere else

>the compiler is free to decide to
>make it an unsigned type if it desires. This in turn leads to bugs when
>code does
>
>    if ((def->foo = virDomainFooTypeFromString(str)) < 0)
>       ...
>
>because 'def->foo' can't technically have an unsigned value from the
>compiler's POV. While it is possible to add (int) casts in the code
>example above,

What I proposed during review was assigning the result to an int
variable before filling the version:
https://www.redhat.com/archives/libvir-list/2018-June/msg00114.html

>this is not desirable because it is easy to miss out
>such casts. eg the code fixed here caused an error with clang builds

If 'getting a compiler warning' means easy to miss, then we should not
have bothered with all those switch enumerations and virEnumRangeError
stuff.

>
>../../src/conf/domain_conf.c:12838:73: error: comparison of unsigned enum expression < 0 is always false [-Werror,-Wtautological-compare]
>        if ((def->version = virDomainTPMVersionTypeFromString(version)) < 0) {
>            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~
>
>Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>---
> src/conf/domain_conf.c | 4 ++--
> src/conf/domain_conf.h | 6 +++---
> 2 files changed, 5 insertions(+), 5 deletions(-)
>
>Pushed as a FreeBSD build fix
>

[... lots of lines that aren't required to fix the build ;) ...]

>     virDomainDeviceInfo info;
>-    virDomainTPMModel model;
>-    virDomainTPMVersion version;
>+    int model; /* virDomainTPMModel */
>+    int version; /* virDomainTPMVersion */

This deprives us of the -Wswitch-enum warning on all compilers
because some don't detect the bogus negative value comparison.
And the comment has even less power than the clang warning. So:

1. Is it actually worth the trouble to store enum values in
   typedef'd enums?
2. If so, can we make TypeFromString usage less cumbersome?

The fact that the compiler can choose different types rules out
returning the parsed value via a pointer.

A macro cannot both return a value and declare the temporary int
value (can it?), so doing:

if (typeFromString(def->version, virDomainTPMVersion, version) < 0)

won't work either.


I can imagine separating the check and the conversion:

if (!virDomainTPMVersionTypeCheck(version)) {
    virReportError();
    goto cleanup;
}
def->version = virDomainTPMVersionTypeFromString(version)

But not even clang will catch the missing check and it would go
through the array twice.


Alternatively, we can hide some control flow into the macro, which
will look ugly in the middle of a function:

#define VIR_ENUM_PARSE_GOTO(ret, name, type, label)
do {
    int val = name ## TypeFromString(type);

    if (val < 0) {
        virReportError(VIR_ERR_XML_ERROR, "generic parse error");
        goto label;
    }
    ret = val;
} while(0);

VIR_ENUM_PARSE_GOTO(def->version, virDomainTPMVersion, version, cleanup);

Hopefully someone has a better idea.

Jano
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Don't use enums in TPM struct fields
Posted by Daniel P. Berrangé 6 years, 11 months ago 
On Wed, Jun 06, 2018 at 08:32:39PM +0200, Ján Tomko wrote:
> This deprives us of the -Wswitch-enum warning on all compilers
> because some don't detect the bogus negative value comparison.
> And the comment has even less power than the clang warning. So:
> 
> 1. Is it actually worth the trouble to store enum values in
>   typedef'd enums?
> 2. If so, can we make TypeFromString usage less cumbersome?
> 
> The fact that the compiler can choose different types rules out
> returning the parsed value via a pointer.

Actually that is not a problem - the TypeToString methods are
autogenerated from a macro, so we can just make the macro
use the correct typename and adds a cast to deal with the signed
vs unsignedness of the value.

diff --git a/src/util/virutil.c b/src/util/virutil.c
index a908422feb..d8adc57931 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -444,15 +444,20 @@ virParseVersionString(const char *str, unsigned long *version,
 
 int virEnumFromString(const char *const*types,
                       unsigned int ntypes,
-                      const char *type)
+                      const char *type,
+                      int *val)
 {
     size_t i;
+    *val = 0;
     if (!type)
         return -1;
 
-    for (i = 0; i < ntypes; i++)
-        if (STREQ(types[i], type))
-            return i;
+    for (i = 0; i < ntypes; i++) {
+        if (STREQ(types[i], type)) {
+            *val = i;
+            return 0;
+        }
+    }
 
     return -1;
 }
diff --git a/src/util/virutil.h b/src/util/virutil.h
index 1ba9635bd9..7fea6c3a92 100644
--- a/src/util/virutil.h
+++ b/src/util/virutil.h
@@ -90,10 +90,11 @@ const char *virEnumToString(const char *const*types,
                                ARRAY_CARDINALITY(name ## TypeList), \
                                type); \
     } \
-    int name ## TypeFromString(const char *type) { \
+    int name ## TypeFromString(const char *type, name *val) {   \
         return virEnumFromString(name ## TypeList, \
                                  ARRAY_CARDINALITY(name ## TypeList), \
-                                 type); \
+                                 type,                                \
+                                 (int *)val);                         \
     }
 
 # define VIR_ENUM_DECL(name) \



Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Don't use enums in TPM struct fields
Posted by Daniel P. Berrangé 6 years, 11 months ago 
On Wed, Jun 06, 2018 at 08:32:39PM +0200, Ján Tomko wrote:
> This deprives us of the -Wswitch-enum warning on all compilers
> because some don't detect the bogus negative value comparison.
> And the comment has even less power than the clang warning. So:
> 
> 1. Is it actually worth the trouble to store enum values in
>   typedef'd enums?
> 2. If so, can we make TypeFromString usage less cumbersome?

We could add a explicit

   VIR_XXXXX_INVALID = -1,

entry to every single enum, which will force the compiler to
always use a signed int for representing the enum.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Don't use enums in TPM struct fields
Posted by Peter Krempa 6 years, 11 months ago 
On Thu, Jun 07, 2018 at 09:15:06 +0100, Daniel Berrange wrote:
> On Wed, Jun 06, 2018 at 08:32:39PM +0200, Ján Tomko wrote:
> > This deprives us of the -Wswitch-enum warning on all compilers
> > because some don't detect the bogus negative value comparison.
> > And the comment has even less power than the clang warning. So:
> > 
> > 1. Is it actually worth the trouble to store enum values in
> >   typedef'd enums?
> > 2. If so, can we make TypeFromString usage less cumbersome?
> 
> We could add a explicit
> 
>    VIR_XXXXX_INVALID = -1,
> 
> entry to every single enum, which will force the compiler to
> always use a signed int for representing the enum.

That would force us to add that to all switch statements with the
correct type, which seems a waste.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Don't use enums in TPM struct fields
Posted by Daniel P. Berrangé 6 years, 11 months ago 
On Thu, Jun 07, 2018 at 10:25:11AM +0200, Peter Krempa wrote:
> On Thu, Jun 07, 2018 at 09:15:06 +0100, Daniel Berrange wrote:
> > On Wed, Jun 06, 2018 at 08:32:39PM +0200, Ján Tomko wrote:
> > > This deprives us of the -Wswitch-enum warning on all compilers
> > > because some don't detect the bogus negative value comparison.
> > > And the comment has even less power than the clang warning. So:
> > > 
> > > 1. Is it actually worth the trouble to store enum values in
> > >   typedef'd enums?
> > > 2. If so, can we make TypeFromString usage less cumbersome?
> > 
> > We could add a explicit
> > 
> >    VIR_XXXXX_INVALID = -1,
> > 
> > entry to every single enum, which will force the compiler to
> > always use a signed int for representing the enum.
> 
> That would force us to add that to all switch statements with the
> correct type, which seems a waste.

Yeah, that would be annoying, though no different to the need to add
_LAST to every switch too.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-8c64711

© 2016 - 2025 Red Hat, Inc.