[xtables-arptables PATCH v2 0/5] nft changes for xtables-arptables

[xtables-arptables PATCH v2 0/5] nft changes for xtables-arptables

[Giuseppe Longo]

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(-)

-- 

[xtables-arptables PATCH v2 1/5] nft: add builtin_table pointer

[Giuseppe Longo]

Signed-off-by: Giuseppe Longo <giuseppelng@gmail.com>
---
 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(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index 4d6a7a3..f6dccff 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -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;
-	const char *type;
-	uint32_t prio;
-	uint32_t hook;
-};
-
-static struct builtin_table {
-	const char *name;
-	struct builtin_chain chains[NF_INET_NUMHOOKS];
-} tables[TABLES_MAX] = {
+struct builtin_table tables[TABLES_MAX] = {
 	[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;
 }
diff --git a/iptables/nft.h b/iptables/nft.h
index 7a6351b..e4d177e 100644
--- a/iptables/nft.h
+++ b/iptables/nft.h
@@ -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);
 
 /*
diff --git a/iptables/xtables-config.c b/iptables/xtables-config.c
index 515b18b..bb87886 100644
--- a/iptables/xtables-config.c
+++ b/iptables/xtables-config.c
@@ -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;
diff --git a/iptables/xtables-restore.c b/iptables/xtables-restore.c
index 8469ba1..b894173 100644
--- a/iptables/xtables-restore.c
+++ b/iptables/xtables-restore.c
@@ -24,6 +24,8 @@
 #define DEBUGP(x, args...)
 #endif
 
+extern struct builtin_table tables[TABLES_MAX];
+
 static int binary = 0, counters = 0, verbose = 0, noflush = 0;
 
 /* Keeping track of external matches and targets.  */
@@ -177,7 +179,6 @@ xtables_restore_main(int argc, char *argv[])
 	const struct xtc_ops *ops = &iptc_ops;
 	struct nft_chain_list *chain_list;
 	struct nft_chain *chain_obj;
-
 	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,
diff --git a/iptables/xtables-save.c b/iptables/xtables-save.c
index 41ceaf5..8a5c991 100644
--- a/iptables/xtables-save.c
+++ b/iptables/xtables-save.c
@@ -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[] = {
@@ -82,7 +84,6 @@ xtables_save_main(int argc, char *argv[])
 		.family	= AF_INET,	/* default to AF_INET */
 	};
 	int c;
-
 	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,
diff --git a/iptables/xtables-standalone.c b/iptables/xtables-standalone.c
index 3f8b981..bd95ff8 100644
--- a/iptables/xtables-standalone.c
+++ b/iptables/xtables-standalone.c
@@ -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,
diff --git a/iptables/xtables.c b/iptables/xtables.c
index c314b37..65e4882 100644
--- a/iptables/xtables.c
+++ b/iptables/xtables.c
@@ -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' };

Re: [xtables-arptables PATCH v2 1/5] nft: add builtin_table pointer

[Pablo Neira Ayuso]

Hi Giuseppe,

Several comments below.

On Tue, Jul 23, 2013 at 06:12:11PM +0200, Giuseppe Longo wrote:
>

missing patch description, a couple of lines would be just fine: You
have to explain why you need this.

Others may be trying to follow nftables development, and this empty
description does not help.

>
> Signed-off-by: Giuseppe Longo <giuseppelng@gmail.com>
> ---
>  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(-)
> 
> diff --git a/iptables/nft.c b/iptables/nft.c
> index 4d6a7a3..f6dccff 100644
> --- a/iptables/nft.c
> +++ b/iptables/nft.c
> @@ -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;
> -	const char *type;
> -	uint32_t prio;
> -	uint32_t hook;
> -};
> -
> -static struct builtin_table {
> -	const char *name;
> -	struct builtin_chain chains[NF_INET_NUMHOOKS];
> -} tables[TABLES_MAX] = {
> +struct builtin_table tables[TABLES_MAX] = {

please, rename 'tables' to 'xtables_ipv4' in this patch.

>  	[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;
>  }
> diff --git a/iptables/nft.h b/iptables/nft.h
> index 7a6351b..e4d177e 100644
> --- a/iptables/nft.h
> +++ b/iptables/nft.h
> @@ -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);
>  
>  /*
> diff --git a/iptables/xtables-config.c b/iptables/xtables-config.c
> index 515b18b..bb87886 100644
> --- a/iptables/xtables-config.c
> +++ b/iptables/xtables-config.c
> @@ -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;
> diff --git a/iptables/xtables-restore.c b/iptables/xtables-restore.c
> index 8469ba1..b894173 100644
> --- a/iptables/xtables-restore.c
> +++ b/iptables/xtables-restore.c
> @@ -24,6 +24,8 @@
>  #define DEBUGP(x, args...)
>  #endif
>  
> +extern struct builtin_table tables[TABLES_MAX];

You can move this definition to nft.h, as it is again required a bit
below.

> +
>  static int binary = 0, counters = 0, verbose = 0, noflush = 0;
>  
>  /* Keeping track of external matches and targets.  */
> @@ -177,7 +179,6 @@ xtables_restore_main(int argc, char *argv[])
>  	const struct xtc_ops *ops = &iptc_ops;
>  	struct nft_chain_list *chain_list;
>  	struct nft_chain *chain_obj;
> -

I still want that empty line there to separated variable declaration
and function body, please get it back.

>  	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,
> diff --git a/iptables/xtables-save.c b/iptables/xtables-save.c
> index 41ceaf5..8a5c991 100644
> --- a/iptables/xtables-save.c
> +++ b/iptables/xtables-save.c
> @@ -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[] = {
> @@ -82,7 +84,6 @@ xtables_save_main(int argc, char *argv[])
>  		.family	= AF_INET,	/* default to AF_INET */
>  	};
>  	int c;
> -

Same thing here.

>  	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,
> diff --git a/iptables/xtables-standalone.c b/iptables/xtables-standalone.c
> index 3f8b981..bd95ff8 100644
> --- a/iptables/xtables-standalone.c
> +++ b/iptables/xtables-standalone.c
> @@ -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,
> diff --git a/iptables/xtables.c b/iptables/xtables.c
> index c314b37..65e4882 100644
> --- a/iptables/xtables.c
> +++ b/iptables/xtables.c
> @@ -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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[xtables-arptables PATCH v2 2/5] nft: search builtin tables via nft_handle tables pointer

[Giuseppe Longo]

Signed-off-by: Giuseppe Longo <giuseppelng@gmail.com>
---
 iptables/nft.c |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index f6dccff..07ca0f1 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -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);

