src: xml: use nodes instead of attributes

When working with XML, it's desirable to work with nodes better than
attributes.

Table/chain/rules had attributes in their XML representation, and
this patch transform those to nodes, ie:

Before:
<table name="filter">
	<family>ip</family>
	<table_flags>0</table_flags>
</table>

After:
<table>
	<name>filter</name>
	<family>ip</family>
	<table_flags>0</table_flags>
</table>

While at it:
 * There was a lot of redundant code that is now collapsed with the
new nft_mxml_family_parse() helper function.

 * I've added a small fix: additional validation for the name of
the current XML object, and also replace raw strtol calls to nft_strtoi.

 * Also, all XML testfiles are updated to keep passing the parsing tests and
mantain the repo in consisten state.

Signed-off-by: Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

6 files changed, 81 insertions, 94 deletions

diff --git a/src/chain.c b/src/chain.c
index 0dd3461..3ad52fd 100644
--- a/src/chain.c
+++ b/src/chain.c
@@ -587,8 +587,7 @@ static int nft_chain_xml_parse(struct nft_chain *c, char *xml)
 #ifdef XML_PARSING
 	mxml_node_t *tree = NULL;
 	mxml_node_t *node = NULL;
-	char *endptr = NULL;
-	uint64_t utmp;
+	const char *name;
 	const char *hooknum_str;
 	int family, hooknum;
 
@@ -599,54 +598,43 @@ static int nft_chain_xml_parse(struct nft_chain *c, char *xml)
 	if (tree == NULL)
 		return -1;
 
