[iptables PATCH 00/12] Larger xtables-save review

[iptables PATCH 00/12] Larger xtables-save review

[Phil Sutter]

This series started as a fix to program names mentioned in *-save
outputs and ended in merging ebtables-save and arptables-save code into
xtables_save_main used by ip{6,}tables-nft-save.

The first patch is actually unrelated but was discovered when testing
counter output - depending on environment, ebtables-nft might segfault.

The second patch fixes option '-c' of ebtables-nft-save which enables
counter prefixes in dumped rules but failed to disable the classical
ebtables-style counters.

Patch three sorts program names quoted in output of any of the *-save
programs, patch four unifies the header/footer comments in the same. The
latter also drops the extra newline printed in ebtables- and
arptables-save output, so test scripts need adjustments beyond dropping
the new comment lines from output.

Patch five fixes the table compatibility check in ip{6,}tables-nft-save.

Patches six and eight to ten prepare for integrating arptables- and
ebtables-save into the xtables-save code.

Patch seven merely fixes a minor coding-style issue.

Patches eleven and twelve finally perform the actual merge.

Phil Sutter (12):
  ebtables: Fix error message for invalid parameters
  ebtables-save: Fix counter formatting
  xtables-save: Use argv[0] as program name
  xtables-save: Unify *-save header/footer comments
  xtables-save: Fix table compatibility check
  nft: Make nft_for_each_table() more versatile
  xtables-save: Avoid mixed code and declarations
  xtables-save: Pass optstring/longopts to xtables_save_main()
  xtables-save: Make COMMIT line optional
  xtables-save: Pass format flags to do_output()
  arptables-save: Merge into xtables_save_main()
  ebtables-save: Merge into xtables_save_main()

 iptables/nft-bridge.c                         |  39 +--
 iptables/nft.c                                |   6 +-
 iptables/nft.h                                |   2 +-
 .../arptables/0001-arptables-save-restore_0   |   7 +-
 .../0002-arptables-restore-defaults_0         |   6 +-
 .../arptables/0003-arptables-verbose-output_0 |   5 +-
 .../ebtables/0002-ebtables-save-restore_0     |   4 +-
 .../ebtables/0003-ebtables-restore-defaults_0 |   6 +-
 .../testcases/ebtables/0004-save-counters_0   |  64 +++++
 iptables/xtables-eb.c                         |   4 +-
 iptables/xtables-save.c                       | 242 ++++--------------
 11 files changed, 146 insertions(+), 239 deletions(-)
 create mode 100755 iptables/tests/shell/testcases/ebtables/0004-save-counters_0

-- 
2.22.0

[iptables PATCH 01/12] ebtables: Fix error message for invalid parameters

[Phil Sutter]

With empty ruleset, ebtables-nft would report the wrong argv:

