[iptables PATCH 00/10] Reduce code size around arptables-nft

[iptables PATCH 00/10] Reduce code size around arptables-nft

[Phil Sutter]

A review of xtables-arp.c exposed a significant amount of dead, needless
or duplicated code. This series deals with some low hanging fruits. Most
of the changes affect xtables-arp.c and nft-arp.c only, but where common
issues existed or code was to be shared, other files are touched as
well.

Phil Sutter (10):
  ip6tables, xtables-arp: Drop unused struct pprot
  xshared: Share a common add_command() implementation
  xshared: Share a common implementation of parse_rulenumber()
  Merge CMD_* defines
  xtables-arp: Drop generic_opt_check()
  Replace TRUE/FALSE with true/false
  xtables-arp: Integrate OPT_* defines into xshared.h
  xtables-arp: Drop some unused variables
  xtables-arp: Use xtables_parse_interface()
  nft-arp: Use xtables_print_mac_and_mask()

 iptables/ip6tables.c   |  73 +------------
 iptables/iptables.c    |  64 +----------
 iptables/nft-arp.c     |  31 +-----
 iptables/nft-shared.h  |  17 ---
 iptables/xshared.c     |  39 +++++++
 iptables/xshared.h     |  32 ++++++
 iptables/xtables-arp.c | 237 +++--------------------------------------
 iptables/xtables.c     |  48 +--------
 8 files changed, 97 insertions(+), 444 deletions(-)

-- 
2.23.0

[iptables PATCH 01/10] ip6tables, xtables-arp: Drop unused struct pprot

[Phil Sutter]

These seem like leftovers when changing code to use xtables_chain_protos
as struct xtables_pprot is identical to struct pprot removed here.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/ip6tables.c   | 6 ------
 iptables/xtables-arp.c | 7 -------
 2 files changed, 13 deletions(-)

diff --git a/iptables/ip6tables.c b/iptables/ip6tables.c
index c160a2dd4e65b..ee463c9586862 100644
--- a/iptables/ip6tables.c
+++ b/iptables/ip6tables.c
@@ -175,12 +175,6 @@ static const unsigned int inverse_for_options[NUMBER_OF_OPT] =
 #define opts ip6tables_globals.opts
 #define prog_name ip6tables_globals.program_name
 #define prog_vers ip6tables_globals.program_version
-/* A few hardcoded protocols for 'all' and in case the user has no
-   /etc/protocols */
-struct pprot {
-	const char *name;
-	uint8_t num;
-};
 
 static void __attribute__((noreturn))
 exit_tryhelp(int status)
diff --git a/iptables/xtables-arp.c b/iptables/xtables-arp.c
index 1a260e75e3808..8503f47fe2afe 100644
--- a/iptables/xtables-arp.c
+++ b/iptables/xtables-arp.c
@@ -209,13 +209,6 @@ static int inverse_for_options[NUMBER_OF_OPT] =
 /* -c */ 0,
 };
 
-/* A few hardcoded protocols for 'all' and in case the user has no
-   /etc/protocols */
-struct pprot {
-	char *name;
-	u_int8_t num;
-};
-
 /* Primitive headers... */
 /* defined in netinet/in.h */
 #if 0
-- 
2.23.0

[iptables PATCH 02/10] xshared: Share a common add_command() implementation

[Phil Sutter]

The shared definition of cmdflags is a super set of the previous one in
xtables-arp.c so while not being identical, they're compatible.

Avoid accidental array overstep in cmd2char() by incrementing an index
variable and checking its final value before using it as such.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/ip6tables.c   | 23 -----------------------
 iptables/iptables.c    | 23 -----------------------
 iptables/xshared.c     | 27 +++++++++++++++++++++++++++
 iptables/xshared.h     |  4 ++++
 iptables/xtables-arp.c | 22 ----------------------
 iptables/xtables.c     | 23 -----------------------
 6 files changed, 31 insertions(+), 91 deletions(-)

diff --git a/iptables/ip6tables.c b/iptables/ip6tables.c
index ee463c9586862..9a9d71f1cdadc 100644
--- a/iptables/ip6tables.c
+++ b/iptables/ip6tables.c
@@ -69,8 +69,6 @@
 #define CMD_ZERO_NUM		0x2000U
 #define CMD_CHECK		0x4000U
 #define NUMBER_OF_CMD	16
-static const char cmdflags[] = { 'I', 'D', 'D', 'R', 'A', 'L', 'F', 'Z',
-				 'N', 'X', 'P', 'E', 'S', 'Z', 'C' };
 
 #define NUMBER_OF_OPT	ARRAY_SIZE(optflags)
 static const char optflags[]
@@ -336,27 +334,6 @@ opt2char(int option)
 	return *ptr;
 }
 
