[iptables PATCH v2 00/10] nft: Sorted chain listing et al.

[iptables PATCH v2 00/10] nft: Sorted chain listing et al.

[Phil Sutter]

This is a respin of my original series after getting rid of a few
initial ("fallout") patches. It implements structs nft_chain and
nft_chain_list to avoid changes to libnftnl as requested. Obviously this
introduces some code duplication as some bits from libnftnl have to be
replicated within iptables now.

Phil Sutter (10):
  nft: Fix selective chain compatibility checks
  nft: Implement nft_chain_foreach()
  nft: cache: Introduce nft_cache_add_chain()
  nft: Eliminate nft_chain_list_get()
  nft: cache: Move nft_chain_find() over
  nft: Introduce struct nft_chain
  nft: Introduce a dedicated base chain array
  nft: cache: Sort custom chains by name
  tests: shell: Drop any dump sorting in place
  nft: Avoid pointless table/chain creation

 iptables/Makefile.am                          |   2 +-
 iptables/nft-cache.c                          | 165 +++++---
 iptables/nft-cache.h                          |  11 +-
 iptables/nft-chain.c                          |  59 +++
 iptables/nft-chain.h                          |  87 ++++
 iptables/nft.c                                | 382 ++++++++++--------
 iptables/nft.h                                |  10 +-
 .../ebtables/0002-ebtables-save-restore_0     |   2 +-
 .../firewalld-restore/0001-firewalld_0        |  17 +-
 .../ipt-restore/0007-flush-noflush_0          |   4 +-
 .../ipt-restore/0014-verbose-restore_0        |   2 +-
 iptables/xtables-save.c                       |   8 +-
 12 files changed, 494 insertions(+), 255 deletions(-)
 create mode 100644 iptables/nft-chain.c
 create mode 100644 iptables/nft-chain.h

-- 
2.28.0

[iptables PATCH v2 01/10] nft: Fix selective chain compatibility checks

[Phil Sutter]

Since commit 80251bc2a56ed ("nft: remove cache build calls"), 'chain'
parameter passed to nft_chain_list_get() is no longer effective. To
still support running nft_is_chain_compatible() on specific chains only,
add a short path to nft_is_table_compatible().

Follow-up patches will kill nft_chain_list_get(), so don't bother
dropping the unused parameter from its signature.

