6:28 a.m.
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
6:28 a.m.
New subject: [PATCH 01/15] virNetDevBandwidthParse: Don't validate element name
Callers make sure to pass the correct element.
Signed-off-by: Peter Krempa <pkrempa(a)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
6:28 a.m.
New subject: [PATCH 02/15] virNetDevBandwidthParse: Use 'virXMLPropUInt' to parse 'classID'
Signed-off-by: Peter Krempa <pkrempa(a)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
6:28 a.m.
New subject: [PATCH 03/15] virNetDevBandwidthParseRate: Refactor parsing
Remove the unnecessary check for valid arguments and use
virXMLPropULongLong instead of hand-written property parsers.
Signed-off-by: Peter Krempa <pkrempa(a)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
6:28 a.m.
New subject: [PATCH 04/15] virNetDevBandwidthParse: Use virXMLNodeGetSubelement instead of looped parser
Extrac the 'inbound'/'outbound' subelements using
virXMLNodeGetSubelement to simplify the code.
Signed-off-by: Peter Krempa <pkrempa(a)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
7:27 a.m.
New subject: [PATCH 04/15] virNetDevBandwidthParse: Use virXMLNodeGetSubelement instead of looped parser
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(a)redhat.com>
---
src/conf/netdev_bandwidth_conf.c | 34 ++++----------------------------
1 file changed, 4 insertions(+), 30 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
6:28 a.m.
New subject: [PATCH 05/15] virNetworkDHCPDefParseXML: Refactor cleanup
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(a)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
6:28 a.m.
New subject: [PATCH 06/15] util: xml: Introduce virXMLNodeGetSubelementList
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(a)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
7:39 a.m.
New subject: [PATCH 06/15] util: xml: Introduce virXMLNodeGetSubelementList
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(a)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(a)redhat.com>
Jano
6:28 a.m.
New subject: [PATCH 07/15] nwfilterxml2xmltest: Add test case for parser and formatter quirks
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(a)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
7:44 a.m.
New subject: [PATCH 07/15] nwfilterxml2xmltest: Add test case for parser and formatter quirks
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(a)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(a)redhat.com>
Jano
6:28 a.m.
New subject: [PATCH 08/15] conf: network: Refactor XML parsing in virNetworkDHCPDefParseXML
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(a)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
6:28 a.m.
New subject: [PATCH 09/15] conf: nwfilter: Refactor XML formatting in virNWFilterRuleDefFormat
Use virXMLFormatElement to simplify the formatter. Drop return value of
virNWFilterRuleDefFormat as there are no errors to report.
Signed-off-by: Peter Krempa <pkrempa(a)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
6:28 a.m.
New subject: [PATCH 10/15] virNWFilterRuleDef: Turn 'action' and 'tt' into proper enum types
Convert the fields to the proper types and use virXMLPropEnum for
parsing.
Signed-off-by: Peter Krempa <pkrempa(a)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
6:28 a.m.
New subject: [PATCH 11/15] virNWFilterRuleParse: Parse 'priority' via 'virXMLPropInt'
Use modern parsing. Invalid numbers are now rejected. Semantis for
numbers out of range is preserved.
Signed-off-by: Peter Krempa <pkrempa(a)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
6:28 a.m.
New subject: [PATCH 12/15] virNWFilterRuleParse: Refactor attribute parser
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(a)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
6:28 a.m.
New subject: [PATCH 13/15] virNWFilterRuleDefDetailsFormat: Refactor formatter
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(a)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
6:28 a.m.
New subject: [PATCH 14/15] conf: nwfilter: Refactor virNWFilterIncludeParse
Use automatic memory freeing and modern XML parsers to simplify the
function.
Signed-off-by: Peter Krempa <pkrempa(a)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
6:28 a.m.
New subject: [PATCH 15/15] conf: nwfilter: Refactor virNWFilterFormatParamAttributes
Use virXMLFormatElement and simplify the formatter.
Signed-off-by: Peter Krempa <pkrempa(a)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
7:47 a.m.
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(a)redhat.com>
Jano
574
days inactive
574
days old
19 comments
2 participants
participants (2)
-
Ján Tomko -
Peter Krempa