[PATCH 00/15] XML parser cleanups (part 1)
Peter Krempa (15): virNetDevBandwidthParse: Don't validate element name virNetDevBandwidthParse: Use 'virXMLPropUInt' to parse 'classID' virNetDevBandwidthParseRate: Refactor parsing virNetDevBandwidthParse: Use virXMLNodeGetSubelement instead of looped parser virNetworkDHCPDefParseXML: Refactor cleanup util: xml: Introduce virXMLNodeGetSubelementList nwfilterxml2xmltest: Add test case for parser and formatter quirks conf: network: Refactor XML parsing in virNetworkDHCPDefParseXML conf: nwfilter: Refactor XML formatting in virNWFilterRuleDefFormat virNWFilterRuleDef: Turn 'action' and 'tt' into proper enum types virNWFilterRuleParse: Parse 'priority' via 'virXMLPropInt' virNWFilterRuleParse: Refactor attribute parser virNWFilterRuleDefDetailsFormat: Refactor formatter conf: nwfilter: Refactor virNWFilterIncludeParse conf: nwfilter: Refactor virNWFilterFormatParamAttributes src/conf/netdev_bandwidth_conf.c | 141 ++----- src/conf/network_conf.c | 74 ++-- src/conf/nwfilter_conf.c | 410 ++++++++------------ src/conf/nwfilter_conf.h | 4 +- src/conf/nwfilter_params.c | 45 +-- src/libvirt_private.syms | 1 + src/nwfilter/nwfilter_ebiptables_driver.c | 5 + src/util/virxml.c | 34 ++ src/util/virxml.h | 5 + tests/nwfilterxml2xmlin/quirks-invalid.xml | 13 + tests/nwfilterxml2xmlout/quirks-invalid.xml | 7 + tests/nwfilterxml2xmltest.c | 5 + 12 files changed, 326 insertions(+), 418 deletions(-) create mode 100644 tests/nwfilterxml2xmlin/quirks-invalid.xml create mode 100644 tests/nwfilterxml2xmlout/quirks-invalid.xml -- 2.40.1
Callers make sure to pass the correct element. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/netdev_bandwidth_conf.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/conf/netdev_bandwidth_conf.c b/src/conf/netdev_bandwidth_conf.c index 1609b14784..48be9d2f38 100644 --- a/src/conf/netdev_bandwidth_conf.c +++ b/src/conf/netdev_bandwidth_conf.c @@ -112,12 +112,6 @@ virNetDevBandwidthParse(virNetDevBandwidth **bandwidth, def = g_new0(virNetDevBandwidth, 1); - if (!node || !virXMLNodeNameEqual(node, "bandwidth")) { - virReportError(VIR_ERR_INVALID_ARG, "%s", - _("invalid argument supplied")); - return -1; - } - class_id_prop = virXMLPropString(node, "classID"); if (class_id_prop) { if (!class_id) { -- 2.40.1
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/netdev_bandwidth_conf.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/conf/netdev_bandwidth_conf.c b/src/conf/netdev_bandwidth_conf.c index 48be9d2f38..2ac1983dce 100644 --- a/src/conf/netdev_bandwidth_conf.c +++ b/src/conf/netdev_bandwidth_conf.c @@ -108,24 +108,24 @@ virNetDevBandwidthParse(virNetDevBandwidth **bandwidth, g_autoptr(virNetDevBandwidth) def = NULL; xmlNodePtr cur; xmlNodePtr in = NULL, out = NULL; - g_autofree char *class_id_prop = NULL; + unsigned int class_id_value; + int rc; def = g_new0(virNetDevBandwidth, 1); - class_id_prop = virXMLPropString(node, "classID"); - if (class_id_prop) { + + if ((rc = virXMLPropUInt(node, "classID", 10, VIR_XML_PROP_NONE, &class_id_value)) < 0) + return -1; + + if (rc == 1) { if (!class_id) { virReportError(VIR_ERR_XML_DETAIL, "%s", _("classID attribute not supported on <bandwidth> " "in this usage context")); return -1; } - if (virStrToLong_ui(class_id_prop, NULL, 10, class_id) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unable to parse class id '%1$s'"), - class_id_prop); - return -1; - } + + *class_id = class_id_value; } cur = node->children; -- 2.40.1
Remove the unnecessary check for valid arguments and use virXMLPropULongLong instead of hand-written property parsers. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/netdev_bandwidth_conf.c | 89 +++++++++++--------------------- 1 file changed, 29 insertions(+), 60 deletions(-) diff --git a/src/conf/netdev_bandwidth_conf.c b/src/conf/netdev_bandwidth_conf.c index 2ac1983dce..f34d7499ae 100644 --- a/src/conf/netdev_bandwidth_conf.c +++ b/src/conf/netdev_bandwidth_conf.c @@ -25,61 +25,46 @@ #define VIR_FROM_THIS VIR_FROM_NONE static int -virNetDevBandwidthParseRate(xmlNodePtr node, virNetDevBandwidthRate *rate) +virNetDevBandwidthParseRate(xmlNodePtr node, + virNetDevBandwidthRate *rate, + bool allowFloor) { - g_autofree char *average = NULL; - g_autofree char *peak = NULL; - g_autofree char *burst = NULL; - g_autofree char *floor = NULL; - - if (!node || !rate) { - virReportError(VIR_ERR_INVALID_ARG, "%s", - _("invalid argument supplied")); + int rc_average; + int rc_peak; + int rc_burst; + int rc_floor; + + if ((rc_average = virXMLPropULongLong(node, "average", 10, VIR_XML_PROP_NONE, + &rate->average)) < 0) return -1; - } - average = virXMLPropString(node, "average"); - peak = virXMLPropString(node, "peak"); - burst = virXMLPropString(node, "burst"); - floor = virXMLPropString(node, "floor"); + if ((rc_peak = virXMLPropULongLong(node, "peak", 10, VIR_XML_PROP_NONE, + &rate->peak)) < 0) + return -1; - if (average) { - if (virStrToLong_ullp(average, NULL, 10, &rate->average) < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("could not convert bandwidth average value '%1$s'"), - average); - return -1; - } - } else if (!floor) { - virReportError(VIR_ERR_XML_DETAIL, "%s", - _("Missing mandatory average or floor attributes")); + if ((rc_burst = virXMLPropULongLong(node, "burst", 10, VIR_XML_PROP_NONE, + &rate->burst)) < 0) return -1; - } - if ((peak || burst) && !average) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("'peak' and 'burst' require 'average' attribute")); + if ((rc_floor = virXMLPropULongLong(node, "floor", 10, VIR_XML_PROP_NONE, + &rate->floor)) < 0) return -1; - } - if (peak && virStrToLong_ullp(peak, NULL, 10, &rate->peak) < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("could not convert bandwidth peak value '%1$s'"), - peak); + if (!rc_average && !rc_floor) { + virReportError(VIR_ERR_XML_DETAIL, "%s", + _("Missing mandatory average or floor attributes")); return -1; } - if (burst && virStrToLong_ullp(burst, NULL, 10, &rate->burst) < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("could not convert bandwidth burst value '%1$s'"), - burst); + if ((rc_peak || rc_burst) && !rc_average) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("'peak' and 'burst' require 'average' attribute")); return -1; } - if (floor && virStrToLong_ullp(floor, NULL, 10, &rate->floor) < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("could not convert bandwidth floor value '%1$s'"), - floor); + if (rc_floor && !allowFloor) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("floor attribute is not supported for this config")); return -1; } @@ -157,32 +142,16 @@ virNetDevBandwidthParse(virNetDevBandwidth **bandwidth, if (in) { def->in = g_new0(virNetDevBandwidthRate, 1); - if (virNetDevBandwidthParseRate(in, def->in) < 0) { - /* helper reported error for us */ - return -1; - } - - if (def->in->floor && !allowFloor) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("floor attribute is not supported for this config")); + if (virNetDevBandwidthParseRate(in, def->in, allowFloor) < 0) return -1; - } } if (out) { def->out = g_new0(virNetDevBandwidthRate, 1); - if (virNetDevBandwidthParseRate(out, def->out) < 0) { - /* helper reported error for us */ - return -1; - } - - if (def->out->floor) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("'floor' attribute allowed " - "only in <inbound> element")); + /* floor is not allowed for <outbound> */ + if (virNetDevBandwidthParseRate(out, def->out, false) < 0) return -1; - } } if (def->in || def->out) -- 2.40.1
Extrac the 'inbound'/'outbound' subelements using virXMLNodeGetSubelement to simplify the code. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/netdev_bandwidth_conf.c | 34 ++++---------------------------- 1 file changed, 4 insertions(+), 30 deletions(-) diff --git a/src/conf/netdev_bandwidth_conf.c b/src/conf/netdev_bandwidth_conf.c index f34d7499ae..38b7cc10fd 100644 --- a/src/conf/netdev_bandwidth_conf.c +++ b/src/conf/netdev_bandwidth_conf.c @@ -91,8 +91,8 @@ virNetDevBandwidthParse(virNetDevBandwidth **bandwidth, bool allowFloor) { g_autoptr(virNetDevBandwidth) def = NULL; - xmlNodePtr cur; - xmlNodePtr in = NULL, out = NULL; + xmlNodePtr in; + xmlNodePtr out; unsigned int class_id_value; int rc; @@ -113,40 +113,14 @@ virNetDevBandwidthParse(virNetDevBandwidth **bandwidth, *class_id = class_id_value; } - cur = node->children; - - while (cur) { - if (cur->type == XML_ELEMENT_NODE) { - if (virXMLNodeNameEqual(cur, "inbound")) { - if (in) { - virReportError(VIR_ERR_XML_DETAIL, "%s", - _("Only one child <inbound> " - "element allowed")); - return -1; - } - in = cur; - } else if (virXMLNodeNameEqual(cur, "outbound")) { - if (out) { - virReportError(VIR_ERR_XML_DETAIL, "%s", - _("Only one child <outbound> " - "element allowed")); - return -1; - } - out = cur; - } - /* Silently ignore unknown elements */ - } - cur = cur->next; - } - - if (in) { + if ((in = virXMLNodeGetSubelement(node, "inbound"))) { def->in = g_new0(virNetDevBandwidthRate, 1); if (virNetDevBandwidthParseRate(in, def->in, allowFloor) < 0) return -1; } - if (out) { + if ((out = virXMLNodeGetSubelement(node, "outbound"))) { def->out = g_new0(virNetDevBandwidthRate, 1); /* floor is not allowed for <outbound> */ -- 2.40.1
On a Friday in 2023, Peter Krempa wrote:
Extrac the 'inbound'/'outbound' subelements using
Extract
virXMLNodeGetSubelement to simplify the code.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/netdev_bandwidth_conf.c | 34 ++++---------------------------- 1 file changed, 4 insertions(+), 30 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
There's nothing to clean up in the 'host' local variable on error as the function which fills it makes sure to fill it only on success. In such case it's also directly assigned to the array thus the 'host' variable is cleared. Remove the 'cleanup' label and 'ret' variable as we can now directly return -1 on error. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/network_conf.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 73011cb7a2..5add0ef902 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -605,7 +605,6 @@ virNetworkDHCPDefParseXML(const char *networkName, xmlNodePtr node, virNetworkIPDef *def) { - int ret = -1; xmlNodePtr cur; virNetworkDHCPRangeDef range; virNetworkDHCPHostDef host; @@ -619,7 +618,7 @@ virNetworkDHCPDefParseXML(const char *networkName, virXMLNodeNameEqual(cur, "range")) { if (virNetworkDHCPRangeDefParseXML(networkName, def, cur, &range) < 0) - goto cleanup; + return -1; VIR_APPEND_ELEMENT(def->ranges, def->nranges, range); } else if (cur->type == XML_ELEMENT_NODE && @@ -627,7 +626,7 @@ virNetworkDHCPDefParseXML(const char *networkName, if (virNetworkDHCPHostDefParseXML(networkName, def, cur, &host, false) < 0) - goto cleanup; + return -1; VIR_APPEND_ELEMENT(def->hosts, def->nhosts, host); } else if (VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_INET) && cur->type == XML_ELEMENT_NODE && @@ -645,7 +644,7 @@ virNetworkDHCPDefParseXML(const char *networkName, if (server && virSocketAddrParse(&inaddr, server, AF_UNSPEC) < 0) { - goto cleanup; + return -1; } def->bootfile = g_steal_pointer(&file); @@ -655,10 +654,7 @@ virNetworkDHCPDefParseXML(const char *networkName, cur = cur->next; } - ret = 0; - cleanup: - virNetworkDHCPHostDefClear(&host); - return ret; + return 0; } -- 2.40.1
The new helper is similar to virXPathNodeSet list but for cases where we want to get subelements directly rather than using XPath. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virxml.c | 34 ++++++++++++++++++++++++++++++++++ src/util/virxml.h | 5 +++++ 3 files changed, 40 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 27b8e111aa..436d5a0770 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3709,6 +3709,7 @@ virXMLFormatMetadata; virXMLNewNode; virXMLNodeContentString; virXMLNodeGetSubelement; +virXMLNodeGetSubelementList; virXMLNodeNameEqual; virXMLNodeSanitizeNamespaces; virXMLNodeToString; diff --git a/src/util/virxml.c b/src/util/virxml.c index e04455fcaa..0dae15a039 100644 --- a/src/util/virxml.c +++ b/src/util/virxml.c @@ -899,6 +899,40 @@ virXMLNodeGetSubelement(xmlNodePtr node, } +/** + * virXMLNodeGetSubelementList: + * @node: node to get subelement of + * @name: name of subelement to fetch (NULL to fetch all sub-elements) + * @list: If non-NULL, filled with a list of pointers to the nodes. Caller is + * responsible for freeeing the list but not the members. + * + * Find and return a sub-elements node of @node named @name in a list. + * Returns the number of subelements with @name + */ +size_t +virXMLNodeGetSubelementList(xmlNodePtr node, + const char *name, + xmlNodePtr **list) +{ + xmlNodePtr n; + size_t nelems = 0; + + for (n = node->children; n; n = n->next) { + if (n->type == XML_ELEMENT_NODE) { + if (name && !virXMLNodeNameEqual(n, name)) + continue; + + if (list) + VIR_APPEND_ELEMENT_COPY(*list, nelems, n); + else + nelems++; + } + } + + return nelems; +} + + /** * virXPathNode: * @xpath: the XPath string to evaluate diff --git a/src/util/virxml.h b/src/util/virxml.h index cca9f222ab..7af47437bd 100644 --- a/src/util/virxml.h +++ b/src/util/virxml.h @@ -77,6 +77,11 @@ xmlNodePtr virXMLNodeGetSubelement(xmlNodePtr node, const char *name); +size_t +virXMLNodeGetSubelementList(xmlNodePtr node, + const char *name, + xmlNodePtr **list); + xmlNodePtr virXPathNode(const char *xpath, xmlXPathContextPtr ctxt); -- 2.40.1
On a Friday in 2023, Peter Krempa wrote:
The new helper is similar to virXPathNodeSet list but for cases where we want to get subelements directly rather than using XPath.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virxml.c | 34 ++++++++++++++++++++++++++++++++++ src/util/virxml.h | 5 +++++ 3 files changed, 40 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 27b8e111aa..436d5a0770 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3709,6 +3709,7 @@ virXMLFormatMetadata; virXMLNewNode; virXMLNodeContentString; virXMLNodeGetSubelement; +virXMLNodeGetSubelementList; virXMLNodeNameEqual; virXMLNodeSanitizeNamespaces; virXMLNodeToString; diff --git a/src/util/virxml.c b/src/util/virxml.c index e04455fcaa..0dae15a039 100644 --- a/src/util/virxml.c +++ b/src/util/virxml.c @@ -899,6 +899,40 @@ virXMLNodeGetSubelement(xmlNodePtr node, }
+/** + * virXMLNodeGetSubelementList: + * @node: node to get subelement of + * @name: name of subelement to fetch (NULL to fetch all sub-elements) + * @list: If non-NULL, filled with a list of pointers to the nodes. Caller is + * responsible for freeeing the list but not the members.
*freeing
+ * + * Find and return a sub-elements node of @node named @name in a list. + * Returns the number of subelements with @name + */
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
The parser and formatter for nwfilter rules is very strange and has weird quirks. Add a test case trying to capture some of the quirks to vizualize how it will change when the code is refactored. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/nwfilterxml2xmlin/quirks-invalid.xml | 13 +++++++++++++ tests/nwfilterxml2xmlout/quirks-invalid.xml | 7 +++++++ tests/nwfilterxml2xmltest.c | 5 +++++ 3 files changed, 25 insertions(+) create mode 100644 tests/nwfilterxml2xmlin/quirks-invalid.xml create mode 100644 tests/nwfilterxml2xmlout/quirks-invalid.xml diff --git a/tests/nwfilterxml2xmlin/quirks-invalid.xml b/tests/nwfilterxml2xmlin/quirks-invalid.xml new file mode 100644 index 0000000000..dab0e5035f --- /dev/null +++ b/tests/nwfilterxml2xmlin/quirks-invalid.xml @@ -0,0 +1,13 @@ +<filter name='testcase'> + <uuid>01a992d2-f8c8-7c27-f69b-ab0a9d377379</uuid> + + <!-- quirky XML for parser validation --> + <rule action='accept' direction='in' priority='100'> + <tcp match='no' srcipaddr='10.1.2.3'/> + <tcp match='no' srcportstart='22'/> + <tcp dstportstart='22' comment='comment'/> + <tcp match='no' srcporttend='24'/> + <tcp srcipmask='32' dstporttend='24'/> + </rule> + +</filter> diff --git a/tests/nwfilterxml2xmlout/quirks-invalid.xml b/tests/nwfilterxml2xmlout/quirks-invalid.xml new file mode 100644 index 0000000000..f244d45e08 --- /dev/null +++ b/tests/nwfilterxml2xmlout/quirks-invalid.xml @@ -0,0 +1,7 @@ +<filter name='testcase' chain='root'> + <uuid>01a992d2-f8c8-7c27-f69b-ab0a9d377379</uuid> + <rule action='accept' direction='in' priority='100'> + <tcp match='no' srcipaddr='10.1.2.3' srcipmask='32' srcportstart='22' dstportstart='22'/> + <tcp comment='comment'/> + </rule> +</filter> diff --git a/tests/nwfilterxml2xmltest.c b/tests/nwfilterxml2xmltest.c index 5c84c2fee9..c2481481ee 100644 --- a/tests/nwfilterxml2xmltest.c +++ b/tests/nwfilterxml2xmltest.c @@ -117,6 +117,11 @@ mymain(void) DO_TEST("example-1", false); DO_TEST("example-2", false); + /* The parser and formatter for nwfilter rules was written in a quirky way. + * Validate that it still works. Note that the files don't conform to the + * schema */ + DO_TEST("quirks-invalid", false); + DO_TEST("chain_prefixtest1-invalid", true); /* derived from arp-test */ DO_TEST("attr-value-test", false); -- 2.40.1
On a Friday in 2023, Peter Krempa wrote:
The parser and formatter for nwfilter rules is very strange and has weird quirks. Add a test case trying to capture some of the quirks to vizualize how it will change when the code is refactored.
visualize
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/nwfilterxml2xmlin/quirks-invalid.xml | 13 +++++++++++++ tests/nwfilterxml2xmlout/quirks-invalid.xml | 7 +++++++ tests/nwfilterxml2xmltest.c | 5 +++++ 3 files changed, 25 insertions(+) create mode 100644 tests/nwfilterxml2xmlin/quirks-invalid.xml create mode 100644 tests/nwfilterxml2xmlout/quirks-invalid.xml
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Use virXMLNodeGetSubelement(List) instead of the looped parser and simplify the code. Note that handling of the 'bootp' element now conforms to the schema where we allow just one and the 'file' attribute is mandatory. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/network_conf.c | 68 ++++++++++++++++++----------------------- 1 file changed, 29 insertions(+), 39 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 5add0ef902..73788b6d87 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -605,53 +605,43 @@ virNetworkDHCPDefParseXML(const char *networkName, xmlNodePtr node, virNetworkIPDef *def) { - xmlNodePtr cur; - virNetworkDHCPRangeDef range; - virNetworkDHCPHostDef host; + g_autofree xmlNodePtr *rangeNodes = NULL; + size_t nrangeNodes = virXMLNodeGetSubelementList(node, "range", &rangeNodes); + g_autofree xmlNodePtr *hostNodes = NULL; + size_t nhostNodes = virXMLNodeGetSubelementList(node, "host", &hostNodes); + xmlNodePtr bootp = virXMLNodeGetSubelement(node, "bootp"); + size_t i; - memset(&range, 0, sizeof(range)); - memset(&host, 0, sizeof(host)); + for (i = 0; i < nrangeNodes; i++) { + virNetworkDHCPRangeDef range = { 0 }; - cur = node->children; - while (cur != NULL) { - if (cur->type == XML_ELEMENT_NODE && - virXMLNodeNameEqual(cur, "range")) { + if (virNetworkDHCPRangeDefParseXML(networkName, def, rangeNodes[i], &range) < 0) + return -1; - if (virNetworkDHCPRangeDefParseXML(networkName, def, cur, &range) < 0) - return -1; - VIR_APPEND_ELEMENT(def->ranges, def->nranges, range); + VIR_APPEND_ELEMENT(def->ranges, def->nranges, range); + } - } else if (cur->type == XML_ELEMENT_NODE && - virXMLNodeNameEqual(cur, "host")) { + for (i = 0; i < nhostNodes; i++) { + virNetworkDHCPHostDef host = { 0 }; - if (virNetworkDHCPHostDefParseXML(networkName, def, cur, - &host, false) < 0) - return -1; - VIR_APPEND_ELEMENT(def->hosts, def->nhosts, host); - } else if (VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_INET) && - cur->type == XML_ELEMENT_NODE && - virXMLNodeNameEqual(cur, "bootp")) { - g_autofree char *file = NULL; - g_autofree char *server = NULL; - virSocketAddr inaddr; - memset(&inaddr, 0, sizeof(inaddr)); - - if (!(file = virXMLPropString(cur, "file"))) { - cur = cur->next; - continue; - } - server = virXMLPropString(cur, "server"); + if (virNetworkDHCPHostDefParseXML(networkName, def, hostNodes[i], + &host, false) < 0) + return -1; - if (server && - virSocketAddrParse(&inaddr, server, AF_UNSPEC) < 0) { - return -1; - } + VIR_APPEND_ELEMENT(def->hosts, def->nhosts, host); + } - def->bootfile = g_steal_pointer(&file); - def->bootserver = inaddr; - } + if (bootp && + VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_INET)) { + g_autofree char *server = virXMLPropString(bootp, "server"); - cur = cur->next; + if (!(def->bootfile = virXMLPropStringRequired(bootp, "file"))) + return -1; + + if (server && + virSocketAddrParse(&def->bootserver, server, AF_UNSPEC) < 0) { + return -1; + } } return 0; -- 2.40.1
Use virXMLFormatElement to simplify the formatter. Drop return value of virNWFilterRuleDefFormat as there are no errors to report. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/nwfilter_conf.c | 43 ++++++++++++++++------------------------ 1 file changed, 17 insertions(+), 26 deletions(-) diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index 70ad37e63d..98a19f9e4b 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -2877,43 +2877,31 @@ virNWFilterRuleDefDetailsFormat(virBuffer *buf, } -static int +static void virNWFilterRuleDefFormat(virBuffer *buf, virNWFilterRuleDef *def) { + g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER; + g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf); size_t i; - bool subelement = false; - virBufferAsprintf(buf, "<rule action='%s' direction='%s' priority='%d'", + virBufferAsprintf(&attrBuf, " action='%s' direction='%s' priority='%d'", virNWFilterRuleActionTypeToString(def->action), virNWFilterRuleDirectionTypeToString(def->tt), def->priority); if ((def->flags & RULE_FLAG_NO_STATEMATCH)) - virBufferAddLit(buf, " statematch='false'"); + virBufferAddLit(&attrBuf, " statematch='false'"); - virBufferAdjustIndent(buf, 2); - i = 0; - while (virAttr[i].id) { - if (virAttr[i].prtclType == def->prtclType) { - if (!subelement) - virBufferAddLit(buf, ">\n"); - virNWFilterRuleDefDetailsFormat(buf, - virAttr[i].id, - virAttr[i].att, - def); - subelement = true; - break; - } - i++; + for (i = 0; virAttr[i].id; i++) { + if (virAttr[i].prtclType != def->prtclType) + continue; + + virNWFilterRuleDefDetailsFormat(&childBuf, virAttr[i].id, virAttr[i].att, def); + break; } - virBufferAdjustIndent(buf, -2); - if (subelement) - virBufferAddLit(buf, "</rule>\n"); - else - virBufferAddLit(buf, "/>\n"); - return 0; + virXMLFormatElement(buf, "rule", &attrBuf, &childBuf); } @@ -2921,8 +2909,11 @@ static int virNWFilterEntryFormat(virBuffer *buf, virNWFilterEntry *entry) { - if (entry->rule) - return virNWFilterRuleDefFormat(buf, entry->rule); + if (entry->rule) { + virNWFilterRuleDefFormat(buf, entry->rule); + return 0; + } + return virNWFilterFormatParamAttributes(buf, entry->include->params, entry->include->filterref); } -- 2.40.1
Convert the fields to the proper types and use virXMLPropEnum for parsing. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/nwfilter_conf.c | 32 +++-------------------- src/conf/nwfilter_conf.h | 4 +-- src/nwfilter/nwfilter_ebiptables_driver.c | 5 ++++ 3 files changed, 11 insertions(+), 30 deletions(-) diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index 98a19f9e4b..13c6096fcd 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -2373,8 +2373,6 @@ virNWFilterRuleDefFixup(virNWFilterRuleDef *rule) static virNWFilterRuleDef * virNWFilterRuleParse(xmlNodePtr node) { - g_autofree char *action = NULL; - g_autofree char *direction = NULL; g_autofree char *prio = NULL; g_autofree char *statematch = NULL; bool found; @@ -2386,38 +2384,16 @@ virNWFilterRuleParse(xmlNodePtr node) ret = g_new0(virNWFilterRuleDef, 1); - action = virXMLPropString(node, "action"); - direction = virXMLPropString(node, "direction"); prio = virXMLPropString(node, "priority"); statematch = virXMLPropString(node, "statematch"); - if (!action) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", - _("rule node requires action attribute")); + if (virXMLPropEnum(node, "action", virNWFilterRuleActionTypeFromString, + VIR_XML_PROP_REQUIRED, &ret->action) < 0) return NULL; - } - if ((ret->action = virNWFilterRuleActionTypeFromString(action)) < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - "%s", - _("unknown rule action attribute value")); + if (virXMLPropEnum(node, "direction", virNWFilterRuleDirectionTypeFromString, + VIR_XML_PROP_REQUIRED, &ret->tt) < 0) return NULL; - } - - if (!direction) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", - _("rule node requires direction attribute")); - return NULL; - } - - if ((ret->tt = virNWFilterRuleDirectionTypeFromString(direction)) < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - "%s", - _("unknown rule direction attribute value")); - return NULL; - } ret->priority = MAX_RULE_PRIORITY / 2; diff --git a/src/conf/nwfilter_conf.h b/src/conf/nwfilter_conf.h index 7c09b3bcb9..22c2fb51f0 100644 --- a/src/conf/nwfilter_conf.h +++ b/src/conf/nwfilter_conf.h @@ -446,8 +446,8 @@ typedef struct _virNWFilterRuleDef virNWFilterRuleDef; struct _virNWFilterRuleDef { virNWFilterRulePriority priority; virNWFilterRuleFlags flags; - int action; /* virNWFilterRuleActionType */ - int tt; /* virNWFilterRuleDirectionType */ + virNWFilterRuleActionType action; + virNWFilterRuleDirectionType tt; virNWFilterRuleProtocolType prtclType; union { ethHdrFilterDef ethHdrFilter; diff --git a/src/nwfilter/nwfilter_ebiptables_driver.c b/src/nwfilter/nwfilter_ebiptables_driver.c index 99a74a60e5..1c5da2ae05 100644 --- a/src/nwfilter/nwfilter_ebiptables_driver.c +++ b/src/nwfilter/nwfilter_ebiptables_driver.c @@ -2361,6 +2361,11 @@ ebtablesCreateRuleInstance(virFirewall *fw, target = virNWFilterJumpTargetTypeToString( VIR_NWFILTER_RULE_ACTION_DROP); break; + case VIR_NWFILTER_RULE_ACTION_DROP: + case VIR_NWFILTER_RULE_ACTION_ACCEPT: + case VIR_NWFILTER_RULE_ACTION_RETURN: + case VIR_NWFILTER_RULE_ACTION_CONTINUE: + case VIR_NWFILTER_RULE_ACTION_LAST: default: target = virNWFilterJumpTargetTypeToString(rule->action); } -- 2.40.1
Use modern parsing. Invalid numbers are now rejected. Semantis for numbers out of range is preserved. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/nwfilter_conf.c | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index 13c6096fcd..b7400b553a 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -2373,18 +2373,15 @@ virNWFilterRuleDefFixup(virNWFilterRuleDef *rule) static virNWFilterRuleDef * virNWFilterRuleParse(xmlNodePtr node) { - g_autofree char *prio = NULL; g_autofree char *statematch = NULL; bool found; int found_i = 0; - int priority; xmlNodePtr cur; g_autoptr(virNWFilterRuleDef) ret = NULL; ret = g_new0(virNWFilterRuleDef, 1); - prio = virXMLPropString(node, "priority"); statematch = virXMLPropString(node, "statematch"); if (virXMLPropEnum(node, "action", virNWFilterRuleActionTypeFromString, @@ -2395,15 +2392,12 @@ virNWFilterRuleParse(xmlNodePtr node) VIR_XML_PROP_REQUIRED, &ret->tt) < 0) return NULL; - ret->priority = MAX_RULE_PRIORITY / 2; + if (virXMLPropInt(node, "priority", 10, VIR_XML_PROP_NONE, &ret->priority, + MAX_RULE_PRIORITY / 2) < 0) + return NULL; - if (prio) { - if (virStrToLong_i(prio, NULL, 10, &priority) >= 0) { - if (priority <= MAX_RULE_PRIORITY && - priority >= MIN_RULE_PRIORITY) - ret->priority = priority; - } - } + if (ret->priority > MAX_RULE_PRIORITY || ret->priority < MIN_RULE_PRIORITY) + ret->priority = MAX_RULE_PRIORITY / 2; if (statematch && (STREQ(statematch, "0") || STRCASEEQ(statematch, "false"))) -- 2.40.1
Use virXMLNodeGetSubelementList to get the elements to process. The new approach documents the complexity of the parser, which is designed to ignore unknown attributes and parse only a single kind of them after finding the first valid one. Note that the XML schema doesn't actually allow having multiple sub-elements, but I'm not sure how that translates to actual configs present. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/nwfilter_conf.c | 65 +++++++++++++++++++--------------------- 1 file changed, 31 insertions(+), 34 deletions(-) diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index b7400b553a..e6f7c0f8b7 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -2374,10 +2374,8 @@ static virNWFilterRuleDef * virNWFilterRuleParse(xmlNodePtr node) { g_autofree char *statematch = NULL; - bool found; - int found_i = 0; - - xmlNodePtr cur; + g_autofree xmlNodePtr *attrNodes = NULL; + size_t nattrNodes = 0; g_autoptr(virNWFilterRuleDef) ret = NULL; ret = g_new0(virNWFilterRuleDef, 1); @@ -2403,43 +2401,42 @@ virNWFilterRuleParse(xmlNodePtr node) (STREQ(statematch, "0") || STRCASEEQ(statematch, "false"))) ret->flags |= RULE_FLAG_NO_STATEMATCH; - cur = node->children; - - found = false; - - while (cur != NULL) { - if (cur->type == XML_ELEMENT_NODE) { - size_t i = 0; - while (1) { - if (found) - i = found_i; - - if (virXMLNodeNameEqual(cur, virAttr[i].id)) { + nattrNodes = virXMLNodeGetSubelementList(node, NULL, &attrNodes); - found_i = i; - found = true; - ret->prtclType = virAttr[i].prtclType; + if (nattrNodes > 0) { + size_t i; + size_t attr; - if (virNWFilterRuleDetailsParse(cur, - ret, - virAttr[i].att) < 0) { - return NULL; - } - if (virNWFilterRuleValidate(ret) < 0) - return NULL; + /* First we look up the type of the first valid element. The rest of + * the parsing then only considers elements with same name. */ + for (i = 0; i < nattrNodes; i++) { + for (attr = 0; virAttr[attr].id; attr++) { + if (virXMLNodeNameEqual(attrNodes[i], virAttr[attr].id)) { + ret->prtclType = virAttr[attr].prtclType; break; } - if (!found) { - i++; - if (!virAttr[i].id) - break; - } else { - break; - } } + + /* if we've found the first correct element end the search */ + if (virAttr[attr].id) + break; + } + + /* parse the correct subelements now */ + for (i = 0; i < nattrNodes; i++) { + /* no valid elements */ + if (!virAttr[attr].id) + break; + + if (!virXMLNodeNameEqual(attrNodes[i], virAttr[attr].id)) + continue; + + if (virNWFilterRuleDetailsParse(attrNodes[i], ret, virAttr[attr].att) < 0) + return NULL; } - cur = cur->next; + if (virNWFilterRuleValidate(ret) < 0) + return NULL; } virNWFilterRuleDefFixup(ret); -- 2.40.1
Format the rule attributes in two passes, first for positive 'match' and second pass for negative. This removes the crazy logic for switching between match modes inside the formatter. The refactor makes it also more clear in which cases we actually do format something. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/nwfilter_conf.c | 232 +++++++++----------- tests/nwfilterxml2xmlout/quirks-invalid.xml | 2 +- 2 files changed, 107 insertions(+), 127 deletions(-) diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index e6f7c0f8b7..f96ae707f9 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -2701,146 +2701,119 @@ static void virNWFilterRuleDefDetailsFormat(virBuffer *buf, const char *type, const virXMLAttr2Struct *att, - virNWFilterRuleDef *def) + virNWFilterRuleDef *def, + bool negative, + bool *hasAttrs) { - size_t i = 0, j; - bool typeShown = false; - bool neverShown = true; - bool asHex; - enum match { - MATCH_NONE = 0, - MATCH_YES, - MATCH_NO - } matchShown = MATCH_NONE; - nwItemDesc *item; + g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER; + bool present = false; + size_t i; + size_t j; - while (att[i].name) { - virNWFilterEntryItemFlags flags; + if (negative) + virBufferAddLit(&attrBuf, " match='no'"); + + for (i = 0; att[i].name; i++) { + nwItemDesc *item; VIR_WARNINGS_NO_CAST_ALIGN item = (nwItemDesc *)((char *)def + att[i].dataIdx); VIR_WARNINGS_RESET - flags = item->flags; - if ((flags & NWFILTER_ENTRY_ITEM_FLAG_EXISTS)) { - if (!typeShown) { - virBufferAsprintf(buf, "<%s", type); - typeShown = true; - neverShown = false; - } + if (!(item->flags & NWFILTER_ENTRY_ITEM_FLAG_EXISTS)) + continue; - if ((flags & NWFILTER_ENTRY_ITEM_FLAG_IS_NEG)) { - if (matchShown == MATCH_NONE) { - virBufferAddLit(buf, " match='no'"); - matchShown = MATCH_NO; - } else if (matchShown == MATCH_YES) { - virBufferAddLit(buf, "/>\n"); - typeShown = false; - matchShown = MATCH_NONE; - continue; - } - } else { - if (matchShown == MATCH_NO) { - virBufferAddLit(buf, "/>\n"); - typeShown = false; - matchShown = MATCH_NONE; - continue; - } - matchShown = MATCH_YES; + if (negative != !!(item->flags & NWFILTER_ENTRY_ITEM_FLAG_IS_NEG)) + continue; + + present = true; + *hasAttrs = true; + + virBufferAsprintf(&attrBuf, " %s='", att[i].name); + + if (att[i].formatter && !(item->flags & NWFILTER_ENTRY_ITEM_FLAG_HAS_VAR)) { + if (!att[i].formatter(&attrBuf, def, item)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("formatter for %1$s %2$s reported error"), + type, att[i].name); + return; } + } else if ((item->flags & NWFILTER_ENTRY_ITEM_FLAG_HAS_VAR)) { + virBufferAddChar(&attrBuf, '$'); + virNWFilterVarAccessPrint(item->varAccess, &attrBuf); + } else { - virBufferAsprintf(buf, " %s='", - att[i].name); - if (att[i].formatter && !(flags & NWFILTER_ENTRY_ITEM_FLAG_HAS_VAR)) { - if (!att[i].formatter(buf, def, item)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("formatter for %1$s %2$s reported error"), - type, - att[i].name); - return; - } - } else if ((flags & NWFILTER_ENTRY_ITEM_FLAG_HAS_VAR)) { - virBufferAddChar(buf, '$'); - virNWFilterVarAccessPrint(item->varAccess, buf); - } else { - asHex = false; - - switch (item->datatype) { - - case DATATYPE_UINT8_HEX: - asHex = true; - G_GNUC_FALLTHROUGH; - case DATATYPE_IPMASK: - case DATATYPE_IPV6MASK: - /* display all masks in CIDR format */ - case DATATYPE_UINT8: - virBufferAsprintf(buf, asHex ? "0x%x" : "%d", - item->u.u8); - break; - - case DATATYPE_UINT16_HEX: - asHex = true; - G_GNUC_FALLTHROUGH; - case DATATYPE_UINT16: - virBufferAsprintf(buf, asHex ? "0x%x" : "%d", - item->u.u16); - break; - - case DATATYPE_UINT32_HEX: - asHex = true; - G_GNUC_FALLTHROUGH; - case DATATYPE_UINT32: - virBufferAsprintf(buf, asHex ? "0x%x" : "%u", - item->u.u32); - break; - - case DATATYPE_IPADDR: - case DATATYPE_IPV6ADDR: - virNWIPAddressFormat(buf, - &item->u.ipaddr); - break; - - case DATATYPE_MACMASK: - case DATATYPE_MACADDR: - for (j = 0; j < 6; j++) - virBufferAsprintf(buf, "%02x%s", - item->u.macaddr.addr[j], - (j < 5) ? ":" : ""); - break; - - case DATATYPE_STRINGCOPY: - virBufferEscapeString(buf, "%s", item->u.string); - break; - - case DATATYPE_BOOLEAN: - if (item->u.boolean) - virBufferAddLit(buf, "true"); - else - virBufferAddLit(buf, "false"); - break; - - case DATATYPE_IPSETNAME: - case DATATYPE_IPSETFLAGS: - case DATATYPE_STRING: - case DATATYPE_LAST: - default: - virBufferAsprintf(buf, - "UNSUPPORTED DATATYPE 0x%02x\n", - att[i].datatype); - } + switch (item->datatype) { + + case DATATYPE_UINT8_HEX: + virBufferAsprintf(&attrBuf, "0x%x", item->u.u8); + break; + + case DATATYPE_IPMASK: + case DATATYPE_IPV6MASK: + /* display all masks in CIDR format */ + case DATATYPE_UINT8: + virBufferAsprintf(&attrBuf, "%d", item->u.u8); + break; + + case DATATYPE_UINT16_HEX: + virBufferAsprintf(&attrBuf, "0x%x", item->u.u16); + break; + + case DATATYPE_UINT16: + virBufferAsprintf(&attrBuf, "%d", item->u.u16); + break; + + case DATATYPE_UINT32_HEX: + virBufferAsprintf(&attrBuf, "0x%x", item->u.u32); + break; + + case DATATYPE_UINT32: + virBufferAsprintf(&attrBuf, "%u", item->u.u32); + break; + + case DATATYPE_IPADDR: + case DATATYPE_IPV6ADDR: + virNWIPAddressFormat(&attrBuf, &item->u.ipaddr); + break; + + case DATATYPE_MACMASK: + case DATATYPE_MACADDR: + for (j = 0; j < 6; j++) + virBufferAsprintf(&attrBuf, "%02x%s", + item->u.macaddr.addr[j], + (j < 5) ? ":" : ""); + break; + + case DATATYPE_STRINGCOPY: + virBufferEscapeString(&attrBuf, "%s", item->u.string); + break; + + case DATATYPE_BOOLEAN: + if (item->u.boolean) + virBufferAddLit(&attrBuf, "true"); + else + virBufferAddLit(&attrBuf, "false"); + break; + + case DATATYPE_IPSETNAME: + case DATATYPE_IPSETFLAGS: + case DATATYPE_STRING: + case DATATYPE_LAST: + default: + virBufferAsprintf(&attrBuf, + "UNSUPPORTED DATATYPE 0x%02x\n", + att[i].datatype); } - virBufferAddLit(buf, "'"); } - i++; + + virBufferAddLit(&attrBuf, "'"); } - if (typeShown) - virBufferAddLit(buf, "/>\n"); - if (neverShown) - virBufferAsprintf(buf, - "<%s/>\n", type); + if (!present) + return; - return; + virXMLFormatElement(buf, type, &attrBuf, NULL); } @@ -2861,10 +2834,17 @@ virNWFilterRuleDefFormat(virBuffer *buf, virBufferAddLit(&attrBuf, " statematch='false'"); for (i = 0; virAttr[i].id; i++) { + bool hasAttrs = false; + if (virAttr[i].prtclType != def->prtclType) continue; - virNWFilterRuleDefDetailsFormat(&childBuf, virAttr[i].id, virAttr[i].att, def); + virNWFilterRuleDefDetailsFormat(&childBuf, virAttr[i].id, virAttr[i].att, def, false, &hasAttrs); + virNWFilterRuleDefDetailsFormat(&childBuf, virAttr[i].id, virAttr[i].att, def, true, &hasAttrs); + + if (!hasAttrs) + virBufferAsprintf(&childBuf, "<%s/>\n", virAttr[i].id); + break; } diff --git a/tests/nwfilterxml2xmlout/quirks-invalid.xml b/tests/nwfilterxml2xmlout/quirks-invalid.xml index f244d45e08..5159eaf21d 100644 --- a/tests/nwfilterxml2xmlout/quirks-invalid.xml +++ b/tests/nwfilterxml2xmlout/quirks-invalid.xml @@ -1,7 +1,7 @@ <filter name='testcase' chain='root'> <uuid>01a992d2-f8c8-7c27-f69b-ab0a9d377379</uuid> <rule action='accept' direction='in' priority='100'> - <tcp match='no' srcipaddr='10.1.2.3' srcipmask='32' srcportstart='22' dstportstart='22'/> <tcp comment='comment'/> + <tcp match='no' srcipaddr='10.1.2.3' srcipmask='32' srcportstart='22' dstportstart='22'/> </rule> </filter> -- 2.40.1
Use automatic memory freeing and modern XML parsers to simplify the function. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/nwfilter_conf.c | 26 +++++++------------------- 1 file changed, 7 insertions(+), 19 deletions(-) diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index f96ae707f9..b79fd2561e 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -298,7 +298,7 @@ virNWFilterIncludeDefFree(virNWFilterIncludeDef *inc) g_free(inc->filterref); g_free(inc); } - +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virNWFilterIncludeDef, virNWFilterIncludeDefFree); static void virNWFilterEntryFree(virNWFilterEntry *entry) @@ -2031,27 +2031,15 @@ virNWFilterRuleDetailsParse(xmlNodePtr node, static virNWFilterIncludeDef * virNWFilterIncludeParse(xmlNodePtr cur) { - virNWFilterIncludeDef *ret; - - ret = g_new0(virNWFilterIncludeDef, 1); - - ret->filterref = virXMLPropString(cur, "filter"); - if (!ret->filterref) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", - _("rule node requires action attribute")); - goto err_exit; - } + g_autoptr(virNWFilterIncludeDef) ret = g_new0(virNWFilterIncludeDef, 1); - ret->params = virNWFilterParseParamAttributes(cur); - if (!ret->params) - goto err_exit; + if (!(ret->filterref = virXMLPropStringRequired(cur, "filter"))) + return NULL; - return ret; + if (!(ret->params = virNWFilterParseParamAttributes(cur))) + return NULL; - err_exit: - virNWFilterIncludeDefFree(ret); - return NULL; + return g_steal_pointer(&ret); } -- 2.40.1
Use virXMLFormatElement and simplify the formatter. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/nwfilter_params.c | 45 ++++++++++++++------------------------ 1 file changed, 17 insertions(+), 28 deletions(-) diff --git a/src/conf/nwfilter_params.c b/src/conf/nwfilter_params.c index 496ed2f0b8..868baf7192 100644 --- a/src/conf/nwfilter_params.c +++ b/src/conf/nwfilter_params.c @@ -731,45 +731,34 @@ virNWFilterFormatParamAttributes(virBuffer *buf, GHashTable *table, const char *filterref) { - virHashKeyValuePair *items; - size_t i, j; - int card, numKeys; - - numKeys = virHashSize(table); + g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER; + g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf); + g_autofree virHashKeyValuePair *items = NULL; + size_t i; + size_t nitems; - if (numKeys < 0) { + if (!(items = virHashGetItems(table, &nitems, true))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("missing filter parameter table")); return -1; } - items = virHashGetItems(table, NULL, true); - if (!items) - return -1; + virBufferAsprintf(&attrBuf, " filter='%s'", filterref); - virBufferAsprintf(buf, "<filterref filter='%s'", filterref); - if (numKeys) { - virBufferAddLit(buf, ">\n"); - virBufferAdjustIndent(buf, 2); - for (i = 0; i < numKeys; i++) { - const virNWFilterVarValue *value = items[i].value; + for (i = 0; i < nitems; i++) { + const virNWFilterVarValue *value = items[i].value; + size_t npar = virNWFilterVarValueGetCardinality(value); + size_t j; - card = virNWFilterVarValueGetCardinality(value); + for (j = 0; j < npar; j++) + virBufferAsprintf(&childBuf, + "<parameter name='%s' value='%s'/>\n", + (const char *)items[i].key, + virNWFilterVarValueGetNthValue(value, j)); - for (j = 0; j < card; j++) - virBufferAsprintf(buf, - "<parameter name='%s' value='%s'/>\n", - (const char *)items[i].key, - virNWFilterVarValueGetNthValue(value, j)); - - } - virBufferAdjustIndent(buf, -2); - virBufferAddLit(buf, "</filterref>\n"); - } else { - virBufferAddLit(buf, "/>\n"); } - VIR_FREE(items); + virXMLFormatElement(buf, "filterref", &attrBuf, &childBuf); return 0; } -- 2.40.1
On a Friday in 2023, Peter Krempa wrote:
Peter Krempa (15): virNetDevBandwidthParse: Don't validate element name virNetDevBandwidthParse: Use 'virXMLPropUInt' to parse 'classID' virNetDevBandwidthParseRate: Refactor parsing virNetDevBandwidthParse: Use virXMLNodeGetSubelement instead of looped parser virNetworkDHCPDefParseXML: Refactor cleanup util: xml: Introduce virXMLNodeGetSubelementList nwfilterxml2xmltest: Add test case for parser and formatter quirks conf: network: Refactor XML parsing in virNetworkDHCPDefParseXML conf: nwfilter: Refactor XML formatting in virNWFilterRuleDefFormat virNWFilterRuleDef: Turn 'action' and 'tt' into proper enum types virNWFilterRuleParse: Parse 'priority' via 'virXMLPropInt' virNWFilterRuleParse: Refactor attribute parser virNWFilterRuleDefDetailsFormat: Refactor formatter conf: nwfilter: Refactor virNWFilterIncludeParse conf: nwfilter: Refactor virNWFilterFormatParamAttributes
src/conf/netdev_bandwidth_conf.c | 141 ++----- src/conf/network_conf.c | 74 ++-- src/conf/nwfilter_conf.c | 410 ++++++++------------ src/conf/nwfilter_conf.h | 4 +- src/conf/nwfilter_params.c | 45 +-- src/libvirt_private.syms | 1 + src/nwfilter/nwfilter_ebiptables_driver.c | 5 + src/util/virxml.c | 34 ++ src/util/virxml.h | 5 + tests/nwfilterxml2xmlin/quirks-invalid.xml | 13 + tests/nwfilterxml2xmlout/quirks-invalid.xml | 7 + tests/nwfilterxml2xmltest.c | 5 + 12 files changed, 326 insertions(+), 418 deletions(-) create mode 100644 tests/nwfilterxml2xmlin/quirks-invalid.xml create mode 100644 tests/nwfilterxml2xmlout/quirks-invalid.xml
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Peter Krempa