[libnftnl PATCH] src/utils: Add a common dev_array parser

[libnftnl PATCH] src/utils: Add a common dev_array parser

[Phil Sutter]

Parsing of dev_array fields in flowtable and chain are identical, merge
them into a shared function nftnl_parse_devs() which does a quick scan
through the nested attributes to check validity and calculate required
array size instead of calling realloc() as needed.

This required to align structs nftnl_chain and nftnl_flowtable field
dev_array_len types, though uint32_t should match the size of int on
both 32 and 64 bit architectures.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/utils.h |  4 ++++
 src/chain.c     | 41 +++--------------------------------------
 src/flowtable.c | 40 ++--------------------------------------
 src/utils.c     | 39 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 48 insertions(+), 76 deletions(-)

diff --git a/include/utils.h b/include/utils.h
index eed61277595e2..dd8fbd05213b0 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -82,4 +82,8 @@ int nftnl_fprintf(FILE *fpconst, const void *obj, uint32_t cmd, uint32_t type,
 int nftnl_set_str_attr(const char **dptr, uint32_t *flags,
 		       uint16_t attr, const void *data, uint32_t data_len);
 
+struct nlattr;
+int nftnl_parse_devs(struct nlattr *nest,
+		     const char ***dev_array_p, uint32_t *len_p);
+
 #endif
diff --git a/src/chain.c b/src/chain.c
index 0b68939fe21a7..5bd2fc45b2137 100644
--- a/src/chain.c
+++ b/src/chain.c
@@ -38,7 +38,7 @@ struct nftnl_chain {
 	const char	*table;
 	const char	*dev;
 	const char	**dev_array;
-	int		dev_array_len;
+	uint32_t	dev_array_len;
 	uint32_t	family;
 	uint32_t	policy;
 	uint32_t	hooknum;
@@ -664,42 +664,6 @@ static int nftnl_chain_parse_hook_cb(const struct nlattr *attr, void *data)
 	return MNL_CB_OK;
 }
 
-static int nftnl_chain_parse_devs(struct nlattr *nest, struct nftnl_chain *c)
-{
-	const char **dev_array, **tmp;
-	int len = 0, size = 8;
-	struct nlattr *attr;
-
-	dev_array = calloc(8, sizeof(char *));
-	if (!dev_array)
-		return -1;
-
-	mnl_attr_for_each_nested(attr, nest) {
-		if (mnl_attr_get_type(attr) != NFTA_DEVICE_NAME)
-			goto err;
-		dev_array[len++] = strdup(mnl_attr_get_str(attr));
-		if (len >= size) {
-			tmp = realloc(dev_array, size * 2 * sizeof(char *));
-			if (!tmp)
-				goto err;
-
-			size *= 2;
-			memset(&tmp[len], 0, (size - len) * sizeof(char *));
-			dev_array = tmp;
-		}
-	}
-
-	c->dev_array = dev_array;
-	c->dev_array_len = len;
-
-	return 0;
-err:
-	while (len--)
-		xfree(dev_array[len]);
-	xfree(dev_array);
-	return -1;
-}
-
 static int nftnl_chain_parse_hook(struct nlattr *attr, struct nftnl_chain *c)
 {
 	struct nlattr *tb[NFTA_HOOK_MAX+1] = {};
@@ -723,7 +687,8 @@ static int nftnl_chain_parse_hook(struct nlattr *attr, struct nftnl_chain *c)
 		c->flags |= (1 << NFTNL_CHAIN_DEV);
 	}
 	if (tb[NFTA_HOOK_DEVS]) {
-		ret = nftnl_chain_parse_devs(tb[NFTA_HOOK_DEVS], c);
+		ret = nftnl_parse_devs(tb[NFTA_HOOK_DEVS],
+				       &c->dev_array, &c->dev_array_len);
 		if (ret < 0)
 			return -1;
 		c->flags |= (1 << NFTNL_CHAIN_DEVICES);
diff --git a/src/flowtable.c b/src/flowtable.c
index 41a1456bb19b2..d54b3627371ef 100644
--- a/src/flowtable.c
+++ b/src/flowtable.c
@@ -402,43 +402,6 @@ static int nftnl_flowtable_parse_hook_cb(const struct nlattr *attr, void *data)
 	return MNL_CB_OK;
 }
 
-static int nftnl_flowtable_parse_devs(struct nlattr *nest,
-				      struct nftnl_flowtable *c)
-{
-	const char **dev_array, **tmp;
-	int len = 0, size = 8;
-	struct nlattr *attr;
-
-	dev_array = calloc(8, sizeof(char *));
-	if (!dev_array)
-		return -1;
-
-	mnl_attr_for_each_nested(attr, nest) {
-		if (mnl_attr_get_type(attr) != NFTA_DEVICE_NAME)
-			goto err;
-		dev_array[len++] = strdup(mnl_attr_get_str(attr));
-		if (len >= size) {
-			tmp = realloc(dev_array, size * 2 * sizeof(char *));
-			if (!tmp)
-				goto err;
-
-			size *= 2;
-			memset(&tmp[len], 0, (size - len) * sizeof(char *));
-			dev_array = tmp;
-		}
-	}
-
-	c->dev_array = dev_array;
-	c->dev_array_len = len;
-
-	return 0;
-err:
-	while (len--)
-		xfree(dev_array[len]);
-	xfree(dev_array);
-	return -1;
-}
-
 static int nftnl_flowtable_parse_hook(struct nlattr *attr, struct nftnl_flowtable *c)
 {
 	struct nlattr *tb[NFTA_FLOWTABLE_HOOK_MAX + 1] = {};
@@ -456,7 +419,8 @@ static int nftnl_flowtable_parse_hook(struct nlattr *attr, struct nftnl_flowtabl
 		c->flags |= (1 << NFTNL_FLOWTABLE_PRIO);
 	}
 	if (tb[NFTA_FLOWTABLE_HOOK_DEVS]) {
-		ret = nftnl_flowtable_parse_devs(tb[NFTA_FLOWTABLE_HOOK_DEVS], c);
+		ret = nftnl_parse_devs(tb[NFTA_FLOWTABLE_HOOK_DEVS],
+				       &c->dev_array, &c->dev_array_len);
 		if (ret < 0)
 			return -1;
 		c->flags |= (1 << NFTNL_FLOWTABLE_DEVICES);
diff --git a/src/utils.c b/src/utils.c
index 2f1ffd6227583..9235806b2c95e 100644
--- a/src/utils.c
+++ b/src/utils.c
@@ -19,6 +19,7 @@
 
 #include <libnftnl/common.h>
 
+#include <libmnl/libmnl.h>
 #include <linux/netfilter.h>
 #include <linux/netfilter/nf_tables.h>
 
@@ -150,3 +151,41 @@ int nftnl_set_str_attr(const char **dptr, uint32_t *flags,
 	*flags |= (1 << attr);
 	return 0;
 }
+
+int nftnl_parse_devs(struct nlattr *nest,
+		     const char ***dev_array_p, uint32_t *len_p)
+{
+	struct nlattr *attr;
+	const char *dup;
+	int len = 0;
+
+	mnl_attr_for_each_nested(attr, nest) {
+		if (mnl_attr_get_type(attr) != NFTA_DEVICE_NAME)
+			return -1;
+		len++;
+	}
+
+	if (dev_array_p) {
+		*dev_array_p = calloc(len, sizeof(char *));
+		if (!*dev_array_p)
+			return -1;
+
+		len = 0;
+		mnl_attr_for_each_nested(attr, nest) {
+			dup = strdup(mnl_attr_get_str(attr));
+			if (!dup)
+				goto err_free;
+
+			(*dev_array_p)[len++] = dup;
+		}
+	}
+	if (len_p)
+		*len_p = len;
+
+	return 0;
+err_free:
+	while (len--)
+		xfree((*dev_array_p)[len]);
+	xfree(*dev_array_p);
+	return -1;
+}
-- 
2.47.0

Re: [libnftnl PATCH] src/utils: Add a common dev_array parser

[Pablo Neira Ayuso]

On Wed, Oct 16, 2024 at 06:42:23PM +0200, Phil Sutter wrote:
> Parsing of dev_array fields in flowtable and chain are identical, merge
> them into a shared function nftnl_parse_devs() which does a quick scan
> through the nested attributes to check validity and calculate required
> array size instead of calling realloc() as needed.
> 
> This required to align structs nftnl_chain and nftnl_flowtable field
> dev_array_len types, though uint32_t should match the size of int on
> both 32 and 64 bit architectures.

Maybe go the extra mile and add an internal object for string arrays:

struct nftnl_str_array {
        const char      **array;
        uint32_t        len;
};

and use it in chain and flowtable?

Re: [libnftnl PATCH] src/utils: Add a common dev_array parser

[Phil Sutter]

On Tue, Oct 22, 2024 at 03:07:19PM +0200, Pablo Neira Ayuso wrote:
> On Wed, Oct 16, 2024 at 06:42:23PM +0200, Phil Sutter wrote:
> > Parsing of dev_array fields in flowtable and chain are identical, merge
> > them into a shared function nftnl_parse_devs() which does a quick scan
> > through the nested attributes to check validity and calculate required
> > array size instead of calling realloc() as needed.
> > 
> > This required to align structs nftnl_chain and nftnl_flowtable field
> > dev_array_len types, though uint32_t should match the size of int on
> > both 32 and 64 bit architectures.
> 
> Maybe go the extra mile and add an internal object for string arrays:
> 
> struct nftnl_str_array {
>         const char      **array;
>         uint32_t        len;
> };
> 
> and use it in chain and flowtable?

ACK, will do.

Thanks, Phil