[iptables PATCH 0/8] A bit of *tables-restore review fallout

[iptables PATCH 0/8] A bit of *tables-restore review fallout

[Phil Sutter]

Preparing for upcoming xtables-restore performance improvements I
noticed a few things to smooth out. Patches in this series are not
functionally related.

Phil Sutter (8):
  xtables-restore: Treat struct nft_xt_restore_parse as const
  xtables-restore: Use xt_params->program_name
  xtables-restore: Introduce rule counter tokenizer function
  xtables-restore: Constify struct nft_xt_restore_cb
  iptables-restore: Constify struct iptables_restore_cb
  xtables-restore: Drop pointless newargc reset
  xtables-restore: Drop local xtc_ops instance
  xtables-restore: Drop chain_list callback

 iptables/iptables-restore.c                   | 44 +++-------
 iptables/iptables-xml.c                       | 31 +------
 iptables/nft-shared.h                         |  7 +-
 .../ipt-restore/0008-restore-counters_0       | 22 +++++
 iptables/xshared.c                            | 37 +++++++++
 iptables/xshared.h                            |  1 +
 iptables/xtables-restore.c                    | 81 ++++---------------
 iptables/xtables-translate.c                  |  4 +-
 8 files changed, 90 insertions(+), 137 deletions(-)
 create mode 100755 iptables/tests/shell/testcases/ipt-restore/0008-restore-counters_0

-- 
2.23.0

[iptables PATCH 1/8] xtables-restore: Treat struct nft_xt_restore_parse as const

[Phil Sutter]

This structure contains restore parser configuration, parser is not
supposed to alter it.

Suggested-by: Pablo Neira Ayuso <pablo@netfilter.org>
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft-shared.h      | 2 +-
 iptables/xtables-restore.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/iptables/nft-shared.h b/iptables/nft-shared.h
index 9d62913461fa4..a330aceb9785c 100644
--- a/iptables/nft-shared.h
+++ b/iptables/nft-shared.h
@@ -261,7 +261,7 @@ struct nft_xt_restore_cb {
 };
 
 void xtables_restore_parse(struct nft_handle *h,
-			   struct nft_xt_restore_parse *p,
+			   const struct nft_xt_restore_parse *p,
 			   struct nft_xt_restore_cb *cb,
 			   int argc, char *argv[]);
 
