[iptables PATCH 0/3] xshared: Review option parsing

[iptables PATCH 0/3] xshared: Review option parsing

[Phil Sutter]

Faced with the need to extend optflags, inverse_for_options and
commands_v_options arrays in order to integrate ebtables-specific
options, I chose to get rid of them all instead.

Patch 1 replaces opt2char() function which didn't work well since not
every option has printable short-option character. The callback-based
replacement returns the long-option and should make error messages more
readable this way.

Patch 2 elminates the inverse_for_options array along with the loop
turning a given option into its bit's position for use in the array. The
switch statement in the replacing callback is much easier to maintain
and extend.

Patch 3 makes use of the fact that no command has a mandatory option
anymore. So every combination is either allowed or not, and a single bit
may indicate that.

Phil Sutter (3):
  xshared: Introduce xt_cmd_parse_ops::option_name
  xshared: Introduce xt_cmd_parse_ops::option_invert
  xshared: Simplify generic_opt_check()

 iptables/ip6tables.c |   2 +
 iptables/iptables.c  |   2 +
 iptables/nft-arp.c   |  32 ++++++
 iptables/nft-ipv4.c  |   2 +
 iptables/nft-ipv6.c  |   2 +
 iptables/xshared.c   | 249 ++++++++++++++++++++-----------------------
 iptables/xshared.h   |   6 ++
 7 files changed, 159 insertions(+), 136 deletions(-)

-- 
2.41.0

[iptables PATCH 1/3] xshared: Introduce xt_cmd_parse_ops::option_name

[Phil Sutter]

The old opt2char() function was flawed: Since not every field in
optflags contains a printable character, typical use of its return value
in print statements could lead to garbage on screen.

Replace this by a mechanism to retrieve an option's long name which
supports family-specific overrides. and get rid of optflags field
altogether and define NUMBER_OF_OPT similar to NUMBER_OF_CMD.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/ip6tables.c |   1 +
 iptables/iptables.c  |   1 +
 iptables/nft-arp.c   |  18 ++++++
 iptables/nft-ipv4.c  |   1 +
 iptables/nft-ipv6.c  |   1 +
 iptables/xshared.c   | 140 +++++++++++++++++++++++--------------------
 iptables/xshared.h   |   4 ++
 7 files changed, 100 insertions(+), 66 deletions(-)