Re: [xtables-arptables PATCH v2 2/5] nft: search builtin tables via nft_handle tables pointer

[Pablo Neira Ayuso]

On Tue, Jul 23, 2013 at 06:12:29PM +0200, Giuseppe Longo wrote:
>

Again missing description.

>
> Signed-off-by: Giuseppe Longo <giuseppelng@gmail.com>
> ---
>  iptables/nft.c |   13 +++++++------
>  1 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/iptables/nft.c b/iptables/nft.c
> index f6dccff..07ca0f1 100644
> --- a/iptables/nft.c
> +++ b/iptables/nft.c
> @@ -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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[xtables-arptables PATCH v2 3/5] nft: nft_xtables_config_load() called only in nft_init()

[Giuseppe Longo]

Signed-off-by: Giuseppe Longo <giuseppelng@gmail.com>
---
 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(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index 07ca0f1..589cba7 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -373,7 +373,8 @@ 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, struct builtin_table *t)
+int nft_init(struct nft_handle *h, struct builtin_table *t,
+	     const char *filename)
 {
 	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 */
+	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);
+		}
+	}
 	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);
diff --git a/iptables/nft.h b/iptables/nft.h
index e4d177e..abf0463 100644
--- a/iptables/nft.h
+++ b/iptables/nft.h
@@ -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);
 
 /*
diff --git a/iptables/xtables-config.c b/iptables/xtables-config.c
index bb87886..277e33e 100644
--- a/iptables/xtables-config.c
+++ b/iptables/xtables-config.c
@@ -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;
 }
diff --git a/iptables/xtables-restore.c b/iptables/xtables-restore.c
index b894173..3893734 100644
--- a/iptables/xtables-restore.c
+++ b/iptables/xtables-restore.c
@@ -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) {
diff --git a/iptables/xtables-save.c b/iptables/xtables-save.c
index 8a5c991..897e805 100644
--- a/iptables/xtables-save.c
+++ b/iptables/xtables-save.c
@@ -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);
diff --git a/iptables/xtables-standalone.c b/iptables/xtables-standalone.c
index bd95ff8..212b293 100644
--- a/iptables/xtables-standalone.c
+++ b/iptables/xtables-standalone.c
@@ -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) {
diff --git a/iptables/xtables.c b/iptables/xtables.c
index 65e4882..d4b8709 100644
--- a/iptables/xtables.c
+++ b/iptables/xtables.c
@@ -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 &&

Re: [xtables-arptables PATCH v2 3/5] nft: nft_xtables_config_load() called only in nft_init()

[Pablo Neira Ayuso]

On Tue, Jul 23, 2013 at 06:12:47PM +0200, Giuseppe Longo wrote:
> Signed-off-by: Giuseppe Longo <giuseppelng@gmail.com>
> ---
>  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(-)
> 
> diff --git a/iptables/nft.c b/iptables/nft.c
> index 07ca0f1..589cba7 100644
> --- a/iptables/nft.c
> +++ b/iptables/nft.c
> @@ -373,7 +373,8 @@ 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, struct builtin_table *t)
> +int nft_init(struct nft_handle *h, struct builtin_table *t,
> +	     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.

>  {
>  	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 */
> +	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.

