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

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

[Phil Sutter]

Work in this series centered around Harald's complaint about seemingly
random custom chain ordering in iptables-nft-save output. In fact,
nftables returns chains in the order they were created which differs
from legacy iptables which sorts by name.

The intuitive approach of simply sorting chains in tables'
nftnl_chain_lists is problematic since base chains, which shall be
dumped first, are contained in there as well. Patch 15 solves this by
introducing a per-table array of nftnl_chain pointers to hold only base
chains (the hook values determine the array index). The old
nftnl_chain_list now contains merely non-base chains and is sorted upon
population by the new nftnl_chain_list_add_sorted() function.

Having dedicated slots for base chains allows for another neat trick,
namely to create only immediately required base chains. Apart from the
obvious case, where adding a rule to OUTPUT chain doesn't cause creation
of INPUT or FORWARD chains, this means ruleset modifications can be
avoided completely when listing, flushing or zeroing counters (unless
chains exist).

The first 14 patches are mostly just preliminary work and some
distinct optimizations found while working on the actual features.

Patch 15 introduces the mentioned base chain array and updates related
routines to be aware of the potential other location a given chain name
may be found in.

Patch 16 enables custom chain sorting at cache population time (or when
new chains are created by the user). It depends on my recent patch sent
for libnftnl.

Patch 17 drops the various workarounds from tests/shell dealing with
differing iptables-save output. This implicitly also enables testing of
the sorting feature.

Patch 18 Changes nft_xt_builtin_init() to accept a specific chain which
should be created, adds nft_xt_builtin_table_init() to create just the
table and no chains at all and nft_xt_fake_builtin_chains() to populate
empty base chain slots with fake entries for ruleset listing commands.

Phil Sutter (18):
  nft: Make table creation purely implicit
  nft: Be lazy when flushing
  nft: cache: Drop duplicate chain check
  nft: Drop pointless nft_xt_builtin_init() call
  nft: Turn nft_chain_save() into a foreach-callback
  nft: Use nft_chain_find() in two more places
  nft: Reorder enum nft_table_type
  nft: cache: Fetch only interesting tables from kernel
  nft: Use nftnl_chain_list_foreach in nft_rule_list{,_save}
  nft: Use nftnl_chain_list_foreach in nft_rule_flush
  nft: Use nftnl_chain_foreach in nft_rule_save
  nft: Fold nftnl_rule_list_chain_save() into caller
  nft: Implement nft_chain_foreach()
  nft: cache: Introduce nft_cache_add_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/nft-cache.c                          | 107 ++--
 iptables/nft-cache.h                          |   3 +
 iptables/nft-cmd.c                            |   5 -
 iptables/nft.c                                | 466 +++++++++---------
 iptables/nft.h                                |  15 +-
 .../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-restore.c                    |   3 -
 iptables/xtables-save.c                       |   8 +-
 11 files changed, 335 insertions(+), 297 deletions(-)

-- 
2.27.0

[iptables PATCH 01/18] nft: Make table creation purely implicit

[Phil Sutter]

While asserting a required builtin chain exists, its table is created
implicitly if missing. Exploit this from xtables-restore, too: The only
actions which need adjustment are chain_new and chain_restore, i.e. when
restoring (either builtin or custom) chains.

Note: The call to nft_table_builtin_add() wasn't sufficient as it
doesn't set the table as initialized and therefore a following call to
nft_xt_builtin_init() would override non-default base chain policies.

Note2: The 'table_new' callback in 'nft_xt_restore_cb' is left in place
as xtables-translate uses it to print an explicit 'add table' command.

Note3: nft_table_new() function was already unused since a7f1e208cdf9c
("nft: split parsing from netlink commands").

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft-cmd.c         |  5 -----
 iptables/nft.c             | 17 +++--------------
 iptables/nft.h             |  2 --
 iptables/xtables-restore.c |  3 ---
 4 files changed, 3 insertions(+), 24 deletions(-)

diff --git a/iptables/nft-cmd.c b/iptables/nft-cmd.c
index 51cdfed41519c..5d33f1f00f574 100644
--- a/iptables/nft-cmd.c
+++ b/iptables/nft-cmd.c
@@ -393,8 +393,3 @@ int ebt_cmd_user_chain_policy(struct nft_handle *h, const char *table,
 
 	return 1;
 }
-
-void nft_cmd_table_new(struct nft_handle *h, const char *table)
-{
-	nft_cmd_new(h, NFT_COMPAT_TABLE_NEW, table, NULL, NULL, -1, false);
-}
diff --git a/iptables/nft.c b/iptables/nft.c
index 0c5a74fc232c6..c5ab0dbe8d6e7 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -350,7 +350,6 @@ static int mnl_append_error(const struct nft_handle *h,
 	case NFT_COMPAT_RULE_SAVE:
 	case NFT_COMPAT_RULE_ZERO:
 	case NFT_COMPAT_BRIDGE_USER_CHAIN_UPDATE:
-	case NFT_COMPAT_TABLE_NEW:
 		assert(0);
 		break;
 	}
@@ -892,7 +891,7 @@ static struct nftnl_chain *nft_chain_new(struct nft_handle *h,
 	}
 
 	/* if this built-in table does not exists, create it */