-static char
-cmd2char(int option)
-{
-	const char *ptr;
-	for (ptr = cmdflags; option > 1; option >>= 1, ptr++);
-
-	return *ptr;
-}
-
-static void
-add_command(unsigned int *cmd, const int newcmd, const int othercmds,
-	    int invert)
-{
-	if (invert)
-		xtables_error(PARAMETER_PROBLEM, "unexpected '!' flag");
-	if (*cmd & (~othercmds))
-		xtables_error(PARAMETER_PROBLEM, "Cannot use -%c with -%c\n",
-			   cmd2char(newcmd), cmd2char(*cmd & (~othercmds)));
-	*cmd |= newcmd;
-}
-
 /*
  *	All functions starting with "parse" should succeed, otherwise
  *	the program fails.
diff --git a/iptables/iptables.c b/iptables/iptables.c
index 544e87596e7e4..5fec25376c24f 100644
--- a/iptables/iptables.c
+++ b/iptables/iptables.c
@@ -65,8 +65,6 @@
 #define CMD_ZERO_NUM		0x2000U
 #define CMD_CHECK		0x4000U
 #define NUMBER_OF_CMD	16
-static const char cmdflags[] = { 'I', 'D', 'D', 'R', 'A', 'L', 'F', 'Z',
-				 'N', 'X', 'P', 'E', 'S', 'Z', 'C' };
 
 #define OPT_FRAGMENT    0x00800U
 #define NUMBER_OF_OPT	ARRAY_SIZE(optflags)
@@ -335,27 +333,6 @@ opt2char(int option)
 	return *ptr;
 }
 
-static char
-cmd2char(int option)
-{
-	const char *ptr;
-	for (ptr = cmdflags; option > 1; option >>= 1, ptr++);
-
-	return *ptr;
-}
-
-static void
-add_command(unsigned int *cmd, const int newcmd, const int othercmds, 
-	    int invert)
-{
-	if (invert)
-		xtables_error(PARAMETER_PROBLEM, "unexpected ! flag");
-	if (*cmd & (~othercmds))
-		xtables_error(PARAMETER_PROBLEM, "Cannot use -%c with -%c\n",
-			   cmd2char(newcmd), cmd2char(*cmd & (~othercmds)));
-	*cmd |= newcmd;
-}
-
 /*
  *	All functions starting with "parse" should succeed, otherwise
  *	the program fails.
diff --git a/iptables/xshared.c b/iptables/xshared.c
index 97f1b5d22fdbe..3baa805c64e6d 100644
--- a/iptables/xshared.c
+++ b/iptables/xshared.c
@@ -732,3 +732,30 @@ void command_jump(struct iptables_command_state *cs, const char *jumpto)
 		xtables_error(OTHER_PROBLEM, "can't alloc memory!");
 	xt_params->opts = opts;
 }
+
+char cmd2char(int option)
+{
+	/* cmdflags index corresponds with position of bit in CMD_* values */
+	static const char cmdflags[] = { 'I', 'D', 'D', 'R', 'A', 'L', 'F', 'Z',
+					 'N', 'X', 'P', 'E', 'S', 'Z', 'C' };
+	int i;
+
+	for (i = 0; option > 1; option >>= 1, i++)
+		;
+	if (i >= ARRAY_SIZE(cmdflags))
+		xtables_error(OTHER_PROBLEM,
+			      "cmd2char(): Invalid command number %u.\n",
+			      1 << i);
+	return cmdflags[i];
+}
+
+void add_command(unsigned int *cmd, const int newcmd,
+		 const int othercmds, int invert)
+{
+	if (invert)
+		xtables_error(PARAMETER_PROBLEM, "unexpected '!' flag");
+	if (*cmd & (~othercmds))
+		xtables_error(PARAMETER_PROBLEM, "Cannot use -%c with -%c\n",
+			   cmd2char(newcmd), cmd2char(*cmd & (~othercmds)));
+	*cmd |= newcmd;
+}
diff --git a/iptables/xshared.h b/iptables/xshared.h
index 64b7e8fc4b690..0b9b357c7bdaa 100644
--- a/iptables/xshared.h
+++ b/iptables/xshared.h
@@ -183,4 +183,8 @@ void command_match(struct iptables_command_state *cs);
 const char *xt_parse_target(const char *targetname);
 void command_jump(struct iptables_command_state *cs, const char *jumpto);
 
+char cmd2char(int option);
+void add_command(unsigned int *cmd, const int newcmd,
+		 const int othercmds, int invert);
+
 #endif /* IPTABLES_XSHARED_H */
diff --git a/iptables/xtables-arp.c b/iptables/xtables-arp.c
index 8503f47fe2afe..584b6f0646821 100644
--- a/iptables/xtables-arp.c
+++ b/iptables/xtables-arp.c
@@ -81,8 +81,6 @@ typedef char arpt_chainlabel[32];
 #define CMD_CHECK		0x0800U
 #define CMD_RENAME_CHAIN	0x1000U
 #define NUMBER_OF_CMD	13
-static const char cmdflags[] = { 'I', 'D', 'D', 'R', 'A', 'L', 'F', 'Z',
-				 'N', 'X', 'P', 'E' };
 
 #define OPTION_OFFSET 256
 
@@ -462,26 +460,6 @@ opt2char(int option)
 	return *ptr;
 }
 
-static char
-cmd2char(int option)
-{
-	const char *ptr;
-	for (ptr = cmdflags; option > 1; option >>= 1, ptr++);
-
-	return *ptr;
-}
-
-static void
-add_command(unsigned int *cmd, const int newcmd, const unsigned int othercmds, int invert)
-{
-	if (invert)
-		xtables_error(PARAMETER_PROBLEM, "unexpected ! flag");
-	if (*cmd & (~othercmds))
-		xtables_error(PARAMETER_PROBLEM, "Can't use -%c with -%c\n",
-			      cmd2char(newcmd), cmd2char(*cmd & (~othercmds)));
-	*cmd |= newcmd;
-}
-
 static int
 check_inverse(const char option[], int *invert, int *optidx, int argc)
 {
diff --git a/iptables/xtables.c b/iptables/xtables.c
index 8a9e0edc3bea2..6dfa3f1171183 100644
--- a/iptables/xtables.c
+++ b/iptables/xtables.c
@@ -51,8 +51,6 @@
 #endif
 
 #define NUMBER_OF_CMD	16
-static const char cmdflags[] = { 'I', 'D', 'D', 'R', 'A', 'L', 'F', 'Z',
-				 'N', 'X', 'P', 'E', 'S', 'Z', 'C' };
 
 #define OPT_FRAGMENT	0x00800U
 #define NUMBER_OF_OPT	ARRAY_SIZE(optflags)
@@ -319,27 +317,6 @@ opt2char(int option)
 	return *ptr;
 }
 
