[libvirt PATCH v2 09/51] conf: Use virTristateXXX in virDomainMemballoonDef

[Peter Krempa]

On Fri, Mar 19, 2021 at 15:16:49 +0100, Tim Wiederhake wrote:
> On Fri, 2021-03-19 at 15:00 +0100, Peter Krempa wrote:
> > On Fri, Mar 19, 2021 at 14:40:23 +0100, Tim Wiederhake wrote:
> > > Signed-off-by: Tim Wiederhake <twiederh at redhat.com>
> > > ---
> > >  src/conf/domain_conf.h | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> > > index 09b697432d..2d342effb1 100644
> > > --- a/src/conf/domain_conf.h
> > > +++ b/src/conf/domain_conf.h
> > > @@ -1921,8 +1921,8 @@ struct _virDomainMemballoonDef {
> > >      int model;
> > >      virDomainDeviceInfo info;
> > >      int period; /* seconds between collections */
> > > -    int autodeflate; /* enum virTristateSwitch */
> > > -    int free_page_reporting; /* enum virTristateSwitch */
> > > +    virTristateSwitch autodeflate;
> > > +    virTristateSwitch free_page_reporting;
> > 
> > Sorry to jump in in the middle of the series without the intention to
> > continue review, but this is wong, when coupled with the code
> > assigning
> > to the enum value directly from virTristateSwitchTypeFromString:
> > 
> > Such as:
> > 
> > (def->autodeflate = virTristateSwitchTypeFromString(deflate)) <= 0)
> > 
> > virTristateSwitch is an enum value, thus treated as an unsigned
> > value,
> > while here you compare it with a signed value.
> > 
> > Comparing it with a negative value doesn't work.
> > 
> > Here the compiler doesn't moan as it's a <= 0 check, but if you
> > change
> > it to < you get a compiler warning that the statement is a
> > contradiction:
> > 
> > ../../../libvirt/src/conf/domain_conf.c:14520:71: error: result of
> > comparison of unsigned enum expression < 0 is always false [-Werror,-
> > Wtautological-unsigned-enum-zero-compare]
> >         (def->autodeflate = virTristateSwitchTypeFromString(deflate))
> > < 0) {
> >         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > ^ ~
> > 
> > 
> > As of such, return value of virTristateSwitchTypeFromString must
> > never
> > be directly assigned to a variable of virTristateSwitch type.
> > 
> 
> Ah, thanks, I thought I caught all of those (see e.g. 
> https://listman.redhat.com/archives/libvir-list/2021-March/msg01006.html
> ).
> 
> Will double check, and to your all delight, resend the entire series.

Well, in that case you must reorder the series first or squash
appropriate commits , because here at 9/51 in this series this is
introducing a bug, which only gets fixed later in

[libvirt PATCH v2 36/51] domain_conf: Use virXMLPropTristateXXX
in virDomainMemballoonDefParseXML