[iptables PATCH v3 0/7] Improve xtables-restore performance

[iptables PATCH v3 0/7] Improve xtables-restore performance

[Phil Sutter]

This series speeds up xtables-restore calls with --noflush (typically
used to batch a few commands for faster execution) by preliminary input
inspection.

Before, setting --noflush flag would inevitably lead to full cache
population. With this series in place, if input can be fully buffered
and no commands requiring full cache is contained, no initial cache
population happens and each rule parsed will cause fetching of cache
bits as required.

The input buffer size is arbitrarily chosen to be 64KB.

Patches one and two prepare code for patch three which moves the loop
content parsing each line of input into a separate function. The
reduction of code indenting is used by patch four which deals with
needless line breaks.

Patch five deals with another requirement of input buffering, namely
stripping newline characters from each line. This is not a problem by
itself, but add_param_to_argv() replaces them by nul-chars and so
strings stop being consistently terminated (some by a single, some by
two nul-chars).

Patch six then finally adds the buffering and caching decision code.

Patch seven is pretty unrelated but tests a specific behaviour of
*tables-restore I wasn't sure of at first.

Phil Sutter (7):
  xtables-restore: Integrate restore callbacks into struct
    nft_xt_restore_parse
  xtables-restore: Introduce struct nft_xt_restore_state
  xtables-restore: Introduce line parsing function
  xtables-restore: Remove some pointless linebreaks
  xtables-restore: Allow lines without trailing newline character
  xtables-restore: Improve performance of --noflush operation
  tests: shell: Add ipt-restore/0007-flush-noflush_0

 iptables/nft-shared.h                         |  18 +-
 .../ipt-restore/0007-flush-noflush_0          |  42 ++
 iptables/xshared.c                            |   4 +
 iptables/xtables-restore.c                    | 443 +++++++++++-------
 iptables/xtables-translate.c                  |   6 +-
 5 files changed, 323 insertions(+), 190 deletions(-)
 create mode 100755 iptables/tests/shell/testcases/ipt-restore/0007-flush-noflush_0

-- 
2.23.0

[iptables PATCH v3 1/7] xtables-restore: Integrate restore callbacks into struct nft_xt_restore_parse

[Phil Sutter]

There's really no point in passing those as separate parameter. While
being at it, make them static const everywhere.

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

diff --git a/iptables/nft-shared.h b/iptables/nft-shared.h
index 8b073b18fb0d9..3ff7251f9f7ec 100644
--- a/iptables/nft-shared.h
+++ b/iptables/nft-shared.h
@@ -232,13 +232,6 @@ void do_parse(struct nft_handle *h, int argc, char *argv[],
 	      struct nft_xt_cmd_parse *p, struct iptables_command_state *cs,
 	      struct xtables_args *args);
 
-struct nft_xt_restore_parse {
-	FILE		*in;
-	int		testing;
-	const char	*tablename;
-	bool		commit;
-};
-
 struct nftnl_chain_list;
 
 struct nft_xt_restore_cb {
@@ -258,9 +251,16 @@ struct nft_xt_restore_cb {
 	int (*abort)(struct nft_handle *h);
 };
 
+struct nft_xt_restore_parse {
+	FILE				*in;
+	int				testing;
+	const char			*tablename;
+	bool				commit;
+	const struct nft_xt_restore_cb	*cb;
+};
+
 void xtables_restore_parse(struct nft_handle *h,
-			   const struct nft_xt_restore_parse *p,
-			   const struct nft_xt_restore_cb *cb);
+			   const struct nft_xt_restore_parse *p);
 
 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 8d6cb7a97ea37..341579bd8d1c3 100644
--- a/iptables/xtables-restore.c
+++ b/iptables/xtables-restore.c
@@ -69,10 +69,10 @@ static const struct nft_xt_restore_cb restore_cb = {
 };
 
 void xtables_restore_parse(struct nft_handle *h,
-			   const struct nft_xt_restore_parse *p,
-			   const struct nft_xt_restore_cb *cb)
+			   const struct nft_xt_restore_parse *p)
 {
 	const struct builtin_table *curtable = NULL;
+	const struct nft_xt_restore_cb *cb = p->cb;
 	struct argv_store av_store = {};
 	char buffer[10240];
 	int in_table = 0;
@@ -279,6 +279,7 @@ xtables_restore_main(int family, const char *progname, int argc, char *argv[])
 	int c;
 	struct nft_xt_restore_parse p = {
 		.commit = true,
+		.cb = &restore_cb,
 	};
 
 	line = 0;
@@ -383,7 +384,7 @@ xtables_restore_main(int family, const char *progname, int argc, char *argv[])
 		exit(EXIT_FAILURE);
 	}
 
-	xtables_restore_parse(&h, &p, &restore_cb);
+	xtables_restore_parse(&h, &p);
 
 	nft_fini(&h);
 	fclose(p.in);
@@ -427,6 +428,7 @@ int xtables_eb_restore_main(int argc, char *argv[])
 {
 	struct nft_xt_restore_parse p = {
 		.in = stdin,
+		.cb = &ebt_restore_cb,
 	};
 	bool noflush = false;
 	struct nft_handle h;
@@ -448,7 +450,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);
+	xtables_restore_parse(&h, &p);
 	nft_fini(&h);
 
 	return 0;
@@ -467,11 +469,12 @@ int xtables_arp_restore_main(int argc, char *argv[])
 {
 	struct nft_xt_restore_parse p = {
 		.in = stdin,
+		.cb = &arp_restore_cb,
 	};
 	struct nft_handle h;
 
 	nft_init_arp(&h, "arptables-restore");
-	xtables_restore_parse(&h, &p, &arp_restore_cb);
+	xtables_restore_parse(&h, &p);
 	nft_fini(&h);
 
 	return 0;
diff --git a/iptables/xtables-translate.c b/iptables/xtables-translate.c
index 43607901fc62b..a42c60a3b64c6 100644
--- a/iptables/xtables-translate.c
+++ b/iptables/xtables-translate.c
@@ -498,7 +498,9 @@ static int xtables_restore_xlate_main(int family, const char *progname,
 		.family = family,
 	};
 	const char *file = NULL;
-	struct nft_xt_restore_parse p = {};
+	struct nft_xt_restore_parse p = {
+		.cb = &cb_xlate,
+	};
 	time_t now = time(NULL);
 	int c;
 
@@ -535,7 +537,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);
+	xtables_restore_parse(&h, &p);
 	printf("# Completed on %s", ctime(&now));
 
 	nft_fini(&h);