-static char
-cmd2char(int option)
-{
-	const char *ptr;
-	for (ptr = cmdflags; option > 1; option >>= 1, ptr++);
-
-	return *ptr;
-}
-
-static void
-add_command(unsigned int *cmd, const int newcmd, const int othercmds,
-	    int invert)
-{
-	if (invert)
-		xtables_error(PARAMETER_PROBLEM, "unexpected ! flag");
-	if (*cmd & (~othercmds))
-		xtables_error(PARAMETER_PROBLEM, "Cannot use -%c with -%c\n",
-			   cmd2char(newcmd), cmd2char(*cmd & (~othercmds)));
-	*cmd |= newcmd;
-}
-
 /*
  *	All functions starting with "parse" should succeed, otherwise
  *	the program fails.
-- 
2.23.0

[iptables PATCH 03/10] xshared: Share a common implementation of parse_rulenumber()

[Phil Sutter]

The function is really small, but still copied four times.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/ip6tables.c   | 13 -------------
 iptables/iptables.c    | 12 ------------
 iptables/xshared.c     | 12 ++++++++++++
 iptables/xshared.h     |  1 +
 iptables/xtables-arp.c | 13 -------------
 iptables/xtables.c     | 12 ------------
 6 files changed, 13 insertions(+), 50 deletions(-)

diff --git a/iptables/ip6tables.c b/iptables/ip6tables.c
index 9a9d71f1cdadc..f4ccfc60de953 100644
--- a/iptables/ip6tables.c
+++ b/iptables/ip6tables.c
@@ -352,19 +352,6 @@ static int is_exthdr(uint16_t proto)
 		proto == IPPROTO_DSTOPTS);
 }
 
-/* Can't be zero. */
-static int
-parse_rulenumber(const char *rule)
-{
-	unsigned int rulenum;
-
-	if (!xtables_strtoui(rule, NULL, &rulenum, 1, INT_MAX))
-		xtables_error(PARAMETER_PROBLEM,
-			   "Invalid rule number `%s'", rule);
-
-	return rulenum;
-}
-
 static void
 parse_chain(const char *chainname)
 {
diff --git a/iptables/iptables.c b/iptables/iptables.c
index 5fec25376c24f..df371f410a9c2 100644
--- a/iptables/iptables.c
+++ b/iptables/iptables.c
@@ -343,18 +343,6 @@ opt2char(int option)
 */
 
 /* Christophe Burki wants `-p 6' to imply `-m tcp'.  */
-/* Can't be zero. */
-static int
-parse_rulenumber(const char *rule)
-{
-	unsigned int rulenum;
-
-	if (!xtables_strtoui(rule, NULL, &rulenum, 1, INT_MAX))
-		xtables_error(PARAMETER_PROBLEM,
-			   "Invalid rule number `%s'", rule);
-
-	return rulenum;
-}
 
 static void
 parse_chain(const char *chainname)
diff --git a/iptables/xshared.c b/iptables/xshared.c
index 3baa805c64e6d..2a0077d9da846 100644
--- a/iptables/xshared.c
+++ b/iptables/xshared.c
@@ -759,3 +759,15 @@ void add_command(unsigned int *cmd, const int newcmd,
 			   cmd2char(newcmd), cmd2char(*cmd & (~othercmds)));
 	*cmd |= newcmd;
 }
+
+/* Can't be zero. */
+int parse_rulenumber(const char *rule)
+{
+	unsigned int rulenum;
+
+	if (!xtables_strtoui(rule, NULL, &rulenum, 1, INT_MAX))
+		xtables_error(PARAMETER_PROBLEM,
+			   "Invalid rule number `%s'", rule);
+
+	return rulenum;
+}
diff --git a/iptables/xshared.h b/iptables/xshared.h
index 0b9b357c7bdaa..85bbfa1250aa3 100644
--- a/iptables/xshared.h
+++ b/iptables/xshared.h
@@ -186,5 +186,6 @@ void command_jump(struct iptables_command_state *cs, const char *jumpto);
 char cmd2char(int option);
 void add_command(unsigned int *cmd, const int newcmd,
 		 const int othercmds, int invert);
+int parse_rulenumber(const char *rule);
 
 #endif /* IPTABLES_XSHARED_H */
diff --git a/iptables/xtables-arp.c b/iptables/xtables-arp.c
index 584b6f0646821..79cc83d354fc5 100644
--- a/iptables/xtables-arp.c
+++ b/iptables/xtables-arp.c
@@ -518,19 +518,6 @@ parse_interface(const char *arg, char *vianame, unsigned char *mask)
 	}
 }
 
-/* Can't be zero. */
-static int
-parse_rulenumber(const char *rule)
-{
-	unsigned int rulenum;
-
-	if (!xtables_strtoui(rule, NULL, &rulenum, 1, INT_MAX))
-		xtables_error(PARAMETER_PROBLEM,
-			      "Invalid rule number `%s'", rule);
-
-	return rulenum;
-}
-
 static void
 set_option(unsigned int *options, unsigned int option, u_int16_t *invflg,
 	   int invert)
diff --git a/iptables/xtables.c b/iptables/xtables.c
index 6dfa3f1171183..bb76e6a7a1ce8 100644
--- a/iptables/xtables.c
+++ b/iptables/xtables.c
@@ -327,18 +327,6 @@ opt2char(int option)
 */
 
 /* Christophe Burki wants `-p 6' to imply `-m tcp'.  */
