Convenience function to return value of a yes / no attribute.

Does not use virTristateBoolTypeFromString to disallow "default".

Signed-off-by: Tim Wiederhake <twiederh@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;
+
+    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;
+}
+
+
 /**
  * virXPathBoolean:
  * @xpath: the XPath string to evaluate
diff --git a/src/util/virxml.h b/src/util/virxml.h
index d32f77b867..072948b3e2 100644
--- a/src/util/virxml.h
+++ b/src/util/virxml.h
@@ -28,6 +28,7 @@
 #include <libxml/relaxng.h>
 
 #include "virbuffer.h"
+#include "virenum.h"
 
 xmlXPathContextPtr virXMLXPathContextNew(xmlDocPtr xml)
     G_GNUC_WARN_UNUSED_RESULT;
@@ -77,6 +78,9 @@ char *     virXMLPropStringLimit(xmlNodePtr node,
                                  const char *name,
                                  size_t maxlen);
 char *   virXMLNodeContentString(xmlNodePtr node);
+int              virXMLPropYesNo(xmlNodePtr node,
+                                 const char *name,
+                                 virTristateBool *value);
 
 /* Internal function; prefer the macros below.  */
 xmlDocPtr      virXMLParseHelper(int domcode,
-- 
2.26.2

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@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;
> +
> +    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

On Thu, Mar 18, 2021 at 04:03:17PM +0100, Michal Privoznik wrote:
> 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@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)

I kind of feel this should be called

   virXMLPropTristateBool


and the next patch virXMLPropTristateSwitch, so that they
both match their output variable types.

> > +{
> > +    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;
> }

Yep, I think we really should do this, not open code the same
thing.


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 :|

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