nft changes for xtables-arptables

The following series implements changes in nft code that permit to reuse some functions in other tool (like xtables-arptables).

I changed nft.h, now struct nft_handle gets a struct builtin_table pointer used in functions to works with a properly tables and not with tables declared in nft.c.

---

Giuseppe Longo (5):
nft: add builtin_table
nft: search builtin tables via nft_handle tables pointer
nft: nft_xtables_config_load() called only in nft_init()
nft: make functions public
nft: replace args.family

  iptables/nft.c                |   82 +++++++++++++++--------------------------
  iptables/nft.h                |   44 ++++++++++++++++++++++
  iptables/xtables-config.c     |    9 +++--
  iptables/xtables-restore.c    |   19 +++++-----
  iptables/xtables-save.c       |   18 +++++----
  iptables/xtables-standalone.c |   16 +++-----
  iptables/xtables.c            |    9 ++++-
  7 files changed, 111 insertions(+), 86 deletions(-)

Signed-off-by Giuseppe Longo <[email protected]>

---
 iptables/nft.c                |   22 +++-------------------
 iptables/nft.h                |   22 +++++++++++++++++++++-
 iptables/xtables-config.c     |    4 +++-
 iptables/xtables-restore.c    |    5 +++--
 iptables/xtables-save.c       |    5 +++--
 iptables/xtables-standalone.c |    4 +++-
 iptables/xtables.c            |    2 ++
 7 files changed, 38 insertions(+), 26 deletions(-)

iptables/nft.c +0 -9

index 4d6a7a3..f6dccff 100644
@@ -80,24 +80,7 @@ static int mnl_talk(struct nft_handle *h, struct nlmsghdr *nlh,
 	return 0;
 }
 
