[libvirt PATCH v3 16/51] virxml: Add virXMLPropTristateSwitch

[Tim Wiederhake]

On Mon, 2021-03-22 at 12:31 +0000, Daniel P. Berrangé wrote:
> On Mon, Mar 22, 2021 at 01:23:28PM +0100, Michal Privoznik wrote:
> > On 3/19/21 4:57 PM, Tim Wiederhake wrote:
> > > Convenience function to return value of an on / off attribute.
> > > 
> > > Signed-off-by: Tim Wiederhake <twiederh at redhat.com>
> > > ---
> > >   src/libvirt_private.syms |  1 +
> > >   src/util/virxml.c        | 41
> > > ++++++++++++++++++++++++++++++++++++++++
> > >   src/util/virxml.h        |  6 +++++-
> > >   3 files changed, 47 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/src/util/virxml.h b/src/util/virxml.h
> > > index 3041c37df3..e844cb0713 100644
> > > --- a/src/util/virxml.h
> > > +++ b/src/util/virxml.h
> > > @@ -80,8 +80,12 @@ char *     virXMLPropStringLimit(xmlNodePtr
> > > node,
> > >   char *   virXMLNodeContentString(xmlNodePtr node);
> > >   int       virXMLPropTristateBool(xmlNodePtr node,
> > >                                    const char *name,
> > > -                                 bool mandatory,
> > > +                                 bool required,
> > >                                    virTristateBool *result);
> > 
> > I guess this doesn't belong here. I'll squash it into the previous
> > patch.
> 
> I wonder if we should avoid the boolean arg entirely though.
> Looking at a call
> 
>   if (virXMLPropTristateBool(node, "foo", true, &result) < 0)
> 
> you have no idea whether "true" means mandatory or optional unless
> you're very familiar with the code. In GTK/GNOME they generally
> discourage use of bool parameters in favour of enums for this reason.
> eg they would have a VIR_XML_TRISTATE_REQUIRED enum flag.
> 
> This is quite verbose though, so perhaps we can just use trivial
> wrapper methods:
> 
>    virXMLPropTristateRequiredBool
>    virXMLPropTristateOptionalBool
> 
> Regards,
> Daniel

I agree that having a couple of bool arguments is not perfect, and my
first (unpublished and proof-of-concept) version had one function per
variant.

The problem with this approach becomes apparent in the yet-to-be-
published series that refactors reading integer attributes. In addition
to the "required or optional" variant, these also have "allow two's
complement wraparound for unsigned int or not" variants and "zero is a
valid number or not" variants.

Having dedicated enums for this might be an option, especially as I
believe I will be able to reuse e.g. the enum for "attribute is
required or not". What is your opinion on using bools for the time
being, see how the refactoring for "int attributes" and "enum
attributes" turns out, and then revisiting this issue?

Regards,
Tim