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

[Ján Tomko]

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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180606/f245ba33/attachment-0001.sig>