Fixes: 80251bc2a56ed ("nft: remove cache build calls")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/iptables/nft.c b/iptables/nft.c
index 27bb98d184c7c..669e29d4cf88f 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -3453,6 +3453,12 @@ bool nft_is_table_compatible(struct nft_handle *h,
 {
 	struct nftnl_chain_list *clist;
 
+	if (chain) {
+		struct nftnl_chain *c = nft_chain_find(h, table, chain);
+
+		return c && !nft_is_chain_compatible(c, h);
+	}
+
 	clist = nft_chain_list_get(h, table, chain);
 	if (clist == NULL)
 		return false;
-- 
2.28.0

[iptables PATCH v2 02/10] nft: Implement nft_chain_foreach()

[Phil Sutter]

This is just a fancy wrapper around nftnl_chain_list_foreach() with the
added benefit of detecting invalid table names or uninitialized chain
lists. This in turn allows to drop the checks in flush_rule_cache() and
ignore the return code of nft_chain_foreach() as it fails only if the
dropped checks had failed, too.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
Changes since v1:
- Extend commit message.
- Drop pointless code from flush_rule_cache().
- Use nft_chain_foreach() in flush_cache() and nft_is_table_compatible()
  also.
---
 iptables/nft-cache.c    | 29 +++++----------
 iptables/nft.c          | 80 +++++++++++++++--------------------------
 iptables/nft.h          |  3 ++
 iptables/xtables-save.c |  7 +---
 4 files changed, 41 insertions(+), 78 deletions(-)

diff --git a/iptables/nft-cache.c b/iptables/nft-cache.c
index 32cfd6cf0989a..b94766a751db4 100644
--- a/iptables/nft-cache.c
+++ b/iptables/nft-cache.c
@@ -455,21 +455,16 @@ static int fetch_rule_cache(struct nft_handle *h,
 {
 	int i;
 
-	if (t) {
-		struct nftnl_chain_list *list =
-			h->cache->table[t->type].chains;
-
-		return nftnl_chain_list_foreach(list, nft_rule_list_update, h);
-	}
+	if (t)
+		return nft_chain_foreach(h, t->name, nft_rule_list_update, h);
 
 	for (i = 0; i < NFT_TABLE_MAX; i++) {
-		enum nft_table_type type = h->tables[i].type;
 
 		if (!h->tables[i].name)
 			continue;
 
-		if (nftnl_chain_list_foreach(h->cache->table[type].chains,
-					     nft_rule_list_update, h))
+		if (nft_chain_foreach(h, h->tables[i].name,
+				      nft_rule_list_update, h))
 			return -1;
 	}
 	return 0;
@@ -543,17 +538,11 @@ static int __flush_rule_cache(struct nftnl_chain *c, void *data)
 int flush_rule_cache(struct nft_handle *h, const char *table,
 		     struct nftnl_chain *c)
 {
-	const struct builtin_table *t;
-
 	if (c)
 		return __flush_rule_cache(c, NULL);
 
-	t = nft_table_builtin_find(h, table);
-	if (!t || !h->cache->table[t->type].chains)
-		return 0;
-
-	return nftnl_chain_list_foreach(h->cache->table[t->type].chains,
-					__flush_rule_cache, NULL);
+	nft_chain_foreach(h, table, __flush_rule_cache, NULL);
+	return 0;
 }
 
 static int __flush_chain_cache(struct nftnl_chain *c, void *data)
@@ -582,9 +571,9 @@ static int flush_cache(struct nft_handle *h, struct nft_cache *c,
 		table = nft_table_builtin_find(h, tablename);
 		if (!table)
 			return 0;
-		if (c->table[table->type].chains)
-			nftnl_chain_list_foreach(c->table[table->type].chains,
-						 __flush_chain_cache, NULL);
+
+		nft_chain_foreach(h, tablename, __flush_chain_cache, NULL);
+
 		if (c->table[table->type].sets)
 			nftnl_set_list_foreach(c->table[table->type].sets,
 					       __flush_set_cache, NULL);
diff --git a/iptables/nft.c b/iptables/nft.c
index 669e29d4cf88f..4f40be2e60252 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -1589,14 +1589,9 @@ int nft_rule_save(struct nft_handle *h, const char *table, unsigned int format)
 		.h = h,
 		.format = format,
 	};
-	struct nftnl_chain_list *list;
 	int ret;
 
-	list = nft_chain_list_get(h, table, NULL);
-	if (!list)
-		return 0;
-
-	ret = nftnl_chain_list_foreach(list, nft_rule_save_cb, &d);
+	ret = nft_chain_foreach(h, table, nft_rule_save_cb, &d);
 
 	/* the core expects 1 for success and 0 for error */
 	return ret == 0 ? 1 : 0;
@@ -1668,7 +1663,6 @@ int nft_rule_flush(struct nft_handle *h, const char *chain, const char *table,
 		.table = table,
 		.verbose = verbose,
 	};
-	struct nftnl_chain_list *list;
 	struct nftnl_chain *c = NULL;
 	int ret = 0;
 
@@ -1694,14 +1688,8 @@ int nft_rule_flush(struct nft_handle *h, const char *chain, const char *table,
 		return 1;
 	}
 
-	list = nft_chain_list_get(h, table, chain);
-	if (list == NULL) {
-		ret = 1;
-		goto err;
-	}
+	ret = nft_chain_foreach(h, table, nft_rule_flush_cb, &d);
 
-	ret = nftnl_chain_list_foreach(list, nft_rule_flush_cb, &d);
-err:
 	/* the core expects 1 for success and 0 for error */
 	return ret == 0 ? 1 : 0;
 }
@@ -1824,18 +1812,13 @@ int nft_chain_user_del(struct nft_handle *h, const char *chain,
 		.handle = h,
 		.verbose = verbose,
 	};
-	struct nftnl_chain_list *list;
 	struct nftnl_chain *c;
 	int ret = 0;
 
 	nft_fn = nft_chain_user_del;
 
-	list = nft_chain_list_get(h, table, chain);
-	if (list == NULL)
-		return 0;
-
 	if (chain) {
-		c = nftnl_chain_list_lookup_byname(list, chain);
+		c = nft_chain_find(h, table, chain);
 		if (!c) {
 			errno = ENOENT;
 			return 0;
@@ -1847,7 +1830,7 @@ int nft_chain_user_del(struct nft_handle *h, const char *chain,
 		goto out;
 	}
 
-	ret = nftnl_chain_list_foreach(list, __nft_chain_user_del, &d);
+	ret = nft_chain_foreach(h, table, __nft_chain_user_del, &d);
 out:
 	/* the core expects 1 for success and 0 for error */
 	return ret == 0 ? 1 : 0;
@@ -2377,7 +2360,6 @@ int nft_rule_list(struct nft_handle *h, const char *chain, const char *table,
 		.rulenum = rulenum,
 		.cb = ops->print_rule,
 	};
-	struct nftnl_chain_list *list;
 	struct nftnl_chain *c;
 
 	nft_xt_builtin_init(h, table);
@@ -2397,14 +2379,10 @@ int nft_rule_list(struct nft_handle *h, const char *chain, const char *table,
 		return 1;
 	}
 
-	list = nft_chain_list_get(h, table, chain);
-	if (!list)
-		return 0;
-
 	if (ops->print_table_header)
 		ops->print_table_header(table);
 
-	nftnl_chain_list_foreach(list, nft_rule_list_cb, &d);
+	nft_chain_foreach(h, table, nft_rule_list_cb, &d);
 	return 1;
 }
 
@@ -2415,6 +2393,23 @@ list_save(struct nft_handle *h, struct nftnl_rule *r,
 	nft_rule_print_save(h, r, NFT_RULE_APPEND, format);
 }
 
+int nft_chain_foreach(struct nft_handle *h, const char *table,
+		      int (*cb)(struct nftnl_chain *c, void *data),
+		      void *data)
+{
+	const struct builtin_table *t;
+
+	t = nft_table_builtin_find(h, table);
+	if (!t)
+		return -1;
+
+	if (!h->cache->table[t->type].chains)
+		return -1;
+
+	return nftnl_chain_list_foreach(h->cache->table[t->type].chains,
+					cb, data);
+}
+
 static int nft_rule_list_chain_save(struct nftnl_chain *c, void *data)
 {
 	const char *chain_name = nftnl_chain_get_str(c, NFTNL_CHAIN_NAME);
@@ -2446,24 +2441,19 @@ int nft_rule_list_save(struct nft_handle *h, const char *chain,
 		.save_fmt = true,
 		.cb = list_save,
 	};
-	struct nftnl_chain_list *list;
 	struct nftnl_chain *c;
 	int ret = 0;
 
 	nft_xt_builtin_init(h, table);
 	nft_assert_table_compatible(h, table, chain);
 
-	list = nft_chain_list_get(h, table, chain);
-	if (!list)
-		return 0;
-
 	if (counters < 0)
 		d.format = FMT_C_COUNTS;
 	else if (counters == 0)
 		d.format = FMT_NOCOUNTS;
 
 	if (chain) {
-		c = nftnl_chain_list_lookup_byname(list, chain);
+		c = nft_chain_find(h, table, chain);
 		if (!c)
 			return 0;
 
@@ -2474,10 +2464,10 @@ int nft_rule_list_save(struct nft_handle *h, const char *chain,
 	}
 
 	/* Dump policies and custom chains first */
-	nftnl_chain_list_foreach(list, nft_rule_list_chain_save, &counters);
+	nft_chain_foreach(h, table, nft_rule_list_chain_save, &counters);
 
 	/* Now dump out rules in this table */
-	ret = nftnl_chain_list_foreach(list, nft_rule_list_cb, &d);
+	ret = nft_chain_foreach(h, table, nft_rule_list_cb, &d);
 	return ret == 0 ? 1 : 0;
 }
 
@@ -3339,7 +3329,6 @@ static int __nft_chain_zero_counters(struct nftnl_chain *c, void *data)
 int nft_chain_zero_counters(struct nft_handle *h, const char *chain,
 			    const char *table, bool verbose)
 {
-	struct nftnl_chain_list *list;
 	struct chain_zero_data d = {
 		.handle = h,
 		.verbose = verbose,
@@ -3347,12 +3336,8 @@ int nft_chain_zero_counters(struct nft_handle *h, const char *chain,
 	struct nftnl_chain *c;
 	int ret = 0;
 
-	list = nft_chain_list_get(h, table, chain);
-	if (list == NULL)
-		goto err;
-
 	if (chain) {
-		c = nftnl_chain_list_lookup_byname(list, chain);
+		c = nft_chain_find(h, table, chain);
 		if (!c) {
 			errno = ENOENT;
 			return 0;
@@ -3362,7 +3347,7 @@ int nft_chain_zero_counters(struct nft_handle *h, const char *chain,
 		goto err;
 	}
 
-	ret = nftnl_chain_list_foreach(list, __nft_chain_zero_counters, &d);
+	ret = nft_chain_foreach(h, table, __nft_chain_zero_counters, &d);
 err:
 	/* the core expects 1 for success and 0 for error */
 	return ret == 0 ? 1 : 0;
@@ -3451,22 +3436,13 @@ static int nft_is_chain_compatible(struct nftnl_chain *c, void *data)
 bool nft_is_table_compatible(struct nft_handle *h,
 			     const char *table, const char *chain)
 {
-	struct nftnl_chain_list *clist;
-
 	if (chain) {
 		struct nftnl_chain *c = nft_chain_find(h, table, chain);
 
 		return c && !nft_is_chain_compatible(c, h);
 	}
 
-	clist = nft_chain_list_get(h, table, chain);
-	if (clist == NULL)
-		return false;
-
-	if (nftnl_chain_list_foreach(clist, nft_is_chain_compatible, h))
-		return false;
-
-	return true;
+	return !nft_chain_foreach(h, table, nft_is_chain_compatible, h);
 }
 
 void nft_assert_table_compatible(struct nft_handle *h,
diff --git a/iptables/nft.h b/iptables/nft.h
index 128e09beb805e..949d9d077f23b 100644
--- a/iptables/nft.h
+++ b/iptables/nft.h
@@ -151,6 +151,9 @@ const struct builtin_chain *nft_chain_builtin_find(const struct builtin_table *t
 bool nft_chain_exists(struct nft_handle *h, const char *table, const char *chain);
 void nft_bridge_chain_postprocess(struct nft_handle *h,
 				  struct nftnl_chain *c);
+int nft_chain_foreach(struct nft_handle *h, const char *table,
+		      int (*cb)(struct nftnl_chain *c, void *data),
+		      void *data);
 
 
 /*
diff --git a/iptables/xtables-save.c b/iptables/xtables-save.c
index 92b0c911c5f1c..bf00b0324cc4f 100644
--- a/iptables/xtables-save.c
+++ b/iptables/xtables-save.c
@@ -68,7 +68,6 @@ struct do_output_data {
 static int
 __do_output(struct nft_handle *h, const char *tablename, void *data)
 {
-	struct nftnl_chain_list *chain_list;
 	struct do_output_data *d = data;
 	time_t now;
 
@@ -81,10 +80,6 @@ __do_output(struct nft_handle *h, const char *tablename, void *data)
 		return 0;
 	}
 
-	chain_list = nft_chain_list_get(h, tablename, NULL);
-	if (!chain_list)
-		return 0;
-
 	now = time(NULL);
 	printf("# Generated by %s v%s on %s", prog_name,
 	       prog_vers, ctime(&now));
@@ -92,7 +87,7 @@ __do_output(struct nft_handle *h, const char *tablename, void *data)
 	printf("*%s\n", tablename);
 	/* Dump out chain names first,
 	 * thereby preventing dependency conflicts */
-	nftnl_chain_list_foreach(chain_list, nft_chain_save, h);
+	nft_chain_foreach(h, tablename, nft_chain_save, h);
 	nft_rule_save(h, tablename, d->format);
 	if (d->commit)
 		printf("COMMIT\n");
-- 
2.28.0

[iptables PATCH v2 03/10] nft: cache: Introduce nft_cache_add_chain()

[Phil Sutter]

This is a convenience function for adding a chain to cache, for now just
a simple wrapper around nftnl_chain_list_add_tail().

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
Changes since v1:
- Use the function in nft_chain_builtin_add() as well.
---
 iptables/nft-cache.c | 12 +++++++++---
 iptables/nft-cache.h |  3 +++
 iptables/nft.c       | 16 +++++++---------
 3 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/iptables/nft-cache.c b/iptables/nft-cache.c
index b94766a751db4..a22e693320451 100644
--- a/iptables/nft-cache.c
+++ b/iptables/nft-cache.c
@@ -165,6 +165,13 @@ static int fetch_table_cache(struct nft_handle *h)
 	return 1;
 }
 
+int nft_cache_add_chain(struct nft_handle *h, const struct builtin_table *t,
+			struct nftnl_chain *c)
+{
+	nftnl_chain_list_add_tail(c, h->cache->table[t->type].chains);
+	return 0;
+}
+
 struct nftnl_chain_list_cb_data {
 	struct nft_handle *h;
 	const struct builtin_table *t;
@@ -174,7 +181,6 @@ static int nftnl_chain_list_cb(const struct nlmsghdr *nlh, void *data)
 {
 	struct nftnl_chain_list_cb_data *d = data;
 	const struct builtin_table *t = d->t;
-	struct nftnl_chain_list *list;
 	struct nft_handle *h = d->h;
 	struct nftnl_chain *c;
 	const char *tname;
@@ -196,8 +202,8 @@ static int nftnl_chain_list_cb(const struct nlmsghdr *nlh, void *data)
 		goto out;
 	}
 
-	list = h->cache->table[t->type].chains;
-	nftnl_chain_list_add_tail(c, list);
+	if (nft_cache_add_chain(h, t, c))
+		goto out;
 
 	return MNL_CB_OK;
 out:
diff --git a/iptables/nft-cache.h b/iptables/nft-cache.h
index 76f9fbb6c8ccc..d97f8de255f02 100644
--- a/iptables/nft-cache.h
+++ b/iptables/nft-cache.h
@@ -3,6 +3,7 @@
 
 struct nft_handle;
 struct nft_cmd;
+struct builtin_table;
 
 void nft_cache_level_set(struct nft_handle *h, int level,
 			 const struct nft_cmd *cmd);
@@ -12,6 +13,8 @@ void flush_chain_cache(struct nft_handle *h, const char *tablename);
 int flush_rule_cache(struct nft_handle *h, const char *table,
 		     struct nftnl_chain *c);
 void nft_cache_build(struct nft_handle *h);
+int nft_cache_add_chain(struct nft_handle *h, const struct builtin_table *t,
+			struct nftnl_chain *c);
 
 struct nftnl_chain_list *
 nft_chain_list_get(struct nft_handle *h, const char *table, const char *chain);
diff --git a/iptables/nft.c b/iptables/nft.c
index 4f40be2e60252..8e1a33ba69bf1 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -695,7 +695,7 @@ static void nft_chain_builtin_add(struct nft_handle *h,
 		return;
 
 	batch_chain_add(h, NFT_COMPAT_CHAIN_ADD, c);
-	nftnl_chain_list_add_tail(c, h->cache->table[table->type].chains);
+	nft_cache_add_chain(h, table, c);
 }
 
 /* find if built-in table already exists */
@@ -1696,7 +1696,7 @@ int nft_rule_flush(struct nft_handle *h, const char *chain, const char *table,
 
 int nft_chain_user_add(struct nft_handle *h, const char *chain, const char *table)
 {
-	struct nftnl_chain_list *list;
+	const struct builtin_table *t;
 	struct nftnl_chain *c;
 	int ret;
 
@@ -1720,9 +1720,8 @@ int nft_chain_user_add(struct nft_handle *h, const char *chain, const char *tabl
 
 	ret = batch_chain_add(h, NFT_COMPAT_CHAIN_USER_ADD, c);
 
-	list = nft_chain_list_get(h, table, chain);
-	if (list)
-		nftnl_chain_list_add(c, list);
+	t = nft_table_builtin_find(h, table);
+	nft_cache_add_chain(h, t, c);
 
 	/* the core expects 1 for success and 0 for error */
 	return ret == 0 ? 1 : 0;
@@ -1730,7 +1729,7 @@ int nft_chain_user_add(struct nft_handle *h, const char *chain, const char *tabl
 
 int nft_chain_restore(struct nft_handle *h, const char *chain, const char *table)
 {
-	struct nftnl_chain_list *list;
+	const struct builtin_table *t;
 	struct nftnl_chain *c;
 	bool created = false;
 	int ret;
@@ -1762,9 +1761,8 @@ int nft_chain_restore(struct nft_handle *h, const char *chain, const char *table
 
 	ret = batch_chain_add(h, NFT_COMPAT_CHAIN_USER_ADD, c);
 
-	list = nft_chain_list_get(h, table, chain);
-	if (list)
-		nftnl_chain_list_add(c, list);
+	t = nft_table_builtin_find(h, table);
+	nft_cache_add_chain(h, t, c);
 
 	/* the core expects 1 for success and 0 for error */
 	return ret == 0 ? 1 : 0;
-- 
2.28.0

[iptables PATCH v2 04/10] nft: Eliminate nft_chain_list_get()

[Phil Sutter]

Since introduction of nft_cache_add_chain(), there is merely a single
user of nft_chain_list_get() left. Hence fold the function into its
caller.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft-cache.c | 13 -------------
 iptables/nft-cache.h |  2 --
 iptables/nft.c       |  8 +++++---
 3 files changed, 5 insertions(+), 18 deletions(-)

diff --git a/iptables/nft-cache.c b/iptables/nft-cache.c
index a22e693320451..109524c3fbc79 100644
--- a/iptables/nft-cache.c
+++ b/iptables/nft-cache.c
@@ -684,16 +684,3 @@ nft_set_list_get(struct nft_handle *h, const char *table, const char *set)
 
 	return h->cache->table[t->type].sets;
 }
-
-struct nftnl_chain_list *
-nft_chain_list_get(struct nft_handle *h, const char *table, const char *chain)
-{
-	const struct builtin_table *t;
-
-	t = nft_table_builtin_find(h, table);
-	if (!t)
-		return NULL;
-
-	return h->cache->table[t->type].chains;
-}
-
diff --git a/iptables/nft-cache.h b/iptables/nft-cache.h
index d97f8de255f02..52ad2d396199e 100644
--- a/iptables/nft-cache.h
+++ b/iptables/nft-cache.h
@@ -16,8 +16,6 @@ void nft_cache_build(struct nft_handle *h);
 int nft_cache_add_chain(struct nft_handle *h, const struct builtin_table *t,
 			struct nftnl_chain *c);
 
-struct nftnl_chain_list *
-nft_chain_list_get(struct nft_handle *h, const char *table, const char *chain);
 struct nftnl_set_list *
 nft_set_list_get(struct nft_handle *h, const char *table, const char *set);
 
diff --git a/iptables/nft.c b/iptables/nft.c
index 8e1a33ba69bf1..5967d36038953 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -1837,13 +1837,15 @@ out:
 static struct nftnl_chain *
 nft_chain_find(struct nft_handle *h, const char *table, const char *chain)
 {
+	const struct builtin_table *t;
 	struct nftnl_chain_list *list;
 
-	list = nft_chain_list_get(h, table, chain);
-	if (list == NULL)
+	t = nft_table_builtin_find(h, table);
+	if (!t)
 		return NULL;
 
-	return nftnl_chain_list_lookup_byname(list, chain);
+	list = h->cache->table[t->type].chains;
+	return list ? nftnl_chain_list_lookup_byname(list, chain) : NULL;
 }
 
 bool nft_chain_exists(struct nft_handle *h,
-- 
2.28.0

[iptables PATCH v2 05/10] nft: cache: Move nft_chain_find() over

[Phil Sutter]

It is basically just a cache lookup, hence fits better in here.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft-cache.c | 14 ++++++++++++++
 iptables/nft-cache.h |  3 +++
 iptables/nft.c       | 17 -----------------
 3 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/iptables/nft-cache.c b/iptables/nft-cache.c
index 109524c3fbc79..929fa0fa152c1 100644
--- a/iptables/nft-cache.c
+++ b/iptables/nft-cache.c
@@ -165,6 +165,20 @@ static int fetch_table_cache(struct nft_handle *h)
 	return 1;
 }
 
+struct nftnl_chain *
+nft_chain_find(struct nft_handle *h, const char *table, const char *chain)
+{
+	const struct builtin_table *t;
+	struct nftnl_chain_list *list;
+
+	t = nft_table_builtin_find(h, table);
+	if (!t)
+		return NULL;
+
+	list = h->cache->table[t->type].chains;
+	return list ? nftnl_chain_list_lookup_byname(list, chain) : NULL;
+}
+
 int nft_cache_add_chain(struct nft_handle *h, const struct builtin_table *t,
 			struct nftnl_chain *c)
 {
diff --git a/iptables/nft-cache.h b/iptables/nft-cache.h
index 52ad2d396199e..085594c26457b 100644
--- a/iptables/nft-cache.h
+++ b/iptables/nft-cache.h
@@ -16,6 +16,9 @@ void nft_cache_build(struct nft_handle *h);
 int nft_cache_add_chain(struct nft_handle *h, const struct builtin_table *t,
 			struct nftnl_chain *c);
 
+struct nftnl_chain *
+nft_chain_find(struct nft_handle *h, const char *table, const char *chain);
+
 struct nftnl_set_list *
 nft_set_list_get(struct nft_handle *h, const char *table, const char *set);
 
diff --git a/iptables/nft.c b/iptables/nft.c
index 5967d36038953..4d6a41fabfaa6 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -736,9 +736,6 @@ nft_chain_builtin_find(const struct builtin_table *t, const char *chain)
 	return found ? &t->chains[i] : NULL;
 }
 
-static struct nftnl_chain *
-nft_chain_find(struct nft_handle *h, const char *table, const char *chain);
-
 static void nft_chain_builtin_init(struct nft_handle *h,
 				   const struct builtin_table *table)
 {
@@ -1834,20 +1831,6 @@ out:
 	return ret == 0 ? 1 : 0;
 }
 
-static struct nftnl_chain *
-nft_chain_find(struct nft_handle *h, const char *table, const char *chain)
-{
-	const struct builtin_table *t;
-	struct nftnl_chain_list *list;
-
-	t = nft_table_builtin_find(h, table);
-	if (!t)
-		return NULL;
-
-	list = h->cache->table[t->type].chains;
-	return list ? nftnl_chain_list_lookup_byname(list, chain) : NULL;
-}
-
 bool nft_chain_exists(struct nft_handle *h,
 		      const char *table, const char *chain)
 {
-- 
2.28.0

[iptables PATCH v2 06/10] nft: Introduce struct nft_chain

[Phil Sutter]

Preparing for ordered output of user-defined chains, introduce a local
datatype wrapping nftnl_chain. In order to maintain the chain name hash
table, introduce nft_chain_list as well and use it instead of
nftnl_chain_list.

Put everything into a dedicated source file and provide a bunch of
getters for attributes of the embedded libnftnl_chain object.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/Makefile.am |   2 +-
 iptables/nft-cache.c |  65 +++++++++++-----
 iptables/nft-cache.h |   5 +-
 iptables/nft-chain.c |  59 ++++++++++++++
 iptables/nft-chain.h |  87 +++++++++++++++++++++
 iptables/nft.c       | 179 +++++++++++++++++++++----------------------
 iptables/nft.h       |   7 +-
 7 files changed, 287 insertions(+), 117 deletions(-)
 create mode 100644 iptables/nft-chain.c
 create mode 100644 iptables/nft-chain.h

diff --git a/iptables/Makefile.am b/iptables/Makefile.am
index 4bf5742c9dc95..f789521042f87 100644
--- a/iptables/Makefile.am
+++ b/iptables/Makefile.am
@@ -38,7 +38,7 @@ xtables_nft_multi_SOURCES += xtables-save.c xtables-restore.c \
 				nft-shared.c nft-ipv4.c nft-ipv6.c nft-arp.c \
 				xtables-monitor.c nft-cache.c \
 				xtables-arp-standalone.c xtables-arp.c \
-				nft-bridge.c nft-cmd.c \
+				nft-bridge.c nft-cmd.c nft-chain.c \
 				xtables-eb-standalone.c xtables-eb.c \
 				xtables-eb-translate.c \
 				xtables-translate.c
diff --git a/iptables/nft-cache.c b/iptables/nft-cache.c
index 929fa0fa152c1..41ec7b82dc639 100644
--- a/iptables/nft-cache.c
+++ b/iptables/nft-cache.c
@@ -24,6 +24,7 @@
 
 #include "nft.h"
 #include "nft-cache.h"
+#include "nft-chain.h"
 
 static void cache_chain_list_insert(struct list_head *list, const char *name)
 {
@@ -153,9 +154,7 @@ static int fetch_table_cache(struct nft_handle *h)
 		if (!h->tables[i].name)
 			continue;
 
-		h->cache->table[type].chains = nftnl_chain_list_alloc();
-		if (!h->cache->table[type].chains)
-			return 0;
+		h->cache->table[type].chains = nft_chain_list_alloc();
 
 		h->cache->table[type].sets = nftnl_set_list_alloc();
 		if (!h->cache->table[type].sets)
@@ -165,24 +164,50 @@ static int fetch_table_cache(struct nft_handle *h)
 	return 1;
 }
 
-struct nftnl_chain *
+static uint32_t djb_hash(const char *key)
+{
+	uint32_t i, hash = 5381;
+
+	for (i = 0; i < strlen(key); i++)
+		hash = ((hash << 5) + hash) + key[i];
+
+	return hash;
+}
+
+static struct hlist_head *chain_name_hlist(struct nft_handle *h,
+					   const struct builtin_table *t,
+					   const char *chain)
+{
+	int key = djb_hash(chain) % CHAIN_NAME_HSIZE;
+
+	return &h->cache->table[t->type].chains->names[key];
+}
+
+struct nft_chain *
 nft_chain_find(struct nft_handle *h, const char *table, const char *chain)
 {
 	const struct builtin_table *t;
-	struct nftnl_chain_list *list;
+	struct hlist_node *node;
+	struct nft_chain *c;
 
 	t = nft_table_builtin_find(h, table);
 	if (!t)
 		return NULL;
 
-	list = h->cache->table[t->type].chains;
-	return list ? nftnl_chain_list_lookup_byname(list, chain) : NULL;
+	hlist_for_each_entry(c, node, chain_name_hlist(h, t, chain), hnode) {
+		if (!strcmp(nft_chain_name(c), chain))
+			return c;
+	}
+	return NULL;
 }
 
 int nft_cache_add_chain(struct nft_handle *h, const struct builtin_table *t,
 			struct nftnl_chain *c)
 {
-	nftnl_chain_list_add_tail(c, h->cache->table[t->type].chains);
+	struct nft_chain *nc = nft_chain_alloc(c);
+
+	list_add_tail(&nc->head, &h->cache->table[t->type].chains->list);
+	hlist_add_head(&nc->hnode, chain_name_hlist(h, t, nft_chain_name(nc)));
 	return 0;
 }
 
@@ -434,8 +459,9 @@ static int nftnl_rule_list_cb(const struct nlmsghdr *nlh, void *data)
 	return MNL_CB_OK;
 }
 
-static int nft_rule_list_update(struct nftnl_chain *c, void *data)
+static int nft_rule_list_update(struct nft_chain *nc, void *data)
 {
+	struct nftnl_chain *c = nc->nftnl;
 	struct nft_handle *h = data;
 	char buf[16536];
 	struct nlmsghdr *nlh;
@@ -449,10 +475,8 @@ static int nft_rule_list_update(struct nftnl_chain *c, void *data)
 	if (!rule)
 		return -1;
 
-	nftnl_rule_set_str(rule, NFTNL_RULE_TABLE,
-			   nftnl_chain_get_str(c, NFTNL_CHAIN_TABLE));
-	nftnl_rule_set_str(rule, NFTNL_RULE_CHAIN,
-			   nftnl_chain_get_str(c, NFTNL_CHAIN_NAME));
+	nftnl_rule_set_str(rule, NFTNL_RULE_TABLE, nft_chain_table(nc));
+	nftnl_rule_set_str(rule, NFTNL_RULE_CHAIN, nft_chain_name(nc));
 
 	nlh = nftnl_rule_nlmsg_build_hdr(buf, NFT_MSG_GETRULE, h->family,
 					NLM_F_DUMP, h->seq);
@@ -550,13 +574,13 @@ static int ____flush_rule_cache(struct nftnl_rule *r, void *data)
 	return 0;
 }
 
-static int __flush_rule_cache(struct nftnl_chain *c, void *data)
+static int __flush_rule_cache(struct nft_chain *c, void *data)
 {
-	return nftnl_rule_foreach(c, ____flush_rule_cache, NULL);
+	return nftnl_rule_foreach(c->nftnl, ____flush_rule_cache, NULL);
 }
 
 int flush_rule_cache(struct nft_handle *h, const char *table,
-		     struct nftnl_chain *c)
+		     struct nft_chain *c)
 {
 	if (c)
 		return __flush_rule_cache(c, NULL);
@@ -565,10 +589,10 @@ int flush_rule_cache(struct nft_handle *h, const char *table,
 	return 0;
 }
 
-static int __flush_chain_cache(struct nftnl_chain *c, void *data)
+static int __flush_chain_cache(struct nft_chain *c, void *data)
 {
-	nftnl_chain_list_del(c);
-	nftnl_chain_free(c);
+	nft_chain_list_del(c);
+	nft_chain_free(c);
 
 	return 0;
 }
@@ -605,9 +629,10 @@ static int flush_cache(struct nft_handle *h, struct nft_cache *c,
 			continue;
 
 		if (c->table[i].chains) {
-			nftnl_chain_list_free(c->table[i].chains);
+			nft_chain_list_free(c->table[i].chains);
 			c->table[i].chains = NULL;
 		}
+
 		if (c->table[i].sets) {
 			nftnl_set_list_free(c->table[i].sets);
 			c->table[i].sets = NULL;
diff --git a/iptables/nft-cache.h b/iptables/nft-cache.h
index 085594c26457b..20d96beede876 100644
--- a/iptables/nft-cache.h
+++ b/iptables/nft-cache.h
@@ -2,6 +2,7 @@
 #define _NFT_CACHE_H_
 
 struct nft_handle;
+struct nft_chain;
 struct nft_cmd;
 struct builtin_table;
 
@@ -11,12 +12,12 @@ void nft_rebuild_cache(struct nft_handle *h);
 void nft_release_cache(struct nft_handle *h);
 void flush_chain_cache(struct nft_handle *h, const char *tablename);
 int flush_rule_cache(struct nft_handle *h, const char *table,
-		     struct nftnl_chain *c);
+		     struct nft_chain *c);
 void nft_cache_build(struct nft_handle *h);
 int nft_cache_add_chain(struct nft_handle *h, const struct builtin_table *t,
 			struct nftnl_chain *c);
 
-struct nftnl_chain *
+struct nft_chain *
 nft_chain_find(struct nft_handle *h, const char *table, const char *chain);
 
 struct nftnl_set_list *
diff --git a/iptables/nft-chain.c b/iptables/nft-chain.c
new file mode 100644
index 0000000000000..e954170fa7312
--- /dev/null
+++ b/iptables/nft-chain.c
@@ -0,0 +1,59 @@
+/*
+ * Copyright (c) 2020  Red Hat GmbH.  Author: Phil Sutter <phil@nwl.cc>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published
+ * by the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <stdlib.h>
+#include <xtables.h>
+
+#include "nft-chain.h"
+
+struct nft_chain *nft_chain_alloc(struct nftnl_chain *nftnl)
+{
+	struct nft_chain *c = xtables_malloc(sizeof(*c));
+
+	INIT_LIST_HEAD(&c->head);
+	c->nftnl = nftnl;
+
+	return c;
+}
+
+void nft_chain_free(struct nft_chain *c)
+{
+	if (c->nftnl)
+		nftnl_chain_free(c->nftnl);
+	free(c);
+}
+
+struct nft_chain_list *nft_chain_list_alloc(void)
+{
+	struct nft_chain_list *list = xtables_malloc(sizeof(*list));
+	int i;
+
+	INIT_LIST_HEAD(&list->list);
+	for (i = 0; i < CHAIN_NAME_HSIZE; i++)
+		INIT_HLIST_HEAD(&list->names[i]);
+
+	return list;
+}
+
+void nft_chain_list_del(struct nft_chain *c)
+{
+	list_del(&c->head);
+	hlist_del(&c->hnode);
+}
+
+void nft_chain_list_free(struct nft_chain_list *list)
+{
+	struct nft_chain *c, *c2;
+
+	list_for_each_entry_safe(c, c2, &list->list, head) {
+		nft_chain_list_del(c);
+		nft_chain_free(c);
+	}
+	free(list);
+}
diff --git a/iptables/nft-chain.h b/iptables/nft-chain.h
new file mode 100644
index 0000000000000..818bbf1f4b525
--- /dev/null
+++ b/iptables/nft-chain.h
@@ -0,0 +1,87 @@
+#ifndef _NFT_CHAIN_H_
+#define _NFT_CHAIN_H_
+
+#include <libnftnl/chain.h>
+#include <libiptc/linux_list.h>
+
+struct nft_handle;
+
+struct nft_chain {
+	struct list_head	head;
+	struct hlist_node	hnode;
+	struct nftnl_chain	*nftnl;
+};
+
+#define CHAIN_NAME_HSIZE	512
+
+struct nft_chain_list {
+	struct list_head	list;
+	struct hlist_head	names[CHAIN_NAME_HSIZE];
+};
+
+struct nft_chain *nft_chain_alloc(struct nftnl_chain *nftnl);
+void nft_chain_free(struct nft_chain *c);
+
+struct nft_chain_list *nft_chain_list_alloc(void);
+void nft_chain_list_free(struct nft_chain_list *list);
+void nft_chain_list_del(struct nft_chain *c);
+
+static inline const char *nft_chain_name(struct nft_chain *c)
+{
+	return nftnl_chain_get_str(c->nftnl, NFTNL_CHAIN_NAME);
+}
+
+static inline const char *nft_chain_table(struct nft_chain *c)
+{
+	return nftnl_chain_get_str(c->nftnl, NFTNL_CHAIN_TABLE);
+}
+
+static inline const char *nft_chain_type(struct nft_chain *c)
+{
+	return nftnl_chain_get_str(c->nftnl, NFTNL_CHAIN_TYPE);
+}
+
+static inline uint32_t nft_chain_prio(struct nft_chain *c)
+{
+	return nftnl_chain_get_u32(c->nftnl, NFTNL_CHAIN_PRIO);
+}
+
+static inline uint32_t nft_chain_hooknum(struct nft_chain *c)
+{
+	return nftnl_chain_get_u32(c->nftnl, NFTNL_CHAIN_HOOKNUM);
+}
+
+static inline uint64_t nft_chain_packets(struct nft_chain *c)
+{
+	return nftnl_chain_get_u64(c->nftnl, NFTNL_CHAIN_PACKETS);
+}
+
+static inline uint64_t nft_chain_bytes(struct nft_chain *c)
+{
+	return nftnl_chain_get_u64(c->nftnl, NFTNL_CHAIN_BYTES);
+}
+
+static inline bool nft_chain_has_policy(struct nft_chain *c)
+{
+	return nftnl_chain_is_set(c->nftnl, NFTNL_CHAIN_POLICY);
+}
+
+static inline uint32_t nft_chain_policy(struct nft_chain *c)
+{
+	return nftnl_chain_get_u32(c->nftnl, NFTNL_CHAIN_POLICY);
+}
+
+static inline uint32_t nft_chain_use(struct nft_chain *c)
+{
+	return nftnl_chain_get_u32(c->nftnl, NFTNL_CHAIN_USE);
+}
+
+static inline bool nft_chain_builtin(struct nft_chain *c)
+{
+	/* Check if this chain has hook number, in that case is built-in.
+	 * Should we better export the flags to user-space via nf_tables?
+	 */
+	return nftnl_chain_is_set(c->nftnl, NFTNL_CHAIN_HOOKNUM);
+}
+
+#endif /* _NFT_CHAIN_H_ */
diff --git a/iptables/nft.c b/iptables/nft.c
index 4d6a41fabfaa6..16355a78c257b 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -772,14 +772,6 @@ static int nft_xt_builtin_init(struct nft_handle *h, const char *table)
 	return 0;
 }
 
-static bool nft_chain_builtin(struct nftnl_chain *c)
-{
-	/* Check if this chain has hook number, in that case is built-in.
-	 * Should we better export the flags to user-space via nf_tables?
-	 */
-	return nftnl_chain_get(c, NFTNL_CHAIN_HOOKNUM) != NULL;
-}
-
 int nft_restart(struct nft_handle *h)
 {
 	mnl_socket_close(h->nl);
@@ -1384,7 +1376,7 @@ int
 nft_rule_append(struct nft_handle *h, const char *chain, const char *table,
 		struct nftnl_rule *r, struct nftnl_rule *ref, bool verbose)
 {
-	struct nftnl_chain *c;
+	struct nft_chain *c;
 	int type;
 
 	nft_xt_builtin_init(h, table);
@@ -1414,7 +1406,7 @@ nft_rule_append(struct nft_handle *h, const char *chain, const char *table,
 			errno = ENOENT;
 			return 0;
 		}
-		nftnl_chain_rule_add_tail(r, c);
+		nftnl_chain_rule_add_tail(r, c->nftnl);
 	}
 
 	return 1;
@@ -1536,13 +1528,13 @@ static const char *policy_name[NF_ACCEPT+1] = {
 	[NF_ACCEPT] = "ACCEPT",
 };
 
-int nft_chain_save(struct nftnl_chain *c, void *data)
+int nft_chain_save(struct nft_chain *c, void *data)
 {
 	struct nft_handle *h = data;
 	const char *policy = NULL;
 
-	if (nftnl_chain_is_set(c, NFTNL_CHAIN_POLICY)) {
-		policy = policy_name[nftnl_chain_get_u32(c, NFTNL_CHAIN_POLICY)];
+	if (nft_chain_has_policy(c)) {
+		policy = policy_name[nft_chain_policy(c)];
 	} else if (nft_chain_builtin(c)) {
 		policy = "ACCEPT";
 	} else if (h->family == NFPROTO_BRIDGE) {
@@ -1550,7 +1542,7 @@ int nft_chain_save(struct nftnl_chain *c, void *data)
 	}
 
 	if (h->ops->save_chain)
-		h->ops->save_chain(c, policy);
+		h->ops->save_chain(c->nftnl, policy);
 
 	return 0;
 }
@@ -1560,13 +1552,13 @@ struct nft_rule_save_data {
 	unsigned int format;
 };
 
-static int nft_rule_save_cb(struct nftnl_chain *c, void *data)
+static int nft_rule_save_cb(struct nft_chain *c, void *data)
 {
 	struct nft_rule_save_data *d = data;
 	struct nftnl_rule_iter *iter;
 	struct nftnl_rule *r;
 
-	iter = nftnl_rule_iter_create(c);
+	iter = nftnl_rule_iter_create(c->nftnl);
 	if (iter == NULL)
 		return 1;
 
@@ -1641,9 +1633,9 @@ struct nft_rule_flush_data {
 	bool verbose;
 };
 
-static int nft_rule_flush_cb(struct nftnl_chain *c, void *data)
+static int nft_rule_flush_cb(struct nft_chain *c, void *data)
 {
-	const char *chain = nftnl_chain_get_str(c, NFTNL_CHAIN_NAME);
+	const char *chain = nft_chain_name(c);
 	struct nft_rule_flush_data *d = data;
 
 	batch_chain_flush(d->h, d->table, chain);
@@ -1660,7 +1652,7 @@ int nft_rule_flush(struct nft_handle *h, const char *chain, const char *table,
 		.table = table,
 		.verbose = verbose,
 	};
-	struct nftnl_chain *c = NULL;
+	struct nft_chain *c = NULL;
 	int ret = 0;
 
 	nft_fn = nft_rule_flush;
@@ -1728,18 +1720,20 @@ int nft_chain_restore(struct nft_handle *h, const char *chain, const char *table
 {
 	const struct builtin_table *t;
 	struct nftnl_chain *c;
+	struct nft_chain *nc;
 	bool created = false;
 	int ret;
 
 	nft_xt_builtin_init(h, table);
 
-	c = nft_chain_find(h, table, chain);
-	if (c) {
+	nc = nft_chain_find(h, table, chain);
+	if (nc) {
 		/* Apparently -n still flushes existing user defined
 		 * chains that are redefined.
 		 */
 		if (h->noflush)
 			__nft_rule_flush(h, table, chain, false, true);
+		c = nc->nftnl;
 	} else {
 		c = nftnl_chain_alloc();
 		if (!c)
@@ -1776,7 +1770,7 @@ struct chain_user_del_data {
 	int			builtin_err;
 };
 
-static int __nft_chain_user_del(struct nftnl_chain *c, void *data)
+static int __nft_chain_user_del(struct nft_chain *c, void *data)
 {
 	struct chain_user_del_data *d = data;
 	struct nft_handle *h = d->handle;
@@ -1787,16 +1781,19 @@ static int __nft_chain_user_del(struct nftnl_chain *c, void *data)
 		return d->builtin_err;
 
 	if (d->verbose)
-		fprintf(stdout, "Deleting chain `%s'\n",
-			nftnl_chain_get_str(c, NFTNL_CHAIN_NAME));
+		fprintf(stdout, "Deleting chain `%s'\n", nft_chain_name(c));
 
 	/* XXX This triggers a fast lookup from the kernel. */
-	nftnl_chain_unset(c, NFTNL_CHAIN_HANDLE);
-	ret = batch_chain_add(h, NFT_COMPAT_CHAIN_USER_DEL, c);
+	nftnl_chain_unset(c->nftnl, NFTNL_CHAIN_HANDLE);
+	ret = batch_chain_add(h, NFT_COMPAT_CHAIN_USER_DEL, c->nftnl);
 	if (ret)
 		return -1;
 
-	nftnl_chain_list_del(c);
+	/* nftnl_chain is freed when deleting the batch object */
+	c->nftnl = NULL;
+
+	nft_chain_list_del(c);
+	nft_chain_free(c);
 	return 0;
 }
 
@@ -1807,7 +1804,7 @@ int nft_chain_user_del(struct nft_handle *h, const char *chain,
 		.handle = h,
 		.verbose = verbose,
 	};
-	struct nftnl_chain *c;
+	struct nft_chain *c;
 	int ret = 0;
 
 	nft_fn = nft_chain_user_del;
@@ -1850,6 +1847,7 @@ int nft_chain_user_rename(struct nft_handle *h,const char *chain,
 			  const char *table, const char *newname)
 {
 	struct nftnl_chain *c;
+	struct nft_chain *nc;
 	uint64_t handle;
 	int ret;
 
@@ -1864,12 +1862,12 @@ int nft_chain_user_rename(struct nft_handle *h,const char *chain,
 	errno = 0;
 
 	/* Find the old chain to be renamed */
-	c = nft_chain_find(h, table, chain);
-	if (c == NULL) {
+	nc = nft_chain_find(h, table, chain);
+	if (nc == NULL) {
 		errno = ENOENT;
 		return 0;
 	}
-	handle = nftnl_chain_get_u64(c, NFTNL_CHAIN_HANDLE);
+	handle = nftnl_chain_get_u64(nc->nftnl, NFTNL_CHAIN_HANDLE);
 
 	/* Now prepare the new name for the chain */
 	c = nftnl_chain_alloc();
@@ -2017,9 +2015,10 @@ out:
 }
 
 static struct nftnl_rule *
-nft_rule_find(struct nft_handle *h, struct nftnl_chain *c,
+nft_rule_find(struct nft_handle *h, struct nft_chain *nc,
 	      struct nftnl_rule *rule, int rulenum)
 {
+	struct nftnl_chain *c = nc->nftnl;
 	struct nftnl_rule *r;
 	struct nftnl_rule_iter *iter;
 	bool found = false;
@@ -2048,8 +2047,8 @@ nft_rule_find(struct nft_handle *h, struct nftnl_chain *c,
 int nft_rule_check(struct nft_handle *h, const char *chain,
 		   const char *table, struct nftnl_rule *rule, bool verbose)
 {
-	struct nftnl_chain *c;
 	struct nftnl_rule *r;
+	struct nft_chain *c;
 
 	nft_fn = nft_rule_check;
 
@@ -2074,8 +2073,8 @@ int nft_rule_delete(struct nft_handle *h, const char *chain,
 		    const char *table, struct nftnl_rule *rule, bool verbose)
 {
 	int ret = 0;
-	struct nftnl_chain *c;
 	struct nftnl_rule *r;
+	struct nft_chain *c;
 
 	nft_fn = nft_rule_delete;
 
@@ -2135,7 +2134,7 @@ int nft_rule_insert(struct nft_handle *h, const char *chain,
 		    bool verbose)
 {
 	struct nftnl_rule *r = NULL;
-	struct nftnl_chain *c;
+	struct nft_chain *c;
 
 	nft_xt_builtin_init(h, table);
 
@@ -2170,7 +2169,7 @@ int nft_rule_insert(struct nft_handle *h, const char *chain,
 	if (r)
 		nftnl_chain_rule_insert_at(new_rule, r);
 	else
-		nftnl_chain_rule_add(new_rule, c);
+		nftnl_chain_rule_add(new_rule, c->nftnl);
 
 	return 1;
 err:
@@ -2181,8 +2180,8 @@ int nft_rule_delete_num(struct nft_handle *h, const char *chain,
 			const char *table, int rulenum, bool verbose)
 {
 	int ret = 0;
-	struct nftnl_chain *c;
 	struct nftnl_rule *r;
+	struct nft_chain *c;
 
 	nft_fn = nft_rule_delete_num;
 
@@ -2209,8 +2208,8 @@ int nft_rule_replace(struct nft_handle *h, const char *chain,
 		     int rulenum, bool verbose)
 {
 	int ret = 0;
-	struct nftnl_chain *c;
 	struct nftnl_rule *r;
+	struct nft_chain *c;
 
 	nft_fn = nft_rule_replace;
 
@@ -2289,23 +2288,21 @@ static int nft_rule_count(struct nft_handle *h, struct nftnl_chain *c)
 }
 
 static void __nft_print_header(struct nft_handle *h,
-			       struct nftnl_chain *c, unsigned int format)
+			       struct nft_chain *c, unsigned int format)
 {
-	const char *chain_name = nftnl_chain_get_str(c, NFTNL_CHAIN_NAME);
-	bool basechain = !!nftnl_chain_get(c, NFTNL_CHAIN_HOOKNUM);
-	uint32_t refs = nftnl_chain_get_u32(c, NFTNL_CHAIN_USE);
-	uint32_t entries = nft_rule_count(h, c);
+	uint32_t entries = nft_rule_count(h, c->nftnl);
 	struct xt_counters ctrs = {
-		.pcnt = nftnl_chain_get_u64(c, NFTNL_CHAIN_PACKETS),
-		.bcnt = nftnl_chain_get_u64(c, NFTNL_CHAIN_BYTES),
+		.pcnt = nft_chain_packets(c),
+		.bcnt = nft_chain_bytes(c),
 	};
 	const char *pname = NULL;
 
-	if (nftnl_chain_is_set(c, NFTNL_CHAIN_POLICY))
-		pname = policy_name[nftnl_chain_get_u32(c, NFTNL_CHAIN_POLICY)];
+	if (nft_chain_has_policy(c))
+		pname = policy_name[nft_chain_policy(c)];
 
-	h->ops->print_header(format, chain_name, pname,
-			&ctrs, basechain, refs - entries, entries);
+	h->ops->print_header(format, nft_chain_name(c), pname,
+			     &ctrs, nft_chain_builtin(c),
+			     nft_chain_use(c) - entries, entries);
 }
 
 struct nft_rule_list_cb_data {
@@ -2318,7 +2315,7 @@ struct nft_rule_list_cb_data {
 		   unsigned int num, unsigned int format);
 };
 
-static int nft_rule_list_cb(struct nftnl_chain *c, void *data)
+static int nft_rule_list_cb(struct nft_chain *c, void *data)
 {
 	struct nft_rule_list_cb_data *d = data;
 
@@ -2330,7 +2327,7 @@ static int nft_rule_list_cb(struct nftnl_chain *c, void *data)
 		__nft_print_header(d->h, c, d->format);
 	}
 
-	return __nft_rule_list(d->h, c, d->rulenum, d->format, d->cb);
+	return __nft_rule_list(d->h, c->nftnl, d->rulenum, d->format, d->cb);
 }
 
 int nft_rule_list(struct nft_handle *h, const char *chain, const char *table,
@@ -2343,7 +2340,7 @@ int nft_rule_list(struct nft_handle *h, const char *chain, const char *table,
 		.rulenum = rulenum,
 		.cb = ops->print_rule,
 	};
-	struct nftnl_chain *c;
+	struct nft_chain *c;
 
 	nft_xt_builtin_init(h, table);
 	nft_assert_table_compatible(h, table, chain);
@@ -2377,40 +2374,45 @@ list_save(struct nft_handle *h, struct nftnl_rule *r,
 }
 
 int nft_chain_foreach(struct nft_handle *h, const char *table,
-		      int (*cb)(struct nftnl_chain *c, void *data),
+		      int (*cb)(struct nft_chain *c, void *data),
 		      void *data)
 {
 	const struct builtin_table *t;
+	struct nft_chain_list *list;
+	struct nft_chain *c, *c_bak;
+	int ret;
 
 	t = nft_table_builtin_find(h, table);
 	if (!t)
 		return -1;
 
-	if (!h->cache->table[t->type].chains)
+	list = h->cache->table[t->type].chains;
+	if (!list)
 		return -1;
 
-	return nftnl_chain_list_foreach(h->cache->table[t->type].chains,
-					cb, data);
+	list_for_each_entry_safe(c, c_bak, &list->list, head) {
+		ret = cb(c, data);
+		if (ret < 0)
+			return ret;
+	}
+	return 0;
 }
 
-static int nft_rule_list_chain_save(struct nftnl_chain *c, void *data)
+static int nft_rule_list_chain_save(struct nft_chain *c, void *data)
 {
-	const char *chain_name = nftnl_chain_get_str(c, NFTNL_CHAIN_NAME);
-	uint32_t policy = nftnl_chain_get_u32(c, NFTNL_CHAIN_POLICY);
 	int *counters = data;
 
 	if (!nft_chain_builtin(c)) {
-		printf("-N %s\n", chain_name);
+		printf("-N %s\n", nft_chain_name(c));
 		return 0;
 	}
 
 	/* this is a base chain */
 
-	printf("-P %s %s", chain_name, policy_name[policy]);
+	printf("-P %s %s", nft_chain_name(c), policy_name[nft_chain_policy(c)]);
 	if (*counters)
 		printf(" -c %"PRIu64" %"PRIu64,
-		       nftnl_chain_get_u64(c, NFTNL_CHAIN_PACKETS),
-		       nftnl_chain_get_u64(c, NFTNL_CHAIN_BYTES));
+		       nft_chain_packets(c), nft_chain_bytes(c));
 	printf("\n");
 	return 0;
 }
@@ -2424,7 +2426,7 @@ int nft_rule_list_save(struct nft_handle *h, const char *chain,
 		.save_fmt = true,
 		.cb = list_save,
 	};
-	struct nftnl_chain *c;
+	struct nft_chain *c;
 	int ret = 0;
 
 	nft_xt_builtin_init(h, table);
@@ -2459,7 +2461,7 @@ int nft_rule_zero_counters(struct nft_handle *h, const char *chain,
 {
 	struct iptables_command_state cs = {};
 	struct nftnl_rule *r, *new_rule;
-	struct nftnl_chain *c;
+	struct nft_chain *c;
 	int ret = 0;
 
 	nft_fn = nft_rule_delete;
@@ -2601,7 +2603,7 @@ static void batch_obj_del(struct nft_handle *h, struct obj_update *o)
 static void nft_refresh_transaction(struct nft_handle *h)
 {
 	const char *tablename, *chainname;
-	const struct nftnl_chain *c;
+	const struct nft_chain *c;
 	struct obj_update *n, *tmp;
 	bool exists;
 
@@ -2895,7 +2897,7 @@ err_free_rule:
 int ebt_set_user_chain_policy(struct nft_handle *h, const char *table,
 			      const char *chain, const char *policy)
 {
-	struct nftnl_chain *c = nft_chain_find(h, table, chain);
+	struct nft_chain *c = nft_chain_find(h, table, chain);
 	int pval;
 
 	if (!c)
@@ -2910,14 +2912,15 @@ int ebt_set_user_chain_policy(struct nft_handle *h, const char *table,
 	else
 		return 0;
 
-	nftnl_chain_set_u32(c, NFTNL_CHAIN_POLICY, pval);
+	nftnl_chain_set_u32(c->nftnl, NFTNL_CHAIN_POLICY, pval);
 	return 1;
 }
 
 static void nft_bridge_commit_prepare(struct nft_handle *h)
 {
 	const struct builtin_table *t;
-	struct nftnl_chain_list *list;
+	struct nft_chain_list *list;
+	struct nft_chain *c;
 	int i;
 
 	for (i = 0; i < NFT_TABLE_MAX; i++) {
@@ -2930,7 +2933,9 @@ static void nft_bridge_commit_prepare(struct nft_handle *h)
 		if (!list)
 			continue;
 
-		nftnl_chain_list_foreach(list, ebt_add_policy_rule, h);
+		list_for_each_entry(c, &list->list, head) {
+			ebt_add_policy_rule(c->nftnl, h);
+		}
 	}
 }
 
@@ -3238,18 +3243,18 @@ struct chain_zero_data {
 	bool			verbose;
 };
 
-static int __nft_chain_zero_counters(struct nftnl_chain *c, void *data)
+static int __nft_chain_zero_counters(struct nft_chain *nc, void *data)
 {
+	struct nftnl_chain *c = nc->nftnl;
 	struct chain_zero_data *d = data;
 	struct nft_handle *h = d->handle;
 	struct nftnl_rule_iter *iter;
 	struct nftnl_rule *r;
 
 	if (d->verbose)
-		fprintf(stdout, "Zeroing chain `%s'\n",
-			nftnl_chain_get_str(c, NFTNL_CHAIN_NAME));
+		fprintf(stdout, "Zeroing chain `%s'\n", nft_chain_name(nc));
 
-	if (nftnl_chain_is_set(c, NFTNL_CHAIN_HOOKNUM)) {
+	if (nft_chain_builtin(nc)) {
 		/* zero base chain counters. */
 		nftnl_chain_set_u64(c, NFTNL_CHAIN_PACKETS, 0);
 		nftnl_chain_set_u64(c, NFTNL_CHAIN_BYTES, 0);
@@ -3316,7 +3321,7 @@ int nft_chain_zero_counters(struct nft_handle *h, const char *chain,
 		.handle = h,
 		.verbose = verbose,
 	};
-	struct nftnl_chain *c;
+	struct nft_chain *c;
 	int ret = 0;
 
 	if (chain) {
@@ -3380,37 +3385,29 @@ static int nft_is_rule_compatible(struct nftnl_rule *rule, void *data)
 	return nftnl_expr_foreach(rule, nft_is_expr_compatible, NULL);
 }
 
-static int nft_is_chain_compatible(struct nftnl_chain *c, void *data)
+static int nft_is_chain_compatible(struct nft_chain *c, void *data)
 {
 	const struct builtin_table *table;
 	const struct builtin_chain *chain;
-	const char *tname, *cname, *type;
 	struct nft_handle *h = data;
-	enum nf_inet_hooks hook;
-	int prio;
 
-	if (nftnl_rule_foreach(c, nft_is_rule_compatible, NULL))
+	if (nftnl_rule_foreach(c->nftnl, nft_is_rule_compatible, NULL))
 		return -1;
 
 	if (!nft_chain_builtin(c))
 		return 0;
 
-	tname = nftnl_chain_get_str(c, NFTNL_CHAIN_TABLE);
-	table = nft_table_builtin_find(h, tname);
+	table = nft_table_builtin_find(h, nft_chain_table(c));
 	if (!table)
 		return -1;
 
-	cname = nftnl_chain_get_str(c, NFTNL_CHAIN_NAME);
-	chain = nft_chain_builtin_find(table, cname);
+	chain = nft_chain_builtin_find(table, nft_chain_name(c));
 	if (!chain)
 		return -1;
 
-	type = nftnl_chain_get_str(c, NFTNL_CHAIN_TYPE);
-	prio = nftnl_chain_get_u32(c, NFTNL_CHAIN_PRIO);
-	hook = nftnl_chain_get_u32(c, NFTNL_CHAIN_HOOKNUM);
-	if (strcmp(type, chain->type) ||
-	    prio != chain->prio ||
-	    hook != chain->hook)
+	if (strcmp(nft_chain_type(c), chain->type) ||
+	    nft_chain_prio(c) != chain->prio ||
+	    nft_chain_hooknum(c) != chain->hook)
 		return -1;
 
 	return 0;
@@ -3420,7 +3417,7 @@ bool nft_is_table_compatible(struct nft_handle *h,
 			     const char *table, const char *chain)
 {
 	if (chain) {
-		struct nftnl_chain *c = nft_chain_find(h, table, chain);
+		struct nft_chain *c = nft_chain_find(h, table, chain);
 
 		return c && !nft_is_chain_compatible(c, h);
 	}
diff --git a/iptables/nft.h b/iptables/nft.h
index 949d9d077f23b..ac227b4c6c581 100644
--- a/iptables/nft.h
+++ b/iptables/nft.h
@@ -4,6 +4,7 @@
 #include "xshared.h"
 #include "nft-shared.h"
 #include "nft-cache.h"
+#include "nft-chain.h"
 #include "nft-cmd.h"
 #include <libiptc/linux_list.h>
 
@@ -39,7 +40,7 @@ enum nft_cache_level {
 
 struct nft_cache {
 	struct {
-		struct nftnl_chain_list *chains;
+		struct nft_chain_list	*chains;
 		struct nftnl_set_list	*sets;
 		bool			exists;
 	} table[NFT_TABLE_MAX];
@@ -141,7 +142,7 @@ const struct builtin_table *nft_table_builtin_find(struct nft_handle *h, const c
 struct nftnl_chain;
 
 int nft_chain_set(struct nft_handle *h, const char *table, const char *chain, const char *policy, const struct xt_counters *counters);
-int nft_chain_save(struct nftnl_chain *c, void *data);
+int nft_chain_save(struct nft_chain *c, void *data);
 int nft_chain_user_add(struct nft_handle *h, const char *chain, const char *table);
 int nft_chain_user_del(struct nft_handle *h, const char *chain, const char *table, bool verbose);
 int nft_chain_restore(struct nft_handle *h, const char *chain, const char *table);
@@ -152,7 +153,7 @@ bool nft_chain_exists(struct nft_handle *h, const char *table, const char *chain
 void nft_bridge_chain_postprocess(struct nft_handle *h,
 				  struct nftnl_chain *c);
 int nft_chain_foreach(struct nft_handle *h, const char *table,
-		      int (*cb)(struct nftnl_chain *c, void *data),
+		      int (*cb)(struct nft_chain *c, void *data),
 		      void *data);
 
 
-- 
2.28.0

[iptables PATCH v2 07/10] nft: Introduce a dedicated base chain array

[Phil Sutter]

Preparing for sorted chain output, introduce a per-table array holding
base chains indexed by nf_inet_hooks value. Since the latter is ordered
correctly, iterating over the array will return base chains in expected
order.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
Changes since v1:
- Use nft_chain getters in nft_cache_add_chain().
- Introduce flush_base_chain_cache().
- Simplified patch since nft_cache_add_chain() hides some details.
---
 iptables/nft-cache.c | 34 +++++++++++++++++++++++++++++++++-
 iptables/nft.c       | 12 +++++++++++-
 iptables/nft.h       |  1 +
 3 files changed, 45 insertions(+), 2 deletions(-)

diff --git a/iptables/nft-cache.c b/iptables/nft-cache.c
index 41ec7b82dc639..83ff11e794a65 100644
--- a/iptables/nft-cache.c
+++ b/iptables/nft-cache.c
@@ -206,7 +206,24 @@ int nft_cache_add_chain(struct nft_handle *h, const struct builtin_table *t,
 {
 	struct nft_chain *nc = nft_chain_alloc(c);
 
-	list_add_tail(&nc->head, &h->cache->table[t->type].chains->list);
+	if (nft_chain_builtin(nc)) {
+		uint32_t hooknum = nft_chain_hooknum(nc);
+
+		if (hooknum >= NF_INET_NUMHOOKS) {
+			nft_chain_free(nc);
+			return -EINVAL;
+		}
+
+		if (h->cache->table[t->type].base_chains[hooknum]) {
+			nft_chain_free(nc);
+			return -EEXIST;
+		}
+
+		h->cache->table[t->type].base_chains[hooknum] = nc;
+	} else {
+		list_add_tail(&nc->head,
+			      &h->cache->table[t->type].chains->list);
+	}
 	hlist_add_head(&nc->hnode, chain_name_hlist(h, t, nft_chain_name(nc)));
 	return 0;
 }
@@ -605,6 +622,19 @@ static int __flush_set_cache(struct nftnl_set *s, void *data)
 	return 0;
 }
 
+static void flush_base_chain_cache(struct nft_chain **base_chains)
+{
+	int i;
+
+	for (i = 0; i < NF_INET_NUMHOOKS; i++) {
+		if (!base_chains[i])
+			continue;
+		hlist_del(&base_chains[i]->hnode);
+		nft_chain_free(base_chains[i]);
+		base_chains[i] = NULL;
+	}
+}
+
 static int flush_cache(struct nft_handle *h, struct nft_cache *c,
 		       const char *tablename)
 {
@@ -616,6 +646,7 @@ static int flush_cache(struct nft_handle *h, struct nft_cache *c,
 		if (!table)
 			return 0;
 
+		flush_base_chain_cache(c->table[table->type].base_chains);
 		nft_chain_foreach(h, tablename, __flush_chain_cache, NULL);
 
 		if (c->table[table->type].sets)
@@ -628,6 +659,7 @@ static int flush_cache(struct nft_handle *h, struct nft_cache *c,
 		if (h->tables[i].name == NULL)
 			continue;
 
+		flush_base_chain_cache(c->table[i].base_chains);
 		if (c->table[i].chains) {
 			nft_chain_list_free(c->table[i].chains);
 			c->table[i].chains = NULL;
diff --git a/iptables/nft.c b/iptables/nft.c
index 16355a78c257b..281f1f1962fb2 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -2380,12 +2380,22 @@ int nft_chain_foreach(struct nft_handle *h, const char *table,
 	const struct builtin_table *t;
 	struct nft_chain_list *list;
 	struct nft_chain *c, *c_bak;
-	int ret;
+	int i, ret;
 
 	t = nft_table_builtin_find(h, table);
 	if (!t)
 		return -1;
 
+	for (i = 0; i < NF_INET_NUMHOOKS; i++) {
+		c = h->cache->table[t->type].base_chains[i];
+		if (!c)
+			continue;
+
+		ret = cb(c, data);
+		if (ret < 0)
+			return ret;
+	}
+
 	list = h->cache->table[t->type].chains;
 	if (!list)
 		return -1;
diff --git a/iptables/nft.h b/iptables/nft.h
index ac227b4c6c581..1a2506eea7b6c 100644
--- a/iptables/nft.h
+++ b/iptables/nft.h
@@ -40,6 +40,7 @@ enum nft_cache_level {
 
 struct nft_cache {
 	struct {
+		struct nft_chain	*base_chains[NF_INET_NUMHOOKS];
 		struct nft_chain_list	*chains;
 		struct nftnl_set_list	*sets;
 		bool			exists;
-- 
2.28.0

[iptables PATCH v2 08/10] nft: cache: Sort custom chains by name

[Phil Sutter]

With base chains no longer residing in the tables' chain lists, they can
easily be sorted upon insertion. This on one hand aligns custom chain
ordering with legacy iptables and on the other makes it predictable,
which is very helpful when manually comparing ruleset dumps for
instance.

Adjust the one ebtables-nft test case this change breaks (as wrong
ordering is expected in there). The manual output sorting done for tests
which apply to legacy as well as nft is removed in a separate patch.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
Changes since v1:
- Extended commit message.
- Rewrote to not rely upon changes in libnftnl (namely,
  nftnl_chain_list_add_sorted()).
---
 iptables/nft-cache.c                             | 16 +++++++++++++---
 .../ebtables/0002-ebtables-save-restore_0        |  2 +-
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/iptables/nft-cache.c b/iptables/nft-cache.c
index 83ff11e794a65..f7c51d2f92692 100644
--- a/iptables/nft-cache.c
+++ b/iptables/nft-cache.c
@@ -205,6 +205,7 @@ int nft_cache_add_chain(struct nft_handle *h, const struct builtin_table *t,
 			struct nftnl_chain *c)
 {
 	struct nft_chain *nc = nft_chain_alloc(c);
+	const char *cname = nft_chain_name(nc);
 
 	if (nft_chain_builtin(nc)) {
 		uint32_t hooknum = nft_chain_hooknum(nc);
@@ -221,10 +222,19 @@ int nft_cache_add_chain(struct nft_handle *h, const struct builtin_table *t,
 
 		h->cache->table[t->type].base_chains[hooknum] = nc;
 	} else {
-		list_add_tail(&nc->head,
-			      &h->cache->table[t->type].chains->list);
+		struct nft_chain_list *clist = h->cache->table[t->type].chains;
+		struct list_head *pos = &clist->list;
+		struct nft_chain *cur;
+
+		list_for_each_entry(cur, &clist->list, head) {
+			if (strcmp(cname, nft_chain_name(cur)) <= 0) {
+				pos = &cur->head;
+				break;
+			}
+		}
+		list_add_tail(&nc->head, pos);
 	}
-	hlist_add_head(&nc->hnode, chain_name_hlist(h, t, nft_chain_name(nc)));
+	hlist_add_head(&nc->hnode, chain_name_hlist(h, t, cname));
 	return 0;
 }
 
diff --git a/iptables/tests/shell/testcases/ebtables/0002-ebtables-save-restore_0 b/iptables/tests/shell/testcases/ebtables/0002-ebtables-save-restore_0
index b84f63a7c3672..ccdef19cfb215 100755
--- a/iptables/tests/shell/testcases/ebtables/0002-ebtables-save-restore_0
+++ b/iptables/tests/shell/testcases/ebtables/0002-ebtables-save-restore_0
@@ -70,8 +70,8 @@ DUMP='*filter
 :INPUT ACCEPT
 :FORWARD DROP
 :OUTPUT ACCEPT
-:foo ACCEPT
 :bar RETURN
+:foo ACCEPT
 -A INPUT -p IPv4 -i lo -j ACCEPT
 -A FORWARD -j foo
 -A OUTPUT -s Broadcast -j DROP
-- 
2.28.0

[iptables PATCH v2 09/10] tests: shell: Drop any dump sorting in place

[Phil Sutter]

With iptables-nft-save output now sorted just like legacy one, no
sorting to unify them is needed anymore.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 .../firewalld-restore/0001-firewalld_0          | 17 ++---------------
 .../testcases/ipt-restore/0007-flush-noflush_0  |  4 ++--
 .../ipt-restore/0014-verbose-restore_0          |  2 +-
 3 files changed, 5 insertions(+), 18 deletions(-)

diff --git a/iptables/tests/shell/testcases/firewalld-restore/0001-firewalld_0 b/iptables/tests/shell/testcases/firewalld-restore/0001-firewalld_0
index 0174b03f4ebc7..4900554e7d9e6 100755
--- a/iptables/tests/shell/testcases/firewalld-restore/0001-firewalld_0
+++ b/iptables/tests/shell/testcases/firewalld-restore/0001-firewalld_0
@@ -230,21 +230,8 @@ for table in nat mangle raw filter;do
 	$XT_MULTI iptables-save -t $table | grep -v '^#' >> "$tmpfile"
 done
 
-case "$XT_MULTI" in
-*xtables-nft-multi)
-	# nft-multi displays chain names in different order, work around this for now
-	tmpfile2=$(mktemp)
-	sort "$tmpfile" > "$tmpfile2"
-	sort $(dirname "$0")/dumps/ipt-save-completed.txt > "$tmpfile"
-	diff -u $tmpfile $tmpfile2
-	RET=$?
-	rm -f "$tmpfile2"
-	;;
-*)
-	diff -u $tmpfile  $(dirname "$0")/dumps/ipt-save-completed.txt
-	RET=$?
-	;;
-esac
+diff -u $tmpfile  $(dirname "$0")/dumps/ipt-save-completed.txt
+RET=$?
 
 rm -f "$tmpfile"
 
diff --git a/iptables/tests/shell/testcases/ipt-restore/0007-flush-noflush_0 b/iptables/tests/shell/testcases/ipt-restore/0007-flush-noflush_0
index 029db2235b9a4..e705b28c87359 100755
--- a/iptables/tests/shell/testcases/ipt-restore/0007-flush-noflush_0
+++ b/iptables/tests/shell/testcases/ipt-restore/0007-flush-noflush_0
@@ -18,7 +18,7 @@ EXPECT="*nat
 :POSTROUTING ACCEPT [0:0]
 -A POSTROUTING -j ACCEPT
 COMMIT"
-diff -u -Z <(echo -e "$EXPECT" | sort) <($XT_MULTI iptables-save | grep -v '^#' | sort)
+diff -u -Z <(echo -e "$EXPECT") <($XT_MULTI iptables-save | grep -v '^#')
 
 $XT_MULTI iptables-restore <<EOF
 *filter
@@ -39,4 +39,4 @@ COMMIT
 :POSTROUTING ACCEPT [0:0]
 -A POSTROUTING -j ACCEPT
 COMMIT"
-diff -u -Z <(echo -e "$EXPECT" | sort) <($XT_MULTI iptables-save | grep -v '^#' | sort)
+diff -u -Z <(echo -e "$EXPECT") <($XT_MULTI iptables-save | grep -v '^#')
diff --git a/iptables/tests/shell/testcases/ipt-restore/0014-verbose-restore_0 b/iptables/tests/shell/testcases/ipt-restore/0014-verbose-restore_0
index 94bed0ec29c6b..fc8559c5bac9e 100755
--- a/iptables/tests/shell/testcases/ipt-restore/0014-verbose-restore_0
+++ b/iptables/tests/shell/testcases/ipt-restore/0014-verbose-restore_0
@@ -59,7 +59,7 @@ Flushing chain \`secfoo'
 Deleting chain \`secfoo'"
 
 for ipt in iptables-restore ip6tables-restore; do
-	diff -u -Z <(sort <<< "$EXPECT") <($XT_MULTI $ipt -v <<< "$DUMP" | sort)
+	diff -u -Z <(echo "$EXPECT") <($XT_MULTI $ipt -v <<< "$DUMP")
 done
 
 DUMP="*filter
-- 
2.28.0

[iptables PATCH v2 10/10] nft: Avoid pointless table/chain creation

[Phil Sutter]

Accept a chain name in nft_xt_builtin_init() to limit the base chain
creation to that specific chain only.

Introduce nft_xt_builtin_table_init() to create just the table for
situations where no builtin chains are needed but the command may still
succeed in an empty ruleset, particularly when creating a custom chain,
restoring base chains or adding a set for ebtables among match.

Introduce nft_xt_fake_builtin_chains(), a function to call after cache
has been populated to fill empty base chain slots. This keeps ruleset
listing output intact if some base chains do not exist (or even the
whole ruleset is completely empty).

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
Changes since v1:
- Add 'fake' parameter to nft_chain_builtin_add() to reuse its code for
  chain faking.
- Rebase onto previous changes.
- Accept a chain name in nft_xt_fake_builtin_chains() to fake only that
  specific chain.
---
 iptables/nft.c          | 96 ++++++++++++++++++++++++++++++++++-------
 iptables/nft.h          |  1 +
 iptables/xtables-save.c |  1 +
 3 files changed, 82 insertions(+), 16 deletions(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index 281f1f1962fb2..1a24c51605273 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -686,7 +686,8 @@ nft_chain_builtin_alloc(const struct builtin_table *table,
 
 static void nft_chain_builtin_add(struct nft_handle *h,
 				  const struct builtin_table *table,
-				  const struct builtin_chain *chain)
+				  const struct builtin_chain *chain,
+				  bool fake)
 {
 	struct nftnl_chain *c;
 
@@ -694,7 +695,8 @@ static void nft_chain_builtin_add(struct nft_handle *h,
 	if (c == NULL)
 		return;
 
-	batch_chain_add(h, NFT_COMPAT_CHAIN_ADD, c);
+	if (!fake)
+		batch_chain_add(h, NFT_COMPAT_CHAIN_ADD, c);
 	nft_cache_add_chain(h, table, c);
 }
 
@@ -746,11 +748,12 @@ static void nft_chain_builtin_init(struct nft_handle *h,
 		if (nft_chain_find(h, table->name, table->chains[i].name))
 			continue;
 
-		nft_chain_builtin_add(h, table, &table->chains[i]);
+		nft_chain_builtin_add(h, table, &table->chains[i], false);
 	}
 }
 
-static int nft_xt_builtin_init(struct nft_handle *h, const char *table)
+static const struct builtin_table *
+nft_xt_builtin_table_init(struct nft_handle *h, const char *table)
 {
 	const struct builtin_table *t;
 
@@ -758,20 +761,81 @@ static int nft_xt_builtin_init(struct nft_handle *h, const char *table)
 		return 0;
 
 	t = nft_table_builtin_find(h, table);
-	if (t == NULL)
-		return -1;
+	if (!t)
+		return NULL;
 
 	if (nft_table_builtin_add(h, t) < 0)
+		return NULL;
+
+	return t;
+}
+
+static int nft_xt_builtin_init(struct nft_handle *h, const char *table,
+			       const char *chain)
+{
+	const struct builtin_table *t;
+	const struct builtin_chain *c;
+
+	if (!h->cache_init)
+		return 0;
+
+	t = nft_xt_builtin_table_init(h, table);
+	if (!t)
 		return -1;
 
 	if (h->cache_req.level < NFT_CL_CHAINS)
 		return 0;
 
-	nft_chain_builtin_init(h, t);
+	if (!chain) {
+		nft_chain_builtin_init(h, t);
+		return 0;
+	}
+
+	c = nft_chain_builtin_find(t, chain);
+	if (!c)
+		return -1;
+
+	if (h->cache->table[t->type].base_chains[c->hook])
+		return 0;
+
+	nft_chain_builtin_add(h, t, c, false);
+	return 0;
+}
 
+static int __nft_xt_fake_builtin_chains(struct nft_handle *h,
+				        const char *table, void *data)
+{
+	const char *chain = data ? *(const char **)data : NULL;
+	const struct builtin_table *t;
+	struct nft_chain **bcp;
+	int i;
+
+	t = nft_table_builtin_find(h, table);
+	if (!t)
+		return -1;
+
+	bcp = h->cache->table[t->type].base_chains;
+	for (i = 0; i < NF_INET_NUMHOOKS && t->chains[i].name; i++) {
+		if (bcp[t->chains[i].hook])
+			continue;
+
+		if (chain && strcmp(chain, t->chains[i].name))
+			continue;
+
+		nft_chain_builtin_add(h, t, &t->chains[i], true);
+	}
 	return 0;
 }
 
+int nft_xt_fake_builtin_chains(struct nft_handle *h,
+			       const char *table, const char *chain)
+{
+	if (table)
+		return __nft_xt_fake_builtin_chains(h, table, &chain);
+
+	return nft_for_each_table(h, __nft_xt_fake_builtin_chains, &chain);
+}
+
 int nft_restart(struct nft_handle *h)
 {
 	mnl_socket_close(h->nl);
@@ -864,7 +928,7 @@ static struct nftnl_chain *nft_chain_new(struct nft_handle *h,
 	}
 
 	/* if this built-in table does not exists, create it */
-	nft_xt_builtin_init(h, table);
+	nft_xt_builtin_init(h, table, chain);
 
 	_c = nft_chain_builtin_find(_t, chain);
 	if (_c != NULL) {
@@ -1379,7 +1443,7 @@ nft_rule_append(struct nft_handle *h, const char *chain, const char *table,
 	struct nft_chain *c;
 	int type;
 
-	nft_xt_builtin_init(h, table);
+	nft_xt_builtin_init(h, table, chain);
 
 	nft_fn = nft_rule_append;
 
@@ -1658,7 +1722,7 @@ int nft_rule_flush(struct nft_handle *h, const char *chain, const char *table,
 	nft_fn = nft_rule_flush;
 
 	if (chain || verbose)
-		nft_xt_builtin_init(h, table);
+		nft_xt_builtin_init(h, table, chain);
 	else if (!nft_table_find(h, table))
 		return 1;
 
@@ -1691,7 +1755,7 @@ int nft_chain_user_add(struct nft_handle *h, const char *chain, const char *tabl
 
 	nft_fn = nft_chain_user_add;
 
-	nft_xt_builtin_init(h, table);
+	nft_xt_builtin_table_init(h, table);
 
 	if (nft_chain_exists(h, table, chain)) {
 		errno = EEXIST;
@@ -1724,7 +1788,7 @@ int nft_chain_restore(struct nft_handle *h, const char *chain, const char *table
 	bool created = false;
 	int ret;
 
-	nft_xt_builtin_init(h, table);
+	nft_xt_builtin_table_init(h, table);
 
 	nc = nft_chain_find(h, table, chain);
 	if (nc) {
@@ -2136,7 +2200,7 @@ int nft_rule_insert(struct nft_handle *h, const char *chain,
 	struct nftnl_rule *r = NULL;
 	struct nft_chain *c;
 
-	nft_xt_builtin_init(h, table);
+	nft_xt_builtin_init(h, table, chain);
 
 	nft_fn = nft_rule_insert;
 
@@ -2342,7 +2406,7 @@ int nft_rule_list(struct nft_handle *h, const char *chain, const char *table,
 	};
 	struct nft_chain *c;
 
-	nft_xt_builtin_init(h, table);
+	nft_xt_fake_builtin_chains(h, table, chain);
 	nft_assert_table_compatible(h, table, chain);
 
 	if (chain) {
@@ -2439,7 +2503,7 @@ int nft_rule_list_save(struct nft_handle *h, const char *chain,
 	struct nft_chain *c;
 	int ret = 0;
 
-	nft_xt_builtin_init(h, table);
+	nft_xt_fake_builtin_chains(h, table, chain);
 	nft_assert_table_compatible(h, table, chain);
 
 	if (counters < 0)
@@ -3046,7 +3110,7 @@ static int nft_prepare(struct nft_handle *h)
 							cmd->chain, cmd->policy);
 			break;
 		case NFT_COMPAT_SET_ADD:
-			nft_xt_builtin_init(h, cmd->table);
+			nft_xt_builtin_table_init(h, cmd->table);
 			batch_set_add(h, NFT_COMPAT_SET_ADD, cmd->obj.set);
 			ret = 1;
 			break;
diff --git a/iptables/nft.h b/iptables/nft.h
index 1a2506eea7b6c..0910f82a2773c 100644
--- a/iptables/nft.h
+++ b/iptables/nft.h
@@ -136,6 +136,7 @@ bool nft_table_find(struct nft_handle *h, const char *tablename);
 int nft_table_purge_chains(struct nft_handle *h, const char *table, struct nftnl_chain_list *list);
 int nft_table_flush(struct nft_handle *h, const char *table);
 const struct builtin_table *nft_table_builtin_find(struct nft_handle *h, const char *table);
+int nft_xt_fake_builtin_chains(struct nft_handle *h, const char *table, const char *chain);
 
 /*
  * Operations with chains.
diff --git a/iptables/xtables-save.c b/iptables/xtables-save.c
index bf00b0324cc4f..d7901c650ea70 100644
--- a/iptables/xtables-save.c
+++ b/iptables/xtables-save.c
@@ -236,6 +236,7 @@ xtables_save_main(int family, int argc, char *argv[],
 
 	nft_cache_level_set(&h, NFT_CL_RULES, NULL);
 	nft_cache_build(&h);
+	nft_xt_fake_builtin_chains(&h, tablename, NULL);
 
 	ret = do_output(&h, tablename, &d);
 	nft_fini(&h);
-- 
2.28.0

Re: [iptables PATCH v2 01/10] nft: Fix selective chain compatibility checks

[Pablo Neira Ayuso]

On Wed, Sep 23, 2020 at 07:48:40PM +0200, Phil Sutter wrote:
> Since commit 80251bc2a56ed ("nft: remove cache build calls"), 'chain'
> parameter passed to nft_chain_list_get() is no longer effective. To
> still support running nft_is_chain_compatible() on specific chains only,
> add a short path to nft_is_table_compatible().
> 
> Follow-up patches will kill nft_chain_list_get(), so don't bother
> dropping the unused parameter from its signature.

This has a Fixes: tag.

What is precisely the problem? How does show from the iptables and
iptables-restore interface?

Not sure I understand the problem.

> Fixes: 80251bc2a56ed ("nft: remove cache build calls")
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  iptables/nft.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/iptables/nft.c b/iptables/nft.c
> index 27bb98d184c7c..669e29d4cf88f 100644
> --- a/iptables/nft.c
> +++ b/iptables/nft.c
> @@ -3453,6 +3453,12 @@ bool nft_is_table_compatible(struct nft_handle *h,
>  {
>  	struct nftnl_chain_list *clist;
>  
> +	if (chain) {
> +		struct nftnl_chain *c = nft_chain_find(h, table, chain);
> +
> +		return c && !nft_is_chain_compatible(c, h);
> +	}
> +
>  	clist = nft_chain_list_get(h, table, chain);
>  	if (clist == NULL)
>  		return false;
> -- 
> 2.28.0
> 

Re: [iptables PATCH v2 02/10] nft: Implement nft_chain_foreach()

[Pablo Neira Ayuso]

On Wed, Sep 23, 2020 at 07:48:41PM +0200, Phil Sutter wrote:
> This is just a fancy wrapper around nftnl_chain_list_foreach() with the
> added benefit of detecting invalid table names or uninitialized chain
> lists. This in turn allows to drop the checks in flush_rule_cache() and
> ignore the return code of nft_chain_foreach() as it fails only if the
> dropped checks had failed, too.

At quick glance, this is reducing the LoC.

However, I'm not sure this is better, before this code:

1) You fetch the list
2) You use it from several spots in the function

with this patch you might look up for the chain list several times in
the same function.

+int nft_chain_foreach(struct nft_handle *h, const char *table,                
+                   int (*cb)(struct nftnl_chain *c, void *data),              
+                   void *data)                                                
+{                                                                             
+     const struct builtin_table *t;                                           
+                                                                              
+     t = nft_table_builtin_find(h, table);                                    
+     if (!t)                                                                  
+             return -1;                                                       
+                                                                              
+     if (!h->cache->table[t->type].chains)                                    
+             return -1;                                                       
+                                                                              
+     return nftnl_chain_list_foreach(h->cache->table[t->type].chains,         
+                                     cb, data);                               
+}

I can also see calls to:

nft_chain_find(h, table, chain);

and

nft_chain_foreach(...)

from the same function.

This patch also updates paths in very different ways, there is no
common idiom being replaced.

Re: [iptables PATCH v2 03/10] nft: cache: Introduce nft_cache_add_chain()

[Pablo Neira Ayuso]

On Wed, Sep 23, 2020 at 07:48:42PM +0200, Phil Sutter wrote:
> This is a convenience function for adding a chain to cache, for now just
> a simple wrapper around nftnl_chain_list_add_tail().
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
> Changes since v1:
> - Use the function in nft_chain_builtin_add() as well.
> ---
>  iptables/nft-cache.c | 12 +++++++++---
>  iptables/nft-cache.h |  3 +++
>  iptables/nft.c       | 16 +++++++---------
>  3 files changed, 19 insertions(+), 12 deletions(-)
> 
> diff --git a/iptables/nft-cache.c b/iptables/nft-cache.c
> index b94766a751db4..a22e693320451 100644
> --- a/iptables/nft-cache.c
> +++ b/iptables/nft-cache.c
> @@ -165,6 +165,13 @@ static int fetch_table_cache(struct nft_handle *h)
>  	return 1;
>  }
>  
> +int nft_cache_add_chain(struct nft_handle *h, const struct builtin_table *t,
> +			struct nftnl_chain *c)
> +{
> +	nftnl_chain_list_add_tail(c, h->cache->table[t->type].chains);
> +	return 0;
> +}

This wrapper LGTM.

>  struct nftnl_chain_list_cb_data {
>  	struct nft_handle *h;
>  	const struct builtin_table *t;
> @@ -174,7 +181,6 @@ static int nftnl_chain_list_cb(const struct nlmsghdr *nlh, void *data)
>  {
>  	struct nftnl_chain_list_cb_data *d = data;
>  	const struct builtin_table *t = d->t;
> -	struct nftnl_chain_list *list;
>  	struct nft_handle *h = d->h;
>  	struct nftnl_chain *c;
>  	const char *tname;
> @@ -196,8 +202,8 @@ static int nftnl_chain_list_cb(const struct nlmsghdr *nlh, void *data)
>  		goto out;
>  	}
>  
> -	list = h->cache->table[t->type].chains;
> -	nftnl_chain_list_add_tail(c, list);
> +	if (nft_cache_add_chain(h, t, c))
> +		goto out;
>  
>  	return MNL_CB_OK;
>  out:
> diff --git a/iptables/nft-cache.h b/iptables/nft-cache.h
> index 76f9fbb6c8ccc..d97f8de255f02 100644
> --- a/iptables/nft-cache.h
> +++ b/iptables/nft-cache.h
> @@ -3,6 +3,7 @@
>  
>  struct nft_handle;
>  struct nft_cmd;
> +struct builtin_table;
>  
>  void nft_cache_level_set(struct nft_handle *h, int level,
>  			 const struct nft_cmd *cmd);
> @@ -12,6 +13,8 @@ void flush_chain_cache(struct nft_handle *h, const char *tablename);
>  int flush_rule_cache(struct nft_handle *h, const char *table,
>  		     struct nftnl_chain *c);
>  void nft_cache_build(struct nft_handle *h);
> +int nft_cache_add_chain(struct nft_handle *h, const struct builtin_table *t,
> +			struct nftnl_chain *c);
>  
>  struct nftnl_chain_list *
>  nft_chain_list_get(struct nft_handle *h, const char *table, const char *chain);
> diff --git a/iptables/nft.c b/iptables/nft.c
> index 4f40be2e60252..8e1a33ba69bf1 100644
> --- a/iptables/nft.c
> +++ b/iptables/nft.c
> @@ -695,7 +695,7 @@ static void nft_chain_builtin_add(struct nft_handle *h,
>  		return;
>  
>  	batch_chain_add(h, NFT_COMPAT_CHAIN_ADD, c);
> -	nftnl_chain_list_add_tail(c, h->cache->table[table->type].chains);
> +	nft_cache_add_chain(h, table, c);
>  }
>  
>  /* find if built-in table already exists */
> @@ -1696,7 +1696,7 @@ int nft_rule_flush(struct nft_handle *h, const char *chain, const char *table,
>  
>  int nft_chain_user_add(struct nft_handle *h, const char *chain, const char *table)
>  {
> -	struct nftnl_chain_list *list;
> +	const struct builtin_table *t;
>  	struct nftnl_chain *c;
>  	int ret;
>  
> @@ -1720,9 +1720,8 @@ int nft_chain_user_add(struct nft_handle *h, const char *chain, const char *tabl
>  
>  	ret = batch_chain_add(h, NFT_COMPAT_CHAIN_USER_ADD, c);
>  
> -	list = nft_chain_list_get(h, table, chain);
> -	if (list)
> -		nftnl_chain_list_add(c, list);
> +	t = nft_table_builtin_find(h, table);

I'd add here:

        assert(t);

just in case this is ever crashing here, let's make it nice.

Re: [iptables PATCH v2 04/10] nft: Eliminate nft_chain_list_get()

[Pablo Neira Ayuso]

On Wed, Sep 23, 2020 at 07:48:43PM +0200, Phil Sutter wrote:
> Since introduction of nft_cache_add_chain(), there is merely a single
> user of nft_chain_list_get() left. Hence fold the function into its
> caller.

Why this last update regarding nft_chain_list_get() and not in 02/10 ?

> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  iptables/nft-cache.c | 13 -------------
>  iptables/nft-cache.h |  2 --
>  iptables/nft.c       |  8 +++++---
>  3 files changed, 5 insertions(+), 18 deletions(-)
> 
> diff --git a/iptables/nft-cache.c b/iptables/nft-cache.c
> index a22e693320451..109524c3fbc79 100644
> --- a/iptables/nft-cache.c
> +++ b/iptables/nft-cache.c
> @@ -684,16 +684,3 @@ nft_set_list_get(struct nft_handle *h, const char *table, const char *set)
>  
>  	return h->cache->table[t->type].sets;
>  }
> -
> -struct nftnl_chain_list *
> -nft_chain_list_get(struct nft_handle *h, const char *table, const char *chain)
> -{
> -	const struct builtin_table *t;
> -
> -	t = nft_table_builtin_find(h, table);
> -	if (!t)
> -		return NULL;
> -
> -	return h->cache->table[t->type].chains;
> -}
> -
> diff --git a/iptables/nft-cache.h b/iptables/nft-cache.h
> index d97f8de255f02..52ad2d396199e 100644
> --- a/iptables/nft-cache.h
> +++ b/iptables/nft-cache.h
> @@ -16,8 +16,6 @@ void nft_cache_build(struct nft_handle *h);
>  int nft_cache_add_chain(struct nft_handle *h, const struct builtin_table *t,
>  			struct nftnl_chain *c);
>  
> -struct nftnl_chain_list *
> -nft_chain_list_get(struct nft_handle *h, const char *table, const char *chain);
>  struct nftnl_set_list *
>  nft_set_list_get(struct nft_handle *h, const char *table, const char *set);
>  
> diff --git a/iptables/nft.c b/iptables/nft.c
> index 8e1a33ba69bf1..5967d36038953 100644
> --- a/iptables/nft.c
> +++ b/iptables/nft.c
> @@ -1837,13 +1837,15 @@ out:
>  static struct nftnl_chain *
>  nft_chain_find(struct nft_handle *h, const char *table, const char *chain)
>  {
> +	const struct builtin_table *t;
>  	struct nftnl_chain_list *list;
>  
> -	list = nft_chain_list_get(h, table, chain);
> -	if (list == NULL)
> +	t = nft_table_builtin_find(h, table);
> +	if (!t)
>  		return NULL;
>  
> -	return nftnl_chain_list_lookup_byname(list, chain);
> +	list = h->cache->table[t->type].chains;
> +	return list ? nftnl_chain_list_lookup_byname(list, chain) : NULL;
>  }
>  
>  bool nft_chain_exists(struct nft_handle *h,
> -- 
> 2.28.0
> 

Re: [iptables PATCH v2 06/10] nft: Introduce struct nft_chain

[Pablo Neira Ayuso]

On Wed, Sep 23, 2020 at 07:48:45PM +0200, Phil Sutter wrote:
> Preparing for ordered output of user-defined chains, introduce a local
> datatype wrapping nftnl_chain. In order to maintain the chain name hash
> table, introduce nft_chain_list as well and use it instead of
> nftnl_chain_list.
> 
> Put everything into a dedicated source file and provide a bunch of
> getters for attributes of the embedded libnftnl_chain object.
[...]
> diff --git a/iptables/nft-chain.h b/iptables/nft-chain.h
> new file mode 100644
> index 0000000000000..818bbf1f4b525
> --- /dev/null
> +++ b/iptables/nft-chain.h
> @@ -0,0 +1,87 @@
> +#ifndef _NFT_CHAIN_H_
> +#define _NFT_CHAIN_H_
> +
> +#include <libnftnl/chain.h>
> +#include <libiptc/linux_list.h>
> +
> +struct nft_handle;
> +
> +struct nft_chain {
> +	struct list_head	head;
> +	struct hlist_node	hnode;
> +	struct nftnl_chain	*nftnl;
> +};
> +
> +#define CHAIN_NAME_HSIZE	512
> +
> +struct nft_chain_list {
> +	struct list_head	list;
> +	struct hlist_head	names[CHAIN_NAME_HSIZE];
> +};
> +
> +struct nft_chain *nft_chain_alloc(struct nftnl_chain *nftnl);
> +void nft_chain_free(struct nft_chain *c);
> +
> +struct nft_chain_list *nft_chain_list_alloc(void);
> +void nft_chain_list_free(struct nft_chain_list *list);
> +void nft_chain_list_del(struct nft_chain *c);
> +
> +static inline const char *nft_chain_name(struct nft_chain *c)
> +{
> +	return nftnl_chain_get_str(c->nftnl, NFTNL_CHAIN_NAME);
> +}
> +
> +static inline const char *nft_chain_table(struct nft_chain *c)
> +{
> +	return nftnl_chain_get_str(c->nftnl, NFTNL_CHAIN_TABLE);
> +}
> +
> +static inline const char *nft_chain_type(struct nft_chain *c)
> +{
> +	return nftnl_chain_get_str(c->nftnl, NFTNL_CHAIN_TYPE);
> +}
> +
> +static inline uint32_t nft_chain_prio(struct nft_chain *c)
> +{
> +	return nftnl_chain_get_u32(c->nftnl, NFTNL_CHAIN_PRIO);
> +}
> +
> +static inline uint32_t nft_chain_hooknum(struct nft_chain *c)
> +{
> +	return nftnl_chain_get_u32(c->nftnl, NFTNL_CHAIN_HOOKNUM);
> +}
> +
> +static inline uint64_t nft_chain_packets(struct nft_chain *c)
> +{
> +	return nftnl_chain_get_u64(c->nftnl, NFTNL_CHAIN_PACKETS);
> +}
> +
> +static inline uint64_t nft_chain_bytes(struct nft_chain *c)
> +{
> +	return nftnl_chain_get_u64(c->nftnl, NFTNL_CHAIN_BYTES);
> +}
> +
> +static inline bool nft_chain_has_policy(struct nft_chain *c)
> +{
> +	return nftnl_chain_is_set(c->nftnl, NFTNL_CHAIN_POLICY);
> +}
> +
> +static inline uint32_t nft_chain_policy(struct nft_chain *c)
> +{
> +	return nftnl_chain_get_u32(c->nftnl, NFTNL_CHAIN_POLICY);
> +}
> +
> +static inline uint32_t nft_chain_use(struct nft_chain *c)
> +{
> +	return nftnl_chain_get_u32(c->nftnl, NFTNL_CHAIN_USE);
> +}

Do you need this wrapper functions now? I mean, the intention is to
have a native nft_chain structure so nft_chain_use() become:

static inline uint32_t nft_chain_use(struct nft_chain *c)
{
	return c->use;
}

at some point?

Sorry but I don't see this is happening in this batch?

I remember the original intention was to support for sorting chains,
so the listing is predictable. But this batch is updating more things
than that and I don't see a clear connection with the goal.

Thanks Phil.

Re: [iptables PATCH v2 01/10] nft: Fix selective chain compatibility checks

[Phil Sutter]

Hi Pablo,

On Mon, Oct 12, 2020 at 01:54:24PM +0200, Pablo Neira Ayuso wrote:
> On Wed, Sep 23, 2020 at 07:48:40PM +0200, Phil Sutter wrote:
> > Since commit 80251bc2a56ed ("nft: remove cache build calls"), 'chain'
> > parameter passed to nft_chain_list_get() is no longer effective. To
> > still support running nft_is_chain_compatible() on specific chains only,
> > add a short path to nft_is_table_compatible().
> > 
> > Follow-up patches will kill nft_chain_list_get(), so don't bother
> > dropping the unused parameter from its signature.
> 
> This has a Fixes: tag.
> 
> What is precisely the problem? How does show from the iptables and
> iptables-restore interface?
> 
> Not sure I understand the problem.

Before the big caching rework, nft_chain_list_get() could cause a
cache-fetch to populate table's chain list. If a chain name was passed
to it, that cache-fetch would retrieve only the specified chain (if not
in cache already).

In the current code, nft_is_table_compatible() happens in the second
stage where cache is present already and nft_chain_list_get() really
just returns the table's chain list again. This means that the
compatibility check happens for all chains in cache which belong to that
table and the original optimization (to check only the interesting
chain) is not effective anymore.

The "short path" (actually: short-cut) introduced in this patch allows
to limit compat check to a specific chain again. The impact is not as
big anymore as nft_chain_list_get() does no longer populate the cache,
but still there's no point in checking unintereresting chains'
compatibility.

Above all else though, this is fallout from the preparations to drop
nft_chain_list_get() and given the above, I found it makes sense to push
it as a separate commit.

Cheers, Phil

Re: [iptables PATCH v2 02/10] nft: Implement nft_chain_foreach()

[Phil Sutter]

Hi,

On Mon, Oct 12, 2020 at 02:01:18PM +0200, Pablo Neira Ayuso wrote:
> On Wed, Sep 23, 2020 at 07:48:41PM +0200, Phil Sutter wrote:
> > This is just a fancy wrapper around nftnl_chain_list_foreach() with the
> > added benefit of detecting invalid table names or uninitialized chain
> > lists. This in turn allows to drop the checks in flush_rule_cache() and
> > ignore the return code of nft_chain_foreach() as it fails only if the
> > dropped checks had failed, too.
> 
> At quick glance, this is reducing the LoC.
> 
> However, I'm not sure this is better, before this code:
> 
> 1) You fetch the list
> 2) You use it from several spots in the function
> 
> with this patch you might look up for the chain list several times in
> the same function.

Hmm. There might be exceptions, but typically we should have a function
that takes an optional chain name and the body roughly looks like this:

| if (chain) {
| 	return do_something(nft_chain_find(..., chain));
| }
| return nft_chain_foreach(..., do_something);

[...]
> I can also see calls to:
> 
> nft_chain_find(h, table, chain);
> 
> and
> 
> nft_chain_foreach(...)
> 
> from the same function.
> 
> This patch also updates paths in very different ways, there is no
> common idiom being replaced.

Which in particular are those?

The overall agenda is that looking up a chain won't be a trivial
nftnl_chain_list lookup anymore. And iterating over a table's chains
won't be a matter of iterating over a list, because I split base-chains
from user-defined ones:

* Base-chains sit in an array of size NF_INET_NUMHOOKS.
* User-defined chains sit in an (open-coded) list ordered by name.

So the old nft_chain_list_get() is not possible anymore, therefore I
replace everything by either nft_chain_find() or nft_chain_foreach().
Functions should not use both in the same code-path, so if you spot that
I should have a close look again.

Cheers, Phil

Re: [iptables PATCH v2 04/10] nft: Eliminate nft_chain_list_get()

[Phil Sutter]

On Mon, Oct 12, 2020 at 02:03:41PM +0200, Pablo Neira Ayuso wrote:
> On Wed, Sep 23, 2020 at 07:48:43PM +0200, Phil Sutter wrote:
> > Since introduction of nft_cache_add_chain(), there is merely a single
> > user of nft_chain_list_get() left. Hence fold the function into its
> > caller.
> 
> Why this last update regarding nft_chain_list_get() and not in 02/10 ?

Ah yes, I could have reordered the patches a bit. In patch 3/10, there
are some nft_chain_list_get() removals, too. So I couldn't remove the
function in 2/10 already. I should move 3/10 to before 2/10 and then
combine that with 4/10. ACK?

Thanks, Phil

Re: [iptables PATCH v2 06/10] nft: Introduce struct nft_chain

[Phil Sutter]

On Mon, Oct 12, 2020 at 02:08:55PM +0200, Pablo Neira Ayuso wrote:
> On Wed, Sep 23, 2020 at 07:48:45PM +0200, Phil Sutter wrote:
> > Preparing for ordered output of user-defined chains, introduce a local
> > datatype wrapping nftnl_chain. In order to maintain the chain name hash
> > table, introduce nft_chain_list as well and use it instead of
> > nftnl_chain_list.
> > 
> > Put everything into a dedicated source file and provide a bunch of
> > getters for attributes of the embedded libnftnl_chain object.
> [...]
> > diff --git a/iptables/nft-chain.h b/iptables/nft-chain.h
> > new file mode 100644
> > index 0000000000000..818bbf1f4b525
> > --- /dev/null
> > +++ b/iptables/nft-chain.h
> > @@ -0,0 +1,87 @@
> > +#ifndef _NFT_CHAIN_H_
> > +#define _NFT_CHAIN_H_
> > +
> > +#include <libnftnl/chain.h>
> > +#include <libiptc/linux_list.h>
> > +
> > +struct nft_handle;
> > +
> > +struct nft_chain {
> > +	struct list_head	head;
> > +	struct hlist_node	hnode;
> > +	struct nftnl_chain	*nftnl;
> > +};
> > +
> > +#define CHAIN_NAME_HSIZE	512
> > +
> > +struct nft_chain_list {
> > +	struct list_head	list;
> > +	struct hlist_head	names[CHAIN_NAME_HSIZE];
> > +};
> > +
> > +struct nft_chain *nft_chain_alloc(struct nftnl_chain *nftnl);
> > +void nft_chain_free(struct nft_chain *c);
> > +
> > +struct nft_chain_list *nft_chain_list_alloc(void);
> > +void nft_chain_list_free(struct nft_chain_list *list);
> > +void nft_chain_list_del(struct nft_chain *c);
> > +
> > +static inline const char *nft_chain_name(struct nft_chain *c)
> > +{
> > +	return nftnl_chain_get_str(c->nftnl, NFTNL_CHAIN_NAME);
> > +}
> > +
> > +static inline const char *nft_chain_table(struct nft_chain *c)
> > +{
> > +	return nftnl_chain_get_str(c->nftnl, NFTNL_CHAIN_TABLE);
> > +}
> > +
> > +static inline const char *nft_chain_type(struct nft_chain *c)
> > +{
> > +	return nftnl_chain_get_str(c->nftnl, NFTNL_CHAIN_TYPE);
> > +}
> > +
> > +static inline uint32_t nft_chain_prio(struct nft_chain *c)
> > +{
> > +	return nftnl_chain_get_u32(c->nftnl, NFTNL_CHAIN_PRIO);
> > +}
> > +
> > +static inline uint32_t nft_chain_hooknum(struct nft_chain *c)
> > +{
> > +	return nftnl_chain_get_u32(c->nftnl, NFTNL_CHAIN_HOOKNUM);
> > +}
> > +
> > +static inline uint64_t nft_chain_packets(struct nft_chain *c)
> > +{
> > +	return nftnl_chain_get_u64(c->nftnl, NFTNL_CHAIN_PACKETS);
> > +}
> > +
> > +static inline uint64_t nft_chain_bytes(struct nft_chain *c)
> > +{
> > +	return nftnl_chain_get_u64(c->nftnl, NFTNL_CHAIN_BYTES);
> > +}
> > +
> > +static inline bool nft_chain_has_policy(struct nft_chain *c)
> > +{
> > +	return nftnl_chain_is_set(c->nftnl, NFTNL_CHAIN_POLICY);
> > +}
> > +
> > +static inline uint32_t nft_chain_policy(struct nft_chain *c)
> > +{
> > +	return nftnl_chain_get_u32(c->nftnl, NFTNL_CHAIN_POLICY);
> > +}
> > +
> > +static inline uint32_t nft_chain_use(struct nft_chain *c)
> > +{
> > +	return nftnl_chain_get_u32(c->nftnl, NFTNL_CHAIN_USE);
> > +}
> 
> Do you need this wrapper functions now? I mean, the intention is to
> have a native nft_chain structure so nft_chain_use() become:
> 
> static inline uint32_t nft_chain_use(struct nft_chain *c)
> {
> 	return c->use;
> }

Potentially, yes. I played with "pre-extracting" some attributes from
nftnl_chain object once, populating pointers in nft_chain object and
making above getters just return that pointer.

> at some point?
> 
> Sorry but I don't see this is happening in this batch?

I'm not entirely sure it is much use. In fact, we get nftnl_chain
objects when fetching cache from kernel and when pushing changes to
kernel we need nftnl_chain objects again. So unless we're concerned with
memory use in between or we get rid of the need for nftnl_chain objects,
dropping and recreating them is just extra work.

> I remember the original intention was to support for sorting chains,
> so the listing is predictable. But this batch is updating more things
> than that and I don't see a clear connection with the goal.

The nft_chain struct is needed mostly to allow for sorting the chain
list without adding code to libnftnl. This was your request before this
respin, you claimed it is important to keep libnftnl code size small. So
nft_chain is mostly a reimplementation of nftnl_chain_list, including
chain name hash.

The getter introduction is handy as I had many change like:

- nftnl_rule_set_str(rule, NFTNL_RULE_TABLE,
-                    nftnl_chain_get_str(c, NFTNL_CHAIN_TABLE));
+ nftnl_rule_set_str(rule, NFTNL_RULE_TABLE,
+                    nftnl_chain_get_str(c->nftnl, NFTNL_CHAIN_TABLE));

Given that I have to touch those lines anyway, with introducing a getter
things become a bit cleaner also:

- nftnl_rule_set_str(rule, NFTNL_RULE_TABLE,
-                    nftnl_chain_get_str(c, NFTNL_CHAIN_TABLE));
+ nftnl_rule_set_str(rule, NFTNL_RULE_TABLE, nft_chain_table(c));

Thinking about it, I could introduce the getters in a separate, early
patch. This would reduce the nft_chain introduction patch quite a bit.
But since these getters are (in the current form) specific to nft_chain
objects, I find it all belongs together.

Cheers, Phil

Re: [iptables PATCH v2 03/10] nft: cache: Introduce nft_cache_add_chain()

[Phil Sutter]

On Mon, Oct 12, 2020 at 02:02:31PM +0200, Pablo Neira Ayuso wrote:
> On Wed, Sep 23, 2020 at 07:48:42PM +0200, Phil Sutter wrote:
> > This is a convenience function for adding a chain to cache, for now just
> > a simple wrapper around nftnl_chain_list_add_tail().
> > 
> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > ---
> > Changes since v1:
> > - Use the function in nft_chain_builtin_add() as well.
> > ---
> >  iptables/nft-cache.c | 12 +++++++++---
> >  iptables/nft-cache.h |  3 +++
> >  iptables/nft.c       | 16 +++++++---------
> >  3 files changed, 19 insertions(+), 12 deletions(-)
> > 
> > diff --git a/iptables/nft-cache.c b/iptables/nft-cache.c
> > index b94766a751db4..a22e693320451 100644
> > --- a/iptables/nft-cache.c
> > +++ b/iptables/nft-cache.c
> > @@ -165,6 +165,13 @@ static int fetch_table_cache(struct nft_handle *h)
> >  	return 1;
> >  }
> >  
> > +int nft_cache_add_chain(struct nft_handle *h, const struct builtin_table *t,
> > +			struct nftnl_chain *c)
> > +{
> > +	nftnl_chain_list_add_tail(c, h->cache->table[t->type].chains);
> > +	return 0;
> > +}
> 
> This wrapper LGTM.
> 
> >  struct nftnl_chain_list_cb_data {
> >  	struct nft_handle *h;
> >  	const struct builtin_table *t;
> > @@ -174,7 +181,6 @@ static int nftnl_chain_list_cb(const struct nlmsghdr *nlh, void *data)
> >  {
> >  	struct nftnl_chain_list_cb_data *d = data;
> >  	const struct builtin_table *t = d->t;
> > -	struct nftnl_chain_list *list;
> >  	struct nft_handle *h = d->h;
> >  	struct nftnl_chain *c;
> >  	const char *tname;
> > @@ -196,8 +202,8 @@ static int nftnl_chain_list_cb(const struct nlmsghdr *nlh, void *data)
> >  		goto out;
> >  	}
> >  
> > -	list = h->cache->table[t->type].chains;
> > -	nftnl_chain_list_add_tail(c, list);
> > +	if (nft_cache_add_chain(h, t, c))
> > +		goto out;
> >  
> >  	return MNL_CB_OK;
> >  out:
> > diff --git a/iptables/nft-cache.h b/iptables/nft-cache.h
> > index 76f9fbb6c8ccc..d97f8de255f02 100644
> > --- a/iptables/nft-cache.h
> > +++ b/iptables/nft-cache.h
> > @@ -3,6 +3,7 @@
> >  
> >  struct nft_handle;
> >  struct nft_cmd;
> > +struct builtin_table;
> >  
> >  void nft_cache_level_set(struct nft_handle *h, int level,
> >  			 const struct nft_cmd *cmd);
> > @@ -12,6 +13,8 @@ void flush_chain_cache(struct nft_handle *h, const char *tablename);
> >  int flush_rule_cache(struct nft_handle *h, const char *table,
> >  		     struct nftnl_chain *c);
> >  void nft_cache_build(struct nft_handle *h);
> > +int nft_cache_add_chain(struct nft_handle *h, const struct builtin_table *t,
> > +			struct nftnl_chain *c);
> >  
> >  struct nftnl_chain_list *
> >  nft_chain_list_get(struct nft_handle *h, const char *table, const char *chain);
> > diff --git a/iptables/nft.c b/iptables/nft.c
> > index 4f40be2e60252..8e1a33ba69bf1 100644
> > --- a/iptables/nft.c
> > +++ b/iptables/nft.c
> > @@ -695,7 +695,7 @@ static void nft_chain_builtin_add(struct nft_handle *h,
> >  		return;
> >  
> >  	batch_chain_add(h, NFT_COMPAT_CHAIN_ADD, c);
> > -	nftnl_chain_list_add_tail(c, h->cache->table[table->type].chains);
> > +	nft_cache_add_chain(h, table, c);
> >  }
> >  
> >  /* find if built-in table already exists */
> > @@ -1696,7 +1696,7 @@ int nft_rule_flush(struct nft_handle *h, const char *chain, const char *table,
> >  
> >  int nft_chain_user_add(struct nft_handle *h, const char *chain, const char *table)
> >  {
> > -	struct nftnl_chain_list *list;
> > +	const struct builtin_table *t;
> >  	struct nftnl_chain *c;
> >  	int ret;
> >  
> > @@ -1720,9 +1720,8 @@ int nft_chain_user_add(struct nft_handle *h, const char *chain, const char *tabl
> >  
> >  	ret = batch_chain_add(h, NFT_COMPAT_CHAIN_USER_ADD, c);
> >  
> > -	list = nft_chain_list_get(h, table, chain);
> > -	if (list)
> > -		nftnl_chain_list_add(c, list);
> > +	t = nft_table_builtin_find(h, table);
> 
> I'd add here:
> 
>         assert(t);
> 
> just in case this is ever crashing here, let's make it nice.

It is not needed: In nft_chain_user_add(), there is a call to
nft_chain_exists() earlier which implicitly checks that
nft_table_builtin_find() succeeds with given table name.

The other two places the wrapper is used are fine, too:

In nft_chain_builtin_add(), the builtin_table pointer was passed via
parameters from nft_xt_builtin_init() which itself does a NULL-pointer
check.

In nft_chain_restore(), the table's name is passed from state->curtable
pointer in xtables_restore_parse_line() which in turn was acquired via
nft_table_builtin_find(). (I actually plan to pass this builtin_table
pointer around instead of the table name at some point.)

Cheers, Phil