[iptables PATCH 1/2] nft: Use verbose flag to toggle debug output

[iptables PATCH 1/2] nft: Use verbose flag to toggle debug output

[Phil Sutter]

Copy legacy iptables' behaviour, printing debug output if verbose flag
is given more than once.

Since nft debug output applies to netlink messages which are not created
until nft_action() phase, carrying verbose value is non-trivial -
introduce a field in struct nft_handle for that.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft-shared.h |  1 -
 iptables/nft.c        | 38 ++++++++++++++++++++------------------
 iptables/nft.h        |  1 +
 iptables/xtables.c    |  1 +
 4 files changed, 22 insertions(+), 19 deletions(-)

diff --git a/iptables/nft-shared.h b/iptables/nft-shared.h
index c3241f4b8c726..d4222ae05adc3 100644
--- a/iptables/nft-shared.h
+++ b/iptables/nft-shared.h
@@ -13,7 +13,6 @@
 #include "xshared.h"
 
 #ifdef DEBUG
-#define NLDEBUG
 #define DEBUG_DEL
 #endif
 
diff --git a/iptables/nft.c b/iptables/nft.c
index b5de687c5c4cd..ae41384fe8180 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -926,15 +926,16 @@ void nft_fini(struct nft_handle *h)
 	mnl_socket_close(h->nl);
 }
 
-static void nft_chain_print_debug(struct nftnl_chain *c, struct nlmsghdr *nlh)
+static void nft_chain_print_debug(struct nft_handle *h,
+				  struct nftnl_chain *c, struct nlmsghdr *nlh)
 {
-#ifdef NLDEBUG
-	char tmp[1024];
-
-	nftnl_chain_snprintf(tmp, sizeof(tmp), c, 0, 0);
-	printf("DEBUG: chain: %s\n", tmp);
-	mnl_nlmsg_fprintf(stdout, nlh, nlh->nlmsg_len, sizeof(struct nfgenmsg));
-#endif
+	if (h->verbose > 1) {
+		nftnl_chain_fprintf(stdout, c, 0, 0);
+		fprintf(stdout, "\n");
+	}
+	if (h->verbose > 2)
+		mnl_nlmsg_fprintf(stdout, nlh, nlh->nlmsg_len,
+				  sizeof(struct nfgenmsg));
 }
 
 static struct nftnl_chain *nft_chain_new(struct nft_handle *h,
@@ -1386,15 +1387,16 @@ int add_log(struct nftnl_rule *r, struct iptables_command_state *cs)
 	return 0;
 }
 
-static void nft_rule_print_debug(struct nftnl_rule *r, struct nlmsghdr *nlh)
+static void nft_rule_print_debug(struct nft_handle *h,
+				 struct nftnl_rule *r, struct nlmsghdr *nlh)
 {
-#ifdef NLDEBUG
-	char tmp[1024];
-
-	nftnl_rule_snprintf(tmp, sizeof(tmp), r, 0, 0);
-	printf("DEBUG: rule: %s\n", tmp);
-	mnl_nlmsg_fprintf(stdout, nlh, nlh->nlmsg_len, sizeof(struct nfgenmsg));
-#endif
+	if (h->verbose > 1) {
+		nftnl_rule_fprintf(stdout, r, 0, 0);
+		fprintf(stdout, "\n");
+	}
+	if (h->verbose > 2)
+		mnl_nlmsg_fprintf(stdout, nlh, nlh->nlmsg_len,
+				  sizeof(struct nfgenmsg));
 }
 
 int add_counters(struct nftnl_rule *r, uint64_t packets, uint64_t bytes)
