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

[Michal Privoznik <mprivozn@xxxxxxxxxx>]

On 3/18/21 9:00 AM, 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 <twiederh@xxxxxxxxxx>
> ---
>   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;
> +
> +    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'"),
> +                   name, node->name, tmp);
> +    return -1;


How about:

int
virXMLPropYesNo(xmlNodePtr node, const char* name, virTristateBool *value)
{
    g_autofree char *tmp = virXMLPropString(node, name);
    int val;

    if (!tmp)
        return 0;

    if ((val = virTristateBoolTypeFromString(tmp)) <= 0) {
        virReportError(VIR_ERR_XML_ERROR,
                       _("Invalid value for attribute '%s' in node 
'%s': '%s'. Expected 'yes' or 'no'"),
                       name, node->name, tmp);
        return -1;
    }

    *value = val;
    return 1;
}


"default" which is VIR_TRISTATE_BOOL_ABSENT is explicitly set to 0, so 
that things like g_new0() initialize the value to _ABSENT.

Also, the function should be listed in private symbols we export:

diff --git i/src/libvirt_private.syms w/src/libvirt_private.syms
index 526dcee11a..85bebca23a 100644
--- i/src/libvirt_private.syms
+++ w/src/libvirt_private.syms
@@ -3545,6 +3545,7 @@ virXMLParseHelper;
 virXMLPickShellSafeComment;
 virXMLPropString;
 virXMLPropStringLimit;
+virXMLPropYesNo;
 virXMLSaveFile;
 virXMLValidateAgainstSchema;
 virXMLValidatorFree;

I'm surprised linked did not run into any issues.

No need to resend, I can fix it before pushing. The similar applies to 
the next patch.

Michal