Re: [libvirt PATCH 02/38] virxml: Add virXMLPropYesNo

[Pavel Hrdina]

On Thu, Mar 18, 2021 at 09:00:41AM +0100, Tim Wiederhake wrote:
> Convenience function to return value of a yes / no attribute.
> 
> Does not use virTristateBoolTypeFromString to disallow "default".
> 
> Signed-off-by: Tim Wiederhake <twied...@redhat.com>
> ---
>  src/util/virxml.c | 37 +++++++++++++++++++++++++++++++++++++
>  src/util/virxml.h |  4 ++++
>  2 files changed, 41 insertions(+)
> 
> diff --git a/src/util/virxml.c b/src/util/virxml.c
> index 060b7530fc..47e8414bd5 100644
> --- a/src/util/virxml.c
> +++ b/src/util/virxml.c
> @@ -556,6 +556,43 @@ virXMLNodeContentString(xmlNodePtr node)
>  }
>  
>  
> +/**
> + * virXMLPropYesNo:
> + * @node: XML dom node pointer
> + * @name: Name of the property (attribute) to get
> + * @value: The returned virTristateBool value
> + *
> + * Convenience function to return value of a yes / no attribute.
> + *
> + * Returns 1 in case of success in which case @value is set,
> + *         or 0 if the attribute is not present,
> + *         or -1 and reports an error on failure.
> + */
> +int
> +virXMLPropYesNo(xmlNodePtr node, const char* name, virTristateBool *value)
> +{
> +    g_autofree char *tmp = virXMLPropString(node, name);
> +
> +    if (!tmp)
> +        return 0;
> +

I was wondering if we can return -1 or 0 only by adding addition
argument to the function, for example "bool mandatory".

The condition here could be changed to this:

    if (!tmp) {
        if (mandatory) {
            virReportError(VIR_ERR_XML_ERROR,
                           _(Missing mandatory attribute '%s' in element '%s'.),
                           name, node->name);

            return -1;
        }

        return 0;
    }

This would unify our error message for all the yes/no on/off options as
well.

> +    if (STREQ("yes", tmp)) {
> +        *value = VIR_TRISTATE_BOOL_YES;
> +        return 1;
> +    }
> +
> +    if (STREQ("no", tmp)) {
> +        *value = VIR_TRISTATE_BOOL_NO;
> +        return 1;
> +    }
> +
> +    virReportError(VIR_ERR_XML_ERROR,
> +                   _("Invalid value for attribute '%s' in node '%s': '%s'. 
> Expected 'yes' or 'no'"),

s/node/element/ ? We should unify our terminology and I'm not sure if we
prefer element or node.

Pavel

signature.asc
Description: PGP signature