-	nft_table_builtin_add(h, _t);
+	nft_xt_builtin_init(h, table);
 
 	_c = nft_chain_builtin_find(_t, chain);
 	if (_c != NULL) {
@@ -1789,6 +1788,8 @@ 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);
+
 	c = nft_chain_find(h, table, chain);
 	if (c) {
 		/* Apparently -n still flushes existing user defined
@@ -2099,11 +2100,6 @@ err_out:
 	return ret == 0 ? 1 : 0;
 }
 
-void nft_table_new(struct nft_handle *h, const char *table)
-{
-	nft_xt_builtin_init(h, table);
-}
-
 static int __nft_rule_del(struct nft_handle *h, struct nftnl_rule *r)
 {
 	struct obj_update *obj;
@@ -2735,7 +2731,6 @@ static void batch_obj_del(struct nft_handle *h, struct obj_update *o)
 	case NFT_COMPAT_RULE_SAVE:
 	case NFT_COMPAT_RULE_ZERO:
 	case NFT_COMPAT_BRIDGE_USER_CHAIN_UPDATE:
-	case NFT_COMPAT_TABLE_NEW:
 		assert(0);
 		break;
 	}
@@ -2811,7 +2806,6 @@ static void nft_refresh_transaction(struct nft_handle *h)
 		case NFT_COMPAT_RULE_SAVE:
 		case NFT_COMPAT_RULE_ZERO:
 		case NFT_COMPAT_BRIDGE_USER_CHAIN_UPDATE:
-		case NFT_COMPAT_TABLE_NEW:
 			break;
 		}
 	}
@@ -2915,7 +2909,6 @@ retry:
 		case NFT_COMPAT_RULE_SAVE:
 		case NFT_COMPAT_RULE_ZERO:
 		case NFT_COMPAT_BRIDGE_USER_CHAIN_UPDATE:
-		case NFT_COMPAT_TABLE_NEW:
 			assert(0);
 		}
 
@@ -3178,10 +3171,6 @@ static int nft_prepare(struct nft_handle *h)
 			ret = ebt_set_user_chain_policy(h, cmd->table,
 							cmd->chain, cmd->policy);
 			break;
-		case NFT_COMPAT_TABLE_NEW:
-			nft_xt_builtin_init(h, cmd->table);
-			ret = 1;
-			break;
 		case NFT_COMPAT_SET_ADD:
 			nft_xt_builtin_init(h, cmd->table);
 			batch_set_add(h, NFT_COMPAT_SET_ADD, cmd->obj.set);
diff --git a/iptables/nft.h b/iptables/nft.h
index bd783231156b7..bd944f441caf1 100644
--- a/iptables/nft.h
+++ b/iptables/nft.h
@@ -68,7 +68,6 @@ enum obj_update_type {
 	NFT_COMPAT_RULE_SAVE,
 	NFT_COMPAT_RULE_ZERO,
 	NFT_COMPAT_BRIDGE_USER_CHAIN_UPDATE,
-	NFT_COMPAT_TABLE_NEW,
 };
 
 struct cache_chain {
@@ -135,7 +134,6 @@ int nft_for_each_table(struct nft_handle *h, int (*func)(struct nft_handle *h, c
 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);
-void nft_table_new(struct nft_handle *h, const char *table);
 const struct builtin_table *nft_table_builtin_find(struct nft_handle *h, const char *table);
 
 /*
diff --git a/iptables/xtables-restore.c b/iptables/xtables-restore.c
index eb25ec3dc8398..d27394972d90c 100644
--- a/iptables/xtables-restore.c
+++ b/iptables/xtables-restore.c
@@ -61,7 +61,6 @@ static void print_usage(const char *name, const char *version)
 static const struct nft_xt_restore_cb restore_cb = {
 	.commit		= nft_commit,
 	.abort		= nft_abort,
-	.table_new	= nft_cmd_table_new,
 	.table_flush	= nft_cmd_table_flush,
 	.do_command	= do_commandx,
 	.chain_set	= nft_cmd_chain_set,
@@ -410,7 +409,6 @@ int xtables_ip6_restore_main(int argc, char *argv[])
 
 static const struct nft_xt_restore_cb ebt_restore_cb = {
 	.commit		= nft_bridge_commit,
-	.table_new	= nft_cmd_table_new,
 	.table_flush	= nft_cmd_table_flush,
 	.do_command	= do_commandeb,
 	.chain_set	= nft_cmd_chain_set,
@@ -456,7 +454,6 @@ int xtables_eb_restore_main(int argc, char *argv[])
 
 static const struct nft_xt_restore_cb arp_restore_cb = {
 	.commit		= nft_commit,
-	.table_new	= nft_cmd_table_new,
 	.table_flush	= nft_cmd_table_flush,
 	.do_command	= do_commandarp,
 	.chain_set	= nft_cmd_chain_set,
-- 
2.27.0

[iptables PATCH 02/18] nft: Be lazy when flushing

[Phil Sutter]

If neither chain nor verbose flag was specified and the table to flush
doesn't exist yet, no action is needed (as there is nothing to flush
anyway).

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index c5ab0dbe8d6e7..52ee809b6bc07 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -1699,16 +1699,18 @@ int nft_rule_flush(struct nft_handle *h, const char *chain, const char *table,
 	struct nftnl_chain *c = NULL;
 	int ret = 0;
 
-	nft_xt_builtin_init(h, table);
-
 	nft_fn = nft_rule_flush;
 
 	if (chain || verbose) {
+		nft_xt_builtin_init(h, table);
+
 		list = nft_chain_list_get(h, table, chain);
 		if (list == NULL) {
 			ret = 1;
 			goto err;
 		}
+	} else if (!nft_table_find(h, table)) {
+		return 1;
 	}
 
 	if (chain) {
-- 
2.27.0

[iptables PATCH 03/18] nft: cache: Drop duplicate chain check

[Phil Sutter]

When fetching chains from kernel, checking for duplicate chain names is
not needed: Nftables doesn't support them in the first place. This is
merely a leftover from when multiple cache fetches could happen and so a
bit of sanity checking was in order.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft-cache.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/iptables/nft-cache.c b/iptables/nft-cache.c
index 638b18bc7e382..059f0a7f7891e 100644
--- a/iptables/nft-cache.c
+++ b/iptables/nft-cache.c
@@ -180,8 +180,8 @@ static int nftnl_chain_list_cb(const struct nlmsghdr *nlh, void *data)
 	const struct builtin_table *t = d->t;
 	struct nftnl_chain_list *list;
 	struct nft_handle *h = d->h;
-	const char *tname, *cname;
 	struct nftnl_chain *c;
+	const char *tname;
 
 	c = nftnl_chain_alloc();
 	if (c == NULL)
@@ -201,11 +201,6 @@ static int nftnl_chain_list_cb(const struct nlmsghdr *nlh, void *data)
 	}
 
 	list = h->cache->table[t->type].chains;
-	cname = nftnl_chain_get_str(c, NFTNL_CHAIN_NAME);
-
-	if (nftnl_chain_list_lookup_byname(list, cname))
-		goto out;
-
 	nftnl_chain_list_add_tail(c, list);
 
 	return MNL_CB_OK;
-- 
2.27.0

[iptables PATCH 04/18] nft: Drop pointless nft_xt_builtin_init() call

[Phil Sutter]

When renaming a chain, either everything is in place already or the
command will bail anyway. So just drop this superfluous call.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index 52ee809b6bc07..e3811f5fb20b0 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -1937,8 +1937,6 @@ int nft_chain_user_rename(struct nft_handle *h,const char *chain,
 		return 0;
 	}
 
-	nft_xt_builtin_init(h, table);
-
 	/* Config load changed errno. Ensure genuine info for our callers. */
 	errno = 0;
 
-- 
2.27.0

[iptables PATCH 05/18] nft: Turn nft_chain_save() into a foreach-callback

[Phil Sutter]

Let nftnl_chain_list_foreach() do the chain list iterating instead of
open-coding it. While being at it, simplify the policy value selection
code as well.

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

diff --git a/iptables/nft.c b/iptables/nft.c
index e3811f5fb20b0..c6cfecda1846a 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -1558,46 +1558,23 @@ static const char *policy_name[NF_ACCEPT+1] = {
 	[NF_ACCEPT] = "ACCEPT",
 };
 
-int nft_chain_save(struct nft_handle *h, struct nftnl_chain_list *list)
+int nft_chain_save(struct nftnl_chain *c, void *data)
 {
-	struct nft_family_ops *ops = h->ops;
-	struct nftnl_chain_list_iter *iter;
-	struct nftnl_chain *c;
-
-	iter = nftnl_chain_list_iter_create(list);
-	if (iter == NULL)
-		return 0;
-
-	c = nftnl_chain_list_iter_next(iter);
-	while (c != NULL) {
-		const char *policy = NULL;
-
-		if (nft_chain_builtin(c)) {
-			uint32_t pol = NF_ACCEPT;
-
-			if (nftnl_chain_get(c, NFTNL_CHAIN_POLICY))
-				pol = nftnl_chain_get_u32(c, NFTNL_CHAIN_POLICY);
-			policy = policy_name[pol];
-		} else if (h->family == NFPROTO_BRIDGE) {
-			if (nftnl_chain_is_set(c, NFTNL_CHAIN_POLICY)) {
-				uint32_t pol;
-
-				pol = nftnl_chain_get_u32(c, NFTNL_CHAIN_POLICY);
-				policy = policy_name[pol];
-			} else {
-				policy = "RETURN";
-			}
-		}
-
-		if (ops->save_chain)
-			ops->save_chain(c, policy);
+	struct nft_handle *h = data;
+	const char *policy = NULL;
 
-		c = nftnl_chain_list_iter_next(iter);
+	if (nftnl_chain_is_set(c, NFTNL_CHAIN_POLICY)) {
+		policy = policy_name[nftnl_chain_get_u32(c, NFTNL_CHAIN_POLICY)];
+	} else if (nft_chain_builtin(c)) {
+		policy = "ACCEPT";
+	} else if (h->family == NFPROTO_BRIDGE) {
+		policy = "RETURN";
 	}
 
-	nftnl_chain_list_iter_destroy(iter);
+	if (h->ops->save_chain)
+		h->ops->save_chain(c, policy);
 
-	return 1;
+	return 0;
 }
 
 static int nft_chain_save_rules(struct nft_handle *h,
diff --git a/iptables/nft.h b/iptables/nft.h
index bd944f441caf1..fd390e7f90765 100644
--- a/iptables/nft.h
+++ b/iptables/nft.h
@@ -142,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 nft_handle *h, struct nftnl_chain_list *list);
+int nft_chain_save(struct nftnl_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);
diff --git a/iptables/xtables-save.c b/iptables/xtables-save.c
index bb3d8cd336c38..92b0c911c5f1c 100644
--- a/iptables/xtables-save.c
+++ b/iptables/xtables-save.c
@@ -92,7 +92,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 */
-	nft_chain_save(h, chain_list);
+	nftnl_chain_list_foreach(chain_list, nft_chain_save, h);
 	nft_rule_save(h, tablename, d->format);
 	if (d->commit)
 		printf("COMMIT\n");
-- 
2.27.0

[iptables PATCH 06/18] nft: Use nft_chain_find() in two more places

[Phil Sutter]

This doesn't really increase functions' readability but prepares for
later changes.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index c6cfecda1846a..cc1260dc627d0 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -1678,20 +1678,13 @@ int nft_rule_flush(struct nft_handle *h, const char *chain, const char *table,
 
 	nft_fn = nft_rule_flush;
 
-	if (chain || verbose) {
+	if (chain || verbose)
 		nft_xt_builtin_init(h, table);
-
-		list = nft_chain_list_get(h, table, chain);
-		if (list == NULL) {
-			ret = 1;
-			goto err;
-		}
-	} else if (!nft_table_find(h, table)) {
+	else if (!nft_table_find(h, table))
 		return 1;
-	}
 
 	if (chain) {
-		c = nftnl_chain_list_lookup_byname(list, chain);
+		c = nft_chain_find(h, table, chain);
 		if (!c) {
 			errno = ENOENT;
 			return 0;
@@ -1705,6 +1698,12 @@ 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;
+	}
+
 	iter = nftnl_chain_list_iter_create(list);
 	if (iter == NULL) {
 		ret = 1;
@@ -2437,12 +2436,8 @@ int nft_rule_list(struct nft_handle *h, const char *chain, const char *table,
 	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 (chain) {
-		c = nftnl_chain_list_lookup_byname(list, chain);
+		c = nft_chain_find(h, table, chain);
 		if (!c)
 			return 0;
 
@@ -2455,6 +2450,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;
+
 	iter = nftnl_chain_list_iter_create(list);
 	if (iter == NULL)
 		return 0;
-- 
2.27.0

[iptables PATCH 07/18] nft: Reorder enum nft_table_type

[Phil Sutter]

This list of table types is used internally only, the actual values
don't matter that much. Reorder them to match the order in which
iptables-legacy-save prints them (if present). As a consequence, entries
in builtin_table array 'xtables_ipv4' are correctly sorted as well.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/iptables/nft.h b/iptables/nft.h
index fd390e7f90765..247255ac9e3c5 100644
--- a/iptables/nft.h
+++ b/iptables/nft.h
@@ -8,10 +8,10 @@
 #include <libiptc/linux_list.h>
 
 enum nft_table_type {
-	NFT_TABLE_FILTER	= 0,
-	NFT_TABLE_MANGLE,
-	NFT_TABLE_RAW,
+	NFT_TABLE_MANGLE	= 0,
 	NFT_TABLE_SECURITY,
+	NFT_TABLE_RAW,
+	NFT_TABLE_FILTER,
 	NFT_TABLE_NAT,
 };
 #define NFT_TABLE_MAX	(NFT_TABLE_NAT + 1)
-- 
2.27.0

[iptables PATCH 08/18] nft: cache: Fetch only interesting tables from kernel

[Phil Sutter]

Use the builtin_table array nft_handle->tables points at to gather a
list of table names the calling tool is interested in and fetch only
those instead of requesting a dump of all tables. This increases caching
overhead due to the individual sendmsg() calls but leads to a table list
in defined ordering.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft-cache.c | 40 ++++++++++++++++++++++++++--------------
 1 file changed, 26 insertions(+), 14 deletions(-)

diff --git a/iptables/nft-cache.c b/iptables/nft-cache.c
index 059f0a7f7891e..f8bb2d09c6434 100644
--- a/iptables/nft-cache.c
+++ b/iptables/nft-cache.c
@@ -133,7 +133,8 @@ static int fetch_table_cache(struct nft_handle *h)
 	char buf[16536];
 	struct nlmsghdr *nlh;
 	struct nftnl_table_list *list;
-	int i, ret;
+	struct nftnl_table *t;
+	int i, rc, ret = 1;
 
 	if (h->cache->tables)
 		return 0;
@@ -142,14 +143,9 @@ static int fetch_table_cache(struct nft_handle *h)
 	if (list == NULL)
 		return 0;
 
-	nlh = nftnl_rule_nlmsg_build_hdr(buf, NFT_MSG_GETTABLE, h->family,
-					NLM_F_DUMP, h->seq);
-
-	ret = mnl_talk(h, nlh, nftnl_table_list_cb, list);
-	if (ret < 0 && errno == EINTR)
-		assert(nft_restart(h) >= 0);
-
-	h->cache->tables = list;
+	t = nftnl_table_alloc();
+	if (t == NULL)
+		return 0;
 
 	for (i = 0; i < NFT_TABLE_MAX; i++) {
 		enum nft_table_type type = h->tables[i].type;
@@ -157,16 +153,32 @@ static int fetch_table_cache(struct nft_handle *h)
 		if (!h->tables[i].name)
 			continue;
 
+		nlh = nftnl_rule_nlmsg_build_hdr(buf, NFT_MSG_GETTABLE,
+						 h->family, NLM_F_ACK, h->seq);
+		nftnl_table_set_str(t, NFTNL_TABLE_NAME, h->tables[i].name);
+		nftnl_table_nlmsg_build_payload(nlh, t);
+
+		rc = mnl_talk(h, nlh, nftnl_table_list_cb, list);
+		if (rc < 0 && errno == EINTR)
+			assert(nft_restart(h) >= 0);
+
 		h->cache->table[type].chains = nftnl_chain_list_alloc();
-		if (!h->cache->table[type].chains)
-			return 0;
+		if (!h->cache->table[type].chains) {
+			ret = 0;
+			break;
+		}
 
 		h->cache->table[type].sets = nftnl_set_list_alloc();
-		if (!h->cache->table[type].sets)
-			return 0;
+		if (!h->cache->table[type].sets) {
+			ret = 0;
+			break;
+		}
 	}
 
-	return 1;
+	if (ret == 1)
+		h->cache->tables = list;
+	nftnl_table_free(t);
+	return ret;
 }
 
 struct nftnl_chain_list_cb_data {
-- 
2.27.0

[iptables PATCH 09/18] nft: Use nftnl_chain_list_foreach in nft_rule_list{,_save}

[Phil Sutter]

Introduce a common callback function and data structure to pass via
opaque pointer since chain printing in both functions is pretty similar.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft.c | 89 +++++++++++++++++++++++++++-----------------------
 1 file changed, 49 insertions(+), 40 deletions(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index cc1260dc627d0..66746818f5e0c 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -2424,14 +2424,43 @@ static void __nft_print_header(struct nft_handle *h,
 			&ctrs, basechain, refs - entries, entries);
 }
 
+struct nft_rule_list_cb_data {
+	struct nft_handle *h;
+	unsigned int format;
+	int rulenum;
+	bool found;
+	bool save_fmt;
+	void (*cb)(struct nft_handle *h, struct nftnl_rule *r,
+		   unsigned int num, unsigned int format);
+};
+
+static int nft_rule_list_cb(struct nftnl_chain *c, void *data)
+{
+	struct nft_rule_list_cb_data *d = data;
+
+	if (!d->save_fmt) {
+		if (d->found)
+			printf("\n");
+		d->found = true;
+
+		__nft_print_header(d->h, c, d->format);
+	}
+
+	return __nft_rule_list(d->h, c, d->rulenum, d->format, d->cb);
+}
+
 int nft_rule_list(struct nft_handle *h, const char *chain, const char *table,
 		  int rulenum, unsigned int format)
 {
 	const struct nft_family_ops *ops = h->ops;
+	struct nft_rule_list_cb_data d = {
+		.h = h,
+		.format = format,
+		.rulenum = rulenum,
+		.cb = ops->print_rule,
+	};
 	struct nftnl_chain_list *list;
-	struct nftnl_chain_list_iter *iter;
 	struct nftnl_chain *c;
-	bool found = false;
 
 	nft_xt_builtin_init(h, table);
 	nft_assert_table_compatible(h, table, chain);
@@ -2441,12 +2470,12 @@ int nft_rule_list(struct nft_handle *h, const char *chain, const char *table,
 		if (!c)
 			return 0;
 
-		if (!rulenum) {
-			if (ops->print_table_header)
-				ops->print_table_header(table);
-			__nft_print_header(h, c, format);
-		}
-		__nft_rule_list(h, c, rulenum, format, ops->print_rule);
+		if (rulenum)
+			d.save_fmt = true;	/* skip header printing */
+		else if (ops->print_table_header)
+			ops->print_table_header(table);
+
+		nft_rule_list_cb(c, &d);
 		return 1;
 	}
 
@@ -2454,25 +2483,10 @@ int nft_rule_list(struct nft_handle *h, const char *chain, const char *table,
 	if (!list)
 		return 0;
 
-	iter = nftnl_chain_list_iter_create(list);
-	if (iter == NULL)
-		return 0;
-
 	if (ops->print_table_header)
 		ops->print_table_header(table);
 
-	c = nftnl_chain_list_iter_next(iter);
-	while (c != NULL) {
-		if (found)
-			printf("\n");
-
-		__nft_print_header(h, c, format);
-		__nft_rule_list(h, c, rulenum, format, ops->print_rule);
-
-		found = true;
-		c = nftnl_chain_list_iter_next(iter);
-	}
-	nftnl_chain_list_iter_destroy(iter);
+	nftnl_chain_list_foreach(list, nft_rule_list_cb, &d);
 	return 1;
 }
 
@@ -2527,9 +2541,13 @@ nftnl_rule_list_chain_save(struct nft_handle *h, const char *chain,
 int nft_rule_list_save(struct nft_handle *h, const char *chain,
 		       const char *table, int rulenum, int counters)
 {
+	struct nft_rule_list_cb_data d = {
+		.h = h,
+		.rulenum = rulenum,
+		.save_fmt = true,
+		.cb = list_save,
+	};
 	struct nftnl_chain_list *list;
-	struct nftnl_chain_list_iter *iter;
-	unsigned int format = 0;
 	struct nftnl_chain *c;
 	int ret = 0;
 
@@ -2545,30 +2563,21 @@ int nft_rule_list_save(struct nft_handle *h, const char *chain,
 		nftnl_rule_list_chain_save(h, chain, list, counters);
 
 	if (counters < 0)
-		format = FMT_C_COUNTS;
+		d.format = FMT_C_COUNTS;
 	else if (counters == 0)
-		format = FMT_NOCOUNTS;
+		d.format = FMT_NOCOUNTS;
 
 	if (chain) {
 		c = nftnl_chain_list_lookup_byname(list, chain);
 		if (!c)
 			return 0;
 
-		return __nft_rule_list(h, c, rulenum, format, list_save);
+		return nft_rule_list_cb(c, &d);
 	}
 
 	/* Now dump out rules in this table */
-	iter = nftnl_chain_list_iter_create(list);
-	if (iter == NULL)
-		return 0;
-
-	c = nftnl_chain_list_iter_next(iter);
-	while (c != NULL) {
-		ret = __nft_rule_list(h, c, rulenum, format, list_save);
-		c = nftnl_chain_list_iter_next(iter);
-	}
-	nftnl_chain_list_iter_destroy(iter);
-	return ret;
+	ret = nftnl_chain_list_foreach(list, nft_rule_list_cb, &d);
+	return ret == 0 ? 1 : 0;
 }
 
 int nft_rule_zero_counters(struct nft_handle *h, const char *chain,
-- 
2.27.0

[iptables PATCH 10/18] nft: Use nftnl_chain_list_foreach in nft_rule_flush

[Phil Sutter]

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft.c | 40 +++++++++++++++++++++++-----------------
 1 file changed, 23 insertions(+), 17 deletions(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index 66746818f5e0c..809957c6daeb0 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -1668,10 +1668,31 @@ __nft_rule_flush(struct nft_handle *h, const char *table,
 	obj->implicit = implicit;
 }
 
+struct nft_rule_flush_data {
+	struct nft_handle *h;
+	const char *table;
+	bool verbose;
+};
+
+static int nft_rule_flush_cb(struct nftnl_chain *c, void *data)
+{
+	const char *chain = nftnl_chain_get_str(c, NFTNL_CHAIN_NAME);
+	struct nft_rule_flush_data *d = data;
+
+	batch_chain_flush(d->h, d->table, chain);
+	__nft_rule_flush(d->h, d->table, chain, d->verbose, false);
+	flush_rule_cache(d->h, d->table, c);
+	return 0;
+}
+
 int nft_rule_flush(struct nft_handle *h, const char *chain, const char *table,
 		   bool verbose)
 {
-	struct nftnl_chain_list_iter *iter;
+	struct nft_rule_flush_data d = {
+		.h = h,
+		.table = table,
+		.verbose = verbose,
+	};
 	struct nftnl_chain_list *list;
 	struct nftnl_chain *c = NULL;
 	int ret = 0;
@@ -1704,22 +1725,7 @@ int nft_rule_flush(struct nft_handle *h, const char *chain, const char *table,
 		goto err;
 	}
 
-	iter = nftnl_chain_list_iter_create(list);
-	if (iter == NULL) {
-		ret = 1;
-		goto err;
-	}
-
-	c = nftnl_chain_list_iter_next(iter);
-	while (c != NULL) {
-		chain = nftnl_chain_get_str(c, NFTNL_CHAIN_NAME);
-
-		batch_chain_flush(h, table, chain);
-		__nft_rule_flush(h, table, chain, verbose, false);
-		flush_rule_cache(h, table, c);
-		c = nftnl_chain_list_iter_next(iter);
-	}
-	nftnl_chain_list_iter_destroy(iter);
+	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;
-- 
2.27.0

[iptables PATCH 11/18] nft: Use nftnl_chain_foreach in nft_rule_save

[Phil Sutter]

To do so, turn nft_chain_save_rules() into a suitable callback. It is
not used outside of nft_rule_save anyway.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft.c | 34 ++++++++++++++--------------------
 1 file changed, 14 insertions(+), 20 deletions(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index 809957c6daeb0..51716ff70108d 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -1577,9 +1577,14 @@ int nft_chain_save(struct nftnl_chain *c, void *data)
 	return 0;
 }
 
-static int nft_chain_save_rules(struct nft_handle *h,
-				struct nftnl_chain *c, unsigned int format)
+struct nft_rule_save_data {
+	struct nft_handle *h;
+	unsigned int format;
+};
+
+static int nft_rule_save_cb(struct nftnl_chain *c, void *data)
 {
+	struct nft_rule_save_data *d = data;
 	struct nftnl_rule_iter *iter;
 	struct nftnl_rule *r;
 
@@ -1589,7 +1594,7 @@ static int nft_chain_save_rules(struct nft_handle *h,
 
 	r = nftnl_rule_iter_next(iter);
 	while (r != NULL) {
-		nft_rule_print_save(h, r, NFT_RULE_APPEND, format);
+		nft_rule_print_save(d->h, r, NFT_RULE_APPEND, d->format);
 		r = nftnl_rule_iter_next(iter);
 	}
 
@@ -1599,29 +1604,18 @@ static int nft_chain_save_rules(struct nft_handle *h,
 
 int nft_rule_save(struct nft_handle *h, const char *table, unsigned int format)
 {
-	struct nftnl_chain_list_iter *iter;
+	struct nft_rule_save_data d = {
+		.h = h,
+		.format = format,
+	};
 	struct nftnl_chain_list *list;
-	struct nftnl_chain *c;
-	int ret = 0;
+	int ret;
 
 	list = nft_chain_list_get(h, table, NULL);
 	if (!list)
 		return 0;
 
-	iter = nftnl_chain_list_iter_create(list);
-	if (!iter)
-		return 0;
-
-	c = nftnl_chain_list_iter_next(iter);
-	while (c) {
-		ret = nft_chain_save_rules(h, c, format);
-		if (ret != 0)
-			break;
-
-		c = nftnl_chain_list_iter_next(iter);
-	}
-
-	nftnl_chain_list_iter_destroy(iter);
+	ret = nftnl_chain_list_foreach(list, nft_rule_save_cb, &d);
 
 	/* the core expects 1 for success and 0 for error */
 	return ret == 0 ? 1 : 0;
-- 
2.27.0

[iptables PATCH 12/18] nft: Fold nftnl_rule_list_chain_save() into caller

[Phil Sutter]

Existence of this function was mostly code-duplication: Caller already
branches depending on whether 'chain' is NULL or not and even does the
chain list lookup.

While being at it, simplify __nftnl_rule_list_chain_save function name a
bit now that the non-prefixed name is gone.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft.c | 31 +++++++------------------------
 1 file changed, 7 insertions(+), 24 deletions(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index 51716ff70108d..a5d026e6faa36 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -2497,7 +2497,7 @@ list_save(struct nft_handle *h, struct nftnl_rule *r,
 	nft_rule_print_save(h, r, NFT_RULE_APPEND, format);
 }
 
-static int __nftnl_rule_list_chain_save(struct nftnl_chain *c, void *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);
 	uint32_t policy = nftnl_chain_get_u32(c, NFTNL_CHAIN_POLICY);
@@ -2519,25 +2519,6 @@ static int __nftnl_rule_list_chain_save(struct nftnl_chain *c, void *data)
 	return 0;
 }
 
-static int
-nftnl_rule_list_chain_save(struct nft_handle *h, const char *chain,
-			   struct nftnl_chain_list *list, int counters)
-{
-	struct nftnl_chain *c;
-
-	if (chain) {
-		c = nftnl_chain_list_lookup_byname(list, chain);
-		if (!c)
-			return 0;
-
-		__nftnl_rule_list_chain_save(c, &counters);
-		return 1;
-	}
-
-	nftnl_chain_list_foreach(list, __nftnl_rule_list_chain_save, &counters);
-	return 1;
-}
-
 int nft_rule_list_save(struct nft_handle *h, const char *chain,
 		       const char *table, int rulenum, int counters)
 {
@@ -2558,10 +2539,6 @@ int nft_rule_list_save(struct nft_handle *h, const char *chain,
 	if (!list)
 		return 0;
 
-	/* Dump policies and custom chains first */
-	if (!rulenum)
-		nftnl_rule_list_chain_save(h, chain, list, counters);
-
 	if (counters < 0)
 		d.format = FMT_C_COUNTS;
 	else if (counters == 0)
@@ -2572,9 +2549,15 @@ int nft_rule_list_save(struct nft_handle *h, const char *chain,
 		if (!c)
 			return 0;
 
+		if (!rulenum)
+			nft_rule_list_chain_save(c, &counters);
+
 		return nft_rule_list_cb(c, &d);
 	}
 
+	/* Dump policies and custom chains first */
+	nftnl_chain_list_foreach(list, nft_rule_list_chain_save, &counters);
+
 	/* Now dump out rules in this table */
 	ret = nftnl_chain_list_foreach(list, nft_rule_list_cb, &d);
 	return ret == 0 ? 1 : 0;
-- 
2.27.0

[iptables PATCH 13/18] nft: Implement nft_chain_foreach()

[Phil Sutter]

This is just a fancy wrapper around nftnl_chain_list_foreach() for now.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft-cache.c    | 16 +++-------
 iptables/nft.c          | 69 ++++++++++++++++-------------------------
 iptables/nft.h          |  3 ++
 iptables/xtables-save.c |  7 +----
 4 files changed, 36 insertions(+), 59 deletions(-)

diff --git a/iptables/nft-cache.c b/iptables/nft-cache.c
index f8bb2d09c6434..b897dffb696c1 100644
--- a/iptables/nft-cache.c
+++ b/iptables/nft-cache.c
@@ -471,21 +471,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;
@@ -568,8 +563,7 @@ int flush_rule_cache(struct nft_handle *h, const char *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);
+	return nft_chain_foreach(h, table, __flush_rule_cache, NULL);
 }
 
 static int __flush_chain_cache(struct nftnl_chain *c, void *data)
diff --git a/iptables/nft.c b/iptables/nft.c
index a5d026e6faa36..b2fa3abee6d4a 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -1608,14 +1608,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;
@@ -1687,7 +1682,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;
 
@@ -1713,14 +1707,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;
 }
@@ -1843,18 +1831,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;
@@ -1866,7 +1849,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;
@@ -2459,7 +2442,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);
@@ -2479,14 +2461,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;
 }
 
@@ -2497,6 +2475,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);
@@ -2528,24 +2523,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;
 
@@ -2556,10 +2546,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;
 }
 
@@ -3421,7 +3411,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,
@@ -3429,12 +3418,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;
@@ -3444,7 +3429,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;
diff --git a/iptables/nft.h b/iptables/nft.h
index 247255ac9e3c5..2fe58e7f06d3f 100644
--- a/iptables/nft.h
+++ b/iptables/nft.h
@@ -152,6 +152,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.27.0

[iptables PATCH 14/18] 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>
---
 iptables/nft-cache.c | 12 +++++++++---
 iptables/nft-cache.h |  3 +++
 iptables/nft.c       | 14 ++++++--------
 3 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/iptables/nft-cache.c b/iptables/nft-cache.c
index b897dffb696c1..26771df63bcc2 100644
--- a/iptables/nft-cache.c
+++ b/iptables/nft-cache.c
@@ -181,6 +181,13 @@ static int fetch_table_cache(struct nft_handle *h)
 	return ret;
 }
 
+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;
@@ -190,7 +197,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;
@@ -212,8 +218,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 f429118041be4..d47f7ab6095d9 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 b2fa3abee6d4a..be1275f3357a2 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -1715,7 +1715,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;
 
@@ -1739,9 +1739,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;
@@ -1749,7 +1748,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;
@@ -1781,9 +1780,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.27.0

[iptables PATCH 15/18] 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>
---
 iptables/nft-cache.c | 27 ++++++++++++++++++++++++++-
 iptables/nft.c       | 38 ++++++++++++++++++++++++++++----------
 iptables/nft.h       |  1 +
 3 files changed, 55 insertions(+), 11 deletions(-)

diff --git a/iptables/nft-cache.c b/iptables/nft-cache.c
index 26771df63bcc2..5853bdce82f88 100644
--- a/iptables/nft-cache.c
+++ b/iptables/nft-cache.c
@@ -184,6 +184,19 @@ static int fetch_table_cache(struct nft_handle *h)
 int nft_cache_add_chain(struct nft_handle *h, const struct builtin_table *t,
 			struct nftnl_chain *c)
 {
+	if (nftnl_chain_is_set(c, NFTNL_CHAIN_HOOKNUM)) {
+		uint32_t hooknum = nftnl_chain_get_u32(c, NFTNL_CHAIN_HOOKNUM);
+
+		if (hooknum >= NF_INET_NUMHOOKS)
+			return -EINVAL;
+
+		if (h->cache->table[t->type].base_chains[hooknum])
+			return -EEXIST;
+
+		h->cache->table[t->type].base_chains[hooknum] = c;
+		return 0;
+	}
+
 	nftnl_chain_list_add_tail(c, h->cache->table[t->type].chains);
 	return 0;
 }
@@ -592,12 +605,18 @@ static int flush_cache(struct nft_handle *h, struct nft_cache *c,
 		       const char *tablename)
 {
 	const struct builtin_table *table;
-	int i;
+	int i, j;
 
 	if (tablename) {
 		table = nft_table_builtin_find(h, tablename);
 		if (!table)
 			return 0;
+		for (i = 0; i < NF_INET_NUMHOOKS; i++) {
+			if (!c->table[table->type].base_chains[i])
+				continue;
+			nftnl_chain_free(c->table[table->type].base_chains[i]);
+			c->table[table->type].base_chains[i] = NULL;
+		}
 		if (c->table[table->type].chains)
 			nftnl_chain_list_foreach(c->table[table->type].chains,
 						 __flush_chain_cache, NULL);
@@ -611,6 +630,12 @@ static int flush_cache(struct nft_handle *h, struct nft_cache *c,
 		if (h->tables[i].name == NULL)
 			continue;
 
+		for (j = 0; j < NF_INET_NUMHOOKS; j++) {
+			if (!c->table[i].base_chains[j])
+				continue;
+			nftnl_chain_free(c->table[i].base_chains[j]);
+			c->table[i].base_chains[j] = NULL;
+		}
 		if (c->table[i].chains) {
 			nftnl_chain_list_free(c->table[i].chains);
 			c->table[i].chains = NULL;
diff --git a/iptables/nft.c b/iptables/nft.c
index be1275f3357a2..a83856f16596e 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -701,7 +701,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);
+	h->cache->table[table->type].base_chains[chain->hook] = c;
 }
 
 /* find if built-in table already exists */
@@ -745,19 +745,12 @@ nft_chain_builtin_find(const struct builtin_table *t, const char *chain)
 static void nft_chain_builtin_init(struct nft_handle *h,
 				   const struct builtin_table *table)
 {
-	struct nftnl_chain_list *list;
-	struct nftnl_chain *c;
+	struct nftnl_chain **bcp = h->cache->table[table->type].base_chains;
 	int i;
 
 	/* Initialize built-in chains if they don't exist yet */
 	for (i=0; i < NF_INET_NUMHOOKS && table->chains[i].name != NULL; i++) {
-		list = nft_chain_list_get(h, table->name,
-					  table->chains[i].name);
-		if (!list)
-			continue;
-
-		c = nftnl_chain_list_lookup_byname(list, table->chains[i].name);
-		if (c != NULL)
+		if (bcp[table->chains[i].hook])
 			continue;
 
 		nft_chain_builtin_add(h, table, &table->chains[i]);
@@ -1857,6 +1850,19 @@ static struct nftnl_chain *
 nft_chain_find(struct nft_handle *h, const char *table, const char *chain)
 {
 	struct nftnl_chain_list *list;
+	const struct builtin_table *t;
+	int i;
+
+	t = nft_table_builtin_find(h, table);
+	if (!t)
+		return NULL;
+
+	for (i = 0; i < NF_INET_NUMHOOKS && t->chains[i].name; i++) {
+		if (strcmp(chain, t->chains[i].name))
+			continue;
+
+		return h->cache->table[t->type].base_chains[t->chains[i].hook];
+	}
 
 	list = nft_chain_list_get(h, table, chain);
 	if (list == NULL)
@@ -2478,11 +2484,23 @@ int nft_chain_foreach(struct nft_handle *h, const char *table,
 		      void *data)
 {
 	const struct builtin_table *t;
+	struct nftnl_chain *c;
+	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) /* FIXME */
+			continue;
+
+		ret = cb(c, data);
+		if (ret < 0)
+			return ret;
+	}
+
 	if (!h->cache->table[t->type].chains)
 		return -1;
 
diff --git a/iptables/nft.h b/iptables/nft.h
index 2fe58e7f06d3f..23eebe31e7aa0 100644
--- a/iptables/nft.h
+++ b/iptables/nft.h
@@ -40,6 +40,7 @@ enum nft_cache_level {
 struct nft_cache {
 	struct nftnl_table_list		*tables;
 	struct {
+		struct nftnl_chain	*base_chains[NF_INET_NUMHOOKS];
 		struct nftnl_chain_list *chains;
 		struct nftnl_set_list	*sets;
 		bool			initialized;
-- 
2.27.0

[iptables PATCH 16/18] nft: cache: Sort custom chains by name

[Phil Sutter]

Update nft_cache_add_chain() to make use of libnftnl's new
nftnl_chain_list_add_sorted() function and sort custom chains by name.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft-cache.c                                     | 9 ++++++++-
 .../testcases/ebtables/0002-ebtables-save-restore_0      | 2 +-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/iptables/nft-cache.c b/iptables/nft-cache.c
index 5853bdce82f88..7949bc57b0e1b 100644
--- a/iptables/nft-cache.c
+++ b/iptables/nft-cache.c
@@ -181,6 +181,12 @@ static int fetch_table_cache(struct nft_handle *h)
 	return ret;
 }
 
+static int nftnl_chain_name_cmp(struct nftnl_chain *a, struct nftnl_chain *b)
+{
+	return strcmp(nftnl_chain_get_str(a, NFTNL_CHAIN_NAME),
+		      nftnl_chain_get_str(b, NFTNL_CHAIN_NAME));
+}
+
 int nft_cache_add_chain(struct nft_handle *h, const struct builtin_table *t,
 			struct nftnl_chain *c)
 {
@@ -197,7 +203,8 @@ int nft_cache_add_chain(struct nft_handle *h, const struct builtin_table *t,
 		return 0;
 	}
 
-	nftnl_chain_list_add_tail(c, h->cache->table[t->type].chains);
+	nftnl_chain_list_add_sorted(c, h->cache->table[t->type].chains,
+				    nftnl_chain_name_cmp);
 	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.27.0

[iptables PATCH 17/18] 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.27.0

[iptables PATCH 18/18] 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>
---
 iptables/nft.c          | 90 ++++++++++++++++++++++++++++++++++-------
 iptables/nft.h          |  1 +
 iptables/xtables-save.c |  1 +
 3 files changed, 77 insertions(+), 15 deletions(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index a83856f16596e..6a84bf8ebb3ff 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -757,7 +757,8 @@ static void nft_chain_builtin_init(struct nft_handle *h,
 	}
 }
 
-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;
 
@@ -765,25 +766,84 @@ 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_initialized(h, t->type))
-		return 0;
+		return t;
 
 	if (nft_table_builtin_add(h, t) < 0)
+		return NULL;
+
+	h->cache->table[t->type].initialized = true;
+	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;
 
-	h->cache->table[t->type].initialized = true;
+	if (h->cache->table[t->type].base_chains[c->hook])
+		return 0;
 
+	nft_chain_builtin_add(h, t, c);
 	return 0;
 }
 
+static int __nft_xt_builtin_chain_fake(struct nft_handle *h,
+				       const char *table, void *data)
+{
+	const struct builtin_table *t;
+	struct nftnl_chain **bcp, *c;
+	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;
+
+		c = nft_chain_builtin_alloc(t, &t->chains[i], NF_ACCEPT);
+		if (!c)
+			return -1;
+
+		bcp[t->chains[i].hook] = c;
+	}
+	return 0;
+}
+
+int nft_xt_fake_builtin_chains(struct nft_handle *h, const char *table)
+{
+	if (!table)
+		return nft_for_each_table(h, __nft_xt_builtin_chain_fake, NULL);
+
+	return __nft_xt_builtin_chain_fake(h, table, NULL);
+}
+
 static bool nft_chain_builtin(struct nftnl_chain *c)
 {
 	/* Check if this chain has hook number, in that case is built-in.
@@ -884,7 +944,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) {
@@ -1402,7 +1462,7 @@ nft_rule_append(struct nft_handle *h, const char *chain, const char *table,
 	struct nftnl_chain *c;
 	int type;
 
-	nft_xt_builtin_init(h, table);
+	nft_xt_builtin_init(h, table, chain);
 
 	nft_fn = nft_rule_append;
 
@@ -1681,7 +1741,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;
 
@@ -1714,7 +1774,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;
@@ -1746,7 +1806,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);
 
 	c = nft_chain_find(h, table, chain);
 	if (c) {
@@ -2240,7 +2300,7 @@ int nft_rule_insert(struct nft_handle *h, const char *chain,
 	struct nftnl_rule *r = NULL;
 	struct nftnl_chain *c;
 
-	nft_xt_builtin_init(h, table);
+	nft_xt_builtin_init(h, table, chain);
 
 	nft_fn = nft_rule_insert;
 
@@ -2448,7 +2508,7 @@ int nft_rule_list(struct nft_handle *h, const char *chain, const char *table,
 	};
 	struct nftnl_chain *c;
 
-	nft_xt_builtin_init(h, table);
+	nft_xt_fake_builtin_chains(h, table);
 	nft_assert_table_compatible(h, table, chain);
 
 	if (chain) {
@@ -2542,7 +2602,7 @@ int nft_rule_list_save(struct nft_handle *h, const char *chain,
 	struct nftnl_chain *c;
 	int ret = 0;
 
-	nft_xt_builtin_init(h, table);
+	nft_xt_fake_builtin_chains(h, table);
 	nft_assert_table_compatible(h, table, chain);
 
 	if (counters < 0)
@@ -3146,7 +3206,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 23eebe31e7aa0..7df640338c121 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);
 
 /*
  * Operations with chains.
diff --git a/iptables/xtables-save.c b/iptables/xtables-save.c
index bf00b0324cc4f..d91aa9c354d26 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);
 
 	ret = do_output(&h, tablename, &d);
 	nft_fini(&h);
-- 
2.27.0

Re: [iptables PATCH 00/18] nft: Sorted chain listing et al.

[Pablo Neira Ayuso]

On Sat, Jul 11, 2020 at 12:18:13PM +0200, Phil Sutter wrote:
> Work in this series centered around Harald's complaint about seemingly
> random custom chain ordering in iptables-nft-save output. nftables
> returns chains in the order they were created which differs from
> legacy iptables which sorts by name.
> 
> The intuitive approach of simply sorting chains in tables'
> nftnl_chain_lists is problematic since base chains, which shall be
> dumped first, are contained in there as well. Patch 15 solves this by
> introducing a per-table array of nftnl_chain pointers to hold only base
> chains (the hook values determine the array index). The old
> nftnl_chain_list now contains merely non-base chains and is sorted upon
> population by the new nftnl_chain_list_add_sorted() function.
> 
> Having dedicated slots for base chains allows for another neat trick,
> namely to create only immediately required base chains. Apart from the
> obvious case, where adding a rule to OUTPUT chain doesn't cause creation
> of INPUT or FORWARD chains, this means ruleset modifications can be
> avoided completely when listing, flushing or zeroing counters (unless
> chains exist).

Patches from 1 to 7, they look good to me. Would it be possible to
apply these patches independently from this batch or they are a strong
dependency?

I think it's better if we go slightly different direction?

https://patchwork.ozlabs.org/project/netfilter-devel/patch/20200723121553.7400-1-pablo@netfilter.org/

Instead of adding more functions into libnftnl for specific list
handling, which are not used by nft, use linux list native handling.

I think there is not need to cache the full nftnl_table object,
probably it should be even possible to just use it to collect the
attributes from the kernel to populate the nft_table object that I'm
proposing.

IIRC embedded people complained on the size of libnftnl, going this
direction I suggest, we can probably deprecated iterators for a number
of objects and get it slimmer in the midrun.

WDYT?

Re: [iptables PATCH 00/18] nft: Sorted chain listing et al.

[Phil Sutter]

Hi Pablo,

On Thu, Jul 23, 2020 at 02:22:57PM +0200, Pablo Neira Ayuso wrote:
> On Sat, Jul 11, 2020 at 12:18:13PM +0200, Phil Sutter wrote:
> > Work in this series centered around Harald's complaint about seemingly
> > random custom chain ordering in iptables-nft-save output. nftables
> > returns chains in the order they were created which differs from
> > legacy iptables which sorts by name.
> > 
> > The intuitive approach of simply sorting chains in tables'
> > nftnl_chain_lists is problematic since base chains, which shall be
> > dumped first, are contained in there as well. Patch 15 solves this by
> > introducing a per-table array of nftnl_chain pointers to hold only base
> > chains (the hook values determine the array index). The old
> > nftnl_chain_list now contains merely non-base chains and is sorted upon
> > population by the new nftnl_chain_list_add_sorted() function.
> > 
> > Having dedicated slots for base chains allows for another neat trick,
> > namely to create only immediately required base chains. Apart from the
> > obvious case, where adding a rule to OUTPUT chain doesn't cause creation
> > of INPUT or FORWARD chains, this means ruleset modifications can be
> > avoided completely when listing, flushing or zeroing counters (unless
> > chains exist).
> 
> Patches from 1 to 7, they look good to me. Would it be possible to
> apply these patches independently from this batch or they are a strong
> dependency?

I just pushed them after making sure they don't break any of the
testsuites. Fingers crossed I didn't miss a detail which breaks without
the other patches. :)

> I think it's better if we go slightly different direction?
> 
> https://patchwork.ozlabs.org/project/netfilter-devel/patch/20200723121553.7400-1-pablo@netfilter.org/

That's interesting. At least it would allow us to reorganize the
cache-related data structures, e.g. move the nft_cache->table array
items into nft_cache->table items.

> Instead of adding more functions into libnftnl for specific list
> handling, which are not used by nft, use linux list native handling.

OK.

> I think there is not need to cache the full nftnl_table object,
> probably it should be even possible to just use it to collect the
> attributes from the kernel to populate the nft_table object that I'm
> proposing.

Yes, for iptables-nft at least we should be completely fine with table
name alone.

> IIRC embedded people complained on the size of libnftnl, going this
> direction I suggest, we can probably deprecated iterators for a number
> of objects and get it slimmer in the midrun.

OK. I'll keep that in mind.

So I'll rework my changes based on your nft_table idea and introduce an
nft_chain struct to be organized in a standard list_head list. This will
allow me to perform the sorting in iptables-nft itself. Should I base
this onto your nft_table patch (and exploit it a bit further) or keep
them separate for now?

Thanks, Phil

Re: [iptables PATCH 00/18] nft: Sorted chain listing et al.

[Pablo Neira Ayuso]

Hi Phil,

On Sat, Jul 25, 2020 at 01:55:41PM +0200, Phil Sutter wrote:
> Hi Pablo,
> 
> On Thu, Jul 23, 2020 at 02:22:57PM +0200, Pablo Neira Ayuso wrote:
> > On Sat, Jul 11, 2020 at 12:18:13PM +0200, Phil Sutter wrote:
> > > Work in this series centered around Harald's complaint about seemingly
> > > random custom chain ordering in iptables-nft-save output. nftables
> > > returns chains in the order they were created which differs from
> > > legacy iptables which sorts by name.
> > > 
> > > The intuitive approach of simply sorting chains in tables'
> > > nftnl_chain_lists is problematic since base chains, which shall be
> > > dumped first, are contained in there as well. Patch 15 solves this by
> > > introducing a per-table array of nftnl_chain pointers to hold only base
> > > chains (the hook values determine the array index). The old
> > > nftnl_chain_list now contains merely non-base chains and is sorted upon
> > > population by the new nftnl_chain_list_add_sorted() function.
> > > 
> > > Having dedicated slots for base chains allows for another neat trick,
> > > namely to create only immediately required base chains. Apart from the
> > > obvious case, where adding a rule to OUTPUT chain doesn't cause creation
> > > of INPUT or FORWARD chains, this means ruleset modifications can be
> > > avoided completely when listing, flushing or zeroing counters (unless
> > > chains exist).
> > 
> > Patches from 1 to 7, they look good to me. Would it be possible to
> > apply these patches independently from this batch or they are a strong
> > dependency?
> 
> I just pushed them after making sure they don't break any of the
> testsuites. Fingers crossed I didn't miss a detail which breaks without
> the other patches. :)

Good.

> > I think it's better if we go slightly different direction?
> > 
> > https://patchwork.ozlabs.org/project/netfilter-devel/patch/20200723121553.7400-1-pablo@netfilter.org/
> 
> That's interesting. At least it would allow us to reorganize the
> cache-related data structures, e.g. move the nft_cache->table array
> items into nft_cache->table items.
> 
> > Instead of adding more functions into libnftnl for specific list
> > handling, which are not used by nft, use linux list native handling.
> 
> OK.
> 
> > I think there is not need to cache the full nftnl_table object,
> > probably it should be even possible to just use it to collect the
> > attributes from the kernel to populate the nft_table object that I'm
> > proposing.
> 
> Yes, for iptables-nft at least we should be completely fine with table
> name alone.

Good.

> > IIRC embedded people complained on the size of libnftnl, going this
> > direction I suggest, we can probably deprecated iterators for a number
> > of objects and get it slimmer in the midrun.
> 
> OK. I'll keep that in mind.
> 
> So I'll rework my changes based on your nft_table idea and introduce an
> nft_chain struct to be organized in a standard list_head list. This will
> allow me to perform the sorting in iptables-nft itself.

Good.

> Should I base this onto your nft_table patch (and exploit it a bit
> further) or keep them separate for now?

I'll push it out so you can rebase on top, OK?

Thanks.

Re: [iptables PATCH 00/18] nft: Sorted chain listing et al.

[Phil Sutter]

Hi Pablo,

On Mon, Jul 27, 2020 at 12:20:16PM +0200, Pablo Neira Ayuso wrote:
> On Sat, Jul 25, 2020 at 01:55:41PM +0200, Phil Sutter wrote:
> > Hi Pablo,
> > 
> > On Thu, Jul 23, 2020 at 02:22:57PM +0200, Pablo Neira Ayuso wrote:
> > > On Sat, Jul 11, 2020 at 12:18:13PM +0200, Phil Sutter wrote:
> > > > Work in this series centered around Harald's complaint about seemingly
> > > > random custom chain ordering in iptables-nft-save output. nftables
> > > > returns chains in the order they were created which differs from
> > > > legacy iptables which sorts by name.
> > > > 
> > > > The intuitive approach of simply sorting chains in tables'
> > > > nftnl_chain_lists is problematic since base chains, which shall be
> > > > dumped first, are contained in there as well. Patch 15 solves this by
> > > > introducing a per-table array of nftnl_chain pointers to hold only base
> > > > chains (the hook values determine the array index). The old
> > > > nftnl_chain_list now contains merely non-base chains and is sorted upon
> > > > population by the new nftnl_chain_list_add_sorted() function.
> > > > 
> > > > Having dedicated slots for base chains allows for another neat trick,
> > > > namely to create only immediately required base chains. Apart from the
> > > > obvious case, where adding a rule to OUTPUT chain doesn't cause creation
> > > > of INPUT or FORWARD chains, this means ruleset modifications can be
> > > > avoided completely when listing, flushing or zeroing counters (unless
> > > > chains exist).
> > > 
> > > Patches from 1 to 7, they look good to me. Would it be possible to
> > > apply these patches independently from this batch or they are a strong
> > > dependency?
> > 
> > I just pushed them after making sure they don't break any of the
> > testsuites. Fingers crossed I didn't miss a detail which breaks without
> > the other patches. :)
> 
> Good.
> 
> > > I think it's better if we go slightly different direction?
> > > 
> > > https://patchwork.ozlabs.org/project/netfilter-devel/patch/20200723121553.7400-1-pablo@netfilter.org/
> > 
> > That's interesting. At least it would allow us to reorganize the
> > cache-related data structures, e.g. move the nft_cache->table array
> > items into nft_cache->table items.
> > 
> > > Instead of adding more functions into libnftnl for specific list
> > > handling, which are not used by nft, use linux list native handling.
> > 
> > OK.
> > 
> > > I think there is not need to cache the full nftnl_table object,
> > > probably it should be even possible to just use it to collect the
> > > attributes from the kernel to populate the nft_table object that I'm
> > > proposing.
> > 
> > Yes, for iptables-nft at least we should be completely fine with table
> > name alone.
> 
> Good.
> 
> > > IIRC embedded people complained on the size of libnftnl, going this
> > > direction I suggest, we can probably deprecated iterators for a number
> > > of objects and get it slimmer in the midrun.
> > 
> > OK. I'll keep that in mind.
> > 
> > So I'll rework my changes based on your nft_table idea and introduce an
> > nft_chain struct to be organized in a standard list_head list. This will
> > allow me to perform the sorting in iptables-nft itself.
> 
> Good.
> 
> > Should I base this onto your nft_table patch (and exploit it a bit
> > further) or keep them separate for now?
> 
> I'll push it out so you can rebase on top, OK?

Yes, that's fine with me!

Thanks, Phil