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@xxxxxxxxxx>
> > > ---
> > > 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
