6:19 a.m.
This series lays the groundwork for replacing some recurring boilerplate code
in src/conf/ regarding the extraction of XML attribute values.
For an on / off attribute, the boilerplate code looks roughly like this,
g_autofree char *str = NULL;
if (str = virXMLPropString(node, "x")) {
int val;
if ((val = virTristateBoolTypeFromString(str)) <= 0) {
virReportError(...)
return -1;
}
def->x = val;
}
with some variations regarding how `str` is free'd in case of later re-use,
the exact error message for invalid values, whether or not
`VIR_TRISTATE_(SWITCH|BOOL)_ABSENT` is explicitly checked for (`val < 0` vs.
`val <= 0`), whether an intermediate variable is used or the value is assigned
directly, and in some cases the conditions in the two if-blocks are merged.
After the refactoring, the above code block looks like this:
if (virXMLPropTristateBool(node, "x", VIR_XML_PROP_OPTIONAL, &def->x)
< 0)
return -1;
Similar functions are introduced for integer valued attributes, unsigned
integer valued attributes and enum valued attributes.
Patches #6, #7, and #8 demonstrate the application of these function and stand
representative of more patches that I did not sent along yet as to not drown
the mailing list in spam. These patches remove a total of ~ 1000 lines of code
and fix some places, where e.g. `<foo bar="default"/>` would incorrectly
be
accepted as virXMLTristateBool.
As an added benefit, this refactoring makes the error messages for invalid
values in these XML attributes much more consistent:
$ git diff master | grep "^-.*_(\"" | wc -l
239
$ git diff master | grep "^+.*_(\"" | wc -l
21
V1: https://listman.redhat.com/archives/libvir-list/2021-March/msg00853.html
V2: https://listman.redhat.com/archives/libvir-list/2021-March/msg00994.html
V3: https://listman.redhat.com/archives/libvir-list/2021-March/msg01066.html
V4: https://listman.redhat.com/archives/libvir-list/2021-April/msg00209.html
Changes since V4:
* Added custom error message for virXMLProp(Int|UInt) when the attribute value
is 0 and VIR_XML_PROP_NONZERO is specified.
Cheers,
Tim
Tim Wiederhake (8):
virxml: Add virXMLPropTristateBool
virxml: Add virXMLPropTristateSwitch
virxml: Add virXMLPropInt
virxml: Add virXMLPropUInt
virxml: Add virXMLPropEnum
virNetworkForwardNatDefParseXML: Use virXMLProp*
virDomainIOThreadIDDefParseXML: Use virXMLProp*
virCPUDefParseXML: Use virXMLProp*
src/conf/cpu_conf.c | 14 +-
src/conf/domain_conf.c | 14 +-
src/conf/network_conf.c | 16 +--
src/libvirt_private.syms | 5 +
src/util/virxml.c | 267 +++++++++++++++++++++++++++++++++++++++
src/util/virxml.h | 31 +++++
6 files changed, 312 insertions(+), 35 deletions(-)
--
2.26.2
6:19 a.m.
New subject: [libvirt PATCH v5 1/8] virxml: Add virXMLPropTristateBool
Convenience function to return the value of a yes / no XML attribute.
Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com>
---
src/libvirt_private.syms | 1 +
src/util/virxml.c | 49 ++++++++++++++++++++++++++++++++++++++++
src/util/virxml.h | 10 ++++++++
3 files changed, 60 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index cb9fe7c80a..1aac0de1ae 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -3546,6 +3546,7 @@ virXMLParseHelper;
virXMLPickShellSafeComment;
virXMLPropString;
virXMLPropStringLimit;
+virXMLPropTristateBool;
virXMLSaveFile;
virXMLValidateAgainstSchema;
virXMLValidatorFree;
diff --git a/src/util/virxml.c b/src/util/virxml.c
index 4a6fe09468..0b822d7c4d 100644
--- a/src/util/virxml.c
+++ b/src/util/virxml.c
@@ -558,6 +558,55 @@ virXMLNodeContentString(xmlNodePtr node)
}
+/**
+ * virXMLPropTristateBool:
+ * @node: XML dom node pointer
+ * @name: Name of the property (attribute) to get
+ * @flags: Bitwise or of virXMLPropFlags
+ * @result: The returned value
+ *
+ * Convenience function to return value of a yes / no attribute.
+ *
+ * Returns 1 in case of success in which case @result is set,
+ * or 0 if the attribute is not present,
+ * or -1 and reports an error on failure.
+ */
+int
+virXMLPropTristateBool(xmlNodePtr node, const char* name,
+ virXMLPropFlags flags, virTristateBool *result)
+{
+ g_autofree char *tmp = NULL;
+ int val;
+
+ if (!node || !name || !result) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Invalid argument to %s"),
+ __FUNCTION__);
+ return -1;
+ }
+
+ if (!(tmp = virXMLPropString(node, name))) {
+ if ((flags & VIR_XML_PROP_REQUIRED) != VIR_XML_PROP_REQUIRED)
+ return 0;
+
+ virReportError(VIR_ERR_XML_ERROR,
+ _("Missing required attribute '%s' in element
'%s'"),
+ name, node->name);
+ return -1;
+ }
+
+ if ((val = virTristateBoolTypeFromString(tmp)) <= 0) {
+ virReportError(VIR_ERR_XML_ERROR,
+ _("Invalid value for attribute '%s' in element
'%s': '%s'. Expected 'yes' or 'no'"),
+ name, node->name, tmp);
+ return -1;
+ }
+
+ *result = val;
+ return 1;
+}
+
+
/**
* virXPathBoolean:
* @xpath: the XPath string to evaluate
diff --git a/src/util/virxml.h b/src/util/virxml.h
index d32f77b867..53de416a7f 100644
--- a/src/util/virxml.h
+++ b/src/util/virxml.h
@@ -28,10 +28,16 @@
#include <libxml/relaxng.h>
#include "virbuffer.h"
+#include "virenum.h"
xmlXPathContextPtr virXMLXPathContextNew(xmlDocPtr xml)
G_GNUC_WARN_UNUSED_RESULT;
+typedef enum {
+ VIR_XML_PROP_OPTIONAL = 0, /* Attribute may be absent */
+ VIR_XML_PROP_REQUIRED = 1 << 0, /* Attribute may not be absent */
+} virXMLPropFlags;
+
int virXPathBoolean(const char *xpath,
xmlXPathContextPtr ctxt);
char * virXPathString(const char *xpath,
@@ -77,6 +83,10 @@ char * virXMLPropStringLimit(xmlNodePtr node,
const char *name,
size_t maxlen);
char * virXMLNodeContentString(xmlNodePtr node);
+int virXMLPropTristateBool(xmlNodePtr node,
+ const char *name,
+ virXMLPropFlags flags,
+ virTristateBool *result);
/* Internal function; prefer the macros below. */
xmlDocPtr virXMLParseHelper(int domcode,
--
2.26.2
2:33 a.m.
New subject: [libvirt PATCH v5 1/8] virxml: Add virXMLPropTristateBool
On Thu, Apr 08, 2021 at 13:19:51 +0200, Tim Wiederhake wrote:
Convenience function to return the value of a yes / no XML
attribute.
Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com>
---
src/libvirt_private.syms | 1 +
src/util/virxml.c | 49 ++++++++++++++++++++++++++++++++++++++++
src/util/virxml.h | 10 ++++++++
3 files changed, 60 insertions(+)
Oops, I started reviewing v4 instead of v5. If you follow my comments on
v4:
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
3:36 a.m.
New subject: [libvirt PATCH v5 1/8] virxml: Add virXMLPropTristateBool
On Thu, Apr 08, 2021 at 13:19:51 +0200, Tim Wiederhake wrote:
Convenience function to return the value of a yes / no XML
attribute.
Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com>
---
src/libvirt_private.syms | 1 +
src/util/virxml.c | 49 ++++++++++++++++++++++++++++++++++++++++
src/util/virxml.h | 10 ++++++++
3 files changed, 60 insertions(+)
[...]
diff --git a/src/util/virxml.c b/src/util/virxml.c
index 4a6fe09468..0b822d7c4d 100644
diff --git a/src/util/virxml.h b/src/util/virxml.h
index d32f77b867..53de416a7f 100644
--- a/src/util/virxml.h
+++ b/src/util/virxml.h
@@ -28,10 +28,16 @@
#include <libxml/relaxng.h>
#include "virbuffer.h"
+#include "virenum.h"
xmlXPathContextPtr virXMLXPathContextNew(xmlDocPtr xml)
G_GNUC_WARN_UNUSED_RESULT;
+typedef enum {
+ VIR_XML_PROP_OPTIONAL = 0, /* Attribute may be absent */
+ VIR_XML_PROP_REQUIRED = 1 << 0, /* Attribute may not be absent */
+} virXMLPropFlags;
+
int virXPathBoolean(const char *xpath,
xmlXPathContextPtr ctxt);
char * virXPathString(const char *xpath,
@@ -77,6 +83,10 @@ char * virXMLPropStringLimit(xmlNodePtr node,
const char *name,
size_t maxlen);
char * virXMLNodeContentString(xmlNodePtr node);
+int virXMLPropTristateBool(xmlNodePtr node,
+ const char *name,
+ virXMLPropFlags flags,
+ virTristateBool *result);
I've posted
https://listman.redhat.com/archives/libvir-list/2021-April/msg00659.html
to fix the terrible header formatting so that we don't just keep adding
more and more use.
6:19 a.m.
New subject: [libvirt PATCH v5 2/8] virxml: Add virXMLPropTristateSwitch
Convenience function to return the value of an on / off XML attribute.
Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com>
---
src/libvirt_private.syms | 1 +
src/util/virxml.c | 49 ++++++++++++++++++++++++++++++++++++++++
src/util/virxml.h | 4 ++++
3 files changed, 54 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 1aac0de1ae..776387e6b3 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -3547,6 +3547,7 @@ virXMLPickShellSafeComment;
virXMLPropString;
virXMLPropStringLimit;
virXMLPropTristateBool;
+virXMLPropTristateSwitch;
virXMLSaveFile;
virXMLValidateAgainstSchema;
virXMLValidatorFree;
diff --git a/src/util/virxml.c b/src/util/virxml.c
index 0b822d7c4d..f62c5c39c4 100644
--- a/src/util/virxml.c
+++ b/src/util/virxml.c
@@ -607,6 +607,55 @@ virXMLPropTristateBool(xmlNodePtr node, const char* name,
}
+/**
+ * virXMLPropTristateSwitch:
+ * @node: XML dom node pointer
+ * @name: Name of the property (attribute) to get
+ * @flags: Bitwise or of virXMLPropFlags
+ * @result: The returned value
+ *
+ * Convenience function to return value of an on / off attribute.
+ *
+ * Returns 1 in case of success in which case @result is set,
+ * or 0 if the attribute is not present,
+ * or -1 and reports an error on failure.
+ */
+int
+virXMLPropTristateSwitch(xmlNodePtr node, const char* name,
+ virXMLPropFlags flags, virTristateSwitch *result)
+{
+ g_autofree char *tmp = NULL;
+ int val;
+
+ if (!node || !name || !result) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Invalid argument to %s"),
+ __FUNCTION__);
+ return -1;
+ }
+
+ if (!(tmp = virXMLPropString(node, name))) {
+ if ((flags & VIR_XML_PROP_REQUIRED) != VIR_XML_PROP_REQUIRED)
+ return 0;
+
+ virReportError(VIR_ERR_XML_ERROR,
+ _("Missing required attribute '%s' in element
'%s'"),
+ name, node->name);
+ return -1;
+ }
+
+ if ((val = virTristateSwitchTypeFromString(tmp)) <= 0) {
+ virReportError(VIR_ERR_XML_ERROR,
+ _("Invalid value for attribute '%s' in element
'%s': '%s'. Expected 'on' or 'off'"),
+ name, node->name, tmp);
+ return -1;
+ }
+
+ *result = val;
+ return 1;
+}
+
+
/**
* virXPathBoolean:
* @xpath: the XPath string to evaluate
diff --git a/src/util/virxml.h b/src/util/virxml.h
index 53de416a7f..a5ecfbb01a 100644
--- a/src/util/virxml.h
+++ b/src/util/virxml.h
@@ -87,6 +87,10 @@ int virXMLPropTristateBool(xmlNodePtr node,
const char *name,
virXMLPropFlags flags,
virTristateBool *result);
+int virXMLPropTristateSwitch(xmlNodePtr node,
+ const char *name,
+ virXMLPropFlags flags,
+ virTristateSwitch *result);
/* Internal function; prefer the macros below. */
xmlDocPtr virXMLParseHelper(int domcode,
--
2.26.2
2:34 a.m.
New subject: [libvirt PATCH v5 2/8] virxml: Add virXMLPropTristateSwitch
On Thu, Apr 08, 2021 at 13:19:52 +0200, Tim Wiederhake wrote:
Convenience function to return the value of an on / off XML
attribute.
Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com>
---
src/libvirt_private.syms | 1 +
src/util/virxml.c | 49 ++++++++++++++++++++++++++++++++++++++++
src/util/virxml.h | 4 ++++
3 files changed, 54 insertions(+)
Same comments as on patch 1:
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
6:19 a.m.
New subject: [libvirt PATCH v5 3/8] virxml: Add virXMLPropInt
Convenience function to return the value of an integer XML attribute.
Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com>
---
src/libvirt_private.syms | 1 +
src/util/virxml.c | 57 ++++++++++++++++++++++++++++++++++++++++
src/util/virxml.h | 6 +++++
3 files changed, 64 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 776387e6b3..bdb63349ee 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -3544,6 +3544,7 @@ virXMLNodeSanitizeNamespaces;
virXMLNodeToString;
virXMLParseHelper;
virXMLPickShellSafeComment;
+virXMLPropInt;
virXMLPropString;
virXMLPropStringLimit;
virXMLPropTristateBool;
diff --git a/src/util/virxml.c b/src/util/virxml.c
index f62c5c39c4..bd6b75150e 100644
--- a/src/util/virxml.c
+++ b/src/util/virxml.c
@@ -656,6 +656,63 @@ virXMLPropTristateSwitch(xmlNodePtr node, const char* name,
}
+/**
+ * virXMLPropInt:
+ * @node: XML dom node pointer
+ * @name: Name of the property (attribute) to get
+ * @base: Number base, see strtol
+ * @flags: Bitwise or of virXMLPropFlags
+ * @result: The returned value
+ *
+ * Convenience function to return value of an integer attribute.
+ *
+ * Returns 1 in case of success in which case @result is set,
+ * or 0 if the attribute is not present,
+ * or -1 and reports an error on failure.
+ */
+int
+virXMLPropInt(xmlNodePtr node, const char *name, int base,
+ virXMLPropFlags flags, int *result)
+{
+ g_autofree char *tmp = NULL;
+ int ret;
+
+ if (!node || !name || !result) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Invalid argument to %s"),
+ __FUNCTION__);
+ return -1;
+ }
+
+ if (!(tmp = virXMLPropString(node, name))) {
+ if ((flags & VIR_XML_PROP_REQUIRED) != VIR_XML_PROP_REQUIRED)
+ return 0;
+
+ virReportError(VIR_ERR_XML_ERROR,
+ _("Missing required attribute '%s' in element
'%s'"),
+ name, node->name);
+ return -1;
+ }
+
+ ret = virStrToLong_i(tmp, NULL, base, result);
+
+ if (ret < 0) {
+ virReportError(VIR_ERR_XML_ERROR,
+ _("Invalid value for attribute '%s' in element
'%s': '%s'. Expected integer value"),
+ name, node->name, tmp);
+ return -1;
+ }
+
+ if ((flags & VIR_XML_PROP_NONZERO) && (*result == 0)) {
+ virReportError(VIR_ERR_XML_ERROR,
+ _("Invalid value for attribute '%s' in element
'%s': Zero is not permitted"),
+ name, node->name);
+ }
+
+ return 1;
+}
+
+
/**
* virXPathBoolean:
* @xpath: the XPath string to evaluate
diff --git a/src/util/virxml.h b/src/util/virxml.h
index a5ecfbb01a..aba863ae11 100644
--- a/src/util/virxml.h
+++ b/src/util/virxml.h
@@ -36,6 +36,7 @@ xmlXPathContextPtr virXMLXPathContextNew(xmlDocPtr xml)
typedef enum {
VIR_XML_PROP_OPTIONAL = 0, /* Attribute may be absent */
VIR_XML_PROP_REQUIRED = 1 << 0, /* Attribute may not be absent */
+ VIR_XML_PROP_NONZERO = 1 << 1, /* Attribute may not be zero */
} virXMLPropFlags;
int virXPathBoolean(const char *xpath,
@@ -91,6 +92,11 @@ int virXMLPropTristateSwitch(xmlNodePtr node,
const char *name,
virXMLPropFlags flags,
virTristateSwitch *result);
+int virXMLPropInt(xmlNodePtr node,
+ const char *name,
+ int base,
+ virXMLPropFlags flags,
+ int *result);
/* Internal function; prefer the macros below. */
xmlDocPtr virXMLParseHelper(int domcode,
--
2.26.2
2:38 a.m.
New subject: [libvirt PATCH v5 3/8] virxml: Add virXMLPropInt
On Thu, Apr 08, 2021 at 13:19:53 +0200, Tim Wiederhake wrote:
Convenience function to return the value of an integer XML
attribute.
Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com>
---
src/libvirt_private.syms | 1 +
src/util/virxml.c | 57 ++++++++++++++++++++++++++++++++++++++++
src/util/virxml.h | 6 +++++
3 files changed, 64 insertions(+)
[...]
diff --git a/src/util/virxml.c b/src/util/virxml.c
index f62c5c39c4..bd6b75150e 100644
--- a/src/util/virxml.c
+++ b/src/util/virxml.c
@@ -656,6 +656,63 @@ virXMLPropTristateSwitch(xmlNodePtr node, const char* name,
}
+/**
+ * virXMLPropInt:
+ * @node: XML dom node pointer
+ * @name: Name of the property (attribute) to get
+ * @base: Number base, see strtol
+ * @flags: Bitwise or of virXMLPropFlags
+ * @result: The returned value
+ *
+ * Convenience function to return value of an integer attribute.
+ *
+ * Returns 1 in case of success in which case @result is set,
+ * or 0 if the attribute is not present,
+ * or -1 and reports an error on failure.
Comment doesn't say what happens to @result on 0 or -1, but reading it
implies that @result is left untouched ...
+ */
+int
+virXMLPropInt(xmlNodePtr node, const char *name, int base,
+ virXMLPropFlags flags, int *result)
+{
+ g_autofree char *tmp = NULL;
+ int ret;
+
+ if (!node || !name || !result) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Invalid argument to %s"),
+ __FUNCTION__);
+ return -1;
+ }
+
+ if (!(tmp = virXMLPropString(node, name))) {
+ if ((flags & VIR_XML_PROP_REQUIRED) != VIR_XML_PROP_REQUIRED)
+ return 0;
Same general comments regarding checks and header formatting as in
previous instances.
+
+ virReportError(VIR_ERR_XML_ERROR,
+ _("Missing required attribute '%s' in element
'%s'"),
+ name, node->name);
+ return -1;
+ }
+
+ ret = virStrToLong_i(tmp, NULL, base, result);
+
+ if (ret < 0) {
+ virReportError(VIR_ERR_XML_ERROR,
+ _("Invalid value for attribute '%s' in element
'%s': '%s'. Expected integer value"),
+ name, node->name, tmp);
+ return -1;
+ }
+
+ if ((flags & VIR_XML_PROP_NONZERO) && (*result == 0)) {
+ virReportError(VIR_ERR_XML_ERROR,
+ _("Invalid value for attribute '%s' in element
'%s': Zero is not permitted"),
+ name, node->name);
'return -1;' missing. Also in this case @result is modified despite
returning -1 which breaks expectations from the comment.
+ }
+
+ return 1;
+}
+
+
/**
* virXPathBoolean:
* @xpath: the XPath string to evaluate
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
6:19 a.m.
New subject: [libvirt PATCH v5 4/8] virxml: Add virXMLPropUInt
Convenience function to return the value of an unsigned integer XML attribute.
Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com>
---
src/libvirt_private.syms | 1 +
src/util/virxml.c | 61 ++++++++++++++++++++++++++++++++++++++++
src/util/virxml.h | 6 ++++
3 files changed, 68 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index bdb63349ee..dc6c9191c8 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -3549,6 +3549,7 @@ virXMLPropString;
virXMLPropStringLimit;
virXMLPropTristateBool;
virXMLPropTristateSwitch;
+virXMLPropUInt;
virXMLSaveFile;
virXMLValidateAgainstSchema;
virXMLValidatorFree;
diff --git a/src/util/virxml.c b/src/util/virxml.c
index bd6b75150e..fba02900f5 100644
--- a/src/util/virxml.c
+++ b/src/util/virxml.c
@@ -713,6 +713,67 @@ virXMLPropInt(xmlNodePtr node, const char *name, int base,
}
+/**
+ * virXMLPropUInt:
+ * @node: XML dom node pointer
+ * @name: Name of the property (attribute) to get
+ * @base: Number base, see strtol
+ * @flags: Bitwise or of virXMLPropFlags
+ * @result: The returned value
+ *
+ * Convenience function to return value of an unsigned integer attribute.
+ *
+ * Returns 1 in case of success in which case @result is set,
+ * or 0 if the attribute is not present,
+ * or -1 and reports an error on failure.
+ */
+int
+virXMLPropUInt(xmlNodePtr node, const char* name, int base,
+ virXMLPropFlags flags, unsigned int *result)
+{
+ g_autofree char *tmp = NULL;
+ int ret;
+
+ if (!node || !name || !result) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Invalid argument to %s"),
+ __FUNCTION__);
+ return -1;
+ }
+
+ if (!(tmp = virXMLPropString(node, name))) {
+ if ((flags & VIR_XML_PROP_REQUIRED) != VIR_XML_PROP_REQUIRED)
+ return 0;
+
+ virReportError(VIR_ERR_XML_ERROR,
+ _("Missing required attribute '%s' in element
'%s'"),
+ name, node->name);
+ return -1;
+ }
+
+ if (flags & VIR_XML_PROP_WRAPNEGATIVE) {
+ ret = virStrToLong_ui(tmp, NULL, base, result);
+ } else {
+ ret = virStrToLong_uip(tmp, NULL, base, result);
+ }
+
+ if (ret < 0) {
+ virReportError(VIR_ERR_XML_ERROR,
+ _("Invalid value for attribute '%s' in element
'%s': '%s'. Expected integer value"),
+ name, node->name, tmp);
+ return -1;
+ }
+
+ if ((flags & VIR_XML_PROP_NONZERO) && (*result == 0)) {
+ virReportError(VIR_ERR_XML_ERROR,
+ _("Invalid value for attribute '%s' in element
'%s': Zero is not permitted"),
+ name, node->name);
+ }
+
+ return 1;
+}
+
+
/**
* virXPathBoolean:
* @xpath: the XPath string to evaluate
diff --git a/src/util/virxml.h b/src/util/virxml.h
index aba863ae11..82d5d16375 100644
--- a/src/util/virxml.h
+++ b/src/util/virxml.h
@@ -37,6 +37,7 @@ typedef enum {
VIR_XML_PROP_OPTIONAL = 0, /* Attribute may be absent */
VIR_XML_PROP_REQUIRED = 1 << 0, /* Attribute may not be absent */
VIR_XML_PROP_NONZERO = 1 << 1, /* Attribute may not be zero */
+ VIR_XML_PROP_WRAPNEGATIVE = 1 << 2, /* Wrap around negative values */
} virXMLPropFlags;
int virXPathBoolean(const char *xpath,
@@ -97,6 +98,11 @@ int virXMLPropInt(xmlNodePtr node,
int base,
virXMLPropFlags flags,
int *result);
+int virXMLPropUInt(xmlNodePtr node,
+ const char* name,
+ int base,
+ virXMLPropFlags flags,
+ unsigned int *result);
/* Internal function; prefer the macros below. */
xmlDocPtr virXMLParseHelper(int domcode,
--
2.26.2
2:41 a.m.
New subject: [libvirt PATCH v5 4/8] virxml: Add virXMLPropUInt
On Thu, Apr 08, 2021 at 13:19:54 +0200, Tim Wiederhake wrote:
Convenience function to return the value of an unsigned integer XML
attribute.
Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com>
---
src/libvirt_private.syms | 1 +
src/util/virxml.c | 61 ++++++++++++++++++++++++++++++++++++++++
src/util/virxml.h | 6 ++++
3 files changed, 68 insertions(+)
[...]
Observe same generic comments I had on 3/8.
diff --git a/src/util/virxml.c b/src/util/virxml.c
index bd6b75150e..fba02900f5 100644
--- a/src/util/virxml.c
+++ b/src/util/virxml.c
@@ -713,6 +713,67 @@ virXMLPropInt(xmlNodePtr node, const char *name, int base,
}
+/**
+ * virXMLPropUInt:
+ * @node: XML dom node pointer
+ * @name: Name of the property (attribute) to get
+ * @base: Number base, see strtol
+ * @flags: Bitwise or of virXMLPropFlags
+ * @result: The returned value
+ *
+ * Convenience function to return value of an unsigned integer attribute.
+ *
+ * Returns 1 in case of success in which case @result is set,
+ * or 0 if the attribute is not present,
+ * or -1 and reports an error on failure.
+ */
+int
+virXMLPropUInt(xmlNodePtr node, const char* name, int base,
+ virXMLPropFlags flags, unsigned int *result)
+{
+ g_autofree char *tmp = NULL;
+ int ret;
+
+ if (!node || !name || !result) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Invalid argument to %s"),
+ __FUNCTION__);
+ return -1;
+ }
+
+ if (!(tmp = virXMLPropString(node, name))) {
+ if ((flags & VIR_XML_PROP_REQUIRED) != VIR_XML_PROP_REQUIRED)
+ return 0;
+
+ virReportError(VIR_ERR_XML_ERROR,
+ _("Missing required attribute '%s' in element
'%s'"),
+ name, node->name);
+ return -1;
+ }
+
+ if (flags & VIR_XML_PROP_WRAPNEGATIVE) {
+ ret = virStrToLong_ui(tmp, NULL, base, result);
I find this case to be a very bad anti-feature ...
+ } else {
+ ret = virStrToLong_uip(tmp, NULL, base, result);
+ }
+
+ if (ret < 0) {
+ virReportError(VIR_ERR_XML_ERROR,
+ _("Invalid value for attribute '%s' in element
'%s': '%s'. Expected integer value"),
+ name, node->name, tmp);
+ return -1;
+ }
+
+ if ((flags & VIR_XML_PROP_NONZERO) && (*result == 0)) {
+ virReportError(VIR_ERR_XML_ERROR,
+ _("Invalid value for attribute '%s' in element
'%s': Zero is not permitted"),
+ name, node->name);
+ }
+
+ return 1;
+}
+
+
/**
* virXPathBoolean:
* @xpath: the XPath string to evaluate
diff --git a/src/util/virxml.h b/src/util/virxml.h
index aba863ae11..82d5d16375 100644
--- a/src/util/virxml.h
+++ b/src/util/virxml.h
@@ -37,6 +37,7 @@ typedef enum {
VIR_XML_PROP_OPTIONAL = 0, /* Attribute may be absent */
VIR_XML_PROP_REQUIRED = 1 << 0, /* Attribute may not be absent */
VIR_XML_PROP_NONZERO = 1 << 1, /* Attribute may not be zero */
+ VIR_XML_PROP_WRAPNEGATIVE = 1 << 2, /* Wrap around negative values */
So I'm not very happy to see it being made easy to be abused in the
future.
Under same conditions as 3/8:
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
6:19 a.m.
New subject: [libvirt PATCH v5 5/8] virxml: Add virXMLPropEnum
Convenience function to return the value of an enum XML attribute.
Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com>
---
src/libvirt_private.syms | 1 +
src/util/virxml.c | 51 ++++++++++++++++++++++++++++++++++++++++
src/util/virxml.h | 5 ++++
3 files changed, 57 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index dc6c9191c8..79eb4dba50 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -3544,6 +3544,7 @@ virXMLNodeSanitizeNamespaces;
virXMLNodeToString;
virXMLParseHelper;
virXMLPickShellSafeComment;
+virXMLPropEnum;
virXMLPropInt;
virXMLPropString;
virXMLPropStringLimit;
diff --git a/src/util/virxml.c b/src/util/virxml.c
index fba02900f5..8147266557 100644
--- a/src/util/virxml.c
+++ b/src/util/virxml.c
@@ -774,6 +774,57 @@ virXMLPropUInt(xmlNodePtr node, const char* name, int base,
}
+/**
+ * virXMLPropEnum:
+ * @node: XML dom node pointer
+ * @name: Name of the property (attribute) to get
+ * @strToInt: Conversion function to turn enum name to value. Expected to
+ * return negative value on failure.
+ * @flags: Bitwise or of virXMLPropFlags
+ * @result: The returned value
+ *
+ * Convenience function to return value of an enum attribute.
+ *
+ * Returns 1 in case of success in which case @result is set,
+ * or 0 if the attribute is not present,
+ * or -1 and reports an error on failure.
+ */
+int
+virXMLPropEnum(xmlNodePtr node, const char* name, int (*strToInt)(const char*),
+ virXMLPropFlags flags, unsigned int *result)
+{
+ g_autofree char *tmp = NULL;
+ int ret;
+
+ if (!node || !name || !strToInt || !result) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Invalid argument to %s"),
+ __FUNCTION__);
+ return -1;
+ }
+
+ if (!(tmp = virXMLPropString(node, name))) {
+ if ((flags & VIR_XML_PROP_REQUIRED) != VIR_XML_PROP_REQUIRED)
+ return 0;
+
+ virReportError(VIR_ERR_XML_ERROR,
+ _("Missing required attribute '%s' in element
'%s'"),
+ name, node->name);
+ return -1;
+ }
+
+ ret = strToInt(tmp);
+ if ((ret < 0) || ((flags & VIR_XML_PROP_NONZERO) && (ret == 0))) {
+ virReportError(VIR_ERR_XML_ERROR,
+ _("Invalid value for attribute '%s' in element
'%s': '%s'."),
+ name, node->name, NULLSTR(tmp));
+ return -1;
+ }
+
+ *result = ret;
+ return 0;
+}
+
/**
* virXPathBoolean:
* @xpath: the XPath string to evaluate
diff --git a/src/util/virxml.h b/src/util/virxml.h
index 82d5d16375..cbe4cc32dc 100644
--- a/src/util/virxml.h
+++ b/src/util/virxml.h
@@ -103,6 +103,11 @@ int virXMLPropUInt(xmlNodePtr node,
int base,
virXMLPropFlags flags,
unsigned int *result);
+int virXMLPropEnum(xmlNodePtr node,
+ const char* name,
+ int (*strToInt)(const char*),
+ virXMLPropFlags flags,
+ unsigned int *result);
/* Internal function; prefer the macros below. */
xmlDocPtr virXMLParseHelper(int domcode,
--
2.26.2
2:45 a.m.
New subject: [libvirt PATCH v5 5/8] virxml: Add virXMLPropEnum
On Thu, Apr 08, 2021 at 13:19:55 +0200, Tim Wiederhake wrote:
Convenience function to return the value of an enum XML attribute.
Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com>
---
src/libvirt_private.syms | 1 +
src/util/virxml.c | 51 ++++++++++++++++++++++++++++++++++++++++
src/util/virxml.h | 5 ++++
3 files changed, 57 insertions(+)
[...]
diff --git a/src/util/virxml.c b/src/util/virxml.c
index fba02900f5..8147266557 100644
--- a/src/util/virxml.c
+++ b/src/util/virxml.c
@@ -774,6 +774,57 @@ virXMLPropUInt(xmlNodePtr node, const char* name, int base,
}
+/**
+ * virXMLPropEnum:
+ * @node: XML dom node pointer
+ * @name: Name of the property (attribute) to get
+ * @strToInt: Conversion function to turn enum name to value. Expected to
+ * return negative value on failure.
+ * @flags: Bitwise or of virXMLPropFlags
+ * @result: The returned value
+ *
+ * Convenience function to return value of an enum attribute.
+ *
+ * Returns 1 in case of success in which case @result is set,
+ * or 0 if the attribute is not present,
+ * or -1 and reports an error on failure.
+ */
+int
+virXMLPropEnum(xmlNodePtr node, const char* name, int (*strToInt)(const char*),
+ virXMLPropFlags flags, unsigned int *result)
+{
+ g_autofree char *tmp = NULL;
+ int ret;
+
+ if (!node || !name || !strToInt || !result) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Invalid argument to %s"),
+ __FUNCTION__);
+ return -1;
+ }
+
+ if (!(tmp = virXMLPropString(node, name))) {
+ if ((flags & VIR_XML_PROP_REQUIRED) != VIR_XML_PROP_REQUIRED)
+ return 0;
+
+ virReportError(VIR_ERR_XML_ERROR,
+ _("Missing required attribute '%s' in element
'%s'"),
+ name, node->name);
+ return -1;
+ }
+
+ ret = strToInt(tmp);
+ if ((ret < 0) || ((flags & VIR_XML_PROP_NONZERO) && (ret == 0))) {
The parentheses in this condition make it confusing to read. I suggest
if (ret < 0 ||
((flags & VIR_XML_PROP_NONZERO) && (ret == 0))) {
+ virReportError(VIR_ERR_XML_ERROR,
+ _("Invalid value for attribute '%s' in element
'%s': '%s'."),
+ name, node->name, NULLSTR(tmp));
+ return -1;
+ }
+
+ *result = ret;
+ return 0;
+}
Obviously relevant comments from previous patches apply too:
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
6:19 a.m.
New subject: [libvirt PATCH v5 6/8] virNetworkForwardNatDefParseXML: Use virXMLProp*
Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com>
---
src/conf/network_conf.c | 16 +++-------------
1 file changed, 3 insertions(+), 13 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 4cf4aa4840..19b84cbdd1 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -1321,7 +1321,6 @@ virNetworkForwardNatDefParseXML(const char *networkName,
g_autofree xmlNodePtr *natPortNodes = NULL;
g_autofree char *addrStart = NULL;
g_autofree char *addrEnd = NULL;
- g_autofree char *ipv6 = NULL;
VIR_XPATH_NODE_AUTORESTORE(ctxt)
ctxt->node = node;
@@ -1333,18 +1332,9 @@ virNetworkForwardNatDefParseXML(const char *networkName,
return -1;
}
- ipv6 = virXMLPropString(node, "ipv6");
- if (ipv6) {
- int natIPv6;
- if ((natIPv6 = virTristateBoolTypeFromString(ipv6)) <= 0) {
- virReportError(VIR_ERR_XML_ERROR,
- _("Invalid ipv6 setting '%s' "
- "in network '%s' NAT"),
- ipv6, networkName);
- return -1;
- }
- def->natIPv6 = natIPv6;
- }
+ if (virXMLPropTristateBool(node, "ipv6", VIR_XML_PROP_OPTIONAL,
+ &def->natIPv6) < 0)
+ return -1;
/* addresses for SNAT */
nNatAddrs = virXPathNodeSet("./address", ctxt, &natAddrNodes);
--
2.26.2
2:46 a.m.
New subject: [libvirt PATCH v5 6/8] virNetworkForwardNatDefParseXML: Use virXMLProp*
On Thu, Apr 08, 2021 at 13:19:56 +0200, Tim Wiederhake wrote:
Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com>
---
src/conf/network_conf.c | 16 +++-------------
1 file changed, 3 insertions(+), 13 deletions(-)
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
6:19 a.m.
New subject: [libvirt PATCH v5 7/8] virDomainIOThreadIDDefParseXML: Use virXMLProp*
Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com>
---
src/conf/domain_conf.c | 14 +++-----------
1 file changed, 3 insertions(+), 11 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 1e72171586..fb22695dd3 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -18180,21 +18180,13 @@ static virDomainIOThreadIDDefPtr
virDomainIOThreadIDDefParseXML(xmlNodePtr node)
{
virDomainIOThreadIDDefPtr iothrid;
- g_autofree char *tmp = NULL;
iothrid = g_new0(virDomainIOThreadIDDef, 1);
- if (!(tmp = virXMLPropString(node, "id"))) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("Missing 'id' attribute in <iothread>
element"));
- goto error;
- }
- if (virStrToLong_uip(tmp, NULL, 10, &iothrid->iothread_id) < 0 ||
- iothrid->iothread_id == 0) {
- virReportError(VIR_ERR_XML_ERROR,
- _("invalid iothread 'id' value '%s'"),
tmp);
+ if (virXMLPropUInt(node, "id", 10,
+ VIR_XML_PROP_REQUIRED | VIR_XML_PROP_NONZERO,
+ &iothrid->iothread_id) < 0)
goto error;
- }
return iothrid;
--
2.26.2
2:47 a.m.
New subject: [libvirt PATCH v5 7/8] virDomainIOThreadIDDefParseXML: Use virXMLProp*
On Thu, Apr 08, 2021 at 13:19:57 +0200, Tim Wiederhake wrote:
Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com>
---
src/conf/domain_conf.c | 14 +++-----------
1 file changed, 3 insertions(+), 11 deletions(-)
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
6:19 a.m.
New subject: [libvirt PATCH v5 8/8] virCPUDefParseXML: Use virXMLProp*
Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com>
---
src/conf/cpu_conf.c | 14 +++-----------
1 file changed, 3 insertions(+), 11 deletions(-)
diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c
index 43629068c3..ee7feb1186 100644
--- a/src/conf/cpu_conf.c
+++ b/src/conf/cpu_conf.c
@@ -423,7 +423,6 @@ virCPUDefParseXML(xmlXPathContextPtr ctxt,
if (def->type == VIR_CPU_TYPE_GUEST) {
g_autofree char *match = virXMLPropString(ctxt->node, "match");
- g_autofree char *check = NULL;
if (match) {
def->match = virCPUMatchTypeFromString(match);
@@ -435,16 +434,9 @@ virCPUDefParseXML(xmlXPathContextPtr ctxt,
}
}
- if ((check = virXMLPropString(ctxt->node, "check"))) {
- int value = virCPUCheckTypeFromString(check);
- if (value < 0) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("Invalid check attribute for CPU "
- "specification"));
- return -1;
- }
- def->check = value;
- }
+ if (virXMLPropEnum(ctxt->node, "check", virCPUCheckTypeFromString,
+ VIR_XML_PROP_OPTIONAL, &def->check) < 0)
+ return -1;
}
if (def->type == VIR_CPU_TYPE_HOST) {
--
2.26.2
2:48 a.m.
New subject: [libvirt PATCH v5 8/8] virCPUDefParseXML: Use virXMLProp*
On Thu, Apr 08, 2021 at 13:19:58 +0200, Tim Wiederhake wrote:
Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com>
---
src/conf/cpu_conf.c | 14 +++-----------
1 file changed, 3 insertions(+), 11 deletions(-)
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
3:26 a.m.
polite ping
On Thu, 2021-04-08 at 13:19 +0200, Tim Wiederhake wrote:
This series lays the groundwork for replacing some recurring
boilerplate code
in src/conf/ regarding the extraction of XML attribute values.
For an on / off attribute, the boilerplate code looks roughly like
this,
g_autofree char *str = NULL;
if (str = virXMLPropString(node, "x")) {
int val;
if ((val = virTristateBoolTypeFromString(str)) <= 0) {
virReportError(...)
return -1;
}
def->x = val;
}
with some variations regarding how `str` is free'd in case of later
re-use,
the exact error message for invalid values, whether or not
`VIR_TRISTATE_(SWITCH|BOOL)_ABSENT` is explicitly checked for (`val <
0` vs.
`val <= 0`), whether an intermediate variable is used or the value is
assigned
directly, and in some cases the conditions in the two if-blocks are
merged.
After the refactoring, the above code block looks like this:
if (virXMLPropTristateBool(node, "x", VIR_XML_PROP_OPTIONAL, &def-
>x) < 0)
return -1;
Similar functions are introduced for integer valued attributes,
unsigned
integer valued attributes and enum valued attributes.
Patches #6, #7, and #8 demonstrate the application of these function
and stand
representative of more patches that I did not sent along yet as to
not drown
the mailing list in spam. These patches remove a total of ~ 1000
lines of code
and fix some places, where e.g. `<foo bar="default"/>` would
incorrectly be
accepted as virXMLTristateBool.
As an added benefit, this refactoring makes the error messages for
invalid
values in these XML attributes much more consistent:
$ git diff master | grep "^-.*_(\"" | wc -l
239
$ git diff master | grep "^+.*_(\"" | wc -l
21
V1:
https://listman.redhat.com/archives/libvir-list/2021-March/msg00853.html
V2:
https://listman.redhat.com/archives/libvir-list/2021-March/msg00994.html
V3:
https://listman.redhat.com/archives/libvir-list/2021-March/msg01066.html
V4:
https://listman.redhat.com/archives/libvir-list/2021-April/msg00209.html
Changes since V4:
* Added custom error message for virXMLProp(Int|UInt) when the
attribute value
is 0 and VIR_XML_PROP_NONZERO is specified.
Cheers,
Tim
Tim Wiederhake (8):
virxml: Add virXMLPropTristateBool
virxml: Add virXMLPropTristateSwitch
virxml: Add virXMLPropInt
virxml: Add virXMLPropUInt
virxml: Add virXMLPropEnum
virNetworkForwardNatDefParseXML: Use virXMLProp*
virDomainIOThreadIDDefParseXML: Use virXMLProp*
virCPUDefParseXML: Use virXMLProp*
src/conf/cpu_conf.c | 14 +-
src/conf/domain_conf.c | 14 +-
src/conf/network_conf.c | 16 +--
src/libvirt_private.syms | 5 +
src/util/virxml.c | 267
+++++++++++++++++++++++++++++++++++++++
src/util/virxml.h | 31 +++++
6 files changed, 312 insertions(+), 35 deletions(-)
--
2.26.2
1316
days inactive
1324
days old
18 comments
2 participants
participants (2)
-
Peter Krempa -
Tim Wiederhake