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

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(a)redhat.com&gt; > > --- > > 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