diff --git a/iptables/xtables-restore.c b/iptables/xtables-restore.c
index 4f6d324bdafe9..cb03104e91a7b 100644
--- a/iptables/xtables-restore.c
+++ b/iptables/xtables-restore.c
@@ -86,7 +86,7 @@ static const struct xtc_ops xtc_ops = {
 };
 
 void xtables_restore_parse(struct nft_handle *h,
-			   struct nft_xt_restore_parse *p,
+			   const struct nft_xt_restore_parse *p,
 			   struct nft_xt_restore_cb *cb,
 			   int argc, char *argv[])
 {
-- 
2.23.0

Re: [iptables PATCH 1/8] xtables-restore: Treat struct nft_xt_restore_parse as const

[Pablo Neira Ayuso]

On Fri, Oct 18, 2019 at 12:48:29AM +0200, Phil Sutter wrote:
> This structure contains restore parser configuration, parser is not
> supposed to alter it.
> 
> Suggested-by: Pablo Neira Ayuso <pablo@netfilter.org>
> Signed-off-by: Phil Sutter <phil@nwl.cc>

Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>

[iptables PATCH 2/8] xtables-restore: Use xt_params->program_name

[Phil Sutter]

Instead of setting newargv[0] to argv[0]'s value, just use whatever
xt_params->program_name contains. The latter is arbitrarily defined, but
may still be more correct than real argv[0] which may simply be for
instance xtables-nft-multi. Either way, there is no practical
significance since newargv[0] is used exclusively in debug output.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft-shared.h        |  3 +--
 iptables/xtables-restore.c   | 11 +++++------
 iptables/xtables-translate.c |  2 +-
 3 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/iptables/nft-shared.h b/iptables/nft-shared.h
index a330aceb9785c..5c6641505f3db 100644
--- a/iptables/nft-shared.h
+++ b/iptables/nft-shared.h
@@ -262,8 +262,7 @@ struct nft_xt_restore_cb {
 
 void xtables_restore_parse(struct nft_handle *h,
 			   const struct nft_xt_restore_parse *p,
-			   struct nft_xt_restore_cb *cb,
-			   int argc, char *argv[]);
+			   struct nft_xt_restore_cb *cb);
 
 void nft_check_xt_legacy(int family, bool is_ipt_save);
 #endif
diff --git a/iptables/xtables-restore.c b/iptables/xtables-restore.c
index cb03104e91a7b..24bfce516d34c 100644
--- a/iptables/xtables-restore.c
+++ b/iptables/xtables-restore.c
@@ -87,8 +87,7 @@ static const struct xtc_ops xtc_ops = {
 
 void xtables_restore_parse(struct nft_handle *h,
 			   const struct nft_xt_restore_parse *p,
-			   struct nft_xt_restore_cb *cb,
-			   int argc, char *argv[])
+			   struct nft_xt_restore_cb *cb)
 {
 	const struct builtin_table *curtable = NULL;
 	char buffer[10240];
@@ -264,7 +263,7 @@ void xtables_restore_parse(struct nft_handle *h,
 				parsestart = buffer;
 			}
 
-			add_argv(argv[0], 0);
+			add_argv(xt_params->program_name, 0);
 			add_argv("-t", 0);
 			add_argv(curtable->name, 0);
 
@@ -434,7 +433,7 @@ xtables_restore_main(int family, const char *progname, int argc, char *argv[])
 		exit(EXIT_FAILURE);
 	}
 
-	xtables_restore_parse(&h, &p, &restore_cb, argc, argv);
+	xtables_restore_parse(&h, &p, &restore_cb);
 
 	nft_fini(&h);
 	fclose(p.in);
@@ -500,7 +499,7 @@ int xtables_eb_restore_main(int argc, char *argv[])
 
 	nft_init_eb(&h, "ebtables-restore");
 	h.noflush = noflush;
-	xtables_restore_parse(&h, &p, &ebt_restore_cb, argc, argv);
+	xtables_restore_parse(&h, &p, &ebt_restore_cb);
 	nft_fini(&h);
 
 	return 0;
@@ -524,7 +523,7 @@ int xtables_arp_restore_main(int argc, char *argv[])
 	struct nft_handle h;
 
 	nft_init_arp(&h, "arptables-restore");
-	xtables_restore_parse(&h, &p, &arp_restore_cb, argc, argv);
+	xtables_restore_parse(&h, &p, &arp_restore_cb);
 	nft_fini(&h);
 
 	return 0;
diff --git a/iptables/xtables-translate.c b/iptables/xtables-translate.c
index 4ae9ff57c0eb3..64e7667a253e7 100644
--- a/iptables/xtables-translate.c
+++ b/iptables/xtables-translate.c
@@ -535,7 +535,7 @@ static int xtables_restore_xlate_main(int family, const char *progname,
 
 	printf("# Translated by %s v%s on %s",
 	       argv[0], PACKAGE_VERSION, ctime(&now));
-	xtables_restore_parse(&h, &p, &cb_xlate, argc, argv);
+	xtables_restore_parse(&h, &p, &cb_xlate);
 	printf("# Completed on %s", ctime(&now));
 
 	nft_fini(&h);
-- 
2.23.0

Re: [iptables PATCH 2/8] xtables-restore: Use xt_params->program_name

[Pablo Neira Ayuso]

On Fri, Oct 18, 2019 at 12:48:30AM +0200, Phil Sutter wrote:
> Instead of setting newargv[0] to argv[0]'s value, just use whatever
> xt_params->program_name contains. The latter is arbitrarily defined, but
> may still be more correct than real argv[0] which may simply be for
> instance xtables-nft-multi. Either way, there is no practical
> significance since newargv[0] is used exclusively in debug output.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>

Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>

[iptables PATCH 3/8] xtables-restore: Introduce rule counter tokenizer function

[Phil Sutter]

The same piece of code appears three times, introduce a function to take
care of tokenizing and error reporting.

Pass buffer pointer via reference so it can be updated to point to after
the counters (if found).

While being at it, drop pointless casting when passing pcnt/bcnt to
add_argv().

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/iptables-restore.c                   | 35 ++----------------
 iptables/iptables-xml.c                       | 31 +---------------
 .../ipt-restore/0008-restore-counters_0       | 22 +++++++++++
 iptables/xshared.c                            | 37 +++++++++++++++++++
 iptables/xshared.h                            |  1 +
 iptables/xtables-restore.c                    | 35 ++----------------
 6 files changed, 70 insertions(+), 91 deletions(-)
 create mode 100755 iptables/tests/shell/testcases/ipt-restore/0008-restore-counters_0

diff --git a/iptables/iptables-restore.c b/iptables/iptables-restore.c
index 6bc182bfae4a2..3655b3e84637e 100644
--- a/iptables/iptables-restore.c
+++ b/iptables/iptables-restore.c
@@ -313,44 +313,17 @@ ip46tables_restore_main(struct iptables_restore_cb *cb, int argc, char *argv[])
 			int a;
 			char *pcnt = NULL;
 			char *bcnt = NULL;
-			char *parsestart;
-
-			if (buffer[0] == '[') {
-				/* we have counters in our input */
-				char *ptr = strchr(buffer, ']');
-
-				if (!ptr)
-					xtables_error(PARAMETER_PROBLEM,
-						   "Bad line %u: need ]\n",
-						   line);
-
-				pcnt = strtok(buffer+1, ":");
-				if (!pcnt)
-					xtables_error(PARAMETER_PROBLEM,
-						   "Bad line %u: need :\n",
-						   line);
-
-				bcnt = strtok(NULL, "]");
-				if (!bcnt)
-					xtables_error(PARAMETER_PROBLEM,
-						   "Bad line %u: need ]\n",
-						   line);
-
-				/* start command parsing after counter */
-				parsestart = ptr + 1;
-			} else {
-				/* start command parsing at start of line */
-				parsestart = buffer;
-			}
+			char *parsestart = buffer;
 
 			add_argv(argv[0], 0);
 			add_argv("-t", 0);
 			add_argv(curtable, 0);
 
+			tokenize_rule_counters(&parsestart, &pcnt, &bcnt, line);
 			if (counters && pcnt && bcnt) {
 				add_argv("--set-counters", 0);
-				add_argv((char *) pcnt, 0);
-				add_argv((char *) bcnt, 0);
+				add_argv(pcnt, 0);
+				add_argv(bcnt, 0);
 			}
 
 			add_param_to_argv(parsestart, line);
diff --git a/iptables/iptables-xml.c b/iptables/iptables-xml.c
index 36ad78450b1ef..5255e097eba88 100644
--- a/iptables/iptables-xml.c
+++ b/iptables/iptables-xml.c
@@ -644,7 +644,7 @@ iptables_xml_main(int argc, char *argv[])
 			unsigned int a;
 			char *pcnt = NULL;
 			char *bcnt = NULL;
-			char *parsestart;
+			char *parsestart = buffer;
 			char *chain = NULL;
 
 			/* the parser */
@@ -652,34 +652,7 @@ iptables_xml_main(int argc, char *argv[])
 			int quote_open, quoted;
 			char param_buffer[1024];
 
-			if (buffer[0] == '[') {
-				/* we have counters in our input */
-				char *ptr = strchr(buffer, ']');
-
-				if (!ptr)
-					xtables_error(PARAMETER_PROBLEM,
-						   "Bad line %u: need ]\n",
-						   line);
-
-				pcnt = strtok(buffer + 1, ":");
-				if (!pcnt)
-					xtables_error(PARAMETER_PROBLEM,
-						   "Bad line %u: need :\n",
-						   line);
-
-				bcnt = strtok(NULL, "]");
-				if (!bcnt)
-					xtables_error(PARAMETER_PROBLEM,
-						   "Bad line %u: need ]\n",
-						   line);
-
-				/* start command parsing after counter */
-				parsestart = ptr + 1;
-			} else {
-				/* start command parsing at start of line */
-				parsestart = buffer;
-			}
-
+			tokenize_rule_counters(&parsestart, &pcnt, &bcnt, line);
 
 			/* This is a 'real' parser crafted in artist mode
 			 * not hacker mode. If the author can live with that
diff --git a/iptables/tests/shell/testcases/ipt-restore/0008-restore-counters_0 b/iptables/tests/shell/testcases/ipt-restore/0008-restore-counters_0
new file mode 100755
index 0000000000000..5ac70682b76bf
--- /dev/null
+++ b/iptables/tests/shell/testcases/ipt-restore/0008-restore-counters_0
@@ -0,0 +1,22 @@
+#!/bin/bash
+
+set -e
+
+DUMP="*filter
+:foo - [23:42]
+[13:37] -A foo -j ACCEPT
+COMMIT
+"
+
+EXPECT=":foo - [0:0]
+[0:0] -A foo -j ACCEPT"
+
+$XT_MULTI iptables-restore <<< "$DUMP"
+diff -u -Z <(echo -e "$EXPECT") <($XT_MULTI iptables-save --counters | grep foo)
+
+# iptables-*-restore ignores custom chain counters :(
+EXPECT=":foo - [0:0]
+[13:37] -A foo -j ACCEPT"
+
+$XT_MULTI iptables-restore --counters <<< "$DUMP"
+diff -u -Z <(echo -e "$EXPECT") <($XT_MULTI iptables-save --counters | grep foo)
diff --git a/iptables/xshared.c b/iptables/xshared.c
index 5e6cd4ae7c908..ba723f59dbaad 100644
--- a/iptables/xshared.c
+++ b/iptables/xshared.c
@@ -373,6 +373,43 @@ int parse_counters(const char *string, struct xt_counters *ctr)
 	return ret == 2;
 }
 
+/* Tokenize counters argument of typical iptables-restore format rule.
+ *
+ * If *bufferp contains counters, update *pcntp and *bcntp to point at them,
+ * change bytes after counters in *bufferp to nul-bytes, update *bufferp to
+ * point to after the counters and return true.
+ * If *bufferp does not contain counters, return false.
+ * If syntax is wrong in *bufferp, call xtables_error() and hence exit().
+ * */
+bool tokenize_rule_counters(char **bufferp, char **pcntp, char **bcntp, int line)
+{
+	char *ptr, *buffer = *bufferp, *pcnt, *bcnt;
+
+	if (buffer[0] != '[')
+		return false;
+
+	/* we have counters in our input */
+
+	ptr = strchr(buffer, ']');
+	if (!ptr)
+		xtables_error(PARAMETER_PROBLEM, "Bad line %u: need ]\n", line);
+
+	pcnt = strtok(buffer+1, ":");
+	if (!pcnt)
+		xtables_error(PARAMETER_PROBLEM, "Bad line %u: need :\n", line);
+
+	bcnt = strtok(NULL, "]");
+	if (!bcnt)
+		xtables_error(PARAMETER_PROBLEM, "Bad line %u: need ]\n", line);
+
+	*pcntp = pcnt;
+	*bcntp = bcnt;
+	/* start command parsing after counter */
+	*bufferp = ptr + 1;
+
+	return true;
+}
+
 inline bool xs_has_arg(int argc, char *argv[])
 {
 	return optind < argc &&
diff --git a/iptables/xshared.h b/iptables/xshared.h
index b08c700e1d8b9..21f4e8fdee67c 100644
--- a/iptables/xshared.h
+++ b/iptables/xshared.h
@@ -151,6 +151,7 @@ extern int xtables_lock_or_exit(int wait, struct timeval *tv);
 int parse_wait_time(int argc, char *argv[]);
 void parse_wait_interval(int argc, char *argv[], struct timeval *wait_interval);
 int parse_counters(const char *string, struct xt_counters *ctr);
+bool tokenize_rule_counters(char **bufferp, char **pcnt, char **bcnt, int line);
 bool xs_has_arg(int argc, char *argv[]);
 
 extern const struct xtables_afinfo *afinfo;
diff --git a/iptables/xtables-restore.c b/iptables/xtables-restore.c
index 24bfce516d34c..4652d631d2219 100644
--- a/iptables/xtables-restore.c
+++ b/iptables/xtables-restore.c
@@ -230,47 +230,20 @@ void xtables_restore_parse(struct nft_handle *h,
 			int a;
 			char *pcnt = NULL;
 			char *bcnt = NULL;
-			char *parsestart;
+			char *parsestart = buffer;
 
 			/* reset the newargv */
 			newargc = 0;
 
-			if (buffer[0] == '[') {
-				/* we have counters in our input */
-				char *ptr = strchr(buffer, ']');
-
-				if (!ptr)
-					xtables_error(PARAMETER_PROBLEM,
-						   "Bad line %u: need ]\n",
-						   line);
-
-				pcnt = strtok(buffer+1, ":");
-				if (!pcnt)
-					xtables_error(PARAMETER_PROBLEM,
-						   "Bad line %u: need :\n",
-						   line);
-
-				bcnt = strtok(NULL, "]");
-				if (!bcnt)
-					xtables_error(PARAMETER_PROBLEM,
-						   "Bad line %u: need ]\n",
-						   line);
-
-				/* start command parsing after counter */
-				parsestart = ptr + 1;
-			} else {
-				/* start command parsing at start of line */
-				parsestart = buffer;
-			}
-
 			add_argv(xt_params->program_name, 0);
 			add_argv("-t", 0);
 			add_argv(curtable->name, 0);
 
+			tokenize_rule_counters(&parsestart, &pcnt, &bcnt, line);
 			if (counters && pcnt && bcnt) {
 				add_argv("--set-counters", 0);
-				add_argv((char *) pcnt, 0);
-				add_argv((char *) bcnt, 0);
+				add_argv(pcnt, 0);
+				add_argv(bcnt, 0);
 			}
 
 			add_param_to_argv(parsestart, line);
-- 
2.23.0

Re: [iptables PATCH 3/8] xtables-restore: Introduce rule counter tokenizer function

[Pablo Neira Ayuso]

On Fri, Oct 18, 2019 at 12:48:31AM +0200, Phil Sutter wrote:
> The same piece of code appears three times, introduce a function to take
> care of tokenizing and error reporting.
> 
> Pass buffer pointer via reference so it can be updated to point to after
> the counters (if found).
> 
> While being at it, drop pointless casting when passing pcnt/bcnt to
> add_argv().
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>

Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>

If you get to consolidate more common code between xml and native
parsers, probably you can add a xtables-restore.c file to store all
these functions are common, just an idea for the future.

Re: [iptables PATCH 3/8] xtables-restore: Introduce rule counter tokenizer function

[Phil Sutter]

Hi Pablo,

On Fri, Oct 18, 2019 at 10:11:24AM +0200, Pablo Neira Ayuso wrote:
> On Fri, Oct 18, 2019 at 12:48:31AM +0200, Phil Sutter wrote:
> > The same piece of code appears three times, introduce a function to take
> > care of tokenizing and error reporting.
> > 
> > Pass buffer pointer via reference so it can be updated to point to after
> > the counters (if found).
> > 
> > While being at it, drop pointless casting when passing pcnt/bcnt to
> > add_argv().
> > 
> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> 
> Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>
> 
> If you get to consolidate more common code between xml and native
> parsers, probably you can add a xtables-restore.c file to store all
> these functions are common, just an idea for the future.

I get the point, but we have xtables-restore.c already. Though it
contains *tables-nft-restore code. I would add to xshared.c until we
decide it's large enough to split (currently ~750 lines). AFAICT, this
is the only source file included in both xtables-*-multi binaries. Other
than that, I could extend libxtables to really share code but it's
probably not worth it.

Cheers, Phil

Re: [iptables PATCH 3/8] xtables-restore: Introduce rule counter tokenizer function

[Pablo Neira Ayuso]

On Fri, Oct 18, 2019 at 11:50:54AM +0200, Phil Sutter wrote:
> Hi Pablo,
> 
> On Fri, Oct 18, 2019 at 10:11:24AM +0200, Pablo Neira Ayuso wrote:
> > On Fri, Oct 18, 2019 at 12:48:31AM +0200, Phil Sutter wrote:
> > > The same piece of code appears three times, introduce a function to take
> > > care of tokenizing and error reporting.
> > > 
> > > Pass buffer pointer via reference so it can be updated to point to after
> > > the counters (if found).
> > > 
> > > While being at it, drop pointless casting when passing pcnt/bcnt to
> > > add_argv().
> > > 
> > > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > 
> > Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > 
> > If you get to consolidate more common code between xml and native
> > parsers, probably you can add a xtables-restore.c file to store all
> > these functions are common, just an idea for the future.
> 
> I get the point, but we have xtables-restore.c already. Though it
> contains *tables-nft-restore code. I would add to xshared.c until we
> decide it's large enough to split (currently ~750 lines). AFAICT, this
> is the only source file included in both xtables-*-multi binaries. Other
> than that, I could extend libxtables to really share code but it's
> probably not worth it.

OK. No libxtables for this, I'd suggest. This is a library and we
would need to deal with libversion if you expose this API, xshared.c
is fine, I was just wondering if splitting it to encapsulate all
shared parsing code in another file could be useful at some point.

[iptables PATCH 4/8] xtables-restore: Constify struct nft_xt_restore_cb

[Phil Sutter]

There is no need for dynamic callback mangling, so make all instances
static const.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft-shared.h        | 2 +-
 iptables/xtables-restore.c   | 8 ++++----
 iptables/xtables-translate.c | 2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/iptables/nft-shared.h b/iptables/nft-shared.h
index 5c6641505f3db..b062f3e5792e3 100644
--- a/iptables/nft-shared.h
+++ b/iptables/nft-shared.h
@@ -262,7 +262,7 @@ struct nft_xt_restore_cb {
 
 void xtables_restore_parse(struct nft_handle *h,
 			   const struct nft_xt_restore_parse *p,
-			   struct nft_xt_restore_cb *cb);
+			   const struct nft_xt_restore_cb *cb);
 
 void nft_check_xt_legacy(int family, bool is_ipt_save);
 #endif
diff --git a/iptables/xtables-restore.c b/iptables/xtables-restore.c
index 4652d631d2219..df8844208c273 100644
--- a/iptables/xtables-restore.c
+++ b/iptables/xtables-restore.c
@@ -70,7 +70,7 @@ static struct nftnl_chain_list *get_chain_list(struct nft_handle *h,
 	return chain_list;
 }
 
-struct nft_xt_restore_cb restore_cb = {
+static const struct nft_xt_restore_cb restore_cb = {
 	.chain_list	= get_chain_list,
 	.commit		= nft_commit,
 	.abort		= nft_abort,
@@ -87,7 +87,7 @@ static const struct xtc_ops xtc_ops = {
 
 void xtables_restore_parse(struct nft_handle *h,
 			   const struct nft_xt_restore_parse *p,
-			   struct nft_xt_restore_cb *cb)
+			   const struct nft_xt_restore_cb *cb)
 {
 	const struct builtin_table *curtable = NULL;
 	char buffer[10240];
@@ -432,7 +432,7 @@ static int ebt_table_flush(struct nft_handle *h, const char *table)
 	return nft_table_flush(h, table);
 }
 
-struct nft_xt_restore_cb ebt_restore_cb = {
+static const struct nft_xt_restore_cb ebt_restore_cb = {
 	.chain_list	= get_chain_list,
 	.commit		= nft_bridge_commit,
 	.table_new	= nft_table_new,
@@ -478,7 +478,7 @@ int xtables_eb_restore_main(int argc, char *argv[])
 	return 0;
 }
 
-struct nft_xt_restore_cb arp_restore_cb = {
+static const struct nft_xt_restore_cb arp_restore_cb = {
 	.chain_list	= get_chain_list,
 	.commit		= nft_commit,
 	.table_new	= nft_table_new,
diff --git a/iptables/xtables-translate.c b/iptables/xtables-translate.c
index 64e7667a253e7..43607901fc62b 100644
--- a/iptables/xtables-translate.c
+++ b/iptables/xtables-translate.c
@@ -413,7 +413,7 @@ static int dummy_compat_rev(const char *name, uint8_t rev, int opt)
 	return 1;
 }
 
-static struct nft_xt_restore_cb cb_xlate = {
+static const struct nft_xt_restore_cb cb_xlate = {
 	.table_new	= xlate_table_new,
 	.chain_set	= xlate_chain_set,
 	.chain_restore	= xlate_chain_user_restore,
-- 
2.23.0

Re: [iptables PATCH 4/8] xtables-restore: Constify struct nft_xt_restore_cb

[Pablo Neira Ayuso]

On Fri, Oct 18, 2019 at 12:48:32AM +0200, Phil Sutter wrote:
> There is no need for dynamic callback mangling, so make all instances
> static const.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>

Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>

[iptables PATCH 5/8] iptables-restore: Constify struct iptables_restore_cb

[Phil Sutter]

Just like with xtables-restore, these callbacks don't change at
run-time.

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

diff --git a/iptables/iptables-restore.c b/iptables/iptables-restore.c
index 3655b3e84637e..50d0708eff1f3 100644
--- a/iptables/iptables-restore.c
+++ b/iptables/iptables-restore.c
@@ -70,7 +70,7 @@ struct iptables_restore_cb {
 };
 
 static struct xtc_handle *
-create_handle(struct iptables_restore_cb *cb, const char *tablename)
+create_handle(const struct iptables_restore_cb *cb, const char *tablename)
 {
 	struct xtc_handle *handle;
 
@@ -90,7 +90,8 @@ create_handle(struct iptables_restore_cb *cb, const char *tablename)
 }
 
 static int
-ip46tables_restore_main(struct iptables_restore_cb *cb, int argc, char *argv[])
+ip46tables_restore_main(const struct iptables_restore_cb *cb,
+			int argc, char *argv[])
 {
 	struct xtc_handle *handle = NULL;
 	char buffer[10240];
@@ -360,7 +361,7 @@ ip46tables_restore_main(struct iptables_restore_cb *cb, int argc, char *argv[])
 
 
 #if defined ENABLE_IPV4
-struct iptables_restore_cb ipt_restore_cb = {
+static const struct iptables_restore_cb ipt_restore_cb = {
 	.ops		= &iptc_ops,
 	.for_each_chain	= for_each_chain4,
 	.flush_entries	= flush_entries4,
@@ -391,7 +392,7 @@ iptables_restore_main(int argc, char *argv[])
 #endif
 
 #if defined ENABLE_IPV6
-struct iptables_restore_cb ip6t_restore_cb = {
+static const struct iptables_restore_cb ip6t_restore_cb = {
 	.ops		= &ip6tc_ops,
 	.for_each_chain	= for_each_chain6,
 	.flush_entries	= flush_entries6,
-- 
2.23.0

Re: [iptables PATCH 5/8] iptables-restore: Constify struct iptables_restore_cb

[Pablo Neira Ayuso]

On Fri, Oct 18, 2019 at 12:48:33AM +0200, Phil Sutter wrote:
> Just like with xtables-restore, these callbacks don't change at
> run-time.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>

Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>

[iptables PATCH 6/8] xtables-restore: Drop pointless newargc reset

[Phil Sutter]

This was overlooked when merging argv-related code: newargc is
initialized at declaration and reset in free_argv() again.

Fixes: a2ed880a19d08 ("xshared: Consolidate argv construction routines")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/xtables-restore.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/iptables/xtables-restore.c b/iptables/xtables-restore.c
index df8844208c273..bb6ee78933f7a 100644
--- a/iptables/xtables-restore.c
+++ b/iptables/xtables-restore.c
@@ -232,9 +232,6 @@ void xtables_restore_parse(struct nft_handle *h,
 			char *bcnt = NULL;
 			char *parsestart = buffer;
 
-			/* reset the newargv */
-			newargc = 0;
-
 			add_argv(xt_params->program_name, 0);
 			add_argv("-t", 0);
 			add_argv(curtable->name, 0);
-- 
2.23.0

Re: [iptables PATCH 6/8] xtables-restore: Drop pointless newargc reset

[Pablo Neira Ayuso]

On Fri, Oct 18, 2019 at 12:48:34AM +0200, Phil Sutter wrote:
> This was overlooked when merging argv-related code: newargc is
> initialized at declaration and reset in free_argv() again.
> 
> Fixes: a2ed880a19d08 ("xshared: Consolidate argv construction routines")
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  iptables/xtables-restore.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/iptables/xtables-restore.c b/iptables/xtables-restore.c
> index df8844208c273..bb6ee78933f7a 100644
> --- a/iptables/xtables-restore.c
> +++ b/iptables/xtables-restore.c
> @@ -232,9 +232,6 @@ void xtables_restore_parse(struct nft_handle *h,
>  			char *bcnt = NULL;
>  			char *parsestart = buffer;
>  
> -			/* reset the newargv */
> -			newargc = 0;

Are you sure this is correct? This resets the variable for each table
this is entering.

BTW, newargv, newargc are defined as globals which is very hard to
follow when reading this code. Probably place them in a structure
definition and pass them to functions to make easier to follow track
of this code?

That code would qualify for placing it under
iptables/xtables-restore.c since it is common for the xml and the
native parser as I suggested before.

Re: [iptables PATCH 6/8] xtables-restore: Drop pointless newargc reset

[Phil Sutter]

Hi,

On Fri, Oct 18, 2019 at 10:30:56AM +0200, Pablo Neira Ayuso wrote:
> On Fri, Oct 18, 2019 at 12:48:34AM +0200, Phil Sutter wrote:
> > This was overlooked when merging argv-related code: newargc is
> > initialized at declaration and reset in free_argv() again.
> > 
> > Fixes: a2ed880a19d08 ("xshared: Consolidate argv construction routines")
> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > ---
> >  iptables/xtables-restore.c | 3 ---
> >  1 file changed, 3 deletions(-)
> > 
> > diff --git a/iptables/xtables-restore.c b/iptables/xtables-restore.c
> > index df8844208c273..bb6ee78933f7a 100644
> > --- a/iptables/xtables-restore.c
> > +++ b/iptables/xtables-restore.c
> > @@ -232,9 +232,6 @@ void xtables_restore_parse(struct nft_handle *h,
> >  			char *bcnt = NULL;
> >  			char *parsestart = buffer;
> >  
> > -			/* reset the newargv */
> > -			newargc = 0;
> 
> Are you sure this is correct? This resets the variable for each table
> this is entering.

In fact, the removed line resets newargc before parsing each rule line.
But since newargc is initially zero and after each call to do_command a
call to free_argv() happens which resets newargc again, we're really
save here.

> BTW, newargv, newargc are defined as globals which is very hard to
> follow when reading this code. Probably place them in a structure
> definition and pass them to functions to make easier to follow track
> of this code?

Good point, I'll do that.

> That code would qualify for placing it under
> iptables/xtables-restore.c since it is common for the xml and the
> native parser as I suggested before.

These global variables and related functions currently reside in
xshared.c which is the right spot. :)

Thanks, Phil

[iptables PATCH 7/8] xtables-restore: Drop local xtc_ops instance

[Phil Sutter]

It is merely used to hold nft_strerror() pointer but using that function
in turn does not provide any benefit as it falls back to plain
strerror() if nft_fn is not initialized.

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

diff --git a/iptables/xtables-restore.c b/iptables/xtables-restore.c
index bb6ee78933f7a..900e476eaf968 100644
--- a/iptables/xtables-restore.c
+++ b/iptables/xtables-restore.c
@@ -81,10 +81,6 @@ static const struct nft_xt_restore_cb restore_cb = {
 	.chain_restore  = nft_chain_restore,
 };
 
-static const struct xtc_ops xtc_ops = {
-	.strerror	= nft_strerror,
-};
-
 void xtables_restore_parse(struct nft_handle *h,
 			   const struct nft_xt_restore_parse *p,
 			   const struct nft_xt_restore_cb *cb)
@@ -92,7 +88,6 @@ void xtables_restore_parse(struct nft_handle *h,
 	const struct builtin_table *curtable = NULL;
 	char buffer[10240];
 	int in_table = 0;
-	const struct xtc_ops *ops = &xtc_ops;
 
 	line = 0;
 
@@ -206,7 +201,7 @@ void xtables_restore_parse(struct nft_handle *h,
 						      "Can't set policy `%s'"
 						      " on `%s' line %u: %s\n",
 						      policy, chain, line,
-						      ops->strerror(errno));
+						      strerror(errno));
 				}
 				DEBUGP("Setting policy of chain %s to %s\n",
 				       chain, policy);
@@ -223,7 +218,7 @@ void xtables_restore_parse(struct nft_handle *h,
 					      "Can't set policy `%s'"
 					      " on `%s' line %u: %s\n",
 					      policy, chain, line,
-					      ops->strerror(errno));
+					      strerror(errno));
 			}
 			ret = 1;
 		} else if (in_table) {
-- 
2.23.0

Re: [iptables PATCH 7/8] xtables-restore: Drop local xtc_ops instance

[Pablo Neira Ayuso]

On Fri, Oct 18, 2019 at 12:48:35AM +0200, Phil Sutter wrote:
> It is merely used to hold nft_strerror() pointer but using that function
> in turn does not provide any benefit as it falls back to plain
> strerror() if nft_fn is not initialized.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>

Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>

[iptables PATCH 8/8] xtables-restore: Drop chain_list callback

[Phil Sutter]

Since commit 0baa08fed43fa ("xtables: unify user chain add/flush for
restore case") it is not used anymore, so just drop it.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft-shared.h      |  2 --
 iptables/xtables-restore.c | 15 ---------------
 2 files changed, 17 deletions(-)

diff --git a/iptables/nft-shared.h b/iptables/nft-shared.h
index b062f3e5792e3..8b073b18fb0d9 100644
--- a/iptables/nft-shared.h
+++ b/iptables/nft-shared.h
@@ -243,8 +243,6 @@ struct nftnl_chain_list;
 
 struct nft_xt_restore_cb {
 	void (*table_new)(struct nft_handle *h, const char *table);
-	struct nftnl_chain_list *(*chain_list)(struct nft_handle *h,
-					       const char *table);
 	int (*chain_set)(struct nft_handle *h, const char *table,
 			 const char *chain, const char *policy,
 			 const struct xt_counters *counters);
diff --git a/iptables/xtables-restore.c b/iptables/xtables-restore.c
index 900e476eaf968..18c824b3784ec 100644
--- a/iptables/xtables-restore.c
+++ b/iptables/xtables-restore.c
@@ -58,20 +58,7 @@ static void print_usage(const char *name, const char *version)
 			"	   [ --ipv6 ]\n", name);
 }
 
-static struct nftnl_chain_list *get_chain_list(struct nft_handle *h,
-					       const char *table)
-{
-	struct nftnl_chain_list *chain_list;
-
-	chain_list = nft_chain_list_get(h, table, NULL);
-	if (chain_list == NULL)
-		xtables_error(OTHER_PROBLEM, "cannot retrieve chain list\n");
-
-	return chain_list;
-}
-
 static const struct nft_xt_restore_cb restore_cb = {
-	.chain_list	= get_chain_list,
 	.commit		= nft_commit,
 	.abort		= nft_abort,
 	.table_new	= nft_table_new,
@@ -425,7 +412,6 @@ static int ebt_table_flush(struct nft_handle *h, const char *table)
 }
 
 static const struct nft_xt_restore_cb ebt_restore_cb = {
-	.chain_list	= get_chain_list,
 	.commit		= nft_bridge_commit,
 	.table_new	= nft_table_new,
 	.table_flush	= ebt_table_flush,
@@ -471,7 +457,6 @@ int xtables_eb_restore_main(int argc, char *argv[])
 }
 
 static const struct nft_xt_restore_cb arp_restore_cb = {
-	.chain_list	= get_chain_list,
 	.commit		= nft_commit,
 	.table_new	= nft_table_new,
 	.table_flush	= nft_table_flush,
-- 
2.23.0

Re: [iptables PATCH 8/8] xtables-restore: Drop chain_list callback

[Pablo Neira Ayuso]

On Fri, Oct 18, 2019 at 12:48:36AM +0200, Phil Sutter wrote:
> Since commit 0baa08fed43fa ("xtables: unify user chain add/flush for
> restore case") it is not used anymore, so just drop it.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>

Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>