| % sudo ./install/sbin/ebtables-nft -vnL
| ebtables v1.8.3 (nf_tables): Unknown argument: './install/sbin/ebtables-nft'
| Try `ebtables -h' or 'ebtables --help' for more information.

After a (successful) call to 'ebtables-nft -L', this would even
segfault:

| % sudo ./install/sbin/ebtables-nft -vnL
| zsh: segmentation fault  sudo ./install/sbin/ebtables-nft -vnL

Fixes: acde6be32036f ("ebtables-translate: Fix segfault while parsing extension options")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/xtables-eb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/iptables/xtables-eb.c b/iptables/xtables-eb.c
index 171f41b0f616e..b8d89ad974a42 100644
--- a/iptables/xtables-eb.c
+++ b/iptables/xtables-eb.c
@@ -1180,7 +1180,7 @@ print_zero:
 			if (ebt_command_default(&cs))
 				xtables_error(PARAMETER_PROBLEM,
 					      "Unknown argument: '%s'",
-					      argv[optind - 1]);
+					      argv[optind]);
 
 			if (command != 'A' && command != 'I' &&
 			    command != 'D' && command != 'C')
-- 
2.22.0

[iptables PATCH 02/12] ebtables-save: Fix counter formatting

[Phil Sutter]

The initial problem was 'ebtables-save -c' printing iptables-style
counters but at the same time not disabling ebtables-style counter
output (which was even printed in wrong format for ebtables-save).

The code around counter output was complicated enough to motivate a
larger rework:

* Make FMT_C_COUNTS indicate the appended counter style for ebtables.

* Use FMT_EBT_SAVE to distinguish between '-c' style counters and the
  legacy pcnt/bcnt ones.

Consequently, ebtables-save sets format to:

FMT_NOCOUNTS			- for no counters
FMT_EBT_SAVE			- for iptables-style counters
FMT_EBT_SAVE | FMT_C_COUNTS	- for '-c' style counters

For regular ebtables, list_rules() always sets FMT_C_COUNTS
(iptables-style counters are never used there) and FMT_NOCOUNTS if no
counters are requested.

The big plus is if neither FMT_NOCOUNTS nor FMT_C_COUNTS is set,
iptables-style counters are to be printed - both in iptables and
ebtables. This allows to drop the ebtables-specific 'save_counters'
callback.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft-bridge.c                         | 39 +++--------
 .../testcases/ebtables/0004-save-counters_0   | 67 +++++++++++++++++++
 iptables/xtables-eb.c                         |  2 +-
 iptables/xtables-save.c                       |  3 +-
 4 files changed, 81 insertions(+), 30 deletions(-)
 create mode 100755 iptables/tests/shell/testcases/ebtables/0004-save-counters_0

diff --git a/iptables/nft-bridge.c b/iptables/nft-bridge.c
index ddfbee165da93..2e4b309b86135 100644
--- a/iptables/nft-bridge.c
+++ b/iptables/nft-bridge.c
@@ -21,8 +21,6 @@
 #include "nft-bridge.h"
 #include "nft.h"
 
-static bool ebt_legacy_counter_fmt;
-
 void ebt_cs_clean(struct iptables_command_state *cs)
 {
 	struct ebt_match *m, *nm;
@@ -422,22 +420,6 @@ static void print_protocol(uint16_t ethproto, bool invert, unsigned int bitmask)
 		printf("%s ", ent->e_name);
 }
 
-static void nft_bridge_save_counters(const void *data)
-{
-	const char *ctr;
-
-	if (ebt_legacy_counter_fmt)
-		return;
-
-	ctr = getenv("EBTABLES_SAVE_COUNTER");
-	if (ctr) {
-		ebt_legacy_counter_fmt = true;
-		return;
-	}
-
-	save_counters(data);
-}
-
 static void nft_bridge_save_rule(const void *data, unsigned int format)
 {
 	const struct iptables_command_state *cs = data;
@@ -474,15 +456,16 @@ static void nft_bridge_save_rule(const void *data, unsigned int format)
 		cs->target->print(&cs->fw, cs->target->t, format & FMT_NUMERIC);
 	}
 
-	if (format & FMT_EBT_SAVE)
-		printf(" -c %"PRIu64" %"PRIu64"",
-		       (uint64_t)cs->counters.pcnt,
-		       (uint64_t)cs->counters.bcnt);
-
-	if (!(format & FMT_NOCOUNTS))
-		printf(" , pcnt = %"PRIu64" -- bcnt = %"PRIu64"",
-		       (uint64_t)cs->counters.pcnt,
-		       (uint64_t)cs->counters.bcnt);
+	if ((format & (FMT_NOCOUNTS | FMT_C_COUNTS)) == FMT_C_COUNTS) {
+		if (format & FMT_EBT_SAVE)
+			printf(" -c %"PRIu64" %"PRIu64"",
+			       (uint64_t)cs->counters.pcnt,
+			       (uint64_t)cs->counters.bcnt);
+		else
+			printf(" , pcnt = %"PRIu64" -- bcnt = %"PRIu64"",
+			       (uint64_t)cs->counters.pcnt,
+			       (uint64_t)cs->counters.bcnt);
+	}
 
 	if (!(format & FMT_NONEWLINE))
 		fputc('\n', stdout);
@@ -763,7 +746,7 @@ struct nft_family_ops nft_family_ops_bridge = {
 	.print_header		= nft_bridge_print_header,
 	.print_rule		= nft_bridge_print_rule,
 	.save_rule		= nft_bridge_save_rule,
-	.save_counters		= nft_bridge_save_counters,
+	.save_counters		= save_counters,
 	.save_chain		= nft_bridge_save_chain,
 	.post_parse		= NULL,
 	.rule_to_cs		= nft_rule_to_ebtables_command_state,
diff --git a/iptables/tests/shell/testcases/ebtables/0004-save-counters_0 b/iptables/tests/shell/testcases/ebtables/0004-save-counters_0
new file mode 100755
index 0000000000000..8348dc7ee231f
--- /dev/null
+++ b/iptables/tests/shell/testcases/ebtables/0004-save-counters_0
@@ -0,0 +1,67 @@
+#!/bin/bash
+
+set -e
+
+# there is no legacy backend to test
+[[ $XT_MULTI == */xtables-nft-multi ]] || { echo "skip $XT_MULTI"; exit 0; }
+
+$XT_MULTI ebtables --init-table
+$XT_MULTI ebtables -A FORWARD -i nodev123 -o nodev432 -j ACCEPT
+$XT_MULTI ebtables -A FORWARD -i nodev432 -o nodev123 -j ACCEPT
+
+EXPECT='Bridge table: filter
+
+Bridge chain: FORWARD, entries: 2, policy: ACCEPT
+-i nodev123 -o nodev432 -j ACCEPT
+-i nodev432 -o nodev123 -j ACCEPT'
+
+echo "ebtables -L FORWARD"
+diff -u <(echo -e "$EXPECT") <($XT_MULTI ebtables -L FORWARD)
+
+EXPECT='Bridge table: filter
+
+Bridge chain: FORWARD, entries: 2, policy: ACCEPT
+-i nodev123 -o nodev432 -j ACCEPT , pcnt = 0 -- bcnt = 0
+-i nodev432 -o nodev123 -j ACCEPT , pcnt = 0 -- bcnt = 0'
+
+echo "ebtables -L FORWARD --Lc"
+diff -u <(echo -e "$EXPECT") <($XT_MULTI ebtables -L FORWARD --Lc)
+
+EXPECT='*filter
+:INPUT ACCEPT
+:FORWARD ACCEPT
+:OUTPUT ACCEPT
+-A FORWARD -i nodev123 -o nodev432 -j ACCEPT
+-A FORWARD -i nodev432 -o nodev123 -j ACCEPT
+'
+
+echo "ebtables-save"
+diff -u <(echo -e "$EXPECT") <($XT_MULTI ebtables-save | grep -v '^#')
+
+EXPECT='*filter
+:INPUT ACCEPT
+:FORWARD ACCEPT
+:OUTPUT ACCEPT
+[0:0] -A FORWARD -i nodev123 -o nodev432 -j ACCEPT
+[0:0] -A FORWARD -i nodev432 -o nodev123 -j ACCEPT
+'
+
+echo "ebtables-save -c"
+diff -u <(echo -e "$EXPECT") <($XT_MULTI ebtables-save -c | grep -v '^#')
+
+export EBTABLES_SAVE_COUNTER=yes
+
+# -c flag overrides EBTABLES_SAVE_COUNTER variable
+echo "EBTABLES_SAVE_COUNTER=yes ebtables-save -c"
+diff -u <(echo -e "$EXPECT") <($XT_MULTI ebtables-save -c | grep -v '^#')
+
+EXPECT='*filter
+:INPUT ACCEPT
+:FORWARD ACCEPT
+:OUTPUT ACCEPT
+-A FORWARD -i nodev123 -o nodev432 -j ACCEPT -c 0 0
+-A FORWARD -i nodev432 -o nodev123 -j ACCEPT -c 0 0
+'
+
+echo "EBTABLES_SAVE_COUNTER=yes ebtables-save"
+diff -u <(echo -e "$EXPECT") <($XT_MULTI ebtables-save | grep -v '^#')
diff --git a/iptables/xtables-eb.c b/iptables/xtables-eb.c
index b8d89ad974a42..121ecbecd0b64 100644
--- a/iptables/xtables-eb.c
+++ b/iptables/xtables-eb.c
@@ -410,7 +410,7 @@ static int list_rules(struct nft_handle *h, const char *chain, const char *table
 {
 	unsigned int format;
 
-	format = FMT_OPTIONS;
+	format = FMT_OPTIONS | FMT_C_COUNTS;
 	if (verbose)
 		format |= FMT_VIA;
 
diff --git a/iptables/xtables-save.c b/iptables/xtables-save.c
index 98e004af4b1a4..3f389e8fdc234 100644
--- a/iptables/xtables-save.c
+++ b/iptables/xtables-save.c
@@ -273,7 +273,8 @@ static int __ebt_save(struct nft_handle *h, const char *tablename, bool counters
 	printf("*%s\n", tablename);
 
 	if (counters)
-		format = ebt_legacy_counter_format ? FMT_EBT_SAVE : 0;
+		format = FMT_EBT_SAVE |
+			(ebt_legacy_counter_format ? FMT_C_COUNTS : 0);
 
 	/* Dump out chain names first,
 	 * thereby preventing dependency conflicts */
-- 
2.22.0

[iptables PATCH 03/12] xtables-save: Use argv[0] as program name

[Phil Sutter]

Don't hard-code program names. This also fixes for bogus 'xtables-save'
name which is no longer used.

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

diff --git a/iptables/xtables-save.c b/iptables/xtables-save.c
index 3f389e8fdc234..491122f39bbb0 100644
--- a/iptables/xtables-save.c
+++ b/iptables/xtables-save.c
@@ -9,6 +9,7 @@
 #include "config.h"
 #include <getopt.h>
 #include <errno.h>
+#include <libgen.h>
 #include <stdio.h>
 #include <fcntl.h>
 #include <stdlib.h>
@@ -80,8 +81,8 @@ __do_output(struct nft_handle *h, const char *tablename, bool counters)
 
 	time_t now = time(NULL);
 
-	printf("# Generated by xtables-save v%s on %s",
-	       PACKAGE_VERSION, ctime(&now));
+	printf("# Generated by %s v%s on %s", prog_name,
+	       prog_vers, ctime(&now));
 	printf("*%s\n", tablename);
 
 	/* Dump out chain names first,
@@ -122,7 +123,7 @@ do_output(struct nft_handle *h, const char *tablename, bool counters)
  * rule
  */
 static int
-xtables_save_main(int family, const char *progname, int argc, char *argv[])
+xtables_save_main(int family, int argc, char *argv[])
 {
 	const struct builtin_table *tables;
 	const char *tablename = NULL;
@@ -133,7 +134,7 @@ xtables_save_main(int family, const char *progname, int argc, char *argv[])
 	FILE *file = NULL;
 	int ret, c;
 
-	xtables_globals.program_name = progname;
+	xtables_globals.program_name = basename(*argv);;
 	c = xtables_init_all(&xtables_globals, family);
 	if (c < 0) {
 		fprintf(stderr, "%s/%s Failed to initialize xtables\n",
@@ -237,12 +238,12 @@ xtables_save_main(int family, const char *progname, int argc, char *argv[])
 
 int xtables_ip4_save_main(int argc, char *argv[])
 {
-	return xtables_save_main(NFPROTO_IPV4, "iptables-save", argc, argv);
+	return xtables_save_main(NFPROTO_IPV4, argc, argv);
 }
 
 int xtables_ip6_save_main(int argc, char *argv[])
 {
-	return xtables_save_main(NFPROTO_IPV6, "ip6tables-save", argc, argv);
+	return xtables_save_main(NFPROTO_IPV6, argc, argv);
 }
 
 static int __ebt_save(struct nft_handle *h, const char *tablename, bool counters)
@@ -266,8 +267,8 @@ static int __ebt_save(struct nft_handle *h, const char *tablename, bool counters
 
 	if (first) {
 		now = time(NULL);
-		printf("# Generated by ebtables-save v%s on %s",
-		       PACKAGE_VERSION, ctime(&now));
+		printf("# Generated by %s v%s on %s", prog_name,
+		       prog_vers, ctime(&now));
 		first = false;
 	}
 	printf("*%s\n", tablename);
@@ -308,7 +309,7 @@ int xtables_eb_save_main(int argc_, char *argv_[])
 		}
 	}
 
-	xtables_globals.program_name = "ebtables-save";
+	xtables_globals.program_name = basename(*argv_);
 	c = xtables_init_all(&xtables_globals, h.family);
 	if (c < 0) {
 		fprintf(stderr, "%s/%s Failed to initialize xtables\n",
@@ -362,7 +363,7 @@ int xtables_arp_save_main(int argc, char **argv)
 	};
 	int c;
 
-	xtables_globals.program_name = "arptables-save";
+	xtables_globals.program_name = basename(*argv);;
 	c = xtables_init_all(&xtables_globals, h.family);
 	if (c < 0) {
 		fprintf(stderr, "%s/%s Failed to initialize xtables\n",
-- 
2.22.0

Re: [iptables PATCH 03/12] xtables-save: Use argv[0] as program name

[Phil Sutter]

On Sat, Jul 20, 2019 at 06:30:17PM +0200, Phil Sutter wrote:
> Don't hard-code program names. This also fixes for bogus 'xtables-save'
> name which is no longer used.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>

Argh, I should have pulled upstream first, this one was already
accepted. My series rebases cleanly, should I respin?

Sorry for the mess. :(

Re: [iptables PATCH 03/12] xtables-save: Use argv[0] as program name

[Pablo Neira Ayuso]

On Sat, Jul 20, 2019 at 06:52:04PM +0200, Phil Sutter wrote:
> On Sat, Jul 20, 2019 at 06:30:17PM +0200, Phil Sutter wrote:
> > Don't hard-code program names. This also fixes for bogus 'xtables-save'
> > name which is no longer used.
> > 
> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> 
> Argh, I should have pulled upstream first, this one was already
> accepted. My series rebases cleanly, should I respin?

Please, if not matching asking, please do so. Also addressing
Florian's feedback updating the comments, thanks.

I'll make a quick review tomorrow, sorry but I cannot get on these any
sooner.

[iptables PATCH 04/12] xtables-save: Unify *-save header/footer comments

[Phil Sutter]

Make eb- and arptables-save print both header and footer comments, too.
Also print them for each table separately - the timing information is
worth the extra lines in output.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 .../arptables/0001-arptables-save-restore_0   |  7 +++----
 .../0002-arptables-restore-defaults_0         |  6 ++----
 .../arptables/0003-arptables-verbose-output_0 |  5 ++---
 .../ebtables/0002-ebtables-save-restore_0     |  4 +---
 .../ebtables/0003-ebtables-restore-defaults_0 |  6 ++----
 .../testcases/ebtables/0004-save-counters_0   |  9 +++------
 iptables/xtables-save.c                       | 19 ++++++++++---------
 7 files changed, 23 insertions(+), 33 deletions(-)

diff --git a/iptables/tests/shell/testcases/arptables/0001-arptables-save-restore_0 b/iptables/tests/shell/testcases/arptables/0001-arptables-save-restore_0
index e10f61cc8f95b..bf04dc0a3e15a 100755
--- a/iptables/tests/shell/testcases/arptables/0001-arptables-save-restore_0
+++ b/iptables/tests/shell/testcases/arptables/0001-arptables-save-restore_0
@@ -50,13 +50,12 @@ DUMP='*filter
 -A foo -j MARK --set-mark 12345
 -A foo -j ACCEPT --opcode 1
 -A foo -j ACCEPT --proto-type 0x800
--A foo -j ACCEPT -i lo --opcode 1 --proto-type 0x800
-'
+-A foo -j ACCEPT -i lo --opcode 1 --proto-type 0x800'
 
-diff -u <(echo -e "$DUMP") <($XT_MULTI arptables-save)
+diff -u <(echo -e "$DUMP") <($XT_MULTI arptables-save | grep -v "^#")
 
 # make sure dump can be restored and check it didn't change
 
 $XT_MULTI arptables -F
 $XT_MULTI arptables-restore <<<$DUMP
-diff -u <(echo -e "$DUMP") <($XT_MULTI arptables-save)
+diff -u <(echo -e "$DUMP") <($XT_MULTI arptables-save | grep -v "^#")
diff --git a/iptables/tests/shell/testcases/arptables/0002-arptables-restore-defaults_0 b/iptables/tests/shell/testcases/arptables/0002-arptables-restore-defaults_0
index b2ed95e87bb40..38d387f327ebb 100755
--- a/iptables/tests/shell/testcases/arptables/0002-arptables-restore-defaults_0
+++ b/iptables/tests/shell/testcases/arptables/0002-arptables-restore-defaults_0
@@ -11,8 +11,7 @@ set -e
 DUMP='*filter
 :OUTPUT ACCEPT
 -A OUTPUT -j mangle --mangle-ip-s 10.0.0.1
--A OUTPUT -j mangle --mangle-ip-d 10.0.0.2
-'
+-A OUTPUT -j mangle --mangle-ip-d 10.0.0.2'
 
 # note how mangle-ip-s is unset in second rule
 
@@ -20,8 +19,7 @@ EXPECT='*filter
 :INPUT ACCEPT
 :OUTPUT ACCEPT
 -A OUTPUT -j mangle --mangle-ip-s 10.0.0.1
--A OUTPUT -j mangle --mangle-ip-d 10.0.0.2
-'
+-A OUTPUT -j mangle --mangle-ip-d 10.0.0.2'
 
 $XT_MULTI arptables -F
 $XT_MULTI arptables-restore <<<$DUMP
diff --git a/iptables/tests/shell/testcases/arptables/0003-arptables-verbose-output_0 b/iptables/tests/shell/testcases/arptables/0003-arptables-verbose-output_0
index 3a9807a1cfe0b..10c5ec33ada2c 100755
--- a/iptables/tests/shell/testcases/arptables/0003-arptables-verbose-output_0
+++ b/iptables/tests/shell/testcases/arptables/0003-arptables-verbose-output_0
@@ -58,7 +58,6 @@ EXPECT='*filter
 -A INPUT -j MARK -i eth23 --set-mark 42
 -A OUTPUT -j CLASSIFY -o eth23 --set-class 23:42
 -A OUTPUT -j foo -o eth23
--A foo -j mangle -o eth23 --mangle-ip-s 10.0.0.1
-'
+-A foo -j mangle -o eth23 --mangle-ip-s 10.0.0.1'
 
-diff -u -Z <(echo -e "$EXPECT") <($XT_MULTI arptables-save)
+diff -u -Z <(echo -e "$EXPECT") <($XT_MULTI arptables-save | grep -v '^#')
diff --git a/iptables/tests/shell/testcases/ebtables/0002-ebtables-save-restore_0 b/iptables/tests/shell/testcases/ebtables/0002-ebtables-save-restore_0
index 080ba49a4974d..e18d46551509d 100755
--- a/iptables/tests/shell/testcases/ebtables/0002-ebtables-save-restore_0
+++ b/iptables/tests/shell/testcases/ebtables/0002-ebtables-save-restore_0
@@ -99,7 +99,6 @@ DUMP='*filter
 -A foo --802_3-sap 0x23 --limit 100/sec --limit-burst 5 -j ACCEPT
 -A foo --pkttype-type multicast --log-level notice --log-prefix "" -j CONTINUE
 -A foo --pkttype-type multicast --limit 100/sec --limit-burst 5 -j ACCEPT
-
 *nat
 :PREROUTING ACCEPT
 :OUTPUT DROP
@@ -107,8 +106,7 @@ DUMP='*filter
 :nat_foo DROP
 -A PREROUTING -j redirect 
 -A OUTPUT -j ACCEPT
--A POSTROUTING -j ACCEPT
-'
+-A POSTROUTING -j ACCEPT'
 
 diff -u <(echo -e "$DUMP") <($XT_MULTI ebtables-save | grep -v '^#')
 
diff --git a/iptables/tests/shell/testcases/ebtables/0003-ebtables-restore-defaults_0 b/iptables/tests/shell/testcases/ebtables/0003-ebtables-restore-defaults_0
index c858054764d70..62d224134456b 100755
--- a/iptables/tests/shell/testcases/ebtables/0003-ebtables-restore-defaults_0
+++ b/iptables/tests/shell/testcases/ebtables/0003-ebtables-restore-defaults_0
@@ -13,8 +13,7 @@ DUMP='*filter
 -A FORWARD --limit 100 --limit-burst 42 -j ACCEPT
 -A FORWARD --limit 1000 -j ACCEPT
 -A FORWARD --log --log-prefix "foobar"
--A FORWARD --log
-'
+-A FORWARD --log'
 
 # note how limit-burst is 5 in second rule and log-prefix empty in fourth one
 
@@ -25,8 +24,7 @@ EXPECT='*filter
 -A FORWARD --limit 100/sec --limit-burst 42 -j ACCEPT
 -A FORWARD --limit 1000/sec --limit-burst 5 -j ACCEPT
 -A FORWARD --log-level notice --log-prefix "foobar" -j CONTINUE
--A FORWARD --log-level notice --log-prefix "" -j CONTINUE
-'
+-A FORWARD --log-level notice --log-prefix "" -j CONTINUE'
 
 $XT_MULTI ebtables --init-table
 $XT_MULTI ebtables-restore <<<$DUMP
diff --git a/iptables/tests/shell/testcases/ebtables/0004-save-counters_0 b/iptables/tests/shell/testcases/ebtables/0004-save-counters_0
index 8348dc7ee231f..46966f433139a 100755
--- a/iptables/tests/shell/testcases/ebtables/0004-save-counters_0
+++ b/iptables/tests/shell/testcases/ebtables/0004-save-counters_0
@@ -32,8 +32,7 @@ EXPECT='*filter
 :FORWARD ACCEPT
 :OUTPUT ACCEPT
 -A FORWARD -i nodev123 -o nodev432 -j ACCEPT
--A FORWARD -i nodev432 -o nodev123 -j ACCEPT
-'
+-A FORWARD -i nodev432 -o nodev123 -j ACCEPT'
 
 echo "ebtables-save"
 diff -u <(echo -e "$EXPECT") <($XT_MULTI ebtables-save | grep -v '^#')
@@ -43,8 +42,7 @@ EXPECT='*filter
 :FORWARD ACCEPT
 :OUTPUT ACCEPT
 [0:0] -A FORWARD -i nodev123 -o nodev432 -j ACCEPT
-[0:0] -A FORWARD -i nodev432 -o nodev123 -j ACCEPT
-'
+[0:0] -A FORWARD -i nodev432 -o nodev123 -j ACCEPT'
 
 echo "ebtables-save -c"
 diff -u <(echo -e "$EXPECT") <($XT_MULTI ebtables-save -c | grep -v '^#')
@@ -60,8 +58,7 @@ EXPECT='*filter
 :FORWARD ACCEPT
 :OUTPUT ACCEPT
 -A FORWARD -i nodev123 -o nodev432 -j ACCEPT -c 0 0
--A FORWARD -i nodev432 -o nodev123 -j ACCEPT -c 0 0
-'
+-A FORWARD -i nodev432 -o nodev123 -j ACCEPT -c 0 0'
 
 echo "EBTABLES_SAVE_COUNTER=yes ebtables-save"
 diff -u <(echo -e "$EXPECT") <($XT_MULTI ebtables-save | grep -v '^#')
diff --git a/iptables/xtables-save.c b/iptables/xtables-save.c
index 491122f39bbb0..0cf11f998cc77 100644
--- a/iptables/xtables-save.c
+++ b/iptables/xtables-save.c
@@ -250,7 +250,6 @@ static int __ebt_save(struct nft_handle *h, const char *tablename, bool counters
 {
 	struct nftnl_chain_list *chain_list;
 	unsigned int format = FMT_NOCOUNTS;
-	static bool first = true;
 	time_t now;
 
 	if (!nft_table_find(h, tablename)) {
@@ -265,12 +264,9 @@ static int __ebt_save(struct nft_handle *h, const char *tablename, bool counters
 
 	chain_list = nft_chain_list_get(h, tablename);
 
-	if (first) {
-		now = time(NULL);
-		printf("# Generated by %s v%s on %s", prog_name,
-		       prog_vers, ctime(&now));
-		first = false;
-	}
+	now = time(NULL);
+	printf("# Generated by %s v%s on %s", prog_name,
+	       prog_vers, ctime(&now));
 	printf("*%s\n", tablename);
 
 	if (counters)
@@ -281,7 +277,8 @@ static int __ebt_save(struct nft_handle *h, const char *tablename, bool counters
 	 * thereby preventing dependency conflicts */
 	nft_chain_save(h, chain_list);
 	nft_rule_save(h, tablename, format);
-	printf("\n");
+	now = time(NULL);
+	printf("# Completed on %s", ctime(&now));
 	return 0;
 }
 
@@ -361,6 +358,7 @@ int xtables_arp_save_main(int argc, char **argv)
 	struct nft_handle h = {
 		.family	= NFPROTO_ARP,
 	};
+	time_t now;
 	int c;
 
 	xtables_globals.program_name = basename(*argv);;
@@ -407,10 +405,13 @@ int xtables_arp_save_main(int argc, char **argv)
 		return 0;
 	}
 
+	printf("# Generated by %s v%s on %s", prog_name,
+	       prog_vers, ctime(&now));
 	printf("*filter\n");
 	nft_chain_save(&h, nft_chain_list_get(&h, "filter"));
 	nft_rule_save(&h, "filter", show_counters ? 0 : FMT_NOCOUNTS);
-	printf("\n");
+	now = time(NULL);
+	printf("# Completed on %s", ctime(&now));
 	nft_fini(&h);
 	return 0;
 }
-- 
2.22.0

[iptables PATCH 05/12] xtables-save: Fix table compatibility check

[Phil Sutter]

The builtin table check guarding the 'is incompatible' warning was
wrong: The idea was to print the warning only for incompatible tables
which are builtin, not for others. Yet the code would print the warning
only for non-builtin ones.

Also reorder the checks: nft_table_builtin_find() is fast and therefore
a quick way to bail for uninteresting tables. The compatibility check is
needed for the remaining tables, only.

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

diff --git a/iptables/xtables-save.c b/iptables/xtables-save.c
index 0cf11f998cc77..811ec6330a4cb 100644
--- a/iptables/xtables-save.c
+++ b/iptables/xtables-save.c
@@ -67,11 +67,12 @@ __do_output(struct nft_handle *h, const char *tablename, bool counters)
 {
 	struct nftnl_chain_list *chain_list;
 
+	if (!nft_table_builtin_find(h, tablename))
+		return 0;
 
 	if (!nft_is_table_compatible(h, tablename)) {
-		if (!nft_table_builtin_find(h, tablename))
-			printf("# Table `%s' is incompatible, use 'nft' tool.\n",
-			       tablename);
+		printf("# Table `%s' is incompatible, use 'nft' tool.\n",
+		       tablename);
 		return 0;
 	}
 
