[iptables-nftables - PATCH 0/2] fixes

[iptables-nftables - PATCH 0/2] fixes

[Tomasz Bursztyka]

Hi,

First patch is in reply of my previous "[iptables-nftables - PATCH 3/9] nft: Refactor and optimize nft_rule_list"
Since there is no need to refactor anything anymore. And same fixe applies on rule save actually.

Second patch, well I am not found of "style" patches, but I am also annoyed by lines being way too long.
Your call Pablo, take that one or scrap it. 

Tomasz Bursztyka (2):
  nft: Optimize chain listing if only one is looked for
  nft: Fix code style issues

 iptables/nft.c | 94 ++++++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 62 insertions(+), 32 deletions(-)

-- 
1.8.2.1

[iptables-nftables - PATCH 1/2] nft: Optimize chain listing if only one is looked for

[Tomasz Bursztyka]

Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com>
---
 iptables/nft.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/iptables/nft.c b/iptables/nft.c
index f9a88c9..f33faa5 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -2495,6 +2495,9 @@ int nft_rule_list(struct nft_handle *h, const char *chain, const char *table,
 		}
 		__nft_rule_list(h, c, table, rulenum, format, print_firewall);
 
+		if (chain && strcmp(chain, chain_name) == 0)
+			break;
+
 		found = true;
 
 next:
@@ -2593,6 +2596,9 @@ int nft_rule_list_save(struct nft_handle *h, const char *chain,
 
 		ret = __nft_rule_list(h, c, table, rulenum,
 				      counters ? 0 : FMT_NOCOUNTS, list_save);
+
+		if (chain && strcmp(chain, chain_name) != 0)
+			break;
 next:
 		c = nft_chain_list_iter_next(iter);
 	}
-- 
1.8.2.1

Re: [iptables-nftables - PATCH 1/2] nft: Optimize chain listing if only one is looked for

[Pablo Neira Ayuso]

Hi Tomasz,

On Wed, Jul 17, 2013 at 10:34:14AM +0300, Tomasz Bursztyka wrote:
> Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com>
> ---
>  iptables/nft.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/iptables/nft.c b/iptables/nft.c
> index f9a88c9..f33faa5 100644
> --- a/iptables/nft.c
> +++ b/iptables/nft.c
> @@ -2495,6 +2495,9 @@ int nft_rule_list(struct nft_handle *h, const char *chain, const char *table,
>  		}
>  		__nft_rule_list(h, c, table, rulenum, format, print_firewall);
>  
> +		if (chain && strcmp(chain, chain_name) == 0)
> +			break;
> +
>  		found = true;
>  
>  next:
> @@ -2593,6 +2596,9 @@ int nft_rule_list_save(struct nft_handle *h, const char *chain,
>  
>  		ret = __nft_rule_list(h, c, table, rulenum,
>  				      counters ? 0 : FMT_NOCOUNTS, list_save);
> +
> +		if (chain && strcmp(chain, chain_name) != 0)

This should be == 0. There's the same checking above __nft_rule_list.

I noticed that we don't need to strcmp(chain, chain_name) again,
checking for chain is sufficient.

Pushed this patch, is based on yours:

http://git.netfilter.org/iptables-nftables/commit/?id=db6d43c979954b1a0e2a3d2d1fa4494c43d921c1

While at it, I also noticed that selective listing per chain with -S
was also broken, fixed here:

http://git.netfilter.org/iptables-nftables/commit/?id=eaa70f580a3e3b7675d75005ab71c00494a3ee6e

Regards.

Re: [iptables-nftables - PATCH 1/2] nft: Optimize chain listing if only one is looked for

[Tomasz Bursztyka]

Hi Pablo,

>> +		if (chain && strcmp(chain, chain_name) != 0)
> This should be == 0. There's the same checking above __nft_rule_list.
>
> I noticed that we don't need to strcmp(chain, chain_name) again,
> checking for chain is sufficient.

Indeed, was not really awaken. Thanks for fixing further.

Tomasz

[iptables-nftables - PATCH 2/2] nft: Fix code style issues

[Tomasz Bursztyka]

Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com>
---
 iptables/nft.c | 88 +++++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 56 insertions(+), 32 deletions(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index f33faa5..1004ec8 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -485,7 +485,8 @@ static void nft_chain_print_debug(struct nft_chain *c, struct nlmsghdr *nlh)
 
 	nft_chain_snprintf(tmp, sizeof(tmp), c, 0, 0);
 	printf("DEBUG: chain: %s", tmp);
-	mnl_nlmsg_fprintf(stdout, nlh, nlh->nlmsg_len, sizeof(struct nfgenmsg));
+	mnl_nlmsg_fprintf(stdout, nlh, nlh->nlmsg_len,
+						sizeof(struct nfgenmsg));
 #endif
 }
 
@@ -560,7 +561,8 @@ static int __add_match(struct nft_rule_expr *e, struct xt_entry_match *m)
 {
 	void *info;
 
-	nft_rule_expr_set(e, NFT_EXPR_MT_NAME, m->u.user.name, strlen(m->u.user.name));
+	nft_rule_expr_set(e, NFT_EXPR_MT_NAME, m->u.user.name,
+						strlen(m->u.user.name));
 	nft_rule_expr_set_u32(e, NFT_EXPR_MT_REV, m->u.user.revision);
 
 	info = calloc(1, m->u.match_size);
@@ -568,7 +570,8 @@ static int __add_match(struct nft_rule_expr *e, struct xt_entry_match *m)
 		return -ENOMEM;
 
 	memcpy(info, m->data, m->u.match_size);
-	nft_rule_expr_set(e, NFT_EXPR_MT_INFO, info, m->u.match_size - sizeof(*m));
+	nft_rule_expr_set(e, NFT_EXPR_MT_INFO,
+					info, m->u.match_size - sizeof(*m));
 
 	return 0;
 }
@@ -604,7 +607,8 @@ static int __add_target(struct nft_rule_expr *e, struct xt_entry_target *t)
 		memcpy(info, t->data, t->u.target_size);
 	}
 
