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