>  	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);
> diff --git a/iptables/nft.h b/iptables/nft.h
> index e4d177e..abf0463 100644
> --- a/iptables/nft.h
> +++ b/iptables/nft.h
> @@ -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);
>  
>  /*
> diff --git a/iptables/xtables-config.c b/iptables/xtables-config.c
> index bb87886..277e33e 100644
> --- a/iptables/xtables-config.c
> +++ b/iptables/xtables-config.c
> @@ -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;
>  }
> diff --git a/iptables/xtables-restore.c b/iptables/xtables-restore.c
> index b894173..3893734 100644
> --- a/iptables/xtables-restore.c
> +++ b/iptables/xtables-restore.c
> @@ -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) {
> diff --git a/iptables/xtables-save.c b/iptables/xtables-save.c
> index 8a5c991..897e805 100644
> --- a/iptables/xtables-save.c
> +++ b/iptables/xtables-save.c
> @@ -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);
> diff --git a/iptables/xtables-standalone.c b/iptables/xtables-standalone.c
> index bd95ff8..212b293 100644
> --- a/iptables/xtables-standalone.c
> +++ b/iptables/xtables-standalone.c
> @@ -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) {
> diff --git a/iptables/xtables.c b/iptables/xtables.c
> index 65e4882..d4b8709 100644
> --- a/iptables/xtables.c
> +++ b/iptables/xtables.c
> @@ -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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [xtables-arptables PATCH v2 3/5] nft: nft_xtables_config_load() called only in nft_init()

[Tomasz Bursztyka]

Hi Pablo,

>> +int nft_init(struct nft_handle *h, struct builtin_table *t,
>> >+	     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.

>
>> >  {
>> >  	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 */
>> >+	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.

Cheers,

Tomasz

Re: [xtables-arptables PATCH v2 3/5] nft: nft_xtables_config_load() called only in nft_init()

[Pablo Neira Ayuso]

Hi,

On Wed, Jul 24, 2013 at 12:36:27PM +0300, Tomasz Bursztyka wrote:
> Hi Pablo,
> 
> >>+int nft_init(struct nft_handle *h, struct builtin_table *t,
> >>>+	     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.

Yes, that's the way to go.

> >>>  {
> >>>  	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 */
> >>>+	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.

OK, but there are other chunks in this patch that are unclear to me:

@@ -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 &&

By looking at patch 1, h->tables always point to some array of tables,
so that should always evaluates true.

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.

Thanks.

Re: [xtables-arptables PATCH v2 3/5] nft: nft_xtables_config_load() called only in nft_init()

[Tomasz Bursztyka]

Hi Pablo,

Indeed h->tables is always not NULL, so this test should be removed.

> 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.

Getting xtables-arptables bootstrapped is an open issue.
And while doing some necessary refactoring (like making some functions 
public), such cleanups are also quite handy.
I don't see the point to spread unclear code in a new place and cleaning 
it up afterwards when it can be done just right now. It's would be more 
time consuming.

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

Cheers,

Tomasz

Re: [xtables-arptables PATCH v2 3/5] nft: nft_xtables_config_load() called only in nft_init()

[Pablo Neira Ayuso]

On Wed, Jul 24, 2013 at 02:14:37PM +0300, Tomasz Bursztyka wrote:
[...]
> Please, consider these patches - after being reworked because they
> obviously need it yes - to be included.

Sure, fix them and get back to me with a new round.

Regards.

Re: [xtables-arptables PATCH v2 3/5] nft: nft_xtables_config_load() called only in nft_init()

[Pablo Neira Ayuso]

On Tue, Jul 23, 2013 at 06:12:47PM +0200, Giuseppe Longo wrote:
[...]
> @@ -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);
> +        }
   ^^^^^^^^

BTW, this is indented with spaces, not with tabs.

It happens several times in the patchset. Please, make sure your
patches are correctly indented.

Thanks.

[xtables-arptables PATCH v2 4/5] nft: make functions public

[Giuseppe Longo]