diff --git a/iptables/ip6tables.c b/iptables/ip6tables.c
index 9afc32c1a21ed..85cb211d2ec12 100644
--- a/iptables/ip6tables.c
+++ b/iptables/ip6tables.c
@@ -669,6 +669,7 @@ int do_command6(int argc, char *argv[], char **table,
 	struct xt_cmd_parse_ops cmd_parse_ops = {
 		.proto_parse	= ipv6_proto_parse,
 		.post_parse	= ipv6_post_parse,
+		.option_name	= ip46t_option_name,
 	};
 	struct xt_cmd_parse p = {
 		.table		= *table,
diff --git a/iptables/iptables.c b/iptables/iptables.c
index 6f7b34762ea40..4bfce62dd5d86 100644
--- a/iptables/iptables.c
+++ b/iptables/iptables.c
@@ -663,6 +663,7 @@ int do_command4(int argc, char *argv[], char **table,
 	struct xt_cmd_parse_ops cmd_parse_ops = {
 		.proto_parse	= ipv4_proto_parse,
 		.post_parse	= ipv4_post_parse,
+		.option_name	= ip46t_option_name,
 	};
 	struct xt_cmd_parse p = {
 		.table		= *table,
diff --git a/iptables/nft-arp.c b/iptables/nft-arp.c
index 38b2ab3993128..6f8e1952db3b8 100644
--- a/iptables/nft-arp.c
+++ b/iptables/nft-arp.c
@@ -815,6 +815,23 @@ static int nft_arp_xlate(const struct iptables_command_state *cs,
 	return xlate_action(cs, false, xl);
 }
 
+static const char *nft_arp_option_name(int option)
+{
+	switch (option) {
+	default:		return ip46t_option_name(option);
+	/* different name than iptables */
+	case OPT_SOURCE:	return "--source-ip";
+	case OPT_DESTINATION:	return "--destination-ip";
+	/* arptables specific ones */
+	case OPT_S_MAC:		return "--source-mac";
+	case OPT_D_MAC:		return "--destination-mac";
+	case OPT_H_LENGTH:	return "--h-length";
+	case OPT_OPCODE:	return "--opcode";
+	case OPT_H_TYPE:	return "--h-type";
+	case OPT_P_TYPE:	return "--proto-type";
+	}
+}
+
 struct nft_family_ops nft_family_ops_arp = {
 	.add			= nft_arp_add,
 	.is_same		= nft_arp_is_same,
@@ -826,6 +843,7 @@ struct nft_family_ops nft_family_ops_arp = {
 	.rule_parse		= &nft_ruleparse_ops_arp,
 	.cmd_parse		= {
 		.post_parse	= nft_arp_post_parse,
+		.option_name	= nft_arp_option_name,
 	},
 	.rule_to_cs		= nft_rule_to_iptables_command_state,
 	.init_cs		= nft_arp_init_cs,
diff --git a/iptables/nft-ipv4.c b/iptables/nft-ipv4.c
index 75912847aea3e..166680b3eb07c 100644
--- a/iptables/nft-ipv4.c
+++ b/iptables/nft-ipv4.c
@@ -353,6 +353,7 @@ struct nft_family_ops nft_family_ops_ipv4 = {
 	.cmd_parse		= {
 		.proto_parse	= ipv4_proto_parse,
 		.post_parse	= ipv4_post_parse,
+		.option_name	= ip46t_option_name,
 	},
 	.rule_to_cs		= nft_rule_to_iptables_command_state,
 	.clear_cs		= xtables_clear_iptables_command_state,
diff --git a/iptables/nft-ipv6.c b/iptables/nft-ipv6.c
index 5aef365b79f2a..2cc45944f6c04 100644
--- a/iptables/nft-ipv6.c
+++ b/iptables/nft-ipv6.c
@@ -344,6 +344,7 @@ struct nft_family_ops nft_family_ops_ipv6 = {
 	.cmd_parse		= {
 		.proto_parse	= ipv6_proto_parse,
 		.post_parse	= ipv6_post_parse,
+		.option_name	= ip46t_option_name,
 	},
 	.rule_to_cs		= nft_rule_to_iptables_command_state,
 	.clear_cs		= xtables_clear_iptables_command_state,
diff --git a/iptables/xshared.c b/iptables/xshared.c
index 1b02f35a9de3a..31a3019592317 100644
--- a/iptables/xshared.c
+++ b/iptables/xshared.c
@@ -920,10 +920,6 @@ static int parse_rulenumber(const char *rule)
 	return rulenum;
 }
 
-#define NUMBER_OF_OPT	ARRAY_SIZE(optflags)
-static const char optflags[]
-= { 'n', 's', 'd', 'p', 'j', 'v', 'x', 'i', 'o', '0', 'c', 'f', 2, 3, 'l', 4, 5, 6 };
-
 /* 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).
@@ -953,7 +949,8 @@ static const char commands_v_options[NUMBER_OF_CMD][NUMBER_OF_OPT] =
 /*CHECK*/     {'x',' ',' ',' ',' ',' ','x',' ',' ','x','x',' ',' ',' ',' ',' ',' ',' '},
 };
 
-static void generic_opt_check(int command, int options)
+static void generic_opt_check(struct xt_cmd_parse_ops *ops,
+			      int command, int options)
 {
 	int i, j, legal = 0;
 
@@ -971,8 +968,8 @@ static void generic_opt_check(int command, int options)
 			if (!(options & (1<<i))) {
 				if (commands_v_options[j][i] == '+')
 					xtables_error(PARAMETER_PROBLEM,
-						      "You need to supply the `-%c' option for this command",
-						      optflags[i]);
+						      "You need to supply the `%s' option for this command",
+						      ops->option_name(1<<i));
 			} else {
 				if (commands_v_options[j][i] != 'x')
 					legal = 1;
@@ -982,19 +979,28 @@ static void generic_opt_check(int command, int options)
 		}
 		if (legal == -1)
 			xtables_error(PARAMETER_PROBLEM,
-				      "Illegal option `-%c' with this command",
-				      optflags[i]);
+				      "Illegal option `%s' with this command",
+				      ops->option_name(1<<i));
 	}
 }
 
-static char opt2char(int option)
+const char *ip46t_option_name(int option)
 {
-	const char *ptr;
-
-	for (ptr = optflags; option > 1; option >>= 1, ptr++)
-		;
-
-	return *ptr;
+	switch (option) {
+	case OPT_NUMERIC:	return "--numeric";
+	case OPT_SOURCE:	return "--source";
+	case OPT_DESTINATION:	return "--destination";
+	case OPT_PROTOCOL:	return "--protocol";
+	case OPT_JUMP:		return "--jump";
+	case OPT_VERBOSE:	return "--verbose";
+	case OPT_EXPANDED:	return "--exact";
+	case OPT_VIANAMEIN:	return "--in-interface";
+	case OPT_VIANAMEOUT:	return "--out-interface";
+	case OPT_LINENUMBERS:	return "--line-numbers";
+	case OPT_COUNTERS:	return "--set-counters";
+	case OPT_FRAGMENT:	return "--fragments";
+	default:		return "unknown option";
+	}
 }
 
 static const int inverse_for_options[NUMBER_OF_OPT] =
@@ -1020,12 +1026,14 @@ static const int inverse_for_options[NUMBER_OF_OPT] =
 };
 
 static void
-set_option(unsigned int *options, unsigned int option, uint16_t *invflg,
-	   bool invert)
+set_option(struct xt_cmd_parse_ops *ops,
+	   unsigned int *options, unsigned int option,
+	   uint16_t *invflg, bool invert)
 {
 	if (*options & option)
-		xtables_error(PARAMETER_PROBLEM, "multiple -%c flags not allowed",
-			   opt2char(option));
+		xtables_error(PARAMETER_PROBLEM,
+			      "multiple %s options not allowed",
+			      ops->option_name(option));
 	*options |= option;
 
 	if (invert) {
@@ -1034,8 +1042,8 @@ set_option(unsigned int *options, unsigned int option, uint16_t *invflg,
 
 		if (!inverse_for_options[i])
 			xtables_error(PARAMETER_PROBLEM,
-				   "cannot have ! before -%c",
-				   opt2char(option));
+				      "cannot have ! before %s",
+				      ops->option_name(option));
 		*invflg |= inverse_for_options[i];
 	}
 }
@@ -1543,7 +1551,7 @@ void do_parse(int argc, char *argv[],
 			 */
 		case 'p':
 			check_inverse(args, optarg, &invert, argc, argv);
-			set_option(&cs->options, OPT_PROTOCOL,
+			set_option(p->ops, &cs->options, OPT_PROTOCOL,
 				   &args->invflags, invert);
 
 			/* Canonicalize into lower case */
@@ -1566,22 +1574,22 @@ void do_parse(int argc, char *argv[],
 
 		case 's':
 			check_inverse(args, optarg, &invert, argc, argv);
-			set_option(&cs->options, OPT_SOURCE,
+			set_option(p->ops, &cs->options, OPT_SOURCE,
 				   &args->invflags, invert);
 			args->shostnetworkmask = optarg;
 			break;
 
 		case 'd':
 			check_inverse(args, optarg, &invert, argc, argv);
-			set_option(&cs->options, OPT_DESTINATION,
+			set_option(p->ops, &cs->options, OPT_DESTINATION,
 				   &args->invflags, invert);
 			args->dhostnetworkmask = optarg;
 			break;
 
 #ifdef IPT_F_GOTO
 		case 'g':
-			set_option(&cs->options, OPT_JUMP, &args->invflags,
-				   invert);
+			set_option(p->ops, &cs->options, OPT_JUMP,
+				   &args->invflags, invert);
 			args->goto_set = true;
 			cs->jumpto = xt_parse_target(optarg);
 			break;
@@ -1589,22 +1597,22 @@ void do_parse(int argc, char *argv[],
 
 		case 2:/* src-mac */
 			check_inverse(args, optarg, &invert, argc, argv);
-			set_option(&cs->options, OPT_S_MAC, &args->invflags,
-				   invert);
+			set_option(p->ops, &cs->options, OPT_S_MAC,
+				   &args->invflags, invert);
 			args->src_mac = optarg;
 			break;
 
 		case 3:/* dst-mac */
 			check_inverse(args, optarg, &invert, argc, argv);
-			set_option(&cs->options, OPT_D_MAC, &args->invflags,
-				   invert);
+			set_option(p->ops, &cs->options, OPT_D_MAC,
+				   &args->invflags, invert);
 			args->dst_mac = optarg;
 			break;
 
 		case 'l':/* hardware length */
 			check_inverse(args, optarg, &invert, argc, argv);
-			set_option(&cs->options, OPT_H_LENGTH, &args->invflags,
-				   invert);
+			set_option(p->ops, &cs->options, OPT_H_LENGTH,
+				   &args->invflags, invert);
 			args->arp_hlen = optarg;
 			break;
 
@@ -1612,28 +1620,28 @@ void do_parse(int argc, char *argv[],
 			xtables_error(PARAMETER_PROBLEM, "not supported");
 		case 4:/* opcode */
 			check_inverse(args, optarg, &invert, argc, argv);
-			set_option(&cs->options, OPT_OPCODE, &args->invflags,
-				   invert);
+			set_option(p->ops, &cs->options, OPT_OPCODE,
+				   &args->invflags, invert);
 			args->arp_opcode = optarg;
 			break;
 
 		case 5:/* h-type */
 			check_inverse(args, optarg, &invert, argc, argv);
-			set_option(&cs->options, OPT_H_TYPE, &args->invflags,
-				   invert);
+			set_option(p->ops, &cs->options, OPT_H_TYPE,
+				   &args->invflags, invert);
 			args->arp_htype = optarg;
 			break;
 
 		case 6:/* proto-type */
 			check_inverse(args, optarg, &invert, argc, argv);
-			set_option(&cs->options, OPT_P_TYPE, &args->invflags,
-				   invert);
+			set_option(p->ops, &cs->options, OPT_P_TYPE,
+				   &args->invflags, invert);
 			args->arp_ptype = optarg;
 			break;
 
 		case 'j':
-			set_option(&cs->options, OPT_JUMP, &args->invflags,
-				   invert);
+			set_option(p->ops, &cs->options, OPT_JUMP,
+				   &args->invflags, invert);
 			if (strcmp(optarg, "CONTINUE"))
 				command_jump(cs, optarg);
 			break;
@@ -1641,7 +1649,7 @@ void do_parse(int argc, char *argv[],
 		case 'i':
 			check_empty_interface(args, optarg);
 			check_inverse(args, optarg, &invert, argc, argv);
-			set_option(&cs->options, OPT_VIANAMEIN,
+			set_option(p->ops, &cs->options, OPT_VIANAMEIN,
 				   &args->invflags, invert);
 			xtables_parse_interface(optarg,
 						args->iniface,
@@ -1651,7 +1659,7 @@ void do_parse(int argc, char *argv[],
 		case 'o':
 			check_empty_interface(args, optarg);
 			check_inverse(args, optarg, &invert, argc, argv);
-			set_option(&cs->options, OPT_VIANAMEOUT,
+			set_option(p->ops, &cs->options, OPT_VIANAMEOUT,
 				   &args->invflags, invert);
 			xtables_parse_interface(optarg,
 						args->outiface,
@@ -1664,14 +1672,14 @@ void do_parse(int argc, char *argv[],
 					"`-f' is not supported in IPv6, "
 					"use -m frag instead");
 			}
-			set_option(&cs->options, OPT_FRAGMENT, &args->invflags,
-				   invert);
+			set_option(p->ops, &cs->options, OPT_FRAGMENT,
+				   &args->invflags, invert);
 			args->flags |= IPT_F_FRAG;
 			break;
 
 		case 'v':
 			if (!p->verbose)
-				set_option(&cs->options, OPT_VERBOSE,
+				set_option(p->ops, &cs->options, OPT_VERBOSE,
 					   &args->invflags, invert);
 			p->verbose++;
 			break;
@@ -1681,8 +1689,8 @@ void do_parse(int argc, char *argv[],
 			break;
 
 		case 'n':
-			set_option(&cs->options, OPT_NUMERIC, &args->invflags,
-				   invert);
+			set_option(p->ops, &cs->options, OPT_NUMERIC,
+				   &args->invflags, invert);
 			break;
 
 		case 't':
@@ -1698,8 +1706,8 @@ void do_parse(int argc, char *argv[],
 			break;
 
 		case 'x':
-			set_option(&cs->options, OPT_EXPANDED, &args->invflags,
-				   invert);
+			set_option(p->ops, &cs->options, OPT_EXPANDED,
+				   &args->invflags, invert);
 			break;
 
 		case 'V':
@@ -1734,7 +1742,7 @@ void do_parse(int argc, char *argv[],
 			break;
 
 		case '0':
-			set_option(&cs->options, OPT_LINENUMBERS,
+			set_option(p->ops, &cs->options, OPT_LINENUMBERS,
 				   &args->invflags, invert);
 			break;
 
@@ -1743,8 +1751,8 @@ void do_parse(int argc, char *argv[],
 			break;
 
 		case 'c':
-			set_option(&cs->options, OPT_COUNTERS, &args->invflags,
-				   invert);
+			set_option(p->ops, &cs->options, OPT_COUNTERS,
+				   &args->invflags, invert);
 			args->pcnt = optarg;
 			args->bcnt = strchr(args->pcnt + 1, ',');
 			if (args->bcnt)
@@ -1753,18 +1761,18 @@ void do_parse(int argc, char *argv[],
 				args->bcnt = argv[optind++];
 			if (!args->bcnt)
 				xtables_error(PARAMETER_PROBLEM,
-					"-%c requires packet and byte counter",
-					opt2char(OPT_COUNTERS));
+					      "%s requires packet and byte counter",
+					      p->ops->option_name(OPT_COUNTERS));
 
 			if (sscanf(args->pcnt, "%llu", &args->pcnt_cnt) != 1)
 				xtables_error(PARAMETER_PROBLEM,
-					"-%c packet counter not numeric",
-					opt2char(OPT_COUNTERS));
+					      "%s packet counter not numeric",
+					      p->ops->option_name(OPT_COUNTERS));
 
 			if (sscanf(args->bcnt, "%llu", &args->bcnt_cnt) != 1)
 				xtables_error(PARAMETER_PROBLEM,
-					"-%c byte counter not numeric",
-					opt2char(OPT_COUNTERS));
+					      "%s byte counter not numeric",
+					      p->ops->option_name(OPT_COUNTERS));
 			break;
 
 		case '4':
@@ -1837,7 +1845,7 @@ void do_parse(int argc, char *argv[],
 	if (p->ops->post_parse)
 		p->ops->post_parse(p->command, cs, args);
 
-	generic_opt_check(p->command, cs->options);
+	generic_opt_check(p->ops, p->command, cs->options);
 
 	if (p->chain != NULL && strlen(p->chain) >= XT_EXTENSION_MAXNAMELEN)
 		xtables_error(PARAMETER_PROBLEM,
@@ -1855,9 +1863,9 @@ void do_parse(int argc, char *argv[],
 			/* -o not valid with incoming packets. */
 			if (cs->options & OPT_VIANAMEOUT)
 				xtables_error(PARAMETER_PROBLEM,
-					   "Can't use -%c with %s\n",
-					   opt2char(OPT_VIANAMEOUT),
-					   p->chain);
+					      "Can't use %s with %s\n",
+					      p->ops->option_name(OPT_VIANAMEOUT),
+					      p->chain);
 		}
 
 		if (strcmp(p->chain, "POSTROUTING") == 0
@@ -1865,9 +1873,9 @@ void do_parse(int argc, char *argv[],
 			/* -i not valid with outgoing packets */
 			if (cs->options & OPT_VIANAMEIN)
 				xtables_error(PARAMETER_PROBLEM,
-					   "Can't use -%c with %s\n",
-					   opt2char(OPT_VIANAMEIN),
-					   p->chain);
+					      "Can't use %s with %s\n",
+					      p->ops->option_name(OPT_VIANAMEIN),
+					      p->chain);
 		}
 	}
 }
diff --git a/iptables/xshared.h b/iptables/xshared.h
index 815b9d3e98726..2470acbb46b7d 100644
--- a/iptables/xshared.h
+++ b/iptables/xshared.h
@@ -50,6 +50,7 @@ enum {
 	OPT_COMMAND	= 1 << 20,
 	OPT_ZERO	= 1 << 21,
 };
+#define NUMBER_OF_OPT	23
 
 enum {
 	CMD_NONE		= 0,
@@ -272,6 +273,7 @@ struct xt_cmd_parse_ops {
 	void	(*post_parse)(int command,
 			      struct iptables_command_state *cs,
 			      struct xtables_args *args);
+	const char *(*option_name)(int option);
 };
 
 struct xt_cmd_parse {
@@ -287,6 +289,8 @@ struct xt_cmd_parse {
 	struct xt_cmd_parse_ops		*ops;
 };
 
+const char *ip46t_option_name(int option);
+
 void do_parse(int argc, char *argv[],
 	      struct xt_cmd_parse *p, struct iptables_command_state *cs,
 	      struct xtables_args *args);
-- 
2.41.0

[iptables PATCH 2/3] xshared: Introduce xt_cmd_parse_ops::option_invert

[Phil Sutter]

Replace the awkward inverse_for_options array with basically a few
switch() statements clearly identifying the relation between option and
inverse values and relieve callers from having to find the option flag
bit's position.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/ip6tables.c |  1 +
 iptables/iptables.c  |  1 +
 iptables/nft-arp.c   | 14 ++++++++++++++
 iptables/nft-ipv4.c  |  1 +
 iptables/nft-ipv6.c  |  1 +
 iptables/xshared.c   | 38 ++++++++++++++------------------------
 iptables/xshared.h   |  2 ++
 7 files changed, 34 insertions(+), 24 deletions(-)

diff --git a/iptables/ip6tables.c b/iptables/ip6tables.c
index 85cb211d2ec12..08da04b456787 100644
--- a/iptables/ip6tables.c
+++ b/iptables/ip6tables.c
@@ -670,6 +670,7 @@ int do_command6(int argc, char *argv[], char **table,
 		.proto_parse	= ipv6_proto_parse,
 		.post_parse	= ipv6_post_parse,
 		.option_name	= ip46t_option_name,
+		.option_invert	= ip46t_option_invert,
 	};
 	struct xt_cmd_parse p = {
 		.table		= *table,
diff --git a/iptables/iptables.c b/iptables/iptables.c
index 4bfce62dd5d86..a73e8eed9028a 100644
--- a/iptables/iptables.c
+++ b/iptables/iptables.c
@@ -664,6 +664,7 @@ int do_command4(int argc, char *argv[], char **table,
 		.proto_parse	= ipv4_proto_parse,
 		.post_parse	= ipv4_post_parse,
 		.option_name	= ip46t_option_name,
+		.option_invert	= ip46t_option_invert,
 	};
 	struct xt_cmd_parse p = {
 		.table		= *table,
diff --git a/iptables/nft-arp.c b/iptables/nft-arp.c
index 6f8e1952db3b8..c009dd83e26cf 100644
--- a/iptables/nft-arp.c
+++ b/iptables/nft-arp.c
@@ -832,6 +832,19 @@ static const char *nft_arp_option_name(int option)
 	}
 }
 
+static int nft_arp_option_invert(int option)
+{
+	switch (option) {
+	case OPT_S_MAC:		return IPT_INV_SRCDEVADDR;
+	case OPT_D_MAC:		return IPT_INV_TGTDEVADDR;
+	case OPT_H_LENGTH:	return IPT_INV_ARPHLN;
+	case OPT_OPCODE:	return IPT_INV_ARPOP;
+	case OPT_H_TYPE:	return IPT_INV_ARPHRD;
+	case OPT_P_TYPE:	return IPT_INV_PROTO;
+	default:		return ip46t_option_invert(option);
+	}
+}
+
 struct nft_family_ops nft_family_ops_arp = {
 	.add			= nft_arp_add,
 	.is_same		= nft_arp_is_same,
@@ -844,6 +857,7 @@ struct nft_family_ops nft_family_ops_arp = {
 	.cmd_parse		= {
 		.post_parse	= nft_arp_post_parse,
 		.option_name	= nft_arp_option_name,
+		.option_invert	= nft_arp_option_invert,
 	},
 	.rule_to_cs		= nft_rule_to_iptables_command_state,
 	.init_cs		= nft_arp_init_cs,
diff --git a/iptables/nft-ipv4.c b/iptables/nft-ipv4.c
index 166680b3eb07c..7fb71ed4a8056 100644
--- a/iptables/nft-ipv4.c
+++ b/iptables/nft-ipv4.c
@@ -354,6 +354,7 @@ struct nft_family_ops nft_family_ops_ipv4 = {
 		.proto_parse	= ipv4_proto_parse,
 		.post_parse	= ipv4_post_parse,
 		.option_name	= ip46t_option_name,
+		.option_invert	= ip46t_option_invert,
 	},
 	.rule_to_cs		= nft_rule_to_iptables_command_state,
 	.clear_cs		= xtables_clear_iptables_command_state,
diff --git a/iptables/nft-ipv6.c b/iptables/nft-ipv6.c
index 2cc45944f6c04..bb417356629a9 100644
--- a/iptables/nft-ipv6.c
+++ b/iptables/nft-ipv6.c
@@ -345,6 +345,7 @@ struct nft_family_ops nft_family_ops_ipv6 = {
 		.proto_parse	= ipv6_proto_parse,
 		.post_parse	= ipv6_post_parse,
 		.option_name	= ip46t_option_name,
+		.option_invert	= ip46t_option_invert,
 	},
 	.rule_to_cs		= nft_rule_to_iptables_command_state,
 	.clear_cs		= xtables_clear_iptables_command_state,
diff --git a/iptables/xshared.c b/iptables/xshared.c
index 31a3019592317..f939a988fa59d 100644
--- a/iptables/xshared.c
+++ b/iptables/xshared.c
@@ -1003,27 +1003,18 @@ const char *ip46t_option_name(int option)
 	}
 }
 
-static const int inverse_for_options[NUMBER_OF_OPT] =
+int ip46t_option_invert(int option)
 {
-/* -n */ 0,
-/* -s */ IPT_INV_SRCIP,
-/* -d */ IPT_INV_DSTIP,
-/* -p */ XT_INV_PROTO,
-/* -j */ 0,
-/* -v */ 0,
-/* -x */ 0,
-/* -i */ IPT_INV_VIA_IN,
-/* -o */ IPT_INV_VIA_OUT,
-/*--line*/ 0,
-/* -c */ 0,
-/* -f */ IPT_INV_FRAG,
-/* 2 */ IPT_INV_SRCDEVADDR,
-/* 3 */ IPT_INV_TGTDEVADDR,
-/* -l */ IPT_INV_ARPHLN,
-/* 4 */ IPT_INV_ARPOP,
-/* 5 */ IPT_INV_ARPHRD,
-/* 6 */ IPT_INV_PROTO,
-};
+	switch (option) {
+	case OPT_SOURCE:	return IPT_INV_SRCIP;
+	case OPT_DESTINATION:	return IPT_INV_DSTIP;
+	case OPT_PROTOCOL:	return XT_INV_PROTO;
+	case OPT_VIANAMEIN:	return IPT_INV_VIA_IN;
+	case OPT_VIANAMEOUT:	return IPT_INV_VIA_OUT;
+	case OPT_FRAGMENT:	return IPT_INV_FRAG;
+	default:		return -1;
+	}
+}
 
 static void
 set_option(struct xt_cmd_parse_ops *ops,
@@ -1037,14 +1028,13 @@ set_option(struct xt_cmd_parse_ops *ops,
 	*options |= option;
 
 	if (invert) {
-		unsigned int i;
-		for (i = 0; 1 << i != option; i++);
+		int invopt = ops->option_invert(option);
 
-		if (!inverse_for_options[i])
+		if (invopt < 0)
 			xtables_error(PARAMETER_PROBLEM,
 				      "cannot have ! before %s",
 				      ops->option_name(option));
-		*invflg |= inverse_for_options[i];
+		*invflg |= invopt;
 	}
 }
 
diff --git a/iptables/xshared.h b/iptables/xshared.h
index 2470acbb46b7d..28efd73cf470a 100644
--- a/iptables/xshared.h
+++ b/iptables/xshared.h
@@ -274,6 +274,7 @@ struct xt_cmd_parse_ops {
 			      struct iptables_command_state *cs,
 			      struct xtables_args *args);
 	const char *(*option_name)(int option);
+	int	(*option_invert)(int option);
 };
 
 struct xt_cmd_parse {
@@ -290,6 +291,7 @@ struct xt_cmd_parse {
 };
 
 const char *ip46t_option_name(int option);
+int ip46t_option_invert(int option);
 
 void do_parse(int argc, char *argv[],
 	      struct xt_cmd_parse *p, struct iptables_command_state *cs,
-- 
2.41.0

[iptables PATCH 3/3] xshared: Simplify generic_opt_check()

[Phil Sutter]

The option/command matrix does not contain any '+' entries anymore, so
each option/command combination is either allowed (and optional) or not.

Reduce the matrix to an array of unsigned ints which specify the
commands a given option is allowed with.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/xshared.c | 77 +++++++++++++++++-----------------------------
 1 file changed, 28 insertions(+), 49 deletions(-)

diff --git a/iptables/xshared.c b/iptables/xshared.c
index f939a988fa59d..ca17479811df3 100644
--- a/iptables/xshared.c
+++ b/iptables/xshared.c
@@ -920,67 +920,46 @@ static int parse_rulenumber(const char *rule)
 	return rulenum;
 }
 
-/* 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 const 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 --line -c -f 2 3 l 4 5 6 */
-/*INSERT*/    {'x',' ',' ',' ',' ',' ','x',' ',' ','x',' ',' ',' ',' ',' ',' ',' ',' '},
-/*DELETE*/    {'x',' ',' ',' ',' ',' ','x',' ',' ','x','x',' ',' ',' ',' ',' ',' ',' '},
-/*DELETE_NUM*/{'x','x','x','x','x',' ','x','x','x','x','x','x','x','x','x','x','x','x'},
-/*REPLACE*/   {'x',' ',' ',' ',' ',' ','x',' ',' ','x',' ',' ',' ',' ',' ',' ',' ',' '},
-/*APPEND*/    {'x',' ',' ',' ',' ',' ','x',' ',' ','x',' ',' ',' ',' ',' ',' ',' ',' '},
-/*LIST*/      {' ','x','x','x','x',' ',' ','x','x',' ','x','x','x','x','x','x','x','x'},
-/*FLUSH*/     {'x','x','x','x','x',' ','x','x','x','x','x','x','x','x','x','x','x','x'},
-/*ZERO*/      {'x','x','x','x','x',' ','x','x','x','x','x','x','x','x','x','x','x','x'},
-/*NEW_CHAIN*/ {'x','x','x','x','x',' ','x','x','x','x','x','x','x','x','x','x','x','x'},
-/*DEL_CHAIN*/ {'x','x','x','x','x',' ','x','x','x','x','x','x','x','x','x','x','x','x'},
-/*SET_POLICY*/{'x','x','x','x','x',' ','x','x','x','x',' ','x','x','x','x','x','x','x'},
-/*RENAME*/    {'x','x','x','x','x',' ','x','x','x','x','x','x','x','x','x','x','x','x'},
-/*LIST_RULES*/{'x','x','x','x','x',' ','x','x','x','x','x','x','x','x','x','x','x','x'},
-/*ZERO_NUM*/  {'x','x','x','x','x',' ','x','x','x','x','x','x','x','x','x','x','x','x'},
-/*CHECK*/     {'x',' ',' ',' ',' ',' ','x',' ',' ','x','x',' ',' ',' ',' ',' ',' ',' '},
+/* list the commands an option is allowed with */
+#define CMD_IDRAC	CMD_INSERT | CMD_DELETE | CMD_REPLACE | \
+			CMD_APPEND | CMD_CHECK
+static const unsigned int options_v_commands[NUMBER_OF_OPT] = {
+/*OPT_NUMERIC*/		CMD_LIST,
+/*OPT_SOURCE*/		CMD_IDRAC,
+/*OPT_DESTINATION*/	CMD_IDRAC,
+/*OPT_PROTOCOL*/	CMD_IDRAC,
+/*OPT_JUMP*/		CMD_IDRAC,
+/*OPT_VERBOSE*/		UINT_MAX,
+/*OPT_EXPANDED*/	CMD_LIST,
+/*OPT_VIANAMEIN*/	CMD_IDRAC,
+/*OPT_VIANAMEOUT*/	CMD_IDRAC,
+/*OPT_LINENUMBERS*/	CMD_LIST,
+/*OPT_COUNTERS*/	CMD_INSERT | CMD_REPLACE | CMD_APPEND | CMD_SET_POLICY,
+/*OPT_FRAGMENT*/	CMD_IDRAC,
+/*OPT_S_MAC*/		CMD_IDRAC,
+/*OPT_D_MAC*/		CMD_IDRAC,
+/*OPT_H_LENGTH*/	CMD_IDRAC,
+/*OPT_OPCODE*/		CMD_IDRAC,
+/*OPT_H_TYPE*/		CMD_IDRAC,
+/*OPT_P_TYPE*/		CMD_IDRAC,
 };
+#undef CMD_IDRAC
 
 static void generic_opt_check(struct xt_cmd_parse_ops *ops,
 			      int command, int options)
 {
-	int i, j, legal = 0;
+	int i, optval;
 
 	/* 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 `%s' option for this command",
-						      ops->option_name(1<<i));
-			} else {
-				if (commands_v_options[j][i] != 'x')
-					legal = 1;
-				else if (legal == 0)
-					legal = -1;
-			}
-		}
-		if (legal == -1)
+	for (i = 0, optval = 1; i < NUMBER_OF_OPT; optval = (1 << ++i)) {
+		if ((options & optval) &&
+		    (options_v_commands[i] & command) != command)
 			xtables_error(PARAMETER_PROBLEM,
 				      "Illegal option `%s' with this command",
-				      ops->option_name(1<<i));
+				      ops->option_name(optval));
 	}
 }
 
-- 
2.41.0

Re: [iptables PATCH 0/3] xshared: Review option parsing

[Phil Sutter]

On Fri, Nov 24, 2023 at 12:13:22PM +0100, Phil Sutter wrote:
> Faced with the need to extend optflags, inverse_for_options and
> commands_v_options arrays in order to integrate ebtables-specific
> options, I chose to get rid of them all instead.
> 
> Patch 1 replaces opt2char() function which didn't work well since not
> every option has printable short-option character. The callback-based
> replacement returns the long-option and should make error messages more
> readable this way.
> 
> Patch 2 elminates the inverse_for_options array along with the loop
> turning a given option into its bit's position for use in the array. The
> switch statement in the replacing callback is much easier to maintain
> and extend.
> 
> Patch 3 makes use of the fact that no command has a mandatory option
> anymore. So every combination is either allowed or not, and a single bit
> may indicate that.
> 
> Phil Sutter (3):
>   xshared: Introduce xt_cmd_parse_ops::option_name
>   xshared: Introduce xt_cmd_parse_ops::option_invert
>   xshared: Simplify generic_opt_check()

Series applied.