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>
> ---
> 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
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|