[libvirt PATCH v5 0/8] Refactor XML parsing boilerplate code
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
Convenience function to return the value of a yes / no XML attribute. Signed-off-by: Tim Wiederhake <twiederh@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
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@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@redhat.com>
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@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.
Convenience function to return the value of an on / off XML attribute. Signed-off-by: Tim Wiederhake <twiederh@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
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@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@redhat.com>
Convenience function to return the value of an integer XML attribute. Signed-off-by: Tim Wiederhake <twiederh@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
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@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@redhat.com>
Convenience function to return the value of an unsigned integer XML attribute. Signed-off-by: Tim Wiederhake <twiederh@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
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@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@redhat.com>
Convenience function to return the value of an enum XML attribute. Signed-off-by: Tim Wiederhake <twiederh@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
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@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@redhat.com>
Signed-off-by: Tim Wiederhake <twiederh@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
On Thu, Apr 08, 2021 at 13:19:56 +0200, Tim Wiederhake wrote:
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/network_conf.c | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Tim Wiederhake <twiederh@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
On Thu, Apr 08, 2021 at 13:19:57 +0200, Tim Wiederhake wrote:
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/domain_conf.c | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Tim Wiederhake <twiederh@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
On Thu, Apr 08, 2021 at 13:19:58 +0200, Tim Wiederhake wrote:
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/cpu_conf.c | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
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
participants (2)
-
Peter Krempa -
Tim Wiederhake