-/* Can't be zero. */
-static int
-parse_rulenumber(const char *rule)
-{
-	unsigned int rulenum;
-
-	if (!xtables_strtoui(rule, NULL, &rulenum, 1, INT_MAX))
-		xtables_error(PARAMETER_PROBLEM,
-			   "Invalid rule number `%s'", rule);
-
-	return rulenum;
-}
 
 static void
 set_option(unsigned int *options, unsigned int option, uint8_t *invflg,
-- 
2.23.0

[iptables PATCH 04/10] Merge CMD_* defines

[Phil Sutter]

They are mostly identical, just xtables-arp ones differ slightly. Though
since they are internal use only and their actual value doesn't matter
(as long as it's a distinct bit), they can be merged anyway.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/ip6tables.c   | 17 -----------------
 iptables/iptables.c    | 17 -----------------
 iptables/nft-shared.h  | 17 -----------------
 iptables/xshared.h     | 20 ++++++++++++++++++++
 iptables/xtables-arp.c | 20 --------------------
 iptables/xtables.c     |  2 --
 6 files changed, 20 insertions(+), 73 deletions(-)

diff --git a/iptables/ip6tables.c b/iptables/ip6tables.c
index f4ccfc60de953..e48fdeb1248bd 100644
--- a/iptables/ip6tables.c
+++ b/iptables/ip6tables.c
@@ -52,23 +52,6 @@
 #define FALSE 0
 #endif
 
-#define CMD_NONE		0x0000U
-#define CMD_INSERT		0x0001U
-#define CMD_DELETE		0x0002U
-#define CMD_DELETE_NUM		0x0004U
-#define CMD_REPLACE		0x0008U
-#define CMD_APPEND		0x0010U
-#define CMD_LIST		0x0020U
-#define CMD_FLUSH		0x0040U
-#define CMD_ZERO		0x0080U
-#define CMD_NEW_CHAIN		0x0100U
-#define CMD_DELETE_CHAIN	0x0200U
-#define CMD_SET_POLICY		0x0400U
-#define CMD_RENAME_CHAIN	0x0800U
-#define CMD_LIST_RULES		0x1000U
-#define CMD_ZERO_NUM		0x2000U
-#define CMD_CHECK		0x4000U
-#define NUMBER_OF_CMD	16
 
 #define NUMBER_OF_OPT	ARRAY_SIZE(optflags)
 static const char optflags[]
diff --git a/iptables/iptables.c b/iptables/iptables.c
index df371f410a9c2..255b61b11ec89 100644
--- a/iptables/iptables.c
+++ b/iptables/iptables.c
@@ -48,23 +48,6 @@
 #define FALSE 0
 #endif
 
-#define CMD_NONE		0x0000U
-#define CMD_INSERT		0x0001U
-#define CMD_DELETE		0x0002U
-#define CMD_DELETE_NUM		0x0004U
-#define CMD_REPLACE		0x0008U
-#define CMD_APPEND		0x0010U
-#define CMD_LIST		0x0020U
-#define CMD_FLUSH		0x0040U
-#define CMD_ZERO		0x0080U
-#define CMD_NEW_CHAIN		0x0100U
-#define CMD_DELETE_CHAIN	0x0200U
-#define CMD_SET_POLICY		0x0400U
-#define CMD_RENAME_CHAIN	0x0800U
-#define CMD_LIST_RULES		0x1000U
-#define CMD_ZERO_NUM		0x2000U
-#define CMD_CHECK		0x4000U
-#define NUMBER_OF_CMD	16
 
 #define OPT_FRAGMENT    0x00800U
 #define NUMBER_OF_OPT	ARRAY_SIZE(optflags)
diff --git a/iptables/nft-shared.h b/iptables/nft-shared.h
index 8b073b18fb0d9..e236a981119ac 100644
--- a/iptables/nft-shared.h
+++ b/iptables/nft-shared.h
@@ -199,23 +199,6 @@ struct xtables_args {
 	unsigned long long pcnt_cnt, bcnt_cnt;
 };
 
-#define CMD_NONE		0x0000U
-#define CMD_INSERT		0x0001U
-#define CMD_DELETE		0x0002U
-#define CMD_DELETE_NUM		0x0004U
-#define CMD_REPLACE		0x0008U
-#define CMD_APPEND		0x0010U
-#define CMD_LIST		0x0020U
-#define CMD_FLUSH		0x0040U
-#define CMD_ZERO		0x0080U
-#define CMD_NEW_CHAIN		0x0100U
-#define CMD_DELETE_CHAIN	0x0200U
-#define CMD_SET_POLICY		0x0400U
-#define CMD_RENAME_CHAIN	0x0800U
-#define CMD_LIST_RULES		0x1000U
-#define CMD_ZERO_NUM		0x2000U
-#define CMD_CHECK		0x4000U
-
 struct nft_xt_cmd_parse {
 	unsigned int			command;
 	unsigned int			rulenum;
diff --git a/iptables/xshared.h b/iptables/xshared.h
index 85bbfa1250aa3..b0738b042e95a 100644
--- a/iptables/xshared.h
+++ b/iptables/xshared.h
@@ -31,6 +31,26 @@ enum {
 	OPT_COUNTERS    = 1 << 10,
 };
 
+enum {
+	CMD_NONE		= 0,
+	CMD_INSERT		= 1 << 0,
+	CMD_DELETE		= 1 << 1,
+	CMD_DELETE_NUM		= 1 << 2,
+	CMD_REPLACE		= 1 << 3,
+	CMD_APPEND		= 1 << 4,
+	CMD_LIST		= 1 << 5,
+	CMD_FLUSH		= 1 << 6,
+	CMD_ZERO		= 1 << 7,
+	CMD_NEW_CHAIN		= 1 << 8,
+	CMD_DELETE_CHAIN	= 1 << 9,
+	CMD_SET_POLICY		= 1 << 10,
+	CMD_RENAME_CHAIN	= 1 << 11,
+	CMD_LIST_RULES		= 1 << 12,
+	CMD_ZERO_NUM		= 1 << 13,
+	CMD_CHECK		= 1 << 14,
+};
+#define NUMBER_OF_CMD		16
+
 struct xtables_globals;
 struct xtables_rule_match;
 struct xtables_target;
diff --git a/iptables/xtables-arp.c b/iptables/xtables-arp.c
index 79cc83d354fc5..88a7d534da4f1 100644
--- a/iptables/xtables-arp.c
+++ b/iptables/xtables-arp.c
@@ -62,26 +62,6 @@ typedef char arpt_chainlabel[32];
 #define FALSE 0
 #endif
 
-/* XXX: command defined by nft-shared.h do not overlap with these two */
-#undef CMD_CHECK
-#undef CMD_RENAME_CHAIN
-
-#define CMD_NONE		0x0000U
-#define CMD_INSERT		0x0001U
-#define CMD_DELETE		0x0002U
-#define CMD_DELETE_NUM		0x0004U
-#define CMD_REPLACE		0x0008U
-#define CMD_APPEND		0x0010U
-#define CMD_LIST		0x0020U
-#define CMD_FLUSH		0x0040U
-#define CMD_ZERO		0x0080U
-#define CMD_NEW_CHAIN		0x0100U
-#define CMD_DELETE_CHAIN	0x0200U
-#define CMD_SET_POLICY		0x0400U
-#define CMD_CHECK		0x0800U
-#define CMD_RENAME_CHAIN	0x1000U
-#define NUMBER_OF_CMD	13
-
 #define OPTION_OFFSET 256
 
 #define OPT_NONE	0x00000U
diff --git a/iptables/xtables.c b/iptables/xtables.c
index bb76e6a7a1ce8..805bd801fb060 100644
--- a/iptables/xtables.c
+++ b/iptables/xtables.c
@@ -50,8 +50,6 @@
 #define FALSE 0
 #endif
 
-#define NUMBER_OF_CMD	16
-
 #define OPT_FRAGMENT	0x00800U
 #define NUMBER_OF_OPT	ARRAY_SIZE(optflags)
 static const char optflags[]
-- 
2.23.0

[iptables PATCH 05/10] xtables-arp: Drop generic_opt_check()

[Phil Sutter]

With all fields in commands_v_options[][] being whitespace, the function
is effectively a noop.

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

diff --git a/iptables/xtables-arp.c b/iptables/xtables-arp.c
index 88a7d534da4f1..ae69baf2a9346 100644
--- a/iptables/xtables-arp.c
+++ b/iptables/xtables-arp.c
@@ -139,34 +139,6 @@ struct xtables_globals arptables_globals = {
 	.compat_rev		= nft_compatible_revision,
 };
 
-/* Table of legal combinations of commands and options.  If any of the
- * given commands make an option legal, that option is legal (applies to
- * CMD_LIST and CMD_ZERO only).
- * Key:
- *  +  compulsory
- *  x  illegal
- *     optional
- */
-
-static char commands_v_options[NUMBER_OF_CMD][NUMBER_OF_OPT] =
-/* Well, it's better than "Re: Linux vs FreeBSD" */
-{
-	/*     -n  -s  -d  -p  -j  -v  -x  -i  -o  -f  --line */
-/*INSERT*/    {' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' '},
-/*DELETE*/    {' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' '},
-/*DELETE_NUM*/{' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' '},
-/*REPLACE*/   {' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' '},
-/*APPEND*/    {' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' '},
-/*LIST*/      {' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' '},
-/*FLUSH*/     {' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' '},
-/*ZERO*/      {' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' '},
-/*NEW_CHAIN*/ {' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' '},
-/*DEL_CHAIN*/ {' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' '},
-/*SET_POLICY*/{' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' '},
-/*CHECK*/     {' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' '},
-/*RENAME*/    {' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ',' '}
-};
-
 static int inverse_for_options[NUMBER_OF_OPT] =
 {
 /* -n */ 0,
@@ -395,42 +367,6 @@ exit_printhelp(void)
 	exit(0);
 }
 
-static void
-generic_opt_check(int command, int options)
-{
-	int i, j, legal = 0;
-
-	/* Check that commands are valid with options.  Complicated by the
-	 * fact that if an option is legal with *any* command given, it is
-	 * legal overall (ie. -z and -l).
-	 */
-	for (i = 0; i < NUMBER_OF_OPT; i++) {
-		legal = 0; /* -1 => illegal, 1 => legal, 0 => undecided. */
-
-		for (j = 0; j < NUMBER_OF_CMD; j++) {
-			if (!(command & (1<<j)))
-				continue;
-
-			if (!(options & (1<<i))) {
-				if (commands_v_options[j][i] == '+')
-					xtables_error(PARAMETER_PROBLEM,
-						      "You need to supply the `-%c' "
-						      "option for this command\n",
-						      optflags[i]);
-			} else {
-				if (commands_v_options[j][i] != 'x')
-					legal = 1;
-				else if (legal == 0)
-					legal = -1;
-			}
-		}
-		if (legal == -1)
-			xtables_error(PARAMETER_PROBLEM,
-				      "Illegal option `-%c' with this command\n",
-				      optflags[i]);
-	}
-}
-
 static char
 opt2char(int option)
 {
@@ -1059,8 +995,6 @@ int do_commandarp(struct nft_handle *h, int argc, char *argv[], char **table,
 		xtables_error(PARAMETER_PROBLEM, "Replacement rule does not "
 						 "specify a unique address");
 
-	generic_opt_check(command, options);
-
 	if (chain && strlen(chain) > ARPT_FUNCTION_MAXNAMELEN)
 		xtables_error(PARAMETER_PROBLEM,
 				"chain name `%s' too long (must be under %i chars)",
-- 
2.23.0

[iptables PATCH 06/10] Replace TRUE/FALSE with true/false

[Phil Sutter]

And drop the conditional defines.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/ip6tables.c   | 14 +++-----------
 iptables/iptables.c    | 12 ++----------
 iptables/xtables-arp.c | 17 +++++------------
 iptables/xtables.c     | 11 ++---------
 4 files changed, 12 insertions(+), 42 deletions(-)

diff --git a/iptables/ip6tables.c b/iptables/ip6tables.c
index e48fdeb1248bd..576c2cf8b0d9f 100644
--- a/iptables/ip6tables.c
+++ b/iptables/ip6tables.c
@@ -45,14 +45,6 @@
 #include "ip6tables-multi.h"
 #include "xshared.h"
 
-#ifndef TRUE
-#define TRUE 1
-#endif
-#ifndef FALSE
-#define FALSE 0
-#endif
-
-
 #define NUMBER_OF_OPT	ARRAY_SIZE(optflags)
 static const char optflags[]
 = { 'n', 's', 'd', 'p', 'j', 'v', 'x', 'i', 'o', '0', 'c'};
@@ -1525,7 +1517,7 @@ int do_command6(int argc, char *argv[], char **table,
 					xtables_error(PARAMETER_PROBLEM,
 						   "multiple consecutive ! not"
 						   " allowed");
-				cs.invert = TRUE;
+				cs.invert = true;
 				optarg[0] = '\0';
 				continue;
 			}
@@ -1537,12 +1529,12 @@ int do_command6(int argc, char *argv[], char **table,
 				/*
 				 * If new options were loaded, we must retry
 				 * getopt immediately and not allow
-				 * cs.invert=FALSE to be executed.
+				 * cs.invert=false to be executed.
 				 */
 				continue;
 			break;
 		}
-		cs.invert = FALSE;
+		cs.invert = false;
 	}
 
 	if (!wait && wait_interval_set)
diff --git a/iptables/iptables.c b/iptables/iptables.c
index 255b61b11ec89..88ef6cf666d4b 100644
--- a/iptables/iptables.c
+++ b/iptables/iptables.c
@@ -41,14 +41,6 @@
 #include <fcntl.h>
 #include "xshared.h"
 
-#ifndef TRUE
-#define TRUE 1
-#endif
-#ifndef FALSE
-#define FALSE 0
-#endif
-
-
 #define OPT_FRAGMENT    0x00800U
 #define NUMBER_OF_OPT	ARRAY_SIZE(optflags)
 static const char optflags[]
@@ -1518,7 +1510,7 @@ int do_command4(int argc, char *argv[], char **table,
 					xtables_error(PARAMETER_PROBLEM,
 						   "multiple consecutive ! not"
 						   " allowed");
-				cs.invert = TRUE;
+				cs.invert = true;
 				optarg[0] = '\0';
 				continue;
 			}
@@ -1531,7 +1523,7 @@ int do_command4(int argc, char *argv[], char **table,
 				continue;
 			break;
 		}
-		cs.invert = FALSE;
+		cs.invert = false;
 	}
 
 	if (!wait && wait_interval_set)
diff --git a/iptables/xtables-arp.c b/iptables/xtables-arp.c
index ae69baf2a9346..4949ddd3d486c 100644
--- a/iptables/xtables-arp.c
+++ b/iptables/xtables-arp.c
@@ -55,13 +55,6 @@
 
 typedef char arpt_chainlabel[32];
 
-#ifndef TRUE
-#define TRUE 1
-#endif
-#ifndef FALSE
-#define FALSE 0
-#endif
-
 #define OPTION_OFFSET 256
 
 #define OPT_NONE	0x00000U
@@ -383,7 +376,7 @@ check_inverse(const char option[], int *invert, int *optidx, int argc)
 		if (*invert)
 			xtables_error(PARAMETER_PROBLEM,
 				      "Multiple `!' flags not allowed");
-		*invert = TRUE;
+		*invert = true;
 		if (optidx) {
 			*optidx = *optidx+1;
 			if (argc && *optidx > argc)
@@ -391,9 +384,9 @@ check_inverse(const char option[], int *invert, int *optidx, int argc)
 					      "no argument following `!'");
 		}
 
-		return TRUE;
+		return true;
 	}
-	return FALSE;
+	return false;
 }
 
 static void
@@ -942,7 +935,7 @@ int do_commandarp(struct nft_handle *h, int argc, char *argv[], char **table,
 					xtables_error(PARAMETER_PROBLEM,
 						      "multiple consecutive ! not"
 						      " allowed");
-				invert = TRUE;
+				invert = true;
 				optarg[0] = '\0';
 				continue;
 			}
@@ -956,7 +949,7 @@ int do_commandarp(struct nft_handle *h, int argc, char *argv[], char **table,
 			}
 			break;
 		}
-		invert = FALSE;
+		invert = false;
 	}
 
 	if (cs.target)
diff --git a/iptables/xtables.c b/iptables/xtables.c
index 805bd801fb060..8f9dc628d0029 100644
--- a/iptables/xtables.c
+++ b/iptables/xtables.c
@@ -43,13 +43,6 @@
 #include "nft-shared.h"
 #include "nft.h"
 
-#ifndef TRUE
-#define TRUE 1
-#endif
-#ifndef FALSE
-#define FALSE 0
-#endif
-
 #define OPT_FRAGMENT	0x00800U
 #define NUMBER_OF_OPT	ARRAY_SIZE(optflags)
 static const char optflags[]
@@ -952,7 +945,7 @@ void do_parse(struct nft_handle *h, int argc, char *argv[],
 					xtables_error(PARAMETER_PROBLEM,
 						   "multiple consecutive ! not"
 						   " allowed");
-				cs->invert = TRUE;
+				cs->invert = true;
 				optarg[0] = '\0';
 				continue;
 			}
@@ -965,7 +958,7 @@ void do_parse(struct nft_handle *h, int argc, char *argv[],
 				continue;
 			break;
 		}
-		cs->invert = FALSE;
+		cs->invert = false;
 	}
 
 	if (strcmp(p->table, "nat") == 0 &&
-- 
2.23.0

[iptables PATCH 07/10] xtables-arp: Integrate OPT_* defines into xshared.h

[Phil Sutter]

These defines are internal use only, so their actual value doesn't
matter as long as they're unique.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/xshared.h     |  7 +++++++
 iptables/xtables-arp.c | 25 ++++---------------------
 2 files changed, 11 insertions(+), 21 deletions(-)

diff --git a/iptables/xshared.h b/iptables/xshared.h
index b0738b042e95a..490b19ade5106 100644
--- a/iptables/xshared.h
+++ b/iptables/xshared.h
@@ -29,6 +29,13 @@ enum {
 	OPT_VIANAMEOUT  = 1 << 8,
 	OPT_LINENUMBERS = 1 << 9,
 	OPT_COUNTERS    = 1 << 10,
+	/* below are for arptables only */
+	OPT_S_MAC	= 1 << 11,
+	OPT_D_MAC	= 1 << 12,
+	OPT_H_LENGTH	= 1 << 13,
+	OPT_OPCODE	= 1 << 14,
+	OPT_H_TYPE	= 1 << 15,
+	OPT_P_TYPE	= 1 << 16,
 };
 
 enum {
diff --git a/iptables/xtables-arp.c b/iptables/xtables-arp.c
index 4949ddd3d486c..e4614b57a437f 100644
--- a/iptables/xtables-arp.c
+++ b/iptables/xtables-arp.c
@@ -57,23 +57,6 @@ typedef char arpt_chainlabel[32];
 
 #define OPTION_OFFSET 256
 
-#define OPT_NONE	0x00000U
-#define OPT_NUMERIC	0x00001U
-#define OPT_S_IP	0x00002U
-#define OPT_D_IP	0x00004U
-#define OPT_S_MAC	0x00008U
-#define OPT_D_MAC	0x00010U
-#define OPT_H_LENGTH	0x00020U
-#define OPT_P_LENGTH	0x00040U
-#define OPT_OPCODE	0x00080U
-#define OPT_H_TYPE	0x00100U
-#define OPT_P_TYPE	0x00200U
-#define OPT_JUMP	0x00400U
-#define OPT_VERBOSE	0x00800U
-#define OPT_VIANAMEIN	0x01000U
-#define OPT_VIANAMEOUT	0x02000U
-#define OPT_LINENUMBERS 0x04000U
-#define OPT_COUNTERS	0x08000U
 #define NUMBER_OF_OPT	16
 static const char optflags[NUMBER_OF_OPT]
 = { 'n', 's', 'd', 2, 3, 7, 8, 4, 5, 6, 'j', 'v', 'i', 'o', '0', 'c'};
@@ -747,14 +730,14 @@ int do_commandarp(struct nft_handle *h, int argc, char *argv[], char **table,
 			break;
 		case 's':
 			check_inverse(optarg, &invert, &optind, argc);
-			set_option(&options, OPT_S_IP, &cs.arp.arp.invflags,
+			set_option(&options, OPT_SOURCE, &cs.arp.arp.invflags,
 				   invert);
 			shostnetworkmask = argv[optind-1];
 			break;
 
 		case 'd':
 			check_inverse(optarg, &invert, &optind, argc);
-			set_option(&options, OPT_D_IP, &cs.arp.arp.invflags,
+			set_option(&options, OPT_DESTINATION, &cs.arp.arp.invflags,
 				   invert);
 			dhostnetworkmask = argv[optind-1];
 			break;
@@ -965,9 +948,9 @@ int do_commandarp(struct nft_handle *h, int argc, char *argv[], char **table,
 			      "nothing appropriate following !");
 
 	if (command & (CMD_REPLACE | CMD_INSERT | CMD_DELETE | CMD_APPEND)) {
-		if (!(options & OPT_D_IP))
+		if (!(options & OPT_DESTINATION))
 			dhostnetworkmask = "0.0.0.0/0";
-		if (!(options & OPT_S_IP))
+		if (!(options & OPT_SOURCE))
 			shostnetworkmask = "0.0.0.0/0";
 	}
 
-- 
2.23.0

[iptables PATCH 08/10] xtables-arp: Drop some unused variables

[Phil Sutter]

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/xtables-arp.c | 17 -----------------
 1 file changed, 17 deletions(-)

diff --git a/iptables/xtables-arp.c b/iptables/xtables-arp.c
index e4614b57a437f..11ca7f869730e 100644
--- a/iptables/xtables-arp.c
+++ b/iptables/xtables-arp.c
@@ -53,10 +53,6 @@
 #include "nft-arp.h"
 #include <linux/netfilter_arp/arp_tables.h>
 
-typedef char arpt_chainlabel[32];
-
-#define OPTION_OFFSET 256
-
 #define NUMBER_OF_OPT	16
 static const char optflags[NUMBER_OF_OPT]
 = { 'n', 's', 'd', 2, 3, 7, 8, 4, 5, 6, 'j', 'v', 'i', 'o', '0', 'c'};
@@ -102,8 +98,6 @@ static struct option original_opts[] = {
 	{ 0 }
 };
 
-int RUNTIME_NF_ARP_NUMHOOKS = 3;
-
 #define opts xt_params->opts
 
 extern void xtables_exit_error(enum xtables_exittype status, const char *msg, ...) __attribute__((noreturn, format(printf,2,3)));
@@ -135,17 +129,6 @@ static int inverse_for_options[NUMBER_OF_OPT] =
 /* -c */ 0,
 };
 
-/* Primitive headers... */
-/* defined in netinet/in.h */
-#if 0
-#ifndef IPPROTO_ESP
-#define IPPROTO_ESP 50
-#endif
-#ifndef IPPROTO_AH
-#define IPPROTO_AH 51
-#endif
-#endif
-
 /***********************************************/
 /* ARPTABLES SPECIFIC NEW FUNCTIONS ADDED HERE */
 /***********************************************/
-- 
2.23.0

[iptables PATCH 09/10] xtables-arp: Use xtables_parse_interface()

[Phil Sutter]

The local implementation differs just slightly but libxtables version
seems more correct (no needless memsetting of mask, more relevant
illegal character checking) so use that one.

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

diff --git a/iptables/xtables-arp.c b/iptables/xtables-arp.c
index 11ca7f869730e..d6040e0c73f4b 100644
--- a/iptables/xtables-arp.c
+++ b/iptables/xtables-arp.c
@@ -355,44 +355,6 @@ check_inverse(const char option[], int *invert, int *optidx, int argc)
 	return false;
 }
 
-static void
-parse_interface(const char *arg, char *vianame, unsigned char *mask)
-{
-	int vialen = strlen(arg);
-	unsigned int i;
-
-	memset(mask, 0, IFNAMSIZ);
-	memset(vianame, 0, IFNAMSIZ);
-
-	if (vialen + 1 > IFNAMSIZ)
-		xtables_error(PARAMETER_PROBLEM,
-			      "interface name `%s' must be shorter than IFNAMSIZ"
-			      " (%i)", arg, IFNAMSIZ-1);
-
-	strcpy(vianame, arg);
-	if (vialen == 0)
-		memset(mask, 0, IFNAMSIZ);
-	else if (vianame[vialen - 1] == '+') {
-		memset(mask, 0xFF, vialen - 1);
-		memset(mask + vialen - 1, 0, IFNAMSIZ - vialen + 1);
-		/* Don't remove `+' here! -HW */
-	} else {
-		/* Include nul-terminator in match */
-		memset(mask, 0xFF, vialen + 1);
-		memset(mask + vialen + 1, 0, IFNAMSIZ - vialen - 1);
-		for (i = 0; vianame[i]; i++) {
-			if (!isalnum(vianame[i])
-			    && vianame[i] != '_'
-			    && vianame[i] != '.') {
-				printf("Warning: weird character in interface"
-				       " `%s' (No aliases, :, ! or *).\n",
-				       vianame);
-				break;
-			}
-		}
-	}
-}
-
 static void
 set_option(unsigned int *options, unsigned int option, u_int16_t *invflg,
 	   int invert)
@@ -814,18 +776,18 @@ int do_commandarp(struct nft_handle *h, int argc, char *argv[], char **table,
 			check_inverse(optarg, &invert, &optind, argc);
 			set_option(&options, OPT_VIANAMEIN, &cs.arp.arp.invflags,
 				   invert);
-			parse_interface(argv[optind-1],
-					cs.arp.arp.iniface,
-					cs.arp.arp.iniface_mask);
+			xtables_parse_interface(argv[optind-1],
+						cs.arp.arp.iniface,
+						cs.arp.arp.iniface_mask);
 			break;
 
 		case 'o':
 			check_inverse(optarg, &invert, &optind, argc);
 			set_option(&options, OPT_VIANAMEOUT, &cs.arp.arp.invflags,
 				   invert);
-			parse_interface(argv[optind-1],
-					cs.arp.arp.outiface,
-					cs.arp.arp.outiface_mask);
+			xtables_parse_interface(argv[optind-1],
+						cs.arp.arp.outiface,
+						cs.arp.arp.outiface_mask);
 			break;
 
 		case 'v':
-- 
2.23.0

[iptables PATCH 10/10] nft-arp: Use xtables_print_mac_and_mask()

[Phil Sutter]

This libxtables function does exactly what the local implementation did.
The only noteworthy difference is that it assumes MAC/mask lengths, but
the local implementation was passed ETH_ALEN in each invocation, so no
practical difference.

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

diff --git a/iptables/nft-arp.c b/iptables/nft-arp.c
index 9805bbe0de87b..7068f82c5495a 100644
--- a/iptables/nft-arp.c
+++ b/iptables/nft-arp.c
@@ -114,29 +114,6 @@ mask_to_dotted(const struct in_addr *mask)
 	return buf;
 }
 
-static void print_mac(const unsigned char *mac, int l)
-{
-	int j;
-
-	for (j = 0; j < l; j++)
-		printf("%02x%s", mac[j],
-			(j==l-1) ? "" : ":");
-}
-
-static void print_mac_and_mask(const unsigned char *mac, const unsigned char *mask, int l)
-{
-	int i;
-
-	print_mac(mac, l);
-	for (i = 0; i < l ; i++)
-		if (mask[i] != 255)
-			break;
-	if (i == l)
-		return;
-	printf("/");
-	print_mac(mask, l);
-}
-
 static bool need_devaddr(struct arpt_devaddr_info *info)
 {
 	int i;
@@ -506,8 +483,8 @@ static void nft_arp_print_rule_details(const struct iptables_command_state *cs,
 	printf("%s%s", sep, fw->arp.invflags & ARPT_INV_SRCDEVADDR
 		? "! " : "");
 	printf("--src-mac ");
-	print_mac_and_mask((unsigned char *)fw->arp.src_devaddr.addr,
-		(unsigned char *)fw->arp.src_devaddr.mask, ETH_ALEN);
+	xtables_print_mac_and_mask((unsigned char *)fw->arp.src_devaddr.addr,
+				   (unsigned char *)fw->arp.src_devaddr.mask);
 	sep = " ";
 after_devsrc:
 
@@ -532,8 +509,8 @@ after_devsrc:
 	printf("%s%s", sep, fw->arp.invflags & ARPT_INV_TGTDEVADDR
 		? "! " : "");
 	printf("--dst-mac ");
-	print_mac_and_mask((unsigned char *)fw->arp.tgt_devaddr.addr,
-		(unsigned char *)fw->arp.tgt_devaddr.mask, ETH_ALEN);
+	xtables_print_mac_and_mask((unsigned char *)fw->arp.tgt_devaddr.addr,
+				   (unsigned char *)fw->arp.tgt_devaddr.mask);
 	sep = " ";
 
 after_devdst:
-- 
2.23.0

Re: [iptables PATCH 00/10] Reduce code size around arptables-nft

[Pablo Neira Ayuso]

On Mon, Oct 28, 2019 at 03:04:21PM +0100, Phil Sutter wrote:
> A review of xtables-arp.c exposed a significant amount of dead, needless
> or duplicated code. This series deals with some low hanging fruits. Most
> of the changes affect xtables-arp.c and nft-arp.c only, but where common
> issues existed or code was to be shared, other files are touched as
> well.

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

Re: [iptables PATCH 00/10] Reduce code size around arptables-nft

[Phil Sutter]

On Mon, Oct 28, 2019 at 03:17:05PM +0100, Pablo Neira Ayuso wrote:
> On Mon, Oct 28, 2019 at 03:04:21PM +0100, Phil Sutter wrote:
> > A review of xtables-arp.c exposed a significant amount of dead, needless
> > or duplicated code. This series deals with some low hanging fruits. Most
> > of the changes affect xtables-arp.c and nft-arp.c only, but where common
> > issues existed or code was to be shared, other files are touched as
> > well.
> 
> Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>

I'll have to respin, patch 7 breaks arptables-nft for inverted options -
I missed how the actual values are used in inverse_for_options variable
and ran 'iptables-test.py -n' just now.

Sorry for the noise!

Cheers, Phil