-	/* Get and set <chain name="xxx" ... >*/
-	if (mxmlElementGetAttr(tree, "name") == NULL) {
+	if (strcmp(tree->value.opaque, "chain") != 0) {
 		mxmlDelete(tree);
 		return -1;
 	}
-	strncpy(c->name, mxmlElementGetAttr(tree, "name"),
-		NFT_CHAIN_MAXNAMELEN);
-	c->flags |= (1 << NFT_CHAIN_ATTR_NAME);
 
-	/* Get and set <chain handle="x" ... >*/
-	if (mxmlElementGetAttr(tree, "handle") == NULL) {
+	name = nft_mxml_str_parse(tree, "name", MXML_DESCEND_FIRST);
+	if (name == NULL) {
 		mxmlDelete(tree);
 		return -1;
 	}
 
-	utmp = strtoull(mxmlElementGetAttr(tree, "handle"), &endptr, 10);
-	if (utmp == UINT64_MAX || utmp < 0 || *endptr) {
+	strncpy(c->name, name, NFT_CHAIN_MAXNAMELEN);
+	xfree(name);
+	c->flags |= (1 << NFT_CHAIN_ATTR_NAME);
+
+	if (nft_mxml_num_parse(tree, "handle", MXML_DESCEND_FIRST, BASE_DEC,
+			       &c->handle, NFT_TYPE_U64) != 0) {
 		mxmlDelete(tree);
 		return -1;
 	}
 
-	c->handle = utmp;
 	c->flags |= (1 << NFT_CHAIN_ATTR_HANDLE);
 
-	/* Get and set <chain bytes="x" ... >*/
-	if (mxmlElementGetAttr(tree, "bytes") == NULL) {
+	if (nft_mxml_num_parse(tree, "bytes", MXML_DESCEND_FIRST, BASE_DEC,
+			       &c->bytes, NFT_TYPE_U64) != 0) {
 		mxmlDelete(tree);
 		return -1;
 	}
-	utmp = strtoull(mxmlElementGetAttr(tree, "bytes"), &endptr, 10);
-	if (utmp == UINT64_MAX || utmp < 0 || *endptr) {
-		mxmlDelete(tree);
-		return -1;
-	}
-	c->bytes = utmp;
+
 	c->flags |= (1 << NFT_CHAIN_ATTR_BYTES);
 
-	/* Get and set <chain packets="x" ... > */
-	if (mxmlElementGetAttr(tree, "packets") == NULL) {
+	if (nft_mxml_num_parse(tree, "packets", MXML_DESCEND_FIRST, BASE_DEC,
+			       &c->packets, NFT_TYPE_U64) != 0) {
 		mxmlDelete(tree);
 		return -1;
 	}
-	utmp = strtoull(mxmlElementGetAttr(tree, "packets"), &endptr, 10);
-	if (utmp == UINT64_MAX || utmp < 0 || *endptr) {
-		mxmlDelete(tree);
-		return -1;
-	}
-	c->packets = utmp;
+
 	c->flags |= (1 << NFT_CHAIN_ATTR_PACKETS);
 
 	/* Get and set <type> */
@@ -724,13 +712,7 @@ static int nft_chain_xml_parse(struct nft_chain *c, char *xml)
 	c->flags |= (1 << NFT_CHAIN_ATTR_POLICY);
 
 	/* Get and set <family> */
-	node = mxmlFindElement(tree, tree, "family", NULL, NULL, MXML_DESCEND);
-	if (node == NULL) {
-		mxmlDelete(tree);
-		return -1;
-	}
-
-	family = nft_str2family(node->child->value.opaque);
+	family = nft_mxml_family_parse(tree, "family", MXML_DESCEND_FIRST);
 	if (family < 0) {
 		mxmlDelete(tree);
 		return -1;
@@ -810,11 +792,11 @@ static int nft_chain_snprintf_xml(char *buf, size_t size, struct nft_chain *c)
 {
 	int ret, len = size, offset = 0;
 
-	ret = snprintf(buf, size,
-		       "<chain name=\"%s\" handle=\"%"PRIu64"\""
-		       " bytes=\"%"PRIu64"\" packets=\"%"PRIu64"\">"
-		       "<type>%s</type><table>%s</table><prio>%d</prio>"
-		       "<use>%d</use><hooknum>%s</hooknum>",
+	ret = snprintf(buf, size, "<chain><name>%s</name>"
+		       "<handle>%"PRIu64"</handle><bytes>%"PRIu64"</bytes>"
+		       "<packets>%"PRIu64"</packets><type>%s</type>"
+		       "<table>%s</table><prio>%d</prio><use>%d</use>"
+		       "<hooknum>%s</hooknum>",
 		       c->name, c->handle, c->bytes, c->packets,
 		       c->type, c->table,
 		       c->prio, c->use, hooknum2str_array[c->hooknum]);
diff --git a/src/expr/nat.c b/src/expr/nat.c
index 7446258..4b7ec27 100644
--- a/src/expr/nat.c
+++ b/src/expr/nat.c
@@ -188,7 +188,7 @@ static int nft_rule_expr_nat_xml_parse(struct nft_rule_expr *e, mxml_node_t *tre
 {
 #ifdef XML_PARSING
 	struct nft_expr_nat *nat = nft_expr_data(e);
-	const char *nat_type, *family_str;
+	const char *nat_type;
 	int32_t reg;
 	int family;
 
@@ -205,13 +205,11 @@ static int nft_rule_expr_nat_xml_parse(struct nft_rule_expr *e, mxml_node_t *tre
 
 	e->flags |= (1 << NFT_EXPR_NAT_TYPE);
 
-	family_str = nft_mxml_str_parse(tree, "family", MXML_DESCEND_FIRST);
-	if (family_str == NULL)
-		return -1;
-
-	family = nft_str2family(family_str);
-	if (family < 0)
+	family = nft_mxml_family_parse(tree, "family", MXML_DESCEND_FIRST);
+	if (family < 0) {
+		mxmlDelete(tree);
 		return -1;
+	}
 
 	nat->family = family;
 	e->flags |= (1 << NFT_EXPR_NAT_FAMILY);
diff --git a/src/internal.h b/src/internal.h
index 8d11acf..1ebdb1a 100644
--- a/src/internal.h
+++ b/src/internal.h
@@ -36,6 +36,7 @@ union nft_data_reg;
 int nft_mxml_data_reg_parse(mxml_node_t *tree, const char *node_name, union nft_data_reg *data_reg);
 int nft_mxml_num_parse(mxml_node_t *tree, const char *node_name, uint32_t mxml_flags, int base, void *number, enum nft_type type);
 const char *nft_mxml_str_parse(mxml_node_t *tree, const char *node_name, uint32_t mxml_flags);
+int nft_mxml_family_parse(mxml_node_t *tree, const char *node_name, uint32_t mxml_flags);
 struct nft_set_elem *nft_mxml_set_elem_parse(mxml_node_t *node);
 #endif
 
diff --git a/src/mxml.c b/src/mxml.c
index ed1134f..1f0a2df 100644
--- a/src/mxml.c
+++ b/src/mxml.c
@@ -166,6 +166,25 @@ const char *nft_mxml_str_parse(mxml_node_t *tree, const char *node_name,
 	return strdup(node->child->value.opaque);
 }
 
+int nft_mxml_family_parse(mxml_node_t *tree, const char *node_name,
+			  uint32_t mxml_flags)
+{
+	const char *family_str;
+	int family;
+
+	family_str = nft_mxml_str_parse(tree, node_name, mxml_flags);
+	if (family_str == NULL)
+		return -1;
+
+	family = nft_str2family(family_str);
+	xfree(family_str);
+
+	if (family < 0)
+		errno = EAFNOSUPPORT;
+
+	return family;
+}
+
 struct nft_set_elem *nft_mxml_set_elem_parse(mxml_node_t *node)
 {
 	mxml_node_t *save;
diff --git a/src/rule.c b/src/rule.c
index 3e9ca89..a315cca 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -477,8 +477,8 @@ static int nft_rule_xml_parse(struct nft_rule *r, char *xml)
 	mxml_node_t *node = NULL;
 	mxml_node_t *save = NULL;
 	struct nft_rule_expr *e;
-	char *endptr = NULL;
-	uint64_t tmp;
+	const char *table;
+	const char *chain;
 	int family;
 
 	/* Load the tree */
@@ -486,13 +486,12 @@ static int nft_rule_xml_parse(struct nft_rule *r, char *xml)
 	if (tree == NULL)
 		return -1;
 
-	/* get and set <rule ... family=X ... > */
-	if (mxmlElementGetAttr(tree, "family") == NULL) {
+	if (strcmp(tree->value.opaque, "rule") != 0) {
 		mxmlDelete(tree);
 		return -1;
 	}
 
-	family = nft_str2family(mxmlElementGetAttr(tree, "family"));
+	family = nft_mxml_family_parse(tree, "family", MXML_DESCEND_FIRST);
 	if (family < 0) {
 		mxmlDelete(tree);
 		return -1;
@@ -501,8 +500,8 @@ static int nft_rule_xml_parse(struct nft_rule *r, char *xml)
 	r->family = family;
 	r->flags |= (1 << NFT_RULE_ATTR_FAMILY);
 
-	/* get and set <rule ... table=X ...> */
-	if (mxmlElementGetAttr(tree, "table") == NULL) {
+	table = nft_mxml_str_parse(tree, "table", MXML_DESCEND_FIRST);
+	if (table == NULL) {
 		mxmlDelete(tree);
 		return -1;
 	}
@@ -510,11 +509,11 @@ static int nft_rule_xml_parse(struct nft_rule *r, char *xml)
 	if (r->table)
 		xfree(r->table);
 
-	r->table = strdup(mxmlElementGetAttr(tree, "table"));
+	r->table = (char *)table;
 	r->flags |= (1 << NFT_RULE_ATTR_TABLE);
 
-	/* get and set <rule ... chain=X ...> */
-	if (mxmlElementGetAttr(tree, "chain") == NULL) {
+	chain = nft_mxml_str_parse(tree, "chain", MXML_DESCEND_FIRST);
+	if (chain == NULL) {
 		mxmlDelete(tree);
 		return -1;
 	}
@@ -522,21 +521,15 @@ static int nft_rule_xml_parse(struct nft_rule *r, char *xml)
 	if (r->chain)
 		xfree(r->chain);
 
-	r->chain = strdup(mxmlElementGetAttr(tree, "chain"));
+	r->chain = (char *)chain;
 	r->flags |= (1 << NFT_RULE_ATTR_CHAIN);
 
-	/* get and set <rule ... handle=X ...> */
-	if (mxmlElementGetAttr(tree, "handle") == NULL) {
-		mxmlDelete(tree);
-		return -1;
-	}
-	tmp = strtoull(mxmlElementGetAttr(tree, "handle"), &endptr, 10);
-	if (tmp == UINT64_MAX || tmp < 0 || *endptr) {
+	if (nft_mxml_num_parse(tree, "handle", MXML_DESCEND_FIRST, BASE_DEC,
+			       &r->handle, NFT_TYPE_U64) != 0) {
 		mxmlDelete(tree);
 		return -1;
 	}
 
-	r->handle = tmp;
 	r->flags |= (1 << NFT_RULE_ATTR_HANDLE);
 
 	/* get and set <rule_flags> */
@@ -551,28 +544,26 @@ static int nft_rule_xml_parse(struct nft_rule *r, char *xml)
 	/* <compat_proto> is optional */
 	node = mxmlFindElement(tree, tree, "compat_proto", NULL, NULL,
 			       MXML_DESCEND);
-	if (node != NULL) {
-		tmp = strtoull(node->child->value.opaque, &endptr, 10);
-		if (tmp > UINT32_MAX || tmp < 0 || *endptr) {
+	if (node != NULL && node->child != NULL) {
+		if (nft_strtoi(node->child->value.opaque, BASE_DEC,
+			       &r->compat.proto, NFT_TYPE_U32) != 0) {
 			mxmlDelete(tree);
 			return -1;
 		}
 
-		r->compat.proto = tmp;
 		r->flags |= (1 << NFT_RULE_ATTR_COMPAT_PROTO);
 	}
 
 	/* <compat_flags> is optional */
 	node = mxmlFindElement(tree, tree, "compat_flags", NULL, NULL,
 			       MXML_DESCEND);
-	if (node != NULL) {
-		tmp = strtoull(node->child->value.opaque, &endptr, 10);
-		if (tmp > UINT32_MAX || tmp < 0 || *endptr) {
+	if (node != NULL && node->child != NULL) {
+		if (nft_strtoi(node->child->value.opaque, BASE_DEC,
+			       &r->compat.flags, NFT_TYPE_U32) != 0) {
 			mxmlDelete(tree);
 			return -1;
 		}
 
-		r->compat.flags = tmp;
 		r->flags |= (1 << NFT_RULE_ATTR_COMPAT_FLAGS);
 	}
 
@@ -676,11 +667,11 @@ static int nft_rule_snprintf_xml(char *buf, size_t size, struct nft_rule *r,
 	int ret, len = size, offset = 0;
 	struct nft_rule_expr *expr;
 
-	ret = snprintf(buf, size,
-		"<rule family=\"%s\" table=\"%s\" "
-			"chain=\"%s\" handle=\"%llu\">",
-				nft_family2str(r->family), r->table, r->chain,
-				(unsigned long long)r->handle);
+	ret = snprintf(buf, size, "<rule><family>%s</family>"
+		       "<table>%s</table><chain>%s</chain>"
+		       "<handle>%llu</handle>",
+		       nft_family2str(r->family), r->table, r->chain,
+		       (unsigned long long)r->handle);
 	SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
 
 	ret = snprintf(buf+offset, len, "<rule_flags>%u</rule_flags>",
diff --git a/src/table.c b/src/table.c
index 6875dd7..bb66717 100644
--- a/src/table.c
+++ b/src/table.c
@@ -222,7 +222,7 @@ static int nft_table_xml_parse(struct nft_table *t, char *xml)
 {
 #ifdef XML_PARSING
 	mxml_node_t *tree = NULL;
-	mxml_node_t *node = NULL;
+	const char *name;
 	int family;
 
 	/* NOTE: all XML nodes are mandatory */
@@ -232,8 +232,13 @@ static int nft_table_xml_parse(struct nft_table *t, char *xml)
 	if (tree == NULL)
 		return -1;
 
-	/* Get and set the name of the table */
-	if (mxmlElementGetAttr(tree, "name") == NULL) {
+	if (strcmp(tree->value.opaque, "table") != 0) {
+		mxmlDelete(tree);
+		return -1;
+	}
+
+	name = nft_mxml_str_parse(tree, "name", MXML_DESCEND_FIRST);
+	if (name == NULL) {
 		mxmlDelete(tree);
 		return -1;
 	}
@@ -241,18 +246,10 @@ static int nft_table_xml_parse(struct nft_table *t, char *xml)
 	if (t->name)
 		xfree(t->name);
 
-	t->name = strdup(mxmlElementGetAttr(tree, "name"));
+	t->name = name;
 	t->flags |= (1 << NFT_TABLE_ATTR_NAME);
 
-	/* Get the and set <family> node */
-	node = mxmlFindElement(tree, tree, "family", NULL, NULL,
-			       MXML_DESCEND_FIRST);
-	if (node == NULL) {
-		mxmlDelete(tree);
-		return -1;
-	}
-
-	family = nft_str2family(node->child->value.opaque);
+	family = nft_mxml_family_parse(tree, "family", MXML_DESCEND_FIRST);
 	if (family < 0) {
 		mxmlDelete(tree);
 		return -1;
@@ -261,7 +258,6 @@ static int nft_table_xml_parse(struct nft_table *t, char *xml)
 	t->family = family;
 	t->flags |= (1 << NFT_TABLE_ATTR_FAMILY);
 
-	/* Get and set <table_flags> */
 	if (nft_mxml_num_parse(tree, "table_flags", MXML_DESCEND, BASE_DEC,
 			       &t->table_flags, NFT_TYPE_U32) != 0) {
 		mxmlDelete(tree);
@@ -360,7 +356,7 @@ static int nft_table_snprintf_json(char *buf, size_t size, struct nft_table *t)
 
 static int nft_table_snprintf_xml(char *buf, size_t size, struct nft_table *t)
 {
-	return snprintf(buf, size, "<table name=\"%s\"><family>%s</family>"
+	return snprintf(buf, size, "<table><name>%s</name><family>%s</family>"
 			"<table_flags>%d</table_flags></table>",
 			t->name, nft_family2str(t->family), t->table_flags);
 }