@@ -2698,7 +2700,7 @@ static void nft_compat_chain_batch_add(struct nft_handle *h, uint16_t type,
 	nlh = nftnl_chain_nlmsg_build_hdr(nftnl_batch_buffer(h->batch),
 					type, h->family, flags, seq);
 	nftnl_chain_nlmsg_build_payload(nlh, chain);
-	nft_chain_print_debug(chain, nlh);
+	nft_chain_print_debug(h, chain, nlh);
 }
 
 static void nft_compat_rule_batch_add(struct nft_handle *h, uint16_t type,
@@ -2710,7 +2712,7 @@ static void nft_compat_rule_batch_add(struct nft_handle *h, uint16_t type,
 	nlh = nftnl_rule_nlmsg_build_hdr(nftnl_batch_buffer(h->batch),
 				       type, h->family, flags, seq);
 	nftnl_rule_nlmsg_build_payload(nlh, rule);
-	nft_rule_print_debug(rule, nlh);
+	nft_rule_print_debug(h, rule, nlh);
 }
 
 static void batch_obj_del(struct nft_handle *h, struct obj_update *o)
diff --git a/iptables/nft.h b/iptables/nft.h
index 4c78f761e1c4b..fd116c2e3e198 100644
--- a/iptables/nft.h
+++ b/iptables/nft.h
@@ -109,6 +109,7 @@ struct nft_handle {
 	int8_t			config_done;
 	struct list_head	cmd_list;
 	bool			cache_init;
+	int			verbose;
 
 	/* meta data, for error reporting */
 	struct {
diff --git a/iptables/xtables.c b/iptables/xtables.c
index 051d5c7b7f98b..c44b39acdcd97 100644
--- a/iptables/xtables.c
+++ b/iptables/xtables.c
@@ -163,6 +163,7 @@ int do_commandx(struct nft_handle *h, int argc, char *argv[], char **table,
 		h->ops->init_cs(&cs);
 
 	do_parse(argc, argv, &p, &cs, &args);
+	h->verbose = p.verbose;
 
 	if (!nft_table_builtin_find(h, p.table))
 		xtables_error(VERSION_PROBLEM,
-- 
2.34.1

[iptables PATCH 2/2] iptables-restore: Support for extra debug output

[Phil Sutter]

Treat --verbose just like iptables itself, increasing debug level with
number of invocations.

To propagate the level into do_command() callback, insert virtual '-v'
flags into rule lines.

The only downside of this is that simple verbose output is changed and
now also prints the rules as they are added - which would be useful if
the lines contained the chain they apply to.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/iptables-restore.8.in                           | 1 +
 iptables/iptables-restore.c                              | 6 +++++-
 .../shell/testcases/ipt-restore/0014-verbose-restore_0   | 9 ++++++---
 iptables/xtables-restore.c                               | 6 +++++-
 4 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/iptables/iptables-restore.8.in b/iptables/iptables-restore.8.in
index b4b62f92740d1..883da998b0f7e 100644
--- a/iptables/iptables-restore.8.in
+++ b/iptables/iptables-restore.8.in
@@ -54,6 +54,7 @@ Only parse and construct the ruleset, but do not commit it.
 .TP
 \fB\-v\fP, \fB\-\-verbose\fP
 Print additional debug info during ruleset processing.
+Specify multiple times to increase debug level.
 .TP
 \fB\-V\fP, \fB\-\-version\fP
 Print the program version number.
diff --git a/iptables/iptables-restore.c b/iptables/iptables-restore.c
index a3efb067d3d90..3c0a238917ecd 100644
--- a/iptables/iptables-restore.c
+++ b/iptables/iptables-restore.c
@@ -114,7 +114,7 @@ ip46tables_restore_main(const struct iptables_restore_cb *cb,
 				counters = 1;
 				break;
 			case 'v':
-				verbose = 1;
+				verbose++;
 				break;
 			case 'V':
 				printf("%s v%s\n",
@@ -317,11 +317,15 @@ ip46tables_restore_main(const struct iptables_restore_cb *cb,
 			char *pcnt = NULL;
 			char *bcnt = NULL;
 			char *parsestart = buffer;
+			int i;
 
 			add_argv(&av_store, argv[0], 0);
 			add_argv(&av_store, "-t", 0);
 			add_argv(&av_store, curtable, 0);
 
+			for (i = 0; !noflush && i < verbose; i++)
+				add_argv(&av_store, "-v", 0);
+
 			tokenize_rule_counters(&parsestart, &pcnt, &bcnt, line);
 			if (counters && pcnt && bcnt) {
 				add_argv(&av_store, "--set-counters", 0);
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 fc8559c5bac9e..5daf7a78a5334 100755
--- a/iptables/tests/shell/testcases/ipt-restore/0014-verbose-restore_0
+++ b/iptables/tests/shell/testcases/ipt-restore/0014-verbose-restore_0
@@ -33,6 +33,7 @@ Flushing chain \`bar'
 Flushing chain \`foo'
 Deleting chain \`bar'
 Deleting chain \`foo'
+ACCEPT  all opt -- in * out *  0.0.0.0/0  -> 0.0.0.0/0
 Flushing chain \`PREROUTING'
 Flushing chain \`INPUT'
 Flushing chain \`OUTPUT'
@@ -41,6 +42,7 @@ Flushing chain \`natbar'
 Flushing chain \`natfoo'
 Deleting chain \`natbar'
 Deleting chain \`natfoo'
+ACCEPT  all opt -- in * out *  0.0.0.0/0  -> 0.0.0.0/0
 Flushing chain \`PREROUTING'
 Flushing chain \`OUTPUT'
 Flushing chain \`rawfoo'
@@ -58,9 +60,10 @@ Flushing chain \`OUTPUT'
 Flushing chain \`secfoo'
 Deleting chain \`secfoo'"
 
-for ipt in iptables-restore ip6tables-restore; do
-	diff -u -Z <(echo "$EXPECT") <($XT_MULTI $ipt -v <<< "$DUMP")
-done
+EXPECT6=$(sed -e 's/0\.0\.0\.0/::/g' -e 's/opt --/opt   /' <<< "$EXPECT")
+
+diff -u -Z <(echo "$EXPECT") <($XT_MULTI iptables-restore -v <<< "$DUMP")
+diff -u -Z <(echo "$EXPECT6") <($XT_MULTI ip6tables-restore -v <<< "$DUMP")
 
 DUMP="*filter
 :baz - [0:0]
diff --git a/iptables/xtables-restore.c b/iptables/xtables-restore.c
index 8ca2abffa5d36..f5aabf3cc1944 100644
--- a/iptables/xtables-restore.c
+++ b/iptables/xtables-restore.c
@@ -206,11 +206,15 @@ static void xtables_restore_parse_line(struct nft_handle *h,
 		char *pcnt = NULL;
 		char *bcnt = NULL;
 		char *parsestart = buffer;
+		int i;
 
 		add_argv(&state->av_store, xt_params->program_name, 0);
 		add_argv(&state->av_store, "-t", 0);
 		add_argv(&state->av_store, state->curtable->name, 0);
 
+		for (i = 0; !h->noflush && i < verbose; i++)
+			add_argv(&state->av_store, "-v", 0);
+
 		tokenize_rule_counters(&parsestart, &pcnt, &bcnt, line);
 		if (counters && pcnt && bcnt) {
 			add_argv(&state->av_store, "--set-counters", 0);
@@ -309,7 +313,7 @@ xtables_restore_main(int family, const char *progname, int argc, char *argv[])
 				counters = 1;
 				break;
 			case 'v':
-				verbose = 1;
+				verbose++;
 				break;
 			case 'V':
 				printf("%s v%s\n", prog_name, prog_vers);
-- 
2.34.1

Re: [iptables PATCH 1/2] nft: Use verbose flag to toggle debug output

[Pablo Neira Ayuso]

On Fri, Jan 28, 2022 at 09:36:59PM +0100, Phil Sutter wrote:
> Copy legacy iptables' behaviour, printing debug output if verbose flag
> is given more than once.
> 
> Since nft debug output applies to netlink messages which are not created
> until nft_action() phase, carrying verbose value is non-trivial -
> introduce a field in struct nft_handle for that.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  iptables/nft-shared.h |  1 -
>  iptables/nft.c        | 38 ++++++++++++++++++++------------------
>  iptables/nft.h        |  1 +
>  iptables/xtables.c    |  1 +
>  4 files changed, 22 insertions(+), 19 deletions(-)
> 
> diff --git a/iptables/nft-shared.h b/iptables/nft-shared.h
> index c3241f4b8c726..d4222ae05adc3 100644
> --- a/iptables/nft-shared.h
> +++ b/iptables/nft-shared.h
> @@ -13,7 +13,6 @@
>  #include "xshared.h"
>  
>  #ifdef DEBUG
> -#define NLDEBUG
>  #define DEBUG_DEL
>  #endif
>  
> diff --git a/iptables/nft.c b/iptables/nft.c
> index b5de687c5c4cd..ae41384fe8180 100644
> --- a/iptables/nft.c
> +++ b/iptables/nft.c
> @@ -926,15 +926,16 @@ void nft_fini(struct nft_handle *h)
>  	mnl_socket_close(h->nl);
>  }
>  
> -static void nft_chain_print_debug(struct nftnl_chain *c, struct nlmsghdr *nlh)
> +static void nft_chain_print_debug(struct nft_handle *h,
> +				  struct nftnl_chain *c, struct nlmsghdr *nlh)
>  {
> -#ifdef NLDEBUG
> -	char tmp[1024];
> -
> -	nftnl_chain_snprintf(tmp, sizeof(tmp), c, 0, 0);
> -	printf("DEBUG: chain: %s\n", tmp);
> -	mnl_nlmsg_fprintf(stdout, nlh, nlh->nlmsg_len, sizeof(struct nfgenmsg));
> -#endif
> +	if (h->verbose > 1) {
> +		nftnl_chain_fprintf(stdout, c, 0, 0);
> +		fprintf(stdout, "\n");
> +	}
> +	if (h->verbose > 2)
> +		mnl_nlmsg_fprintf(stdout, nlh, nlh->nlmsg_len,
> +				  sizeof(struct nfgenmsg));

OK, so -v is netlink byte printing and -vv means print netlink message
too. LGTM.

I'd mention this in the commit description before applying.

>  }
>  
>  static struct nftnl_chain *nft_chain_new(struct nft_handle *h,
> @@ -1386,15 +1387,16 @@ int add_log(struct nftnl_rule *r, struct iptables_command_state *cs)
>  	return 0;
>  }
>  
> -static void nft_rule_print_debug(struct nftnl_rule *r, struct nlmsghdr *nlh)
> +static void nft_rule_print_debug(struct nft_handle *h,
> +				 struct nftnl_rule *r, struct nlmsghdr *nlh)
>  {
> -#ifdef NLDEBUG
> -	char tmp[1024];
> -
> -	nftnl_rule_snprintf(tmp, sizeof(tmp), r, 0, 0);
> -	printf("DEBUG: rule: %s\n", tmp);
> -	mnl_nlmsg_fprintf(stdout, nlh, nlh->nlmsg_len, sizeof(struct nfgenmsg));
> -#endif
> +	if (h->verbose > 1) {
> +		nftnl_rule_fprintf(stdout, r, 0, 0);
> +		fprintf(stdout, "\n");
> +	}
> +	if (h->verbose > 2)
> +		mnl_nlmsg_fprintf(stdout, nlh, nlh->nlmsg_len,
> +				  sizeof(struct nfgenmsg));
>  }
>  
>  int add_counters(struct nftnl_rule *r, uint64_t packets, uint64_t bytes)
> @@ -2698,7 +2700,7 @@ static void nft_compat_chain_batch_add(struct nft_handle *h, uint16_t type,
>  	nlh = nftnl_chain_nlmsg_build_hdr(nftnl_batch_buffer(h->batch),
>  					type, h->family, flags, seq);
>  	nftnl_chain_nlmsg_build_payload(nlh, chain);
> -	nft_chain_print_debug(chain, nlh);
> +	nft_chain_print_debug(h, chain, nlh);
>  }
>  
>  static void nft_compat_rule_batch_add(struct nft_handle *h, uint16_t type,
> @@ -2710,7 +2712,7 @@ static void nft_compat_rule_batch_add(struct nft_handle *h, uint16_t type,
>  	nlh = nftnl_rule_nlmsg_build_hdr(nftnl_batch_buffer(h->batch),
>  				       type, h->family, flags, seq);
>  	nftnl_rule_nlmsg_build_payload(nlh, rule);
> -	nft_rule_print_debug(rule, nlh);
> +	nft_rule_print_debug(h, rule, nlh);
>  }
>  
>  static void batch_obj_del(struct nft_handle *h, struct obj_update *o)
> diff --git a/iptables/nft.h b/iptables/nft.h
> index 4c78f761e1c4b..fd116c2e3e198 100644
> --- a/iptables/nft.h
> +++ b/iptables/nft.h
> @@ -109,6 +109,7 @@ struct nft_handle {
>  	int8_t			config_done;
>  	struct list_head	cmd_list;
>  	bool			cache_init;
> +	int			verbose;
>  
>  	/* meta data, for error reporting */
>  	struct {
> diff --git a/iptables/xtables.c b/iptables/xtables.c
> index 051d5c7b7f98b..c44b39acdcd97 100644
> --- a/iptables/xtables.c
> +++ b/iptables/xtables.c
> @@ -163,6 +163,7 @@ int do_commandx(struct nft_handle *h, int argc, char *argv[], char **table,
>  		h->ops->init_cs(&cs);
>  
>  	do_parse(argc, argv, &p, &cs, &args);
> +	h->verbose = p.verbose;
>  
>  	if (!nft_table_builtin_find(h, p.table))
>  		xtables_error(VERSION_PROBLEM,
> -- 
> 2.34.1
> 

Re: [iptables PATCH 1/2] nft: Use verbose flag to toggle debug output

[Phil Sutter]

On Mon, Feb 07, 2022 at 07:02:37PM +0100, Pablo Neira Ayuso wrote:
> On Fri, Jan 28, 2022 at 09:36:59PM +0100, Phil Sutter wrote:
[...]
> > -static void nft_chain_print_debug(struct nftnl_chain *c, struct nlmsghdr *nlh)
> > +static void nft_chain_print_debug(struct nft_handle *h,
> > +				  struct nftnl_chain *c, struct nlmsghdr *nlh)
> >  {
> > -#ifdef NLDEBUG
> > -	char tmp[1024];
> > -
> > -	nftnl_chain_snprintf(tmp, sizeof(tmp), c, 0, 0);
> > -	printf("DEBUG: chain: %s\n", tmp);
> > -	mnl_nlmsg_fprintf(stdout, nlh, nlh->nlmsg_len, sizeof(struct nfgenmsg));
> > -#endif
> > +	if (h->verbose > 1) {
> > +		nftnl_chain_fprintf(stdout, c, 0, 0);
> > +		fprintf(stdout, "\n");
> > +	}
> > +	if (h->verbose > 2)
> > +		mnl_nlmsg_fprintf(stdout, nlh, nlh->nlmsg_len,
> > +				  sizeof(struct nfgenmsg));
> 
> OK, so -v is netlink byte printing and -vv means print netlink message
> too. LGTM.

-v is "normal verbose output", -vv is also nftnl debug and -vvv is also
netlink message dump.

> I'd mention this in the commit description before applying.

Your comment is proof this needs better documentation! :D
Guess I'll describe the behaviour in iptables man page as well.

Thanks, Phil