-- 
2.22.0

[iptables PATCH 06/12] nft: Make nft_for_each_table() more versatile

[Phil Sutter]

Support passing arbitrary data (via void pointer) to the callback.

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

diff --git a/iptables/nft.c b/iptables/nft.c
index 8f0d5e664eca6..cd42af70b54ef 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -2092,8 +2092,8 @@ err:
 }
 
 int nft_for_each_table(struct nft_handle *h,
-		       int (*func)(struct nft_handle *h, const char *tablename, bool counters),
-		       bool counters)
+		       int (*func)(struct nft_handle *h, const char *tablename, void *data),
+		       void *data)
 {
 	struct nftnl_table_list *list;
 	struct nftnl_table_list_iter *iter;
@@ -2112,7 +2112,7 @@ int nft_for_each_table(struct nft_handle *h,
 		const char *tablename =
 			nftnl_table_get(t, NFTNL_TABLE_NAME);
 
-		func(h, tablename, counters);
+		func(h, tablename, data);
 
 		t = nftnl_table_list_iter_next(iter);
 	}
diff --git a/iptables/nft.h b/iptables/nft.h
index dc1161840a38c..da078a44bab7b 100644
--- a/iptables/nft.h
+++ b/iptables/nft.h
@@ -81,7 +81,7 @@ void nft_build_cache(struct nft_handle *h);
 struct nftnl_table;
 struct nftnl_chain_list;
 
-int nft_for_each_table(struct nft_handle *h, int (*func)(struct nft_handle *h, const char *tablename, bool counters), bool counters);
+int nft_for_each_table(struct nft_handle *h, int (*func)(struct nft_handle *h, const char *tablename, void *data), void *data);
 bool nft_table_find(struct nft_handle *h, const char *tablename);
 int nft_table_purge_chains(struct nft_handle *h, const char *table, struct nftnl_chain_list *list);
 int nft_table_flush(struct nft_handle *h, const char *table);
diff --git a/iptables/xtables-save.c b/iptables/xtables-save.c
index 811ec6330a4cb..484450f03354f 100644
--- a/iptables/xtables-save.c
+++ b/iptables/xtables-save.c
@@ -62,10 +62,15 @@ static const struct option ebt_save_options[] = {
 
 static bool ebt_legacy_counter_format;
 
+struct do_output_data {
+	bool counters;
+};
+
 static int
-__do_output(struct nft_handle *h, const char *tablename, bool counters)
+__do_output(struct nft_handle *h, const char *tablename, void *data)
 {
 	struct nftnl_chain_list *chain_list;
+	struct do_output_data *d = data;
 
 	if (!nft_table_builtin_find(h, tablename))
 		return 0;
@@ -89,7 +94,7 @@ __do_output(struct nft_handle *h, const char *tablename, bool counters)
 	/* Dump out chain names first,
 	 * thereby preventing dependency conflicts */
 	nft_chain_save(h, chain_list);
-	nft_rule_save(h, tablename, counters ? 0 : FMT_NOCOUNTS);
+	nft_rule_save(h, tablename, d->counters ? 0 : FMT_NOCOUNTS);
 
 	now = time(NULL);
 	printf("COMMIT\n");
@@ -98,12 +103,12 @@ __do_output(struct nft_handle *h, const char *tablename, bool counters)
 }
 
 static int
-do_output(struct nft_handle *h, const char *tablename, bool counters)
+do_output(struct nft_handle *h, const char *tablename, struct do_output_data *d)
 {
 	int ret;
 
 	if (!tablename) {
-		ret = nft_for_each_table(h, __do_output, counters);
+		ret = nft_for_each_table(h, __do_output, d);
 		nft_check_xt_legacy(h->family, true);
 		return !!ret;
 	}
@@ -114,7 +119,7 @@ do_output(struct nft_handle *h, const char *tablename, bool counters)
 		return 1;
 	}
 
-	ret = __do_output(h, tablename, counters);
+	ret = __do_output(h, tablename, d);
 	nft_check_xt_legacy(h->family, true);
 	return ret;
 }
@@ -128,6 +133,7 @@ xtables_save_main(int family, int argc, char *argv[])
 {
 	const struct builtin_table *tables;
 	const char *tablename = NULL;
+	struct do_output_data d = {};
 	bool dump = false;
 	struct nft_handle h = {
 		.family	= family,
@@ -150,7 +156,7 @@ xtables_save_main(int family, int argc, char *argv[])
 			fprintf(stderr, "-b/--binary option is not implemented\n");
 			break;
 		case 'c':
-			show_counters = true;
+			d.counters = true;
 			break;
 
 		case 't':
@@ -229,7 +235,7 @@ xtables_save_main(int family, int argc, char *argv[])
 		exit(EXIT_FAILURE);
 	}
 
-	ret = do_output(&h, tablename, show_counters);
+	ret = do_output(&h, tablename, &d);
 	nft_fini(&h);
 	if (dump)
 		exit(0);
@@ -247,10 +253,11 @@ int xtables_ip6_save_main(int argc, char *argv[])
 	return xtables_save_main(NFPROTO_IPV6, argc, argv);
 }
 
-static int __ebt_save(struct nft_handle *h, const char *tablename, bool counters)
+static int __ebt_save(struct nft_handle *h, const char *tablename, void *data)
 {
 	struct nftnl_chain_list *chain_list;
 	unsigned int format = FMT_NOCOUNTS;
+	bool *counters = data;
 	time_t now;
 
 	if (!nft_table_find(h, tablename)) {
@@ -286,9 +293,9 @@ static int __ebt_save(struct nft_handle *h, const char *tablename, bool counters
 static int ebt_save(struct nft_handle *h, const char *tablename, bool counters)
 {
 	if (!tablename)
-		return nft_for_each_table(h, __ebt_save, counters);
+		return nft_for_each_table(h, __ebt_save, &counters);
 
-	return __ebt_save(h, tablename, counters);
+	return __ebt_save(h, tablename, &counters);
 }
 
 int xtables_eb_save_main(int argc_, char *argv_[])
-- 
2.22.0

[iptables PATCH 07/12] xtables-save: Avoid mixed code and declarations

[Phil Sutter]

Also move time() calls to where they are used.

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

diff --git a/iptables/xtables-save.c b/iptables/xtables-save.c
index 484450f03354f..ac452f1dd6f14 100644
--- a/iptables/xtables-save.c
+++ b/iptables/xtables-save.c
@@ -71,6 +71,7 @@ __do_output(struct nft_handle *h, const char *tablename, void *data)
 {
 	struct nftnl_chain_list *chain_list;
 	struct do_output_data *d = data;
+	time_t now;
 
 	if (!nft_table_builtin_find(h, tablename))
 		return 0;
@@ -85,19 +86,18 @@ __do_output(struct nft_handle *h, const char *tablename, void *data)
 	if (!chain_list)
 		return 0;
 
-	time_t now = time(NULL);
-
+	now = time(NULL);
 	printf("# Generated by %s v%s on %s", prog_name,
 	       prog_vers, ctime(&now));
-	printf("*%s\n", tablename);
 
+	printf("*%s\n", tablename);
 	/* Dump out chain names first,
 	 * thereby preventing dependency conflicts */
 	nft_chain_save(h, chain_list);
 	nft_rule_save(h, tablename, d->counters ? 0 : FMT_NOCOUNTS);
+	printf("COMMIT\n");
 
 	now = time(NULL);
-	printf("COMMIT\n");
 	printf("# Completed on %s", ctime(&now));
 	return 0;
 }
-- 
2.22.0

[iptables PATCH 08/12] xtables-save: Pass optstring/longopts to xtables_save_main()

[Phil Sutter]

Introduce variables for the different optstrings so short and long
options live side-by-side.

In order to make xtables_save_main() more versatile, pass optstring and
longopts via parameter.

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

diff --git a/iptables/xtables-save.c b/iptables/xtables-save.c
index ac452f1dd6f14..b4d14b5bcd016 100644
--- a/iptables/xtables-save.c
+++ b/iptables/xtables-save.c
@@ -33,7 +33,8 @@
 
 static bool show_counters = false;
 
-static const struct option options[] = {
+static const char *ipt_save_optstring = "bcdt:M:f:46V";
+static const struct option ipt_save_options[] = {
 	{.name = "counters", .has_arg = false, .val = 'c'},
 	{.name = "version",  .has_arg = false, .val = 'V'},
 	{.name = "dump",     .has_arg = false, .val = 'd'},
@@ -45,6 +46,7 @@ static const struct option options[] = {
 	{NULL},
 };
 
+static const char *arp_save_optstring = "cM:V";
 static const struct option arp_save_options[] = {
 	{.name = "counters", .has_arg = false, .val = 'c'},
 	{.name = "version",  .has_arg = false, .val = 'V'},
@@ -52,6 +54,7 @@ static const struct option arp_save_options[] = {
 	{NULL},
 };
 
+static const char *ebt_save_optstring = "ct:M:V";
 static const struct option ebt_save_options[] = {
 	{.name = "counters", .has_arg = false, .val = 'c'},
 	{.name = "version",  .has_arg = false, .val = 'V'},
@@ -129,7 +132,8 @@ do_output(struct nft_handle *h, const char *tablename, struct do_output_data *d)
  * rule
  */
 static int
-xtables_save_main(int family, int argc, char *argv[])
+xtables_save_main(int family, int argc, char *argv[],
+		  const char *optstring, const struct option *longopts)
 {
 	const struct builtin_table *tables;
 	const char *tablename = NULL;
@@ -150,7 +154,7 @@ xtables_save_main(int family, int argc, char *argv[])
 		exit(1);
 	}
 
-	while ((c = getopt_long(argc, argv, "bcdt:M:f:46V", options, NULL)) != -1) {
+	while ((c = getopt_long(argc, argv, optstring, longopts, NULL)) != -1) {
 		switch (c) {
 		case 'b':
 			fprintf(stderr, "-b/--binary option is not implemented\n");
@@ -245,12 +249,14 @@ xtables_save_main(int family, int argc, char *argv[])
 
 int xtables_ip4_save_main(int argc, char *argv[])
 {
-	return xtables_save_main(NFPROTO_IPV4, argc, argv);
+	return xtables_save_main(NFPROTO_IPV4, argc, argv,
+				 ipt_save_optstring, ipt_save_options);
 }
 
 int xtables_ip6_save_main(int argc, char *argv[])
 {
-	return xtables_save_main(NFPROTO_IPV6, argc, argv);
+	return xtables_save_main(NFPROTO_IPV6, argc, argv,
+				 ipt_save_optstring, ipt_save_options);
 }
 
 static int __ebt_save(struct nft_handle *h, const char *tablename, void *data)
@@ -323,7 +329,7 @@ int xtables_eb_save_main(int argc_, char *argv_[])
 		exit(1);
 	}
 
-	while ((c = getopt_long(argc_, argv_, "ct:M:V", ebt_save_options, NULL)) != -1) {
+	while ((c = getopt_long(argc_, argv_, ebt_save_optstring, ebt_save_options, NULL)) != -1) {
 		switch (c) {
 		case 'c':
 			unsetenv("EBTABLES_SAVE_COUNTER");
@@ -378,7 +384,7 @@ int xtables_arp_save_main(int argc, char **argv)
 		exit(1);
 	}
 
-	while ((c = getopt_long(argc, argv, "cM:V", arp_save_options, NULL)) != -1) {
+	while ((c = getopt_long(argc, argv, arp_save_optstring, arp_save_options, NULL)) != -1) {
 		switch (c) {
 		case 'c':
 			show_counters = true;
-- 
2.22.0

[iptables PATCH 09/12] xtables-save: Make COMMIT line optional

[Phil Sutter]

Change xtables_save_main to support optional printing of COMMIT line as
it is not used in arp- or ebtables.

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

diff --git a/iptables/xtables-save.c b/iptables/xtables-save.c
index b4d14b5bcd016..249b396091af4 100644
--- a/iptables/xtables-save.c
+++ b/iptables/xtables-save.c
@@ -67,6 +67,7 @@ static bool ebt_legacy_counter_format;
 
 struct do_output_data {
 	bool counters;
+	bool commit;
 };
 
 static int
@@ -98,7 +99,8 @@ __do_output(struct nft_handle *h, const char *tablename, void *data)
 	 * thereby preventing dependency conflicts */
 	nft_chain_save(h, chain_list);
 	nft_rule_save(h, tablename, d->counters ? 0 : FMT_NOCOUNTS);
-	printf("COMMIT\n");
+	if (d->commit)
+		printf("COMMIT\n");
 
 	now = time(NULL);
 	printf("# Completed on %s", ctime(&now));
@@ -219,6 +221,7 @@ xtables_save_main(int family, int argc, char *argv[],
 		init_extensions4();
 #endif
 		tables = xtables_ipv4;
+		d.commit = true;
 		break;
 	case NFPROTO_ARP:
 		tables = xtables_arp;
-- 
2.22.0

Re: [iptables PATCH 09/12] xtables-save: Make COMMIT line optional

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> Change xtables_save_main to support optional printing of COMMIT line as
> it is not used in arp- or ebtables.

Why?  Is this so ebt-save dumps are compatible with the
old ebt-restore?

Re: [iptables PATCH 09/12] xtables-save: Make COMMIT line optional

[Phil Sutter]

On Sat, Jul 20, 2019 at 09:29:18PM +0200, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > Change xtables_save_main to support optional printing of COMMIT line as
> > it is not used in arp- or ebtables.
> 
> Why?  Is this so ebt-save dumps are compatible with the
> old ebt-restore?

Hmm. I haven't considered deviating in ebtables-nft-save format from
legacy one yet, but it may actually be no problem given that people
shouldn't switch between both. The only bummer might be if someone has a
custom script which then trips over the unknown non-comment line.

I'm undecided, but not against "breaking" with legacy save dumps per se.
If you think it's not a problem, we could just simplify things. OTOH
this is not a big patch, either so maybe not even worth the risk. :)

Thanks, Phil

Re: [iptables PATCH 09/12] xtables-save: Make COMMIT line optional

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> On Sat, Jul 20, 2019 at 09:29:18PM +0200, Florian Westphal wrote:
> > Phil Sutter <phil@nwl.cc> wrote:
> > > Change xtables_save_main to support optional printing of COMMIT line as
> > > it is not used in arp- or ebtables.
> > 
> > Why?  Is this so ebt-save dumps are compatible with the
> > old ebt-restore?
> 
> Hmm. I haven't considered deviating in ebtables-nft-save format from
> legacy one yet, but it may actually be no problem given that people
> shouldn't switch between both. The only bummer might be if someone has a
> custom script which then trips over the unknown non-comment line.
> 
> I'm undecided, but not against "breaking" with legacy save dumps per se.
> If you think it's not a problem, we could just simplify things. OTOH
> this is not a big patch, either so maybe not even worth the risk. :)

I'm fine with this, I was just wondering because the changelog explains
whats happening, not why.

Re: [iptables PATCH 09/12] xtables-save: Make COMMIT line optional

[Phil Sutter]

On Sat, Jul 20, 2019 at 10:13:10PM +0200, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > On Sat, Jul 20, 2019 at 09:29:18PM +0200, Florian Westphal wrote:
> > > Phil Sutter <phil@nwl.cc> wrote:
> > > > Change xtables_save_main to support optional printing of COMMIT line as
> > > > it is not used in arp- or ebtables.
> > > 
> > > Why?  Is this so ebt-save dumps are compatible with the
> > > old ebt-restore?
> > 
> > Hmm. I haven't considered deviating in ebtables-nft-save format from
> > legacy one yet, but it may actually be no problem given that people
> > shouldn't switch between both. The only bummer might be if someone has a
> > custom script which then trips over the unknown non-comment line.
> > 
> > I'm undecided, but not against "breaking" with legacy save dumps per se.
> > If you think it's not a problem, we could just simplify things. OTOH
> > this is not a big patch, either so maybe not even worth the risk. :)
> 
> I'm fine with this, I was just wondering because the changelog explains
> whats happening, not why.

Yes, typical pitfall for me. Maybe I should put a sticky note somewhere.
:)

Thanks, Phil

[iptables PATCH 10/12] xtables-save: Pass format flags to do_output()

[Phil Sutter]

Let callers define the flags to pass to nft_rule_save() instead of just
setting the counters boolean.

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

diff --git a/iptables/xtables-save.c b/iptables/xtables-save.c
index 249b396091af4..980a80ff06f96 100644
--- a/iptables/xtables-save.c
+++ b/iptables/xtables-save.c
@@ -66,7 +66,7 @@ static const struct option ebt_save_options[] = {
 static bool ebt_legacy_counter_format;
 
 struct do_output_data {
-	bool counters;
+	unsigned int format;
 	bool commit;
 };
 
@@ -98,7 +98,7 @@ __do_output(struct nft_handle *h, const char *tablename, void *data)
 	/* Dump out chain names first,
 	 * thereby preventing dependency conflicts */
 	nft_chain_save(h, chain_list);
-	nft_rule_save(h, tablename, d->counters ? 0 : FMT_NOCOUNTS);
+	nft_rule_save(h, tablename, d->format);
 	if (d->commit)
 		printf("COMMIT\n");
 
@@ -139,7 +139,9 @@ xtables_save_main(int family, int argc, char *argv[],
 {
 	const struct builtin_table *tables;
 	const char *tablename = NULL;
-	struct do_output_data d = {};
+	struct do_output_data d = {
+		.format = FMT_NOCOUNTS,
+	};
 	bool dump = false;
 	struct nft_handle h = {
 		.family	= family,
@@ -162,7 +164,7 @@ xtables_save_main(int family, int argc, char *argv[],
 			fprintf(stderr, "-b/--binary option is not implemented\n");
 			break;
 		case 'c':
-			d.counters = true;
+			d.format &= ~FMT_NOCOUNTS;
 			break;
 
 		case 't':
-- 
2.22.0

[iptables PATCH 11/12] arptables-save: Merge into xtables_save_main()

[Phil Sutter]

With all preparations in place, xtables_save_main() can replace it with
not further changes.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/xtables-save.c | 63 ++---------------------------------------
 1 file changed, 3 insertions(+), 60 deletions(-)

diff --git a/iptables/xtables-save.c b/iptables/xtables-save.c
index 980a80ff06f96..0c294e056c7b5 100644
--- a/iptables/xtables-save.c
+++ b/iptables/xtables-save.c
@@ -372,65 +372,8 @@ int xtables_eb_save_main(int argc_, char *argv_[])
 	return 0;
 }
 
-int xtables_arp_save_main(int argc, char **argv)
+int xtables_arp_save_main(int argc, char *argv[])
 {
-	struct nft_handle h = {
-		.family	= NFPROTO_ARP,
-	};
-	time_t now;
-	int c;
-
-	xtables_globals.program_name = basename(*argv);;
-	c = xtables_init_all(&xtables_globals, h.family);
-	if (c < 0) {
-		fprintf(stderr, "%s/%s Failed to initialize xtables\n",
-				xtables_globals.program_name,
-				xtables_globals.program_version);
-		exit(1);
-	}
-
-	while ((c = getopt_long(argc, argv, arp_save_optstring, arp_save_options, NULL)) != -1) {
-		switch (c) {
-		case 'c':
-			show_counters = true;
-			break;
-		case 'M':
-			xtables_modprobe_program = optarg;
-			break;
-		case 'V':
-			printf("%s v%s (nf_tables)\n", prog_name, prog_vers);
-			exit(0);
-		default:
-			fprintf(stderr,
-				"Look at manual page `%s.8' for more information.\n",
-				prog_name);
-			exit(1);
-		}
-	}
-
-	if (nft_init(&h, xtables_arp) < 0) {
-		fprintf(stderr, "%s/%s Failed to initialize nft: %s\n",
-				xtables_globals.program_name,
-				xtables_globals.program_version,
-				strerror(errno));
-		exit(EXIT_FAILURE);
-	}
-
-	if (!nft_table_find(&h, "filter"))
-		return 0;
-
-	if (!nft_is_table_compatible(&h, "filter")) {
-		printf("# Table `filter' is incompatible, use 'nft' tool.\n");
-		return 0;
-	}
-
-	printf("# Generated by %s v%s on %s", prog_name,
-	       prog_vers, ctime(&now));
-	printf("*filter\n");
-	nft_chain_save(&h, nft_chain_list_get(&h, "filter"));
-	nft_rule_save(&h, "filter", show_counters ? 0 : FMT_NOCOUNTS);
-	now = time(NULL);
-	printf("# Completed on %s", ctime(&now));
-	nft_fini(&h);
-	return 0;
+	return xtables_save_main(NFPROTO_ARP, argc, argv,
+				 arp_save_optstring, arp_save_options);
 }
-- 
2.22.0

[iptables PATCH 12/12] ebtables-save: Merge into xtables_save_main()

[Phil Sutter]

The only thing missing was handling of EBTABLES_SAVE_COUNTER env var,
but that can be done after parsing parameters in bridge-specific code.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/xtables-save.c | 123 +++++-----------------------------------
 1 file changed, 13 insertions(+), 110 deletions(-)

diff --git a/iptables/xtables-save.c b/iptables/xtables-save.c
index 0c294e056c7b5..77b13f7ffbcdd 100644
--- a/iptables/xtables-save.c
+++ b/iptables/xtables-save.c
@@ -31,8 +31,6 @@
 #define prog_name xtables_globals.program_name
 #define prog_vers xtables_globals.program_version
 
-static bool show_counters = false;
-
 static const char *ipt_save_optstring = "bcdt:M:f:46V";
 static const struct option ipt_save_options[] = {
 	{.name = "counters", .has_arg = false, .val = 'c'},
@@ -63,8 +61,6 @@ static const struct option ebt_save_options[] = {
 	{NULL},
 };
 
-static bool ebt_legacy_counter_format;
-
 struct do_output_data {
 	unsigned int format;
 	bool commit;
@@ -228,9 +224,18 @@ xtables_save_main(int family, int argc, char *argv[],
 	case NFPROTO_ARP:
 		tables = xtables_arp;
 		break;
-	case NFPROTO_BRIDGE:
+	case NFPROTO_BRIDGE: {
+		const char *ctr = getenv("EBTABLES_SAVE_COUNTER");
+
+		if (!(d.format & FMT_NOCOUNTS)) {
+			d.format |= FMT_EBT_SAVE;
+		} else if (ctr && !strcmp(ctr, "yes")) {
+			d.format &= ~FMT_NOCOUNTS;
+			d.format |= FMT_C_COUNTS | FMT_EBT_SAVE;
+		}
 		tables = xtables_bridge;
 		break;
+	}
 	default:
 		fprintf(stderr, "Unknown family %d\n", family);
 		return 1;
@@ -264,112 +269,10 @@ int xtables_ip6_save_main(int argc, char *argv[])
 				 ipt_save_optstring, ipt_save_options);
 }
 
-static int __ebt_save(struct nft_handle *h, const char *tablename, void *data)
+int xtables_eb_save_main(int argc, char *argv[])
 {
-	struct nftnl_chain_list *chain_list;
-	unsigned int format = FMT_NOCOUNTS;
-	bool *counters = data;
-	time_t now;
-
-	if (!nft_table_find(h, tablename)) {
-		printf("Table `%s' does not exist\n", tablename);
-		return 1;
-	}
-
-	if (!nft_is_table_compatible(h, tablename)) {
-		printf("# Table `%s' is incompatible, use 'nft' tool.\n", tablename);
-		return 0;
-	}
-
-	chain_list = nft_chain_list_get(h, tablename);
-
-	now = time(NULL);
-	printf("# Generated by %s v%s on %s", prog_name,
-	       prog_vers, ctime(&now));
-	printf("*%s\n", tablename);
-
-	if (counters)
-		format = FMT_EBT_SAVE |
-			(ebt_legacy_counter_format ? FMT_C_COUNTS : 0);
-
-	/* Dump out chain names first,
-	 * thereby preventing dependency conflicts */
-	nft_chain_save(h, chain_list);
-	nft_rule_save(h, tablename, format);
-	now = time(NULL);
-	printf("# Completed on %s", ctime(&now));
-	return 0;
-}
-
-static int ebt_save(struct nft_handle *h, const char *tablename, bool counters)
-{
-	if (!tablename)
-		return nft_for_each_table(h, __ebt_save, &counters);
-
-	return __ebt_save(h, tablename, &counters);
-}
-
-int xtables_eb_save_main(int argc_, char *argv_[])
-{
-	const char *ctr = getenv("EBTABLES_SAVE_COUNTER");
-	const char *tablename = NULL;
-	struct nft_handle h = {
-		.family	= NFPROTO_BRIDGE,
-	};
-	int c;
-
-	if (ctr) {
-		if (strcmp(ctr, "yes") == 0) {
-			ebt_legacy_counter_format = true;
-			show_counters = true;
-		}
-	}
-
-	xtables_globals.program_name = basename(*argv_);
-	c = xtables_init_all(&xtables_globals, h.family);
-	if (c < 0) {
-		fprintf(stderr, "%s/%s Failed to initialize xtables\n",
-				xtables_globals.program_name,
-				xtables_globals.program_version);
-		exit(1);
-	}
-
-	while ((c = getopt_long(argc_, argv_, ebt_save_optstring, ebt_save_options, NULL)) != -1) {
-		switch (c) {
-		case 'c':
-			unsetenv("EBTABLES_SAVE_COUNTER");
-			show_counters = true;
-			ebt_legacy_counter_format = false;
-			break;
-		case 't':
-			/* Select specific table. */
-			tablename = optarg;
-			break;
-		case 'M':
-			xtables_modprobe_program = optarg;
-			break;
-		case 'V':
-			printf("%s v%s (nf_tables)\n", prog_name, prog_vers);
-			exit(0);
-		default:
-			fprintf(stderr,
-				"Look at manual page `%s.8' for more information.\n",
-				prog_name);
-			exit(1);
-		}
-	}
-
-	if (nft_init(&h, xtables_bridge) < 0) {
-		fprintf(stderr, "%s/%s Failed to initialize nft: %s\n",
-				xtables_globals.program_name,
-				xtables_globals.program_version,
-				strerror(errno));
-		exit(EXIT_FAILURE);
-	}
-
-	ebt_save(&h, tablename, show_counters);
-	nft_fini(&h);
-	return 0;
+	return xtables_save_main(NFPROTO_BRIDGE, argc, argv,
+				 ebt_save_optstring, ebt_save_options);
 }
 
 int xtables_arp_save_main(int argc, char *argv[])
-- 
2.22.0

Re: [iptables PATCH 00/12] Larger xtables-save review

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> This series started as a fix to program names mentioned in *-save
> outputs and ended in merging ebtables-save and arptables-save code into
> xtables_save_main used by ip{6,}tables-nft-save.
> 
> The first patch is actually unrelated but was discovered when testing
> counter output - depending on environment, ebtables-nft might segfault.
> 
> The second patch fixes option '-c' of ebtables-nft-save which enables
> counter prefixes in dumped rules but failed to disable the classical
> ebtables-style counters.
> 
> Patch three sorts program names quoted in output of any of the *-save
> programs, patch four unifies the header/footer comments in the same. The
> latter also drops the extra newline printed in ebtables- and
> arptables-save output, so test scripts need adjustments beyond dropping
> the new comment lines from output.
> 
> Patch five fixes the table compatibility check in ip{6,}tables-nft-save.
> 
> Patches six and eight to ten prepare for integrating arptables- and
> ebtables-save into the xtables-save code.
> 
> Patch seven merely fixes a minor coding-style issue.
> 
> Patches eleven and twelve finally perform the actual merge.

Looks good, feel free to rebase this on top of master and
then you can push this out.

In case my comment wrt. 'COMMIT line optional' is right, consider
ammending the commit message so that this reasoning is recorded
in the changelog.

Thanks!

Re: [iptables PATCH 00/12] Larger xtables-save review

[Pablo Neira Ayuso]

On Sat, Jul 20, 2019 at 06:30:14PM +0200, Phil Sutter wrote:
> This series started as a fix to program names mentioned in *-save
> outputs and ended in merging ebtables-save and arptables-save code into
> xtables_save_main used by ip{6,}tables-nft-save.
> 
> The first patch is actually unrelated but was discovered when testing
> counter output - depending on environment, ebtables-nft might segfault.
> 
> The second patch fixes option '-c' of ebtables-nft-save which enables
> counter prefixes in dumped rules but failed to disable the classical
> ebtables-style counters.
> 
> Patch three sorts program names quoted in output of any of the *-save
> programs, patch four unifies the header/footer comments in the same. The
> latter also drops the extra newline printed in ebtables- and
> arptables-save output, so test scripts need adjustments beyond dropping
> the new comment lines from output.
> 
> Patch five fixes the table compatibility check in ip{6,}tables-nft-save.
> 
> Patches six and eight to ten prepare for integrating arptables- and
> ebtables-save into the xtables-save code.
> 
> Patch seven merely fixes a minor coding-style issue.
> 
> Patches eleven and twelve finally perform the actual merge.

Series look good after quick review.