-- 
2.23.0

[iptables PATCH v3 2/7] xtables-restore: Introduce struct nft_xt_restore_state

[Phil Sutter]

This data structure holds parser state information. A follow-up patch
will extract line parsing code into a separate function which will need
a place to persistently store this info in between calls.

While being at it, make 'in_table' variable boolean and drop some extra
braces in conditionals checking its value.

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

diff --git a/iptables/xtables-restore.c b/iptables/xtables-restore.c
index 341579bd8d1c3..a9cb4ea55ab8f 100644
--- a/iptables/xtables-restore.c
+++ b/iptables/xtables-restore.c
@@ -68,14 +68,18 @@ static const struct nft_xt_restore_cb restore_cb = {
 	.chain_restore  = nft_chain_restore,
 };
 
+struct nft_xt_restore_state {
+	const struct builtin_table *curtable;
+	struct argv_store av_store;
+	bool in_table;
+};
+
 void xtables_restore_parse(struct nft_handle *h,
 			   const struct nft_xt_restore_parse *p)
 {
-	const struct builtin_table *curtable = NULL;
 	const struct nft_xt_restore_cb *cb = p->cb;
-	struct argv_store av_store = {};
+	struct nft_xt_restore_state state = {};
 	char buffer[10240];
-	int in_table = 0;
 
 	line = 0;
 
@@ -97,7 +101,7 @@ void xtables_restore_parse(struct nft_handle *h,
 			if (verbose)
 				fputs(buffer, stdout);
 			continue;
-		} else if ((strcmp(buffer, "COMMIT\n") == 0) && (in_table)) {
+		} else if ((strcmp(buffer, "COMMIT\n") == 0) && state.in_table) {
 			if (!p->testing) {
 				/* Commit per table, although we support
 				 * global commit at once, stick by now to
@@ -111,9 +115,9 @@ void xtables_restore_parse(struct nft_handle *h,
 				if (cb->abort)
 					ret = cb->abort(h);
 			}
-			in_table = 0;
+			state.in_table = false;
 
-		} else if ((buffer[0] == '*') && (!in_table || !p->commit)) {
+		} else if ((buffer[0] == '*') && (!state.in_table || !p->commit)) {
 			/* New table */
 			char *table;
 
@@ -124,8 +128,8 @@ void xtables_restore_parse(struct nft_handle *h,
 					"%s: line %u table name invalid\n",
 					xt_params->program_name, line);
 
-			curtable = nft_table_builtin_find(h, table);
-			if (!curtable)
+			state.curtable = nft_table_builtin_find(h, table);
+			if (!state.curtable)
 				xtables_error(PARAMETER_PROBLEM,
 					"%s: line %u table name '%s' invalid\n",
 					xt_params->program_name, line, table);
@@ -141,12 +145,12 @@ void xtables_restore_parse(struct nft_handle *h,
 			}
 
 			ret = 1;
-			in_table = 1;
+			state.in_table = true;
 
 			if (cb->table_new)
 				cb->table_new(h, table);
 
-		} else if ((buffer[0] == ':') && (in_table)) {
+		} else if ((buffer[0] == ':') && state.in_table) {
 			/* New chain. */
 			char *policy, *chain = NULL;
 			struct xt_counters count = {};
@@ -171,7 +175,7 @@ void xtables_restore_parse(struct nft_handle *h,
 					   "%s: line %u policy invalid\n",
 					   xt_params->program_name, line);
 
-			if (nft_chain_builtin_find(curtable, chain)) {
+			if (nft_chain_builtin_find(state.curtable, chain)) {
 				if (counters) {
 					char *ctrs;
 					ctrs = strtok(NULL, " \t\n");
@@ -183,7 +187,7 @@ void xtables_restore_parse(struct nft_handle *h,
 
 				}
 				if (cb->chain_set &&
-				    cb->chain_set(h, curtable->name,
+				    cb->chain_set(h, state.curtable->name,
 					          chain, policy, &count) < 0) {
 					xtables_error(OTHER_PROBLEM,
 						      "Can't set policy `%s'"
@@ -193,14 +197,14 @@ void xtables_restore_parse(struct nft_handle *h,
 				}
 				DEBUGP("Setting policy of chain %s to %s\n",
 				       chain, policy);
-			} else if (cb->chain_restore(h, chain, curtable->name) < 0 &&
+			} else if (cb->chain_restore(h, chain, state.curtable->name) < 0 &&
 				   errno != EEXIST) {
 				xtables_error(PARAMETER_PROBLEM,
 					      "cannot create chain "
 					      "'%s' (%s)\n", chain,
 					      strerror(errno));
 			} else if (h->family == NFPROTO_BRIDGE &&
-				   !ebt_set_user_chain_policy(h, curtable->name,
+				   !ebt_set_user_chain_policy(h, state.curtable->name,
 							      chain, policy)) {
 				xtables_error(OTHER_PROBLEM,
 					      "Can't set policy `%s'"
@@ -209,30 +213,30 @@ void xtables_restore_parse(struct nft_handle *h,
 					      strerror(errno));
 			}
 			ret = 1;
-		} else if (in_table) {
+		} else if (state.in_table) {
 			char *pcnt = NULL;
 			char *bcnt = NULL;
 			char *parsestart = buffer;
 
-			add_argv(&av_store, xt_params->program_name, 0);
-			add_argv(&av_store, "-t", 0);
-			add_argv(&av_store, curtable->name, 0);
+			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);
 
 			tokenize_rule_counters(&parsestart, &pcnt, &bcnt, line);
 			if (counters && pcnt && bcnt) {
-				add_argv(&av_store, "--set-counters", 0);
-				add_argv(&av_store, pcnt, 0);
-				add_argv(&av_store, bcnt, 0);
+				add_argv(&state.av_store, "--set-counters", 0);
+				add_argv(&state.av_store, pcnt, 0);
+				add_argv(&state.av_store, bcnt, 0);
 			}
 
-			add_param_to_argv(&av_store, parsestart, line);
+			add_param_to_argv(&state.av_store, parsestart, line);
 
 			DEBUGP("calling do_command4(%u, argv, &%s, handle):\n",
-			       av_store.argc, curtable->name);
-			debug_print_argv(&av_store);
+			       state.av_store.argc, state.curtable->name);
+			debug_print_argv(&state.av_store);
 
-			ret = cb->do_command(h, av_store.argc, av_store.argv,
-					    &av_store.argv[2], true);
+			ret = cb->do_command(h, state.av_store.argc, state.av_store.argv,
+					    &state.av_store.argv[2], true);
 			if (ret < 0) {
 				if (cb->abort)
 					ret = cb->abort(h);
@@ -246,11 +250,11 @@ void xtables_restore_parse(struct nft_handle *h,
 				exit(1);
 			}
 
-			free_argv(&av_store);
+			free_argv(&state.av_store);
 			fflush(stdout);
 		}
-		if (p->tablename && curtable &&
-		    (strcmp(p->tablename, curtable->name) != 0))
+		if (p->tablename && state.curtable &&
+		    (strcmp(p->tablename, state.curtable->name) != 0))
 			continue;
 		if (!ret) {
 			fprintf(stderr, "%s: line %u failed\n",
@@ -258,11 +262,11 @@ void xtables_restore_parse(struct nft_handle *h,
 			exit(1);
 		}
 	}
-	if (in_table && p->commit) {
+	if (state.in_table && p->commit) {
 		fprintf(stderr, "%s: COMMIT expected at line %u\n",
 				xt_params->program_name, line + 1);
 		exit(1);
-	} else if (in_table && cb->commit && !cb->commit(h)) {
+	} else if (state.in_table && cb->commit && !cb->commit(h)) {
 		xtables_error(OTHER_PROBLEM, "%s: final implicit COMMIT failed",
 			      xt_params->program_name);
 	}
-- 
2.23.0

[iptables PATCH v3 3/7] xtables-restore: Introduce line parsing function

[Phil Sutter]

Move the loop code parsing a distinct line of input into a dedicated
function as a preparation for changing input sources. Since loop code
either calls continue or exit() directly, there is no need for a return
code to indicate failure.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
Despite the diff's size, this is actually a small change: Pass
--ignore-space-change to git to see how few changes there actually are
besides the necessary indenting level change.
---
 iptables/xtables-restore.c | 347 +++++++++++++++++++------------------
 1 file changed, 177 insertions(+), 170 deletions(-)

diff --git a/iptables/xtables-restore.c b/iptables/xtables-restore.c
index a9cb4ea55ab8f..d21e9730805a8 100644
--- a/iptables/xtables-restore.c
+++ b/iptables/xtables-restore.c
@@ -74,199 +74,206 @@ struct nft_xt_restore_state {
 	bool in_table;
 };
 
-void xtables_restore_parse(struct nft_handle *h,
-			   const struct nft_xt_restore_parse *p)
+static void xtables_restore_parse_line(struct nft_handle *h,
+				       const struct nft_xt_restore_parse *p,
+				       struct nft_xt_restore_state *state,
+				       char *buffer)
 {
 	const struct nft_xt_restore_cb *cb = p->cb;
-	struct nft_xt_restore_state state = {};
-	char buffer[10240];
-
-	line = 0;
+	int ret = 0;
+
+	if (buffer[0] == '\n')
+		return;
+	else if (buffer[0] == '#') {
+		if (verbose)
+			fputs(buffer, stdout);
+		return;
+	} else if ((strcmp(buffer, "COMMIT\n") == 0) && state->in_table) {
+		if (!p->testing) {
+			/* Commit per table, although we support
+			 * global commit at once, stick by now to
+			 * the existing behaviour.
+			 */
+			DEBUGP("Calling commit\n");
+			if (cb->commit)
+				ret = cb->commit(h);
+		} else {
+			DEBUGP("Not calling commit, testing\n");
+			if (cb->abort)
+				ret = cb->abort(h);
+		}
+		state->in_table = false;
+
+	} else if ((buffer[0] == '*') && (!state->in_table || !p->commit)) {
+		/* New table */
+		char *table;
+
+		table = strtok(buffer+1, " \t\n");
+		DEBUGP("line %u, table '%s'\n", line, table);
+		if (!table)
+			xtables_error(PARAMETER_PROBLEM,
+				"%s: line %u table name invalid\n",
+				xt_params->program_name, line);
+
+		state->curtable = nft_table_builtin_find(h, table);
+		if (!state->curtable)
+			xtables_error(PARAMETER_PROBLEM,
+				"%s: line %u table name '%s' invalid\n",
+				xt_params->program_name, line, table);
+
+		if (p->tablename && (strcmp(p->tablename, table) != 0))
+			return;
+
+		if (h->noflush == 0) {
+			DEBUGP("Cleaning all chains of table '%s'\n",
+				table);
+			if (cb->table_flush)
+				cb->table_flush(h, table);
+		}
 
-	if (!h->noflush)
-		nft_fake_cache(h);
-	else
-		nft_build_cache(h, NULL);
+		ret = 1;
+		state->in_table = true;
+
+		if (cb->table_new)
+			cb->table_new(h, table);
+
+	} else if ((buffer[0] == ':') && state->in_table) {
+		/* New chain. */
+		char *policy, *chain = NULL;
+		struct xt_counters count = {};
+
+		chain = strtok(buffer+1, " \t\n");
+		DEBUGP("line %u, chain '%s'\n", line, chain);
+		if (!chain)
+			xtables_error(PARAMETER_PROBLEM,
+				   "%s: line %u chain name invalid\n",
+				   xt_params->program_name, line);
+
+		if (strlen(chain) >= XT_EXTENSION_MAXNAMELEN)
+			xtables_error(PARAMETER_PROBLEM,
+				   "Invalid chain name `%s' "
+				   "(%u chars max)",
+				   chain, XT_EXTENSION_MAXNAMELEN - 1);
+
+		policy = strtok(NULL, " \t\n");
+		DEBUGP("line %u, policy '%s'\n", line, policy);
+		if (!policy)
+			xtables_error(PARAMETER_PROBLEM,
+				   "%s: line %u policy invalid\n",
+				   xt_params->program_name, line);
+
+		if (nft_chain_builtin_find(state->curtable, chain)) {
+			if (counters) {
+				char *ctrs;
+				ctrs = strtok(NULL, " \t\n");
+
+				if (!ctrs || !parse_counters(ctrs, &count))
+					xtables_error(PARAMETER_PROBLEM,
+						   "invalid policy counters "
+						   "for chain '%s'\n", chain);
 
-	/* Grab standard input. */
-	while (fgets(buffer, sizeof(buffer), p->in)) {
-		int ret = 0;
-
-		line++;
-		h->error.lineno = line;
-
-		if (buffer[0] == '\n')
-			continue;
-		else if (buffer[0] == '#') {
-			if (verbose)
-				fputs(buffer, stdout);
-			continue;
-		} else if ((strcmp(buffer, "COMMIT\n") == 0) && state.in_table) {
-			if (!p->testing) {
-				/* Commit per table, although we support
-				 * global commit at once, stick by now to
-				 * the existing behaviour.
-				 */
-				DEBUGP("Calling commit\n");
-				if (cb->commit)
-					ret = cb->commit(h);
-			} else {
-				DEBUGP("Not calling commit, testing\n");
-				if (cb->abort)
-					ret = cb->abort(h);
-			}
-			state.in_table = false;
-
-		} else if ((buffer[0] == '*') && (!state.in_table || !p->commit)) {
-			/* New table */
-			char *table;
-
-			table = strtok(buffer+1, " \t\n");
-			DEBUGP("line %u, table '%s'\n", line, table);
-			if (!table)
-				xtables_error(PARAMETER_PROBLEM,
-					"%s: line %u table name invalid\n",
-					xt_params->program_name, line);
-
-			state.curtable = nft_table_builtin_find(h, table);
-			if (!state.curtable)
-				xtables_error(PARAMETER_PROBLEM,
-					"%s: line %u table name '%s' invalid\n",
-					xt_params->program_name, line, table);
-
-			if (p->tablename && (strcmp(p->tablename, table) != 0))
-				continue;
-
-			if (h->noflush == 0) {
-				DEBUGP("Cleaning all chains of table '%s'\n",
-					table);
-				if (cb->table_flush)
-					cb->table_flush(h, table);
 			}
-
-			ret = 1;
-			state.in_table = true;
-
-			if (cb->table_new)
-				cb->table_new(h, table);
-
-		} else if ((buffer[0] == ':') && state.in_table) {
-			/* New chain. */
-			char *policy, *chain = NULL;
-			struct xt_counters count = {};
-
-			chain = strtok(buffer+1, " \t\n");
-			DEBUGP("line %u, chain '%s'\n", line, chain);
-			if (!chain)
-				xtables_error(PARAMETER_PROBLEM,
-					   "%s: line %u chain name invalid\n",
-					   xt_params->program_name, line);
-
-			if (strlen(chain) >= XT_EXTENSION_MAXNAMELEN)
-				xtables_error(PARAMETER_PROBLEM,
-					   "Invalid chain name `%s' "
-					   "(%u chars max)",
-					   chain, XT_EXTENSION_MAXNAMELEN - 1);
-
-			policy = strtok(NULL, " \t\n");
-			DEBUGP("line %u, policy '%s'\n", line, policy);
-			if (!policy)
-				xtables_error(PARAMETER_PROBLEM,
-					   "%s: line %u policy invalid\n",
-					   xt_params->program_name, line);
-
-			if (nft_chain_builtin_find(state.curtable, chain)) {
-				if (counters) {
-					char *ctrs;
-					ctrs = strtok(NULL, " \t\n");
-
-					if (!ctrs || !parse_counters(ctrs, &count))
-						xtables_error(PARAMETER_PROBLEM,
-							   "invalid policy counters "
-							   "for chain '%s'\n", chain);
-
-				}
-				if (cb->chain_set &&
-				    cb->chain_set(h, state.curtable->name,
-					          chain, policy, &count) < 0) {
-					xtables_error(OTHER_PROBLEM,
-						      "Can't set policy `%s'"
-						      " on `%s' line %u: %s\n",
-						      policy, chain, line,
-						      strerror(errno));
-				}
-				DEBUGP("Setting policy of chain %s to %s\n",
-				       chain, policy);
-			} else if (cb->chain_restore(h, chain, state.curtable->name) < 0 &&
-				   errno != EEXIST) {
-				xtables_error(PARAMETER_PROBLEM,
-					      "cannot create chain "
-					      "'%s' (%s)\n", chain,
-					      strerror(errno));
-			} else if (h->family == NFPROTO_BRIDGE &&
-				   !ebt_set_user_chain_policy(h, state.curtable->name,
-							      chain, policy)) {
+			if (cb->chain_set &&
+			    cb->chain_set(h, state->curtable->name,
+					  chain, policy, &count) < 0) {
 				xtables_error(OTHER_PROBLEM,
 					      "Can't set policy `%s'"
 					      " on `%s' line %u: %s\n",
 					      policy, chain, line,
 					      strerror(errno));
 			}
-			ret = 1;
-		} else if (state.in_table) {
-			char *pcnt = NULL;
-			char *bcnt = NULL;
-			char *parsestart = buffer;
-
-			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);
-
-			tokenize_rule_counters(&parsestart, &pcnt, &bcnt, line);
-			if (counters && pcnt && bcnt) {
-				add_argv(&state.av_store, "--set-counters", 0);
-				add_argv(&state.av_store, pcnt, 0);
-				add_argv(&state.av_store, bcnt, 0);
-			}
+			DEBUGP("Setting policy of chain %s to %s\n",
+			       chain, policy);
+		} else if (cb->chain_restore(h, chain, state->curtable->name) < 0 &&
+			   errno != EEXIST) {
+			xtables_error(PARAMETER_PROBLEM,
+				      "cannot create chain "
+				      "'%s' (%s)\n", chain,
+				      strerror(errno));
+		} else if (h->family == NFPROTO_BRIDGE &&
+			   !ebt_set_user_chain_policy(h, state->curtable->name,
+						      chain, policy)) {
+			xtables_error(OTHER_PROBLEM,
+				      "Can't set policy `%s'"
+				      " on `%s' line %u: %s\n",
+				      policy, chain, line,
+				      strerror(errno));
+		}
+		ret = 1;
+	} else if (state->in_table) {
+		char *pcnt = NULL;
+		char *bcnt = NULL;
+		char *parsestart = buffer;
+
+		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);
+
+		tokenize_rule_counters(&parsestart, &pcnt, &bcnt, line);
+		if (counters && pcnt && bcnt) {
+			add_argv(&state->av_store, "--set-counters", 0);
+			add_argv(&state->av_store, pcnt, 0);
+			add_argv(&state->av_store, bcnt, 0);
+		}
+
+		add_param_to_argv(&state->av_store, parsestart, line);
 
-			add_param_to_argv(&state.av_store, parsestart, line);
+		DEBUGP("calling do_command4(%u, argv, &%s, handle):\n",
+		       state->av_store.argc, state->curtable->name);
+		debug_print_argv(&state->av_store);
 
-			DEBUGP("calling do_command4(%u, argv, &%s, handle):\n",
-			       state.av_store.argc, state.curtable->name);
-			debug_print_argv(&state.av_store);
+		ret = cb->do_command(h, state->av_store.argc,
+				     state->av_store.argv,
+				     &state->av_store.argv[2], true);
+		if (ret < 0) {
+			if (cb->abort)
+				ret = cb->abort(h);
+			else
+				ret = 0;
 
-			ret = cb->do_command(h, state.av_store.argc, state.av_store.argv,
-					    &state.av_store.argv[2], true);
 			if (ret < 0) {
-				if (cb->abort)
-					ret = cb->abort(h);
-				else
-					ret = 0;
-
-				if (ret < 0) {
-					fprintf(stderr, "failed to abort "
-							"commit operation\n");
-				}
-				exit(1);
+				fprintf(stderr, "failed to abort "
+						"commit operation\n");
 			}
-
-			free_argv(&state.av_store);
-			fflush(stdout);
-		}
-		if (p->tablename && state.curtable &&
-		    (strcmp(p->tablename, state.curtable->name) != 0))
-			continue;
-		if (!ret) {
-			fprintf(stderr, "%s: line %u failed\n",
-					xt_params->program_name, line);
 			exit(1);
 		}
+
+		free_argv(&state->av_store);
+		fflush(stdout);
+	}
+	if (p->tablename && state->curtable &&
+	    (strcmp(p->tablename, state->curtable->name) != 0))
+		return;
+	if (!ret) {
+		fprintf(stderr, "%s: line %u failed\n",
+				xt_params->program_name, line);
+		exit(1);
+	}
+}
+
+void xtables_restore_parse(struct nft_handle *h,
+			   const struct nft_xt_restore_parse *p)
+{
+	struct nft_xt_restore_state state = {};
+	char buffer[10240];
+
+	line = 0;
+
+	if (!h->noflush)
+		nft_fake_cache(h);
+	else
+		nft_build_cache(h, NULL);
+
+	/* Grab standard input. */
+	while (fgets(buffer, sizeof(buffer), p->in)) {
+		h->error.lineno = ++line;
+		xtables_restore_parse_line(h, p, &state, buffer);
 	}
 	if (state.in_table && p->commit) {
 		fprintf(stderr, "%s: COMMIT expected at line %u\n",
 				xt_params->program_name, line + 1);
 		exit(1);
-	} else if (state.in_table && cb->commit && !cb->commit(h)) {
+	} else if (state.in_table && p->cb->commit && !p->cb->commit(h)) {
 		xtables_error(OTHER_PROBLEM, "%s: final implicit COMMIT failed",
 			      xt_params->program_name);
 	}
-- 
2.23.0

[iptables PATCH v3 4/7] xtables-restore: Remove some pointless linebreaks

[Phil Sutter]

Due to reduced indenting level, some linebreaks are no longer needed.
OTOH, strings should not be split to aid in grepping for error output.

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

diff --git a/iptables/xtables-restore.c b/iptables/xtables-restore.c
index d21e9730805a8..83e05102769a7 100644
--- a/iptables/xtables-restore.c
+++ b/iptables/xtables-restore.c
@@ -125,8 +125,7 @@ static void xtables_restore_parse_line(struct nft_handle *h,
 			return;
 
 		if (h->noflush == 0) {
-			DEBUGP("Cleaning all chains of table '%s'\n",
-				table);
+			DEBUGP("Cleaning all chains of table '%s'\n", table);
 			if (cb->table_flush)
 				cb->table_flush(h, table);
 		}
@@ -151,8 +150,7 @@ static void xtables_restore_parse_line(struct nft_handle *h,
 
 		if (strlen(chain) >= XT_EXTENSION_MAXNAMELEN)
 			xtables_error(PARAMETER_PROBLEM,
-				   "Invalid chain name `%s' "
-				   "(%u chars max)",
+				   "Invalid chain name `%s' (%u chars max)",
 				   chain, XT_EXTENSION_MAXNAMELEN - 1);
 
 		policy = strtok(NULL, " \t\n");
@@ -169,16 +167,15 @@ static void xtables_restore_parse_line(struct nft_handle *h,
 
 				if (!ctrs || !parse_counters(ctrs, &count))
 					xtables_error(PARAMETER_PROBLEM,
-						   "invalid policy counters "
-						   "for chain '%s'\n", chain);
+						   "invalid policy counters for chain '%s'\n",
+						   chain);
 
 			}
 			if (cb->chain_set &&
 			    cb->chain_set(h, state->curtable->name,
 					  chain, policy, &count) < 0) {
 				xtables_error(OTHER_PROBLEM,
-					      "Can't set policy `%s'"
-					      " on `%s' line %u: %s\n",
+					      "Can't set policy `%s' on `%s' line %u: %s\n",
 					      policy, chain, line,
 					      strerror(errno));
 			}
@@ -187,15 +184,13 @@ static void xtables_restore_parse_line(struct nft_handle *h,
 		} else if (cb->chain_restore(h, chain, state->curtable->name) < 0 &&
 			   errno != EEXIST) {
 			xtables_error(PARAMETER_PROBLEM,
-				      "cannot create chain "
-				      "'%s' (%s)\n", chain,
-				      strerror(errno));
+				      "cannot create chain '%s' (%s)\n",
+				      chain, strerror(errno));
 		} else if (h->family == NFPROTO_BRIDGE &&
 			   !ebt_set_user_chain_policy(h, state->curtable->name,
 						      chain, policy)) {
 			xtables_error(OTHER_PROBLEM,
-				      "Can't set policy `%s'"
-				      " on `%s' line %u: %s\n",
+				      "Can't set policy `%s' on `%s' line %u: %s\n",
 				      policy, chain, line,
 				      strerror(errno));
 		}
@@ -232,8 +227,8 @@ static void xtables_restore_parse_line(struct nft_handle *h,
 				ret = 0;
 
 			if (ret < 0) {
-				fprintf(stderr, "failed to abort "
-						"commit operation\n");
+				fprintf(stderr,
+					"failed to abort commit operation\n");
 			}
 			exit(1);
 		}
-- 
2.23.0

[iptables PATCH v3 5/7] xtables-restore: Allow lines without trailing newline character

[Phil Sutter]

Old code in add_param_to_argv() assumed the input line would always end
with a newline character. Without it, the last word of input wasn't
recognized. Fix this by adding a final check for param.len (indicating
leftover data in buffer).

In line parsing code itself, only COMMIT line check required presence of
trailing newline. The replaced conditional is not 100% accurate as it
allows for characters after newline to be present, but since fgets() is
used this shouldn't happen anyway.

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

diff --git a/iptables/xshared.c b/iptables/xshared.c
index 97f1b5d22fdbe..681a8fd314f86 100644
--- a/iptables/xshared.c
+++ b/iptables/xshared.c
@@ -530,6 +530,10 @@ void add_param_to_argv(struct argv_store *store, char *parsestart, int line)
 		param.len = 0;
 		quoted = 0;
 	}
+	if (param.len) {
+		param.buffer[param.len] = '\0';
+		add_argv(store, param.buffer, 0);
+	}
 }
 
 #ifdef DEBUG
diff --git a/iptables/xtables-restore.c b/iptables/xtables-restore.c
index 83e05102769a7..5a534ca227379 100644
--- a/iptables/xtables-restore.c
+++ b/iptables/xtables-restore.c
@@ -88,7 +88,9 @@ static void xtables_restore_parse_line(struct nft_handle *h,
 		if (verbose)
 			fputs(buffer, stdout);
 		return;
-	} else if ((strcmp(buffer, "COMMIT\n") == 0) && state->in_table) {
+	} else if (state->in_table &&
+		   (strncmp(buffer, "COMMIT", 6) == 0) &&
+		   (buffer[6] == '\0' || buffer[6] == '\n')) {
 		if (!p->testing) {
 			/* Commit per table, although we support
 			 * global commit at once, stick by now to
-- 
2.23.0

[iptables PATCH v3 6/7] xtables-restore: Improve performance of --noflush operation

[Phil Sutter]

The reason for that full cache fetching when called with --noflush even
before looking at any input data was that there might be a command
requiring a rule cache following some rule add/insert ones which don't.
At that point one needs to fetch rules from kernel and try to insert the
local ones at the right spot which is non-trivial.

At the same time there is a performance-critical use-case for --noflush,
namely fast insertion of a bunch of rules in one go, avoiding the
process spawn overhead.

Optimize for this use-case by preloading input into a 64KB buffer to see
if it fits. If so, search for commands requiring a rule cache. If there
are none, skip initial full cache fetching.

The above algorithm may abort at any point, so actual input parsing must
happen in three stages:

1) parse all preloaded lines from 64KB buffer
2) parse any leftover line in line buffer (happens if input exceeds
   the preload buffer size)
3) parse remaining input from input file pointer

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

diff --git a/iptables/xtables-restore.c b/iptables/xtables-restore.c
index 5a534ca227379..120b444f4b8a5 100644
--- a/iptables/xtables-restore.c
+++ b/iptables/xtables-restore.c
@@ -248,22 +248,96 @@ static void xtables_restore_parse_line(struct nft_handle *h,
 	}
 }
 
+/* Return true if given iptables-restore line will require a full cache.
+ * Typically these are commands referring to an existing rule
+ * (either by number or content) or commands listing the ruleset. */
+static bool cmd_needs_full_cache(char *cmd)
+{
+	char c, chain[32];
+	int rulenum, mcount;
+
+	mcount = sscanf(cmd, "-%c %31s %d", &c, chain, &rulenum);
+
+	if (mcount == 3)
+		return true;
+	if (mcount < 1)
+		return false;
+
+	switch (c) {
+	case 'D':
+	case 'C':
+	case 'S':
+	case 'L':
+		return true;
+	}
+
+	return false;
+}
+
+#define PREBUFSIZ	65536
+
 void xtables_restore_parse(struct nft_handle *h,
 			   const struct nft_xt_restore_parse *p)
 {
 	struct nft_xt_restore_state state = {};
-	char buffer[10240];
+	char preload_buffer[PREBUFSIZ] = {}, buffer[10240], *ptr;
 
-	line = 0;
-
-	if (!h->noflush)
+	if (!h->noflush) {
 		nft_fake_cache(h);
-	else
-		nft_build_cache(h, NULL);
+	} else {
+		ssize_t pblen = sizeof(preload_buffer);
+		bool do_cache = false;
+
+		ptr = preload_buffer;
+		while (fgets(buffer, sizeof(buffer), p->in)) {
+			size_t blen = strlen(buffer);
+
+			/* drop trailing newline; add_param_to_argv() changes
+			 * it into nul, causing unpredictable string delimiting
+			 * in preload_buffer */
+			if (buffer[blen - 1] == '\n')
+				buffer[blen - 1] = '\0';
+			else
+				blen++;
+
+			pblen -= blen;
+			if (pblen <= 0) {
+				/* buffer exhausted */
+				do_cache = true;
+				break;
+			}
 
-	/* Grab standard input. */
+			if (cmd_needs_full_cache(buffer)) {
+				do_cache = true;
+				break;
+			}
+
+			/* copy string including terminating nul-char */
+			memcpy(ptr, buffer, blen);
+			ptr += blen;
+			buffer[0] = '\0';
+		}
+
+		if (do_cache)
+			nft_build_cache(h, NULL);
+	}
+
+	line = 0;
+	ptr = preload_buffer;
+	while (*ptr) {
+		h->error.lineno = ++line;
+		DEBUGP("%s: buffered line %d: '%s'\n", __func__, line, ptr);
+		xtables_restore_parse_line(h, p, &state, ptr);
+		ptr += strlen(ptr) + 1;
+	}
+	if (*buffer) {
+		h->error.lineno = ++line;
+		DEBUGP("%s: overrun line %d: '%s'\n", __func__, line, buffer);
+		xtables_restore_parse_line(h, p, &state, buffer);
+	}
 	while (fgets(buffer, sizeof(buffer), p->in)) {
 		h->error.lineno = ++line;
+		DEBUGP("%s: input line %d: '%s'\n", __func__, line, buffer);
 		xtables_restore_parse_line(h, p, &state, buffer);
 	}
 	if (state.in_table && p->commit) {
-- 
2.23.0

[iptables PATCH v3 7/7] tests: shell: Add ipt-restore/0007-flush-noflush_0

[Phil Sutter]

Simple test to make sure iptables-restore does not touch tables it is
not supposed to.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 .../ipt-restore/0007-flush-noflush_0          | 42 +++++++++++++++++++
 1 file changed, 42 insertions(+)
 create mode 100755 iptables/tests/shell/testcases/ipt-restore/0007-flush-noflush_0

diff --git a/iptables/tests/shell/testcases/ipt-restore/0007-flush-noflush_0 b/iptables/tests/shell/testcases/ipt-restore/0007-flush-noflush_0
new file mode 100755
index 0000000000000..029db2235b9a4
--- /dev/null
+++ b/iptables/tests/shell/testcases/ipt-restore/0007-flush-noflush_0
@@ -0,0 +1,42 @@
+#!/bin/bash
+
+# Make sure iptables-restore without --noflush does not flush tables other than
+# those contained in the dump it's reading from
+
+set -e
+
+$XT_MULTI iptables-restore <<EOF
+*nat
+-A POSTROUTING -j ACCEPT
+COMMIT
+EOF
+
+EXPECT="*nat
+:PREROUTING ACCEPT [0:0]
+:INPUT ACCEPT [0:0]
+:OUTPUT ACCEPT [0:0]
+:POSTROUTING ACCEPT [0:0]
+-A POSTROUTING -j ACCEPT
+COMMIT"
+diff -u -Z <(echo -e "$EXPECT" | sort) <($XT_MULTI iptables-save | grep -v '^#' | sort)
+
+$XT_MULTI iptables-restore <<EOF
+*filter
+-A FORWARD -j ACCEPT
+COMMIT
+EOF
+
+EXPECT="*filter
+:INPUT ACCEPT [0:0]
+:FORWARD ACCEPT [0:0]
+:OUTPUT ACCEPT [0:0]
+-A FORWARD -j ACCEPT
+COMMIT
+*nat
+:PREROUTING ACCEPT [0:0]
+:INPUT ACCEPT [0:0]
+:OUTPUT ACCEPT [0:0]
+:POSTROUTING ACCEPT [0:0]
+-A POSTROUTING -j ACCEPT
+COMMIT"
+diff -u -Z <(echo -e "$EXPECT" | sort) <($XT_MULTI iptables-save | grep -v '^#' | sort)
-- 
2.23.0

Re: [iptables PATCH v3 0/7] Improve xtables-restore performance

[Pablo Neira Ayuso]

On Thu, Oct 24, 2019 at 06:37:05PM +0200, Phil Sutter wrote:
> This series speeds up xtables-restore calls with --noflush (typically
> used to batch a few commands for faster execution) by preliminary input
> inspection.
> 
> Before, setting --noflush flag would inevitably lead to full cache
> population. With this series in place, if input can be fully buffered
> and no commands requiring full cache is contained, no initial cache
> population happens and each rule parsed will cause fetching of cache
> bits as required.
> 
> The input buffer size is arbitrarily chosen to be 64KB.
> 
> Patches one and two prepare code for patch three which moves the loop
> content parsing each line of input into a separate function. The
> reduction of code indenting is used by patch four which deals with
> needless line breaks.

For patches from 1 to 4 in this batch:

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

> Patch five deals with another requirement of input buffering, namely
> stripping newline characters from each line. This is not a problem by
> itself, but add_param_to_argv() replaces them by nul-chars and so
> strings stop being consistently terminated (some by a single, some by
> two nul-chars).
> 
> Patch six then finally adds the buffering and caching decision code.
> 
> Patch seven is pretty unrelated but tests a specific behaviour of
> *tables-restore I wasn't sure of at first.

Do you have any number?

Re: [iptables PATCH v3 0/7] Improve xtables-restore performance

[Phil Sutter]

On Thu, Oct 31, 2019 at 04:02:34PM +0100, Pablo Neira Ayuso wrote:
> On Thu, Oct 24, 2019 at 06:37:05PM +0200, Phil Sutter wrote:
> > This series speeds up xtables-restore calls with --noflush (typically
> > used to batch a few commands for faster execution) by preliminary input
> > inspection.
> > 
> > Before, setting --noflush flag would inevitably lead to full cache
> > population. With this series in place, if input can be fully buffered
> > and no commands requiring full cache is contained, no initial cache
> > population happens and each rule parsed will cause fetching of cache
> > bits as required.
> > 
> > The input buffer size is arbitrarily chosen to be 64KB.
> > 
> > Patches one and two prepare code for patch three which moves the loop
> > content parsing each line of input into a separate function. The
> > reduction of code indenting is used by patch four which deals with
> > needless line breaks.
> 
> For patches from 1 to 4 in this batch:
> 
> Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>
> 
> > Patch five deals with another requirement of input buffering, namely
> > stripping newline characters from each line. This is not a problem by
> > itself, but add_param_to_argv() replaces them by nul-chars and so
> > strings stop being consistently terminated (some by a single, some by
> > two nul-chars).
> > 
> > Patch six then finally adds the buffering and caching decision code.
> > 
> > Patch seven is pretty unrelated but tests a specific behaviour of
> > *tables-restore I wasn't sure of at first.
> 
> Do you have any number?

Yes, I wrote a small benchmark based on some Kubernetes use-case. It
measures loading of dumps like:

| *nat
| :KUBE-SVC-23 - [0:0]
| :KUBE-SEP-23 - [0:0]
| -A KUBE-HOOK ! -s 10.128.0.0/14 -d 172.30.108.136/32 -p tcp -m comment --comment \"openshift-controller-manager/controller-manager:https cluster IP\" -m tcp --dport 443 -j KUBE-MARK-MASQ
| -A KUBE-HOOK -d 172.30.108.136/32 -p tcp -m comment --comment \"openshift-controller-manager/controller-manager:https cluster IP\" -m tcp --dport 443 -j KUBE-SVC-23
| -A KUBE-SVC-23 -j KUBE-SEP-23
| -A KUBE-SEP-23 -s 10.128.0.38/32 -j KUBE-MARK-MASQ
| -A KUBE-SEP-23 -p tcp -m tcp -j DNAT --to-destination 10.128.0.38:8443
| COMMIT

Into a ruleset with increasing size (created by repeating the snippet above):

size (*100) |	legacy     |   nft-pre	   |   nft-post
---------------------------------------------------------
1             .0040366426     .0079313714     .0025598650
10            .0146918664     .0459193868     .0025134858
25            .0361553334     .1195503778     .0024202904
50            .0699177362     .2547542626     .0024351612
75            .1062593206     .4078182120     .0024362044
100           .1614045514     .5636617378     .0024195190

The graph says it all: http://nwl.cc/~n0-1/kube.png

Cheers, Phil

Re: [iptables PATCH v3 0/7] Improve xtables-restore performance

[Pablo Neira Ayuso]

Hi Phil,

On Thu, Oct 31, 2019 at 06:19:47PM +0100, Phil Sutter wrote:
> On Thu, Oct 31, 2019 at 04:02:34PM +0100, Pablo Neira Ayuso wrote:
> > On Thu, Oct 24, 2019 at 06:37:05PM +0200, Phil Sutter wrote:
> > > This series speeds up xtables-restore calls with --noflush (typically
> > > used to batch a few commands for faster execution) by preliminary input
> > > inspection.
> > > 
> > > Before, setting --noflush flag would inevitably lead to full cache
> > > population. With this series in place, if input can be fully buffered
> > > and no commands requiring full cache is contained, no initial cache
> > > population happens and each rule parsed will cause fetching of cache
> > > bits as required.
> > > 
> > > The input buffer size is arbitrarily chosen to be 64KB.
> > > 
> > > Patches one and two prepare code for patch three which moves the loop
> > > content parsing each line of input into a separate function. The
> > > reduction of code indenting is used by patch four which deals with
> > > needless line breaks.
> > 
> > For patches from 1 to 4 in this batch:
> > 
> > Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > 
> > > Patch five deals with another requirement of input buffering, namely
> > > stripping newline characters from each line. This is not a problem by
> > > itself, but add_param_to_argv() replaces them by nul-chars and so
> > > strings stop being consistently terminated (some by a single, some by
> > > two nul-chars).
> > > 
> > > Patch six then finally adds the buffering and caching decision code.
> > > 
> > > Patch seven is pretty unrelated but tests a specific behaviour of
> > > *tables-restore I wasn't sure of at first.
> > 
> > Do you have any number?
> 
> Yes, I wrote a small benchmark based on some Kubernetes use-case. It
> measures loading of dumps like:
> 
> | *nat
> | :KUBE-SVC-23 - [0:0]
> | :KUBE-SEP-23 - [0:0]
> | -A KUBE-HOOK ! -s 10.128.0.0/14 -d 172.30.108.136/32 -p tcp -m comment --comment \"openshift-controller-manager/controller-manager:https cluster IP\" -m tcp --dport 443 -j KUBE-MARK-MASQ
> | -A KUBE-HOOK -d 172.30.108.136/32 -p tcp -m comment --comment \"openshift-controller-manager/controller-manager:https cluster IP\" -m tcp --dport 443 -j KUBE-SVC-23
> | -A KUBE-SVC-23 -j KUBE-SEP-23
> | -A KUBE-SEP-23 -s 10.128.0.38/32 -j KUBE-MARK-MASQ
> | -A KUBE-SEP-23 -p tcp -m tcp -j DNAT --to-destination 10.128.0.38:8443
> | COMMIT
> 
> Into a ruleset with increasing size (created by repeating the snippet above):
> 
> size (*100) |	legacy     |   nft-pre	   |   nft-post
> ---------------------------------------------------------
> 1             .0040366426     .0079313714     .0025598650
> 10            .0146918664     .0459193868     .0025134858
> 25            .0361553334     .1195503778     .0024202904
> 50            .0699177362     .2547542626     .0024351612
> 75            .1062593206     .4078182120     .0024362044
> 100           .1614045514     .5636617378     .0024195190

Thanks, this is nice.

I can see this function:

        static bool cmd_needs_full_cache(char *cmd)

the pre-parsing of the input to calculate the cache, which is good.

One thing: why do you need the conversion from \n to \0. The idea is
to read once from the file and keep it in a buffer, then pass it to
the original parsing function after this pre-parsing to calculate the
cache.

Please, add this to the remaining patches of this series.

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

Thanks.

Re: [iptables PATCH v3 0/7] Improve xtables-restore performance

[Phil Sutter]

Hi Pablo,

On Wed, Nov 06, 2019 at 10:24:52AM +0100, Pablo Neira Ayuso wrote:
[...]
> One thing: why do you need the conversion from \n to \0. The idea is
> to read once from the file and keep it in a buffer, then pass it to
> the original parsing function after this pre-parsing to calculate the
> cache.

Excellent question! It took me quite a while to figure out why it is
necessary to drop the trailing newlines when buffering input: In
add_param_to_argv() I couldn't find what my comment described, yet when
I removed the newline character dropping code some shell tests started
failing.

The real reason is this: When reading a table or chain definition line,
xtables_restore_parse_line() uses strtok() to eliminate trailing
whitespace or newline characters. This in turn mangles input buffer,
replacing the newline chars by nul chars.

The above turns into a problem when xtables_restore_parse() then updates
the pointer to the next string in buffer by calling:

| ptr += strlen(ptr) + 1;

With double nul chars, 'ptr' will point at the second one and that
matches the loop exit condition so we'll lose the remaining buffered
lines.

I'll fix the comment before pushing the commits out.

> Please, add this to the remaining patches of this series.
> 
> Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>

Thanks, Phil