-#define FILTER		0
-#define MANGLE		1
-#define RAW		2
-#define SECURITY	3
-#define NAT		4
-#define TABLES_MAX	5
-
-struct builtin_chain {
-	const char *name;

iptables/nft.c +2 -1

 	[RAW] = {
 		.name	= "raw",
 		.chains = {

@@ -389,7 +372,7 @@ static bool nft_chain_builtin(struct nft_chain *c)
 	return nft_chain_attr_get(c, NFT_CHAIN_ATTR_HOOKNUM) != NULL;
 }
 
-int nft_init(struct nft_handle *h)
+int nft_init(struct nft_handle *h, struct builtin_table *t)
 {
 	h->nl = mnl_socket_open(NETLINK_NETFILTER);
 	if (h->nl == NULL) {

@@ -402,6 +385,7 @@ int nft_init(struct nft_handle *h)
 		return -1;
 	}
 	h->portid = mnl_socket_get_portid(h->nl);
+	h->tables = t;
 
 	return 0;
 }

iptables/nft.h +21 -1

index 7a6351b..e4d177e 100644
@@ -4,6 +4,25 @@
 #include "xshared.h"
 #include "nft-shared.h"
 
+#define FILTER		0
+#define MANGLE		1
+#define RAW		2
+#define SECURITY	3
+#define NAT		4
+#define TABLES_MAX	5
+
+struct builtin_chain {
+	const char *name;
+	const char *type;
+	uint32_t prio;
+	uint32_t hook;
+};
+
+struct builtin_table {
+	const char *name;
+	struct builtin_chain chains[NF_INET_NUMHOOKS];
+};
+
 struct nft_handle {
 	int			family;
 	struct mnl_socket	*nl;

@@ -11,9 +30,10 @@ struct nft_handle {
 	uint32_t		seq;
 	bool			commit;
 	struct nft_family_ops	*ops;
+	struct builtin_table	*tables;
 };
 
-int nft_init(struct nft_handle *h);
+int nft_init(struct nft_handle *h, struct builtin_table *t);
 void nft_fini(struct nft_handle *h);
 
 /*

iptables/xtables-config.c +3 -1

index 515b18b..bb87886 100644
@@ -19,6 +19,8 @@
 #include "xtables-multi.h"
 #include "nft.h"
 
+extern struct builtin_table tables[TABLES_MAX];
+
 int xtables_config_main(int argc, char *argv[])
 {
 	struct nft_handle h = {

@@ -35,7 +37,7 @@ int xtables_config_main(int argc, char *argv[])
 	else
 		filename = argv[1];
 
-	if (nft_init(&h) < 0) {
+	if (nft_init(&h, tables) < 0) {
                 fprintf(stderr, "Failed to initialize nft: %s\n",
 			strerror(errno));
 		return EXIT_FAILURE;

iptables/xtables-restore.c +0 -0

index 8469ba1..b894173 100644

iptables/xtables-restore.c +1 -1

 	line = 0;
 
 	xtables_globals.program_name = "xtables-restore";

@@ -193,7 +194,7 @@ xtables_restore_main(int argc, char *argv[])
 	init_extensions4();
 #endif
 
-	if (nft_init(&h) < 0) {
+	if (nft_init(&h, tables) < 0) {
 		fprintf(stderr, "%s/%s Failed to initialize nft: %s\n",
 				xtables_globals.program_name,
 				xtables_globals.program_version,

iptables/xtables-save.c +2 -0

index 41ceaf5..8a5c991 100644
@@ -25,6 +25,8 @@
 #include <dlfcn.h>
 #endif
 
+extern struct builtin_table tables[TABLES_MAX];
+
 static bool show_counters = false;
 
 static const struct option options[] = {

iptables/xtables-save.c +1 -1

 	xtables_globals.program_name = "xtables-save";
 	/* XXX xtables_init_all does several things we don't want */
 	c = xtables_init_all(&xtables_globals, NFPROTO_IPV4);

@@ -96,7 +97,7 @@ xtables_save_main(int argc, char *argv[])
 	init_extensions();
 	init_extensions4();
 #endif
-	if (nft_init(&h) < 0) {
+	if (nft_init(&h, tables) < 0) {
 		fprintf(stderr, "%s/%s Failed to initialize nft: %s\n",
 				xtables_globals.program_name,
 				xtables_globals.program_version,

iptables/xtables-standalone.c +3 -1

index 3f8b981..bd95ff8 100644
@@ -39,6 +39,8 @@
 #include "xtables-multi.h"
 #include "nft.h"
 
+extern struct builtin_table tables[TABLES_MAX];
+
 int
 xtables_main(int argc, char *argv[])
 {

@@ -61,7 +63,7 @@ xtables_main(int argc, char *argv[])
 	init_extensions4();
 #endif
 
-	if (nft_init(&h) < 0) {
+	if (nft_init(&h, tables) < 0) {
 		fprintf(stderr, "%s/%s Failed to initialize nft: %s\n",
 				xtables_globals.program_name,
 				xtables_globals.program_version,

iptables/xtables.c +2 -1

index c314b37..65e4882 100644
@@ -50,6 +50,8 @@
 #define FALSE 0
 #endif
 
+extern struct builtin_table tables[TABLES_MAX];
+
 #define NUMBER_OF_CMD	16
 static const char cmdflags[] = { 'I', 'D', 'D', 'R', 'A', 'L', 'F', 'Z',
 				 'N', 'X', 'P', 'E', 'S', 'Z', 'C' };
 
--

To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signed-off-by Giuseppe Longo <[email protected]>

---
 iptables/nft.c |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)

iptables/nft.c +7 -7

index f6dccff..07ca0f1 100644
@@ -288,20 +288,21 @@ nft_chain_builtin_add(struct nft_handle *h, struct builtin_table *table,
 }
 
 /* find if built-in table already exists */
-static struct builtin_table *nft_table_builtin_find(const char *table)
+static struct builtin_table *
+nft_table_builtin_find(struct nft_handle *h, const char *table)
 {
 	int i;
 	bool found = false;
 
 	for (i=0; i<TABLES_MAX; i++) {
-		if (strcmp(tables[i].name, table) != 0)
+		if (strcmp(h->tables[i].name, table) != 0)
 			continue;
 
 		found = true;
 		break;
 	}
 
-	return found ? &tables[i] : NULL;
+	return found ? &h->tables[i] : NULL;
 }
 
 /* find if built-in chain already exists */

@@ -349,7 +350,7 @@ nft_chain_builtin_init(struct nft_handle *h, const char *table,
 	int ret = 0;
 	struct builtin_table *t;
 
-	t = nft_table_builtin_find(table);
+	t = nft_table_builtin_find(h, table);
 	if (t == NULL) {
 		ret = -1;
 		goto out;

@@ -424,7 +425,7 @@ int nft_table_set_dormant(struct nft_handle *h, const char *table)
 	int ret = 0, i;
 	struct builtin_table *t;
 
-	t = nft_table_builtin_find(table);
+	t = nft_table_builtin_find(h, table);
 	if (t == NULL) {
 		ret = -1;
 		goto out;

@@ -485,7 +486,7 @@ __nft_chain_set(struct nft_handle *h, const char *table,
 	struct builtin_chain *_c;
 	int ret;
 
-	_t = nft_table_builtin_find(table);
+	_t = nft_table_builtin_find(h, table);
 	/* if this built-in table does not exists, create it */
 	if (_t != NULL)
 		nft_table_builtin_add(h, _t, false);
 
--

To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signed-off-by Giuseppe Longo <[email protected]>

---
 iptables/nft.c                |   33 ++++++++++++---------------------
 iptables/nft.h                |    2 +-
 iptables/xtables-config.c     |    5 ++---
 iptables/xtables-restore.c    |   16 ++++++++--------
 iptables/xtables-save.c       |   15 ++++++++-------
 iptables/xtables-standalone.c |   14 +++-----------
 iptables/xtables.c            |    5 +++++
 7 files changed, 39 insertions(+), 51 deletions(-)

iptables/nft.c +0 -0

index 07ca0f1..589cba7 100644

iptables/nft.c +0 -0

 {
 	h->nl = mnl_socket_open(NETLINK_NETFILTER);
 	if (h->nl == NULL) {

@@ -388,6 +389,16 @@ int nft_init(struct nft_handle *h, struct builtin_table *t)
 	h->portid = mnl_socket_get_portid(h->nl);
 	h->tables = t;
 

iptables/nft.c +0 -20

 	return 0;
 }
 

@@ -742,10 +753,6 @@ nft_rule_append(struct nft_handle *h, const char *chain, const char *table,
 	uint16_t flags = NLM_F_ACK|NLM_F_CREATE;
 	int ret = 1;
 
-	/* If built-in chains don't exist for this table, create them */
-	if (nft_xtables_config_load(h, XTABLES_CONFIG_DEFAULT, 0) < 0)
-		nft_chain_builtin_init(h, table, chain, NF_ACCEPT);
-
 	nft_fn = nft_rule_append;
 
 	r = nft_rule_new(h, chain, table, cs);

@@ -1316,10 +1323,6 @@ int nft_chain_user_add(struct nft_handle *h, const char *chain, const char *tabl
 	struct nft_chain *c;
 	int ret;
 
-	/* If built-in chains don't exist for this table, create them */
-	if (nft_xtables_config_load(h, XTABLES_CONFIG_DEFAULT, 0) < 0)
-		nft_chain_builtin_init(h, table, NULL, NF_ACCEPT);
-
 	c = nft_chain_alloc();
 	if (c == NULL)
 		return 0;

@@ -1472,10 +1475,6 @@ int nft_chain_user_rename(struct nft_handle *h,const char *chain,
 	uint64_t handle;
 	int ret;
 
-	/* If built-in chains don't exist for this table, create them */
-	if (nft_xtables_config_load(h, XTABLES_CONFIG_DEFAULT, 0) < 0)
-		nft_chain_builtin_init(h, table, NULL, NF_ACCEPT);
-
 	/* Find the old chain to be renamed */
 	c = nft_chain_find(h, table, chain);
 	if (c == NULL) {

@@ -2170,10 +2169,6 @@ int nft_rule_insert(struct nft_handle *h, const char *chain,
 	struct nft_rule *r;
 	uint64_t handle;
 
-	/* If built-in chains don't exist for this table, create them */
-	if (nft_xtables_config_load(h, XTABLES_CONFIG_DEFAULT, 0) < 0)
-		nft_chain_builtin_init(h, table, chain, NF_ACCEPT);
-
 	nft_fn = nft_rule_insert;
 
 	list = nft_rule_list_create(h);

@@ -2521,10 +2516,6 @@ int nft_rule_list(struct nft_handle *h, const char *chain, const char *table,
 	struct nft_chain *c;
 	bool found = false;
 
-	/* If built-in chains don't exist for this table, create them */
-	if (nft_xtables_config_load(h, XTABLES_CONFIG_DEFAULT, 0) < 0)
-		nft_chain_builtin_init(h, table, NULL, NF_ACCEPT);
-
 	list = nft_chain_dump(h);
 
 	iter = nft_chain_list_iter_create(list);

iptables/nft.h +1 -1

index e4d177e..abf0463 100644
@@ -33,7 +33,7 @@ struct nft_handle {
 	struct builtin_table	*tables;
 };
 
-int nft_init(struct nft_handle *h, struct builtin_table *t);
+int nft_init(struct nft_handle *h, struct builtin_table *t, const char *filename);
 void nft_fini(struct nft_handle *h);
 
 /*

iptables/xtables-config.c +2 -3

index bb87886..277e33e 100644
@@ -37,12 +37,11 @@ int xtables_config_main(int argc, char *argv[])
 	else
 		filename = argv[1];
 
-	if (nft_init(&h, tables) < 0) {
+	if (nft_init(&h, tables, filename) < 0) {
                 fprintf(stderr, "Failed to initialize nft: %s\n",
 			strerror(errno));
 		return EXIT_FAILURE;
 	}
 
-	return nft_xtables_config_load(&h, filename, NFT_LOAD_VERBOSE) == 0 ?
-						    EXIT_SUCCESS : EXIT_FAILURE;
+	return EXIT_SUCCESS;
 }

iptables/xtables-restore.c +8 -8

index b894173..3893734 100644
@@ -194,14 +194,6 @@ xtables_restore_main(int argc, char *argv[])
 	init_extensions4();
 #endif
 
-	if (nft_init(&h, tables) < 0) {
-		fprintf(stderr, "%s/%s Failed to initialize nft: %s\n",
-				xtables_globals.program_name,
-				xtables_globals.program_version,
-				strerror(errno));
-		exit(EXIT_FAILURE);
-	}
-
 	while ((c = getopt_long(argc, argv, "bcvthnM:T:46", options, NULL)) != -1) {
 		switch (c) {
 			case 'b':

@@ -239,6 +231,14 @@ xtables_restore_main(int argc, char *argv[])
 		}
 	}
 
+        if (nft_init(&h, tables, XTABLES_CONFIG_DEFAULT) < 0) {
+                fprintf(stderr, "%s/%s Failed to initialize nft: %s\n",
+                                xtables_globals.program_name,
+                                xtables_globals.program_version,
+                                strerror(errno));
+                exit(EXIT_FAILURE);
+        }
+
 	if (optind == argc - 1) {
 		in = fopen(argv[optind], "re");
 		if (!in) {

iptables/xtables-save.c +8 -7

index 8a5c991..897e805 100644
@@ -97,13 +97,6 @@ xtables_save_main(int argc, char *argv[])
 	init_extensions();
 	init_extensions4();
 #endif
-	if (nft_init(&h, tables) < 0) {
-		fprintf(stderr, "%s/%s Failed to initialize nft: %s\n",
-				xtables_globals.program_name,
-				xtables_globals.program_version,
-				strerror(errno));
-		exit(EXIT_FAILURE);
-	}
 
 	while ((c = getopt_long(argc, argv, "bcdt:46", options, NULL)) != -1) {
 		switch (c) {

@@ -131,6 +124,14 @@ xtables_save_main(int argc, char *argv[])
 		}
 	}
 
+        if (nft_init(&h, tables, XTABLES_CONFIG_DEFAULT) < 0) {
+                fprintf(stderr, "%s/%s Failed to initialize nft: %s\n",
+                                xtables_globals.program_name,
+                                xtables_globals.program_version,
+                                strerror(errno));
+                exit(EXIT_FAILURE);
+        }
+
 	if (optind < argc) {
 		fprintf(stderr, "Unknown arguments found on commandline\n");
 		exit(1);

iptables/xtables-standalone.c +3 -11

index bd95ff8..212b293 100644
@@ -46,9 +46,9 @@ xtables_main(int argc, char *argv[])
 {
 	int ret;
 	char *table = "filter";
-	struct nft_handle h;
-
-	memset(&h, 0, sizeof(h));
+	struct nft_handle h = {
+		.family = AF_INET,
+	};
 
 	xtables_globals.program_name = "xtables";
 	ret = xtables_init_all(&xtables_globals, NFPROTO_IPV4);

@@ -63,14 +63,6 @@ xtables_main(int argc, char *argv[])
 	init_extensions4();
 #endif
 
-	if (nft_init(&h, tables) < 0) {
-		fprintf(stderr, "%s/%s Failed to initialize nft: %s\n",
-				xtables_globals.program_name,
-				xtables_globals.program_version,
-				strerror(errno));
-		exit(EXIT_FAILURE);
-	}
-
 	ret = do_commandx(&h, argc, argv, &table);
 	if (!ret) {
 		if (errno == EINVAL) {

iptables/xtables.c +5 -1

index 65e4882..d4b8709 100644
@@ -1100,6 +1100,11 @@ int do_commandx(struct nft_handle *h, int argc, char *argv[], char **table)
 	if (h->ops == NULL)
 		xtables_error(PARAMETER_PROBLEM, "Unknown family");
 
+	if (h->tables == NULL) {
+                if (nft_init(h, tables, XTABLES_CONFIG_DEFAULT) < 0)
+                        xtables_error(OTHER_PROBLEM, "Could not initialize nftables layer.");
+        }
+
 	h->ops->post_parse(command, &cs, &args);
 
 	if (command == CMD_REPLACE &&
 
--

To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signed-off-by Giuseppe Longo <[email protected]>

---
 iptables/xtables.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

iptables/xtables.c +0 -0

index d4b8709..d1b04cb 100644

iptables/xtables.c +0 -1

 	if (h->ops == NULL)
 		xtables_error(PARAMETER_PROBLEM, "Unknown family");
 
 
--

To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 2 lines

+int nft_init(struct nft_handle *h, struct builtin_table *t,

iptables/nft.c static bool nft_chain_builtin(struct nft_chain *c) · line 377

Quoted 1 line

iptables/nft.c +1 -0

+	     const char *filename)

                          ^^^^^^^^

why do we need this new parameter?

The optional /etc/xtables.conf file should contain the definition for all the families, that includes IPv4, IPv6, bridge and ARP.

Quoted 8 lines

Quoted 8 lines

{

iptables/nft.c +0 -0

 	h->nl = mnl_socket_open(NETLINK_NETFILTER);
 	if (h->nl == NULL) {

@@ -388,6 +389,16 @@ int nft_init(struct nft_handle *h, struct builtin_table *t)
 	h->portid = mnl_socket_get_portid(h->nl);
 	h->tables = t;
 

Quoted 10 lines

iptables/nft.c int nft_init(struct nft_handle *h, struct builtin_table *t) · line 401

Quoted 10 lines

iptables/nft.c +10 -0

+	/* If built-in chains don't exist for this table, create them */
+	if (nft_xtables_config_load(h, filename, 0) < 0) {
+		int i;
+
+		if (h->tables != NULL) {
+			for (i=0; i<TABLES_MAX; i++)
+				nft_chain_builtin_init(h, h->tables[i].name,
+						       NULL, NF_ACCEPT);
+		}
+	}

I don't see what we get by moving nft_xtables_config_load here.

Quoted 2 lines

iptables/xtables.c int do_commandx(struct nft_handle *h, int argc, char *argv[], char **table) · line 1099

Quoted 2 lines

iptables/xtables.c +1 -1

-	h->ops = nft_family_ops_lookup(args.family);
+	h->ops = nft_family_ops_lookup(h->family);

Due to lack of patch description, I don't know if you're trying to address a bug or this is just a simple cleanup.

Hi Pablo,

Quoted 8 lines

Quoted 2 lines

+int nft_init(struct nft_handle *h, struct builtin_table *t,

iptables/nft.c static bool nft_chain_builtin(struct nft_chain *c) · line 377

Quoted 1 line

iptables/nft.c +1 -0

+	     const char *filename)

                         ^^^^^^^^

why do we need this new parameter?

The optional /etc/xtables.conf file should contain the definition for all the families, that includes IPv4, IPv6, bridge and ARP.

My mistake, I advised that. Imho, it's not relevant to load everything for all families if the user is using only xtables-iptables for instance.

Then ok, let's have one file, but we could make a change in nft_xtables_config_load() so it would load only the tables of the current family.
It's a quick fix to do actually.

Quoted 8 lines

Quoted 8 lines

iptables/nft.c int nft_init(struct nft_handle *h, struct builtin_table *t) · line 391

Quoted 7 lines

{

iptables/nft.c +0 -0

 	h->nl = mnl_socket_open(NETLINK_NETFILTER);
 	if (h->nl == NULL) {

@@ -388,6 +389,16 @@ int nft_init(struct nft_handle *h, struct builtin_table *t)
 	h->portid = mnl_socket_get_portid(h->nl);
 	h->tables = t;
 >+	/* If built-in chains don't exist for this table, create

them */

Quoted 11 lines

Quoted 9 lines

iptables/nft.c int nft_init(struct nft_handle *h, struct builtin_table *t) · line 401

Quoted 9 lines

iptables/nft.c +9 -0

+	if (nft_xtables_config_load(h, filename, 0) < 0) {
+		int i;
+
+		if (h->tables != NULL) {
+			for (i=0; i<TABLES_MAX; i++)
+				nft_chain_builtin_init(h, h->tables[i].name,
+						       NULL, NF_ACCEPT);
+		}
+	}

I don't see what we get by moving nft_xtables_config_load here.

Why not calling this at only one unique place instead of multiple ones? You need to call that anyway currently as soon as you list/change the rule set.
It's relevant to move that here, makes code clearer.

Please, resolve all open issue and get back to me with a patch for this if you think the time is worth to discuss this small refactorization.

Please, consider these patches - after being reworked because they obviously need it yes - to be included.