Signed-off-by: Giuseppe Longo <giuseppelng@gmail.com>
---
 iptables/nft.c |   18 +++++++++---------
 iptables/nft.h |   22 ++++++++++++++++++++++
 2 files changed, 31 insertions(+), 9 deletions(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index 589cba7..682c192 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -53,9 +53,9 @@
 
 static void *nft_fn;
 
-static int mnl_talk(struct nft_handle *h, struct nlmsghdr *nlh,
-		    int (*cb)(const struct nlmsghdr *nlh, void *data),
-		    void *data)
+int mnl_talk(struct nft_handle *h, struct nlmsghdr *nlh,
+	     int (*cb)(const struct nlmsghdr *nlh, void *data),
+	     void *data)
 {
 	int ret;
 	char buf[MNL_SOCKET_BUFFER_SIZE];
@@ -210,7 +210,7 @@ struct builtin_table tables[TABLES_MAX] = {
 	},
 };
 
-static int
+int
 nft_table_builtin_add(struct nft_handle *h, struct builtin_table *_t,
 			bool dormant)
 {
@@ -242,7 +242,7 @@ nft_table_builtin_add(struct nft_handle *h, struct builtin_table *_t,
 	return ret;
 }
 
-static struct nft_chain *
+struct nft_chain *
 nft_chain_builtin_alloc(struct builtin_table *table,
 			struct builtin_chain *chain, int policy)
 {
@@ -262,7 +262,7 @@ nft_chain_builtin_alloc(struct builtin_table *table,
 	return c;
 }
 
-static void
+void
 nft_chain_builtin_add(struct nft_handle *h, struct builtin_table *table,
 		      struct builtin_chain *chain, int policy)
 {
@@ -288,7 +288,7 @@ nft_chain_builtin_add(struct nft_handle *h, struct builtin_table *table,
 }
 
 /* find if built-in table already exists */
-static struct builtin_table *
+struct builtin_table *
 nft_table_builtin_find(struct nft_handle *h, const char *table)
 {
 	int i;
@@ -306,7 +306,7 @@ nft_table_builtin_find(struct nft_handle *h, const char *table)
 }
 
 /* find if built-in chain already exists */
-static struct builtin_chain *
+struct builtin_chain *
 nft_chain_builtin_find(struct builtin_table *t, const char *chain)
 {
 	int i;
@@ -343,7 +343,7 @@ __nft_chain_builtin_init(struct nft_handle *h,
 	}
 }
 
-static int
+int
 nft_chain_builtin_init(struct nft_handle *h, const char *table,
 		       const char *chain, int policy)
 {
diff --git a/iptables/nft.h b/iptables/nft.h
index abf0463..877fadc 100644
--- a/iptables/nft.h
+++ b/iptables/nft.h
@@ -33,6 +33,28 @@ struct nft_handle {
 	struct builtin_table	*tables;
 };
 
+int mnl_talk(struct nft_handle *h, struct nlmsghdr *nlh,
+	     int (*cb)(const struct nlmsghdr *nlh, void *data),
+	     void *data);
+
+int nft_table_builtin_add(struct nft_handle *h, struct builtin_table *_t,
+			  bool dormant);
+
+struct nft_chain *nft_chain_builtin_alloc(struct builtin_table *table,
+					  struct builtin_chain *chain, int policy);
+
+void nft_chain_builtin_add(struct nft_handle *h, struct builtin_table *table,
+			   struct builtin_chain *chain, int policy);
+
+struct builtin_table *nft_table_builtin_find(struct nft_handle *h,
+					     const char *table);
+
+struct builtin_chain *nft_chain_builtin_find(struct builtin_table *t,
+					     const char *chain);
+
+int nft_chain_builtin_init(struct nft_handle *h, const char *table,
+			   const char *chain, int policy);
+
 int nft_init(struct nft_handle *h, struct builtin_table *t, const char *filename);
 void nft_fini(struct nft_handle *h);
 

[xtables-arptables PATCH v2 5/5] nft: replace args.family

[Giuseppe Longo]

Signed-off-by: Giuseppe Longo <giuseppelng@gmail.com>
---
 iptables/xtables.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/iptables/xtables.c b/iptables/xtables.c
index d4b8709..d1b04cb 100644
--- a/iptables/xtables.c
+++ b/iptables/xtables.c
@@ -1096,7 +1096,7 @@ int do_commandx(struct nft_handle *h, int argc, char *argv[], char **table)
 	if (h->family == AF_UNSPEC)
 		h->family = args.family;
 
-	h->ops = nft_family_ops_lookup(args.family);
+	h->ops = nft_family_ops_lookup(h->family);
 	if (h->ops == NULL)
 		xtables_error(PARAMETER_PROBLEM, "Unknown family");
 

Re: [xtables-arptables PATCH v2 5/5] nft: replace args.family

[Pablo Neira Ayuso]

On Tue, Jul 23, 2013 at 06:13:45PM +0200, Giuseppe Longo wrote:
> Signed-off-by: Giuseppe Longo <giuseppelng@gmail.com>
> ---
>  iptables/xtables.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/iptables/xtables.c b/iptables/xtables.c
> index d4b8709..d1b04cb 100644
> --- a/iptables/xtables.c
> +++ b/iptables/xtables.c
> @@ -1096,7 +1096,7 @@ int do_commandx(struct nft_handle *h, int argc, char *argv[], char **table)
>  	if (h->family == AF_UNSPEC)
>  		h->family = args.family;
>  
> -	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.

>  	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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [xtables-arptables PATCH v2 5/5] nft: replace args.family

[Tomasz Bursztyka]

Hi Pablo,

>> >-	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.

It's clearly a bug fix here (using args.family forget about if user has 
used -6)  but indeed a one liner description does not harm :)


Tomasz

Re: [xtables-arptables PATCH v2 0/5] nft changes for xtables-arptables

[Tomasz Bursztyka]

Hi,

This version looks fine to me, tested and so on.

Thanks Giuseppe

Tomasz

> 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(-)
>