-	nft_rule_expr_set(e, NFT_EXPR_TG_INFO, info, t->u.target_size - sizeof(*t));
+	nft_rule_expr_set(e, NFT_EXPR_TG_INFO, info,
+						t->u.target_size - sizeof(*t));
 
 	return 0;
 }
@@ -662,7 +666,8 @@ static void nft_rule_print_debug(struct nft_rule *r, struct nlmsghdr *nlh)
 
 	nft_rule_snprintf(tmp, sizeof(tmp), r, 0, 0);
 	printf("DEBUG: rule: %s", tmp);
-	mnl_nlmsg_fprintf(stdout, nlh, nlh->nlmsg_len, sizeof(struct nfgenmsg));
+	mnl_nlmsg_fprintf(stdout, nlh, nlh->nlmsg_len,
+					sizeof(struct nfgenmsg));
 #endif
 }
 
@@ -996,7 +1001,8 @@ nft_print_counters(struct nft_rule_expr *e, struct nft_rule_expr_iter *iter,
 }
 
 void
-nft_rule_print_save(struct nft_rule *r, enum nft_rule_print type, bool counters)
+nft_rule_print_save(struct nft_rule *r, enum nft_rule_print type,
+		    bool counters)
 {
 	struct nft_rule_expr_iter *iter;
 	struct nft_rule_expr *expr;
@@ -1115,8 +1121,8 @@ static void nft_chain_print_save(struct nft_chain *c, bool basechain)
 		if (nft_chain_attr_get(c, NFT_CHAIN_ATTR_POLICY))
 			pol = nft_chain_attr_get_u32(c, NFT_CHAIN_ATTR_POLICY);
 
-		printf(":%s %s [%"PRIu64":%"PRIu64"]\n", chain, policy_name[pol],
-					     pkts, bytes);
+		printf(":%s %s [%"PRIu64":%"PRIu64"]\n",
+					chain, policy_name[pol], pkts, bytes);
 	} else
 		printf(":%s - [%"PRIu64":%"PRIu64"]\n", chain, pkts, bytes);
 }
@@ -1316,7 +1322,8 @@ err:
 	return ret == 0 ? 1 : 0;
 }
 
-int nft_chain_user_add(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)
 {
 	char buf[MNL_SOCKET_BUFFER_SIZE];
 	struct nlmsghdr *nlh;
@@ -1368,7 +1375,8 @@ static int __nft_chain_del(struct nft_handle *h, struct nft_chain *c)
 	return ret;
 }
 
-int nft_chain_user_del(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)
 {
 	struct nft_chain_list *list;
 	struct nft_chain_list_iter *iter;
@@ -1594,7 +1602,9 @@ err:
 }
 
 int nft_for_each_table(struct nft_handle *h,
-		       int (*func)(struct nft_handle *h, const char *tablename, bool counters),
+		       int (*func)(struct nft_handle *h,
+			           const char *tablename,
+				   bool counters),
 		       bool counters)
 {
 	int ret = 1;
@@ -1641,9 +1651,8 @@ int nft_table_purge_chains(struct nft_handle *h, const char *this_table,
 
 	chain_obj = nft_chain_list_iter_next(iter);
 	while (chain_obj != NULL) {
-		const char *table =
-			nft_chain_attr_get_str(chain_obj, NFT_CHAIN_ATTR_TABLE);
-
+		const char *table = nft_chain_attr_get_str(chain_obj,
+							NFT_CHAIN_ATTR_TABLE);
 		if (strcmp(this_table, table) != 0)
 			goto next;
 
@@ -1819,7 +1828,8 @@ __find_match(struct nft_rule_expr *expr, struct xtables_rule_match *matches)
 			continue;
 		}
 
-		if (memcmp(data, m->data, m->u.user.match_size - sizeof(*m)) != 0) {
+		if (memcmp(data, m->data,
+				m->u.user.match_size - sizeof(*m)) != 0) {
 			DEBUGP("mismatch match data\n");
 			continue;
 		}
@@ -1830,7 +1840,8 @@ __find_match(struct nft_rule_expr *expr, struct xtables_rule_match *matches)
 	return found;
 }
 
-static bool find_matches(struct xtables_rule_match *matches, struct nft_rule *r)
+static bool
+find_matches(struct xtables_rule_match *matches, struct nft_rule *r)
 {
 	struct nft_rule_expr_iter *iter;
 	struct nft_rule_expr *expr;
@@ -1862,7 +1873,8 @@ static bool find_matches(struct xtables_rule_match *matches, struct nft_rule *r)
 	return true;
 }
 
-static bool __find_target(struct nft_rule_expr *expr, struct xt_entry_target *t)
+static bool
+__find_target(struct nft_rule_expr *expr, struct xt_entry_target *t)
 {
 	size_t len;
 	const char *tgname = nft_rule_expr_get_str(expr, NFT_EXPR_TG_NAME);
@@ -1944,7 +1956,8 @@ find_immediate(struct nft_rule *r, const char *jumpto)
 			nft_rule_expr_get_str(expr, NFT_RULE_EXPR_ATTR_NAME);
 
 		if (strcmp(name, "immediate") == 0) {
-			int verdict = nft_rule_expr_get_u32(expr, NFT_EXPR_IMM_VERDICT);
+			int verdict = nft_rule_expr_get_u32(expr,
+							NFT_EXPR_IMM_VERDICT);
 			const char *verdict_name = NULL;
 
 			/* No target specified but immediate shows up, this
@@ -2189,8 +2202,8 @@ int nft_rule_replace(struct nft_handle *h, const char *chain,
 						 NFT_RULE_F_COMMIT);
 		}
 		ret = nft_rule_add(h, chain, table, cs, true,
-				   nft_rule_attr_get_u64(r, NFT_RULE_ATTR_HANDLE),
-				   verbose);
+				nft_rule_attr_get_u64(r, NFT_RULE_ATTR_HANDLE),
+				verbose);
 	} else
 		errno = ENOENT;
 
@@ -2473,8 +2486,10 @@ int nft_rule_list(struct nft_handle *h, const char *chain, const char *table,
 		uint32_t refs =
 			nft_chain_attr_get_u32(c, NFT_CHAIN_ATTR_USE);
 		struct xt_counters ctrs = {
-			.pcnt = nft_chain_attr_get_u64(c, NFT_CHAIN_ATTR_PACKETS),
-			.bcnt = nft_chain_attr_get_u64(c, NFT_CHAIN_ATTR_BYTES),
+			.pcnt = nft_chain_attr_get_u64(c,
+						NFT_CHAIN_ATTR_PACKETS),
+			.bcnt = nft_chain_attr_get_u64(c,
+						NFT_CHAIN_ATTR_BYTES),
 		};
 		bool basechain = false;
 
@@ -2547,8 +2562,10 @@ nft_rule_list_chain_save(struct nft_handle *h, const char *table,
 
 			if (counters) {
 				printf(" -c %"PRIu64" %"PRIu64"\n",
-					nft_chain_attr_get_u64(c, NFT_CHAIN_ATTR_PACKETS),
-					nft_chain_attr_get_u64(c, NFT_CHAIN_ATTR_BYTES));
+					nft_chain_attr_get_u64(c,
+						NFT_CHAIN_ATTR_PACKETS),
+					nft_chain_attr_get_u64(c,
+						NFT_CHAIN_ATTR_BYTES));
 			} else
 				printf("\n");
 		} else {
@@ -2796,11 +2813,13 @@ int nft_xtables_config_load(struct nft_handle *h, const char *filename,
 			if (errno == EEXIST) {
 				xtables_config_perror(flags,
 					"table `%s' already exists, skipping\n",
-					(char *)nft_table_attr_get(table, NFT_TABLE_ATTR_NAME));
+					(char *)nft_table_attr_get(table,
+							NFT_TABLE_ATTR_NAME));
 			} else {
 				xtables_config_perror(flags,
 					"table `%s' cannot be create, reason `%s'. Exitting\n",
-					(char *)nft_table_attr_get(table, NFT_TABLE_ATTR_NAME),
+					(char *)nft_table_attr_get(table,
+							NFT_TABLE_ATTR_NAME),
 					strerror(errno));
 				nft_table_list_iter_destroy(titer);
 				nft_table_list_free(table_list);
@@ -2809,7 +2828,8 @@ int nft_xtables_config_load(struct nft_handle *h, const char *filename,
 			continue;
 		}
 		xtables_config_perror(flags, "table `%s' has been created\n",
-			(char *)nft_table_attr_get(table, NFT_TABLE_ATTR_NAME));
+			(char *)nft_table_attr_get(table,
+							NFT_TABLE_ATTR_NAME));
 	}
 	nft_table_list_iter_destroy(titer);
 	nft_table_list_free(table_list);
@@ -2821,12 +2841,15 @@ int nft_xtables_config_load(struct nft_handle *h, const char *filename,
 			if (errno == EEXIST) {
 				xtables_config_perror(flags,
 					"chain `%s' already exists in table `%s', skipping\n",
-					(char *)nft_chain_attr_get(chain, NFT_CHAIN_ATTR_NAME),
-					(char *)nft_chain_attr_get(chain, NFT_CHAIN_ATTR_TABLE));
+					(char *)nft_chain_attr_get(chain,
+							NFT_CHAIN_ATTR_NAME),
+					(char *)nft_chain_attr_get(chain,
+							NFT_CHAIN_ATTR_TABLE));
 			} else {
 				xtables_config_perror(flags,
 					"chain `%s' cannot be create, reason `%s'. Exitting\n",
-					(char *)nft_chain_attr_get(chain, NFT_CHAIN_ATTR_NAME),
+					(char *)nft_chain_attr_get(chain,
+							NFT_CHAIN_ATTR_NAME),
 					strerror(errno));
 				nft_chain_list_iter_destroy(citer);
 				nft_chain_list_free(chain_list);
@@ -2838,7 +2861,8 @@ int nft_xtables_config_load(struct nft_handle *h, const char *filename,
 		xtables_config_perror(flags,
 			"chain `%s' in table `%s' has been created\n",
 			(char *)nft_chain_attr_get(chain, NFT_CHAIN_ATTR_NAME),
-			(char *)nft_chain_attr_get(chain, NFT_CHAIN_ATTR_TABLE));
+			(char *)nft_chain_attr_get(chain,
+							NFT_CHAIN_ATTR_TABLE));
 	}
 	nft_chain_list_iter_destroy(citer);
 	nft_chain_list_free(chain_list);
@@ -2846,7 +2870,7 @@ int nft_xtables_config_load(struct nft_handle *h, const char *filename,
 	return 0;
 }
 
-int nft_chain_zero_counters(struct nft_handle *h, const char *chain, 
+int nft_chain_zero_counters(struct nft_handle *h, const char *chain,
 			    const char *table)
 {
 	struct nft_chain_list *list;
-- 
1.8.2.1

Re: [iptables-nftables - PATCH 2/2] nft: Fix code style issues

[Pablo Neira Ayuso]

I don't feel like taking this at this moment.

I think many of those style adjustments could be fixed by splitting
functions into smaller chunks and with several helper functions
similar to your "nft: add function to test for a builtin chain" patch,
which I think is the way to go to get this code better.

We can also revisit this later if you don't want to work on this at
this moment.

Thanks for the patch in any case.

Re: [iptables-nftables - PATCH 2/2] nft: Fix code style issues

[Tomasz Bursztyka]

Hi Pablo,

> I think many of those style adjustments could be fixed by splitting
> functions into smaller chunks and with several helper functions
> similar to your "nft: add function to test for a builtin chain" patch,
> which I think is the way to go to get this code better.
>
> We can also revisit this later if you don't want to work on this at
> this moment.

As this is not in a hurry, let's see yes.

Actually Giuseppe's work might help on that since he will move some of 
these common helpers to nft-shared.c (or a new nft-utils.c, let's see)

Tomasz