[iproute2-next,3/3] lib: utils: Add parse_one_of_deprecated(), parse_on_off_deprecated()

Commit Message

Petr Machata Nov. 15, 2023, 3:31 p.m. UTC

The functions parse_on_off() and parse_one_of() currently use matches() for
string comparison under the hood. This has some odd consequences. In
particular, "o" can be used as a shorthand for "off", which is not obvious,
because "o" is the prefix of both. By sheer luck, the end result actually
makes some sense: "on" means on, anything else means off (or errors out).
Similar issues are in principle also possible for parse_one_of() uses,
though currently this does not come up.

Ideally parse_on_off() would accept the strings "on" and "off" and no
others, likewise for parse_one_of().

Therefore in this patch, rename the old matches()-based parsers to
parse_one_of_deprecated() and parse_on_off_deprecated(). Introduce
new strcmp()-based parsers, parse_one_of() and parse_on_off(), for
future users.

All existing users have been converted to the _deprecated() variants,
with one exception for RDMA's sys_set_privileged_qkey_args(), which was
introduced recently enough that it is unlikely to have users relying
on the broken behavior.

Signed-off-by: Petr Machata <[e-mail]>
---
 bridge/link.c            | 48 ++++++++++++++++++++++++++--------------
 bridge/vlan.c            |  4 ++--
 dcb/dcb_ets.c            |  6 +++--
 dcb/dcb_pfc.c            |  5 +++--
 include/utils.h          |  4 ++++
 ip/iplink.c              | 15 ++++++++-----
 ip/iplink_bridge_slave.c |  2 +-
 ip/ipmacsec.c            | 27 +++++++++++++---------
 ip/ipstats.c             |  3 ++-
 lib/utils.c              | 11 +++++++++
 10 files changed, 84 insertions(+), 41 deletions(-)

Comments

Stephen Hemminger Nov. 15, 2023, 3:57 p.m. UTC | #1

On Wed, 15 Nov 2023 16:31:59 +0100
Petr Machata <[e-mail]> wrote:

> The functions parse_on_off() and parse_one_of() currently use matches() for
> string comparison under the hood. This has some odd consequences. In
> particular, "o" can be used as a shorthand for "off", which is not obvious,
> because "o" is the prefix of both. By sheer luck, the end result actually
> makes some sense: "on" means on, anything else means off (or errors out).
> Similar issues are in principle also possible for parse_one_of() uses,
> though currently this does not come up.

If we need to keep accepting shorthands, I would prefer that any
use of shorthand would cause a warning message.

Stephen Hemminger Nov. 15, 2023, 3:59 p.m. UTC | #2

On Wed, 15 Nov 2023 16:31:59 +0100
Petr Machata <[e-mail]> wrote:

> The functions parse_on_off() and parse_one_of() currently use matches() for
> string comparison under the hood. This has some odd consequences. In
> particular, "o" can be used as a shorthand for "off", which is not obvious,
> because "o" is the prefix of both. By sheer luck, the end result actually
> makes some sense: "on" means on, anything else means off (or errors out).
> Similar issues are in principle also possible for parse_one_of() uses,
> though currently this does not come up.

This was probably a bug, I am open to breaking shorthand usage in this case.

Petr Machata Nov. 15, 2023, 4:57 p.m. UTC | #3

Stephen Hemminger <[e-mail]> writes:

> On Wed, 15 Nov 2023 16:31:59 +0100
> Petr Machata <[e-mail]> wrote:
>
>> The functions parse_on_off() and parse_one_of() currently use matches() for
>> string comparison under the hood. This has some odd consequences. In
>> particular, "o" can be used as a shorthand for "off", which is not obvious,
>> because "o" is the prefix of both. By sheer luck, the end result actually
>> makes some sense: "on" means on, anything else means off (or errors out).
>> Similar issues are in principle also possible for parse_one_of() uses,
>> though currently this does not come up.
>
> This was probably a bug, I am open to breaking shorthand usage in this case.

There were uses of matches() for on/off parsing even before adding
parse_on_off(). The bug was converting _everyone_ to matches().

I figured you'd be against just s/matches/strcmp, but if you think it's
OK, I have no problem with that. Shorthanding on/off just makes no
sense to me, not even by mistake.

How about the parse_one_of() users? E.g. the disabled/check/strict for
macsec validate, I could see someone mistyping it as "disable", so now
it lives in some deployment script, or testing harness, or whatever.

Maybe do the warning thing in this case? And retire it a couple releases
down the line in favor of just accepting strcmp?

David Ahern Nov. 20, 2023, 10:27 p.m. UTC | #4

On 11/15/23 8:57 AM, Petr Machata wrote:
> 
> Stephen Hemminger <[e-mail]> writes:
> 
>> On Wed, 15 Nov 2023 16:31:59 +0100
>> Petr Machata <[e-mail]> wrote:
>>
>>> The functions parse_on_off() and parse_one_of() currently use matches() for
>>> string comparison under the hood. This has some odd consequences. In
>>> particular, "o" can be used as a shorthand for "off", which is not obvious,
>>> because "o" is the prefix of both. By sheer luck, the end result actually
>>> makes some sense: "on" means on, anything else means off (or errors out).
>>> Similar issues are in principle also possible for parse_one_of() uses,
>>> though currently this does not come up.
>>
>> This was probably a bug, I am open to breaking shorthand usage in this case.
> 
> There were uses of matches() for on/off parsing even before adding
> parse_on_off(). The bug was converting _everyone_ to matches().
> 
> I figured you'd be against just s/matches/strcmp, but if you think it's
> OK, I have no problem with that. Shorthanding on/off just makes no
> sense to me, not even by mistake.

+1

> 
> How about the parse_one_of() users? E.g. the disabled/check/strict for
> macsec validate, I could see someone mistyping it as "disable", so now
> it lives in some deployment script, or testing harness, or whatever.
> 
> Maybe do the warning thing in this case? And retire it a couple releases
> down the line in favor of just accepting strcmp?

I think the macsec example needs to stay as is; it could be moved to a
parse_one_of_legacy() in ipmacsec to avoid copy-and-paste. The rest seem
safe to convert to your proposal.

Patch

diff --git a/bridge/link.c b/bridge/link.c
index 1c8faa85..2a83c914 100644
--- a/bridge/link.c
+++ b/bridge/link.c
@@ -374,37 +374,44 @@  static int brlink_modify(int argc, char **argv)
 			d = *argv;
 		} else if (strcmp(*argv, "guard") == 0) {
 			NEXT_ARG();
-			bpdu_guard = parse_on_off("guard", *argv, &ret);
+			bpdu_guard = parse_on_off_deprecated("guard",
+							     *argv, &ret);
 			if (ret)
 				return ret;
 		} else if (strcmp(*argv, "hairpin") == 0) {
 			NEXT_ARG();
-			hairpin = parse_on_off("hairpin", *argv, &ret);
+			hairpin = parse_on_off_deprecated("hairpin",
+							  *argv, &ret);
 			if (ret)
 				return ret;
 		} else if (strcmp(*argv, "fastleave") == 0) {
 			NEXT_ARG();
-			fast_leave = parse_on_off("fastleave", *argv, &ret);
+			fast_leave = parse_on_off_deprecated("fastleave",
+							     *argv, &ret);
 			if (ret)
 				return ret;
 		} else if (strcmp(*argv, "root_block") == 0) {
 			NEXT_ARG();
-			root_block = parse_on_off("root_block", *argv, &ret);
+			root_block = parse_on_off_deprecated("root_block",
+							     *argv, &ret);
 			if (ret)
 				return ret;
 		} else if (strcmp(*argv, "learning") == 0) {
 			NEXT_ARG();
-			learning = parse_on_off("learning", *argv, &ret);
+			learning = parse_on_off_deprecated("learning",
+							   *argv, &ret);
 			if (ret)
 				return ret;
 		} else if (strcmp(*argv, "learning_sync") == 0) {
 			NEXT_ARG();
-			learning_sync = parse_on_off("learning_sync", *argv, &ret);
+			learning_sync = parse_on_off_deprecated("learning_sync",
+								*argv, &ret);
 			if (ret)
 				return ret;
 		} else if (strcmp(*argv, "flood") == 0) {
 			NEXT_ARG();
-			flood = parse_on_off("flood", *argv, &ret);
+			flood = parse_on_off_deprecated("flood",
+							*argv, &ret);
 			if (ret)
 				return ret;
 		} else if (strcmp(*argv, "mcast_router") == 0) {
@@ -412,17 +419,20 @@  static int brlink_modify(int argc, char **argv)
 			mcast_router = atoi(*argv);
 		} else if (strcmp(*argv, "mcast_flood") == 0) {
 			NEXT_ARG();
-			mcast_flood = parse_on_off("mcast_flood", *argv, &ret);
+			mcast_flood = parse_on_off_deprecated("mcast_flood",
+							      *argv, &ret);
 			if (ret)
 				return ret;
 		} else if (strcmp(*argv, "bcast_flood") == 0) {
 			NEXT_ARG();
-			bcast_flood = parse_on_off("bcast_flood", *argv, &ret);
+			bcast_flood = parse_on_off_deprecated("bcast_flood",
+							      *argv, &ret);
 			if (ret)
 				return ret;
 		} else if (strcmp(*argv, "mcast_to_unicast") == 0) {
 			NEXT_ARG();
-			mcast_to_unicast = parse_on_off("mcast_to_unicast", *argv, &ret);
+			mcast_to_unicast = parse_on_off_deprecated("mcast_to_unicast",
+								   *argv, &ret);
 			if (ret)
 				return ret;
 		} else if (strcmp(*argv, "mcast_max_groups") == 0) {
@@ -466,33 +476,37 @@  static int brlink_modify(int argc, char **argv)
 			flags |= BRIDGE_FLAGS_MASTER;
 		} else if (strcmp(*argv, "neigh_suppress") == 0) {
 			NEXT_ARG();
-			neigh_suppress = parse_on_off("neigh_suppress", *argv, &ret);
+			neigh_suppress = parse_on_off_deprecated("neigh_suppress",
+								 *argv, &ret);
 			if (ret)
 				return ret;
 		} else if (strcmp(*argv, "neigh_vlan_suppress") == 0) {
 			NEXT_ARG();
-			neigh_vlan_suppress = parse_on_off("neigh_vlan_suppress",
-							   *argv, &ret);
+			neigh_vlan_suppress =
+				parse_on_off_deprecated("neigh_vlan_suppress",
+							*argv, &ret);
 			if (ret)
 				return ret;
 		} else if (strcmp(*argv, "vlan_tunnel") == 0) {
 			NEXT_ARG();
-			vlan_tunnel = parse_on_off("vlan_tunnel", *argv, &ret);
+			vlan_tunnel = parse_on_off_deprecated("vlan_tunnel",
+							      *argv, &ret);
 			if (ret)
 				return ret;
 		} else if (strcmp(*argv, "isolated") == 0) {
 			NEXT_ARG();
-			isolated = parse_on_off("isolated", *argv, &ret);
+			isolated = parse_on_off_deprecated("isolated",
+							   *argv, &ret);
 			if (ret)
 				return ret;
 		} else if (strcmp(*argv, "locked") == 0) {
 			NEXT_ARG();
-			locked = parse_on_off("locked", *argv, &ret);
+			locked = parse_on_off_deprecated("locked", *argv, &ret);
 			if (ret)
 				return ret;
 		} else if (strcmp(*argv, "mab") == 0) {
 			NEXT_ARG();
-			macauth = parse_on_off("mab", *argv, &ret);
+			macauth = parse_on_off_deprecated("mab", *argv, &ret);
 			if (ret)
 				return ret;
 		} else if (strcmp(*argv, "backup_port") == 0) {
diff --git a/bridge/vlan.c b/bridge/vlan.c
index dfc62f83..338b9b0c 100644
--- a/bridge/vlan.c
+++ b/bridge/vlan.c
@@ -361,8 +361,8 @@  static int vlan_option_set(int argc, char **argv)
 			int ret;
 
 			NEXT_ARG();
-			neigh_suppress = parse_on_off("neigh_suppress", *argv,
-						      &ret);
+			neigh_suppress = parse_on_off_deprecated("neigh_suppress",
+								 *argv, &ret);
 			if (ret)
 				return ret;
 			addattr8(&req.n, sizeof(req),
diff --git a/dcb/dcb_ets.c b/dcb/dcb_ets.c
index c2088105..ea8786ff 100644
--- a/dcb/dcb_ets.c
+++ b/dcb/dcb_ets.c
@@ -61,7 +61,8 @@  static int dcb_ets_parse_mapping_tc_tsa(__u32 key, char *value, void *data)
 	__u8 tsa;
 	int ret;
 
-	tsa = parse_one_of("TSA", value, tsa_names, ARRAY_SIZE(tsa_names), &ret);
+	tsa = parse_one_of_deprecated("TSA", value, tsa_names,
+				      ARRAY_SIZE(tsa_names), &ret);
 	if (ret)
 		return ret;
 
@@ -274,7 +275,8 @@  static int dcb_cmd_ets_set(struct dcb *dcb, const char *dev, int argc, char **ar
 			return 0;
 		} else if (matches(*argv, "willing") == 0) {
 			NEXT_ARG();
-			ets.willing = parse_on_off("willing", *argv, &ret);
+			ets.willing = parse_on_off_deprecated("willing",
+							      *argv, &ret);
 			if (ret)
 				return ret;
 		} else if (matches(*argv, "tc-tsa") == 0) {
diff --git a/dcb/dcb_pfc.c b/dcb/dcb_pfc.c
index aaa09022..a89c274f 100644
--- a/dcb/dcb_pfc.c
+++ b/dcb/dcb_pfc.c
@@ -73,7 +73,7 @@  static int dcb_pfc_parse_mapping_prio_pfc(__u32 key, char *value, void *data)
 
 	dcb_pfc_to_array(pfc_en, pfc->pfc_en);
 
-	enabled = parse_on_off("PFC", value, &ret);
+	enabled = parse_on_off_deprecated("PFC", value, &ret);
 	if (ret)
 		return ret;
 
@@ -185,7 +185,8 @@  static int dcb_cmd_pfc_set(struct dcb *dcb, const char *dev, int argc, char **ar
 			continue;
 		} else if (matches(*argv, "macsec-bypass") == 0) {
 			NEXT_ARG();
-			pfc.mbc = parse_on_off("macsec-bypass", *argv, &ret);
+			pfc.mbc = parse_on_off_deprecated("macsec-bypass",
+							  *argv, &ret);
 			if (ret)
 				return ret;
 		} else if (matches(*argv, "delay") == 0) {
diff --git a/include/utils.h b/include/utils.h
index add55bfa..c9318c20 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -342,7 +342,11 @@  int do_batch(const char *name, bool force,
 
 int parse_one_of(const char *msg, const char *realval, const char * const *list,
 		 size_t len, int *p_err);
+int parse_one_of_deprecated(const char *msg, const char *realval,
+			    const char * const *list, size_t len, int *p_err);
+
 bool parse_on_off(const char *msg, const char *realval, int *p_err);
+bool parse_on_off_deprecated(const char *msg, const char *realval, int *p_err);
 
 int parse_mapping_num_all(__u32 *keyp, const char *key);
 int parse_mapping_gen(int *argcp, char ***argvp,
diff --git a/ip/iplink.c b/ip/iplink.c
index 9a548dd3..e868bc66 100644
--- a/ip/iplink.c
+++ b/ip/iplink.c
@@ -470,7 +470,8 @@  static int iplink_parse_vf(int vf, int *argcp, char ***argvp,
 			struct ifla_vf_spoofchk ivs;
 
 			NEXT_ARG();
-			ivs.setting = parse_on_off("spoofchk", *argv, &ret);
+			ivs.setting = parse_on_off_deprecated("spoofchk",
+							      *argv, &ret);
 			if (ret)
 				return ret;
 			ivs.vf = vf;
@@ -481,7 +482,8 @@  static int iplink_parse_vf(int vf, int *argcp, char ***argvp,
 			struct ifla_vf_rss_query_en ivs;
 
 			NEXT_ARG();
-			ivs.setting = parse_on_off("query_rss", *argv, &ret);
+			ivs.setting = parse_on_off_deprecated("query_rss",
+							      *argv, &ret);
 			if (ret)
 				return ret;
 			ivs.vf = vf;
@@ -492,7 +494,8 @@  static int iplink_parse_vf(int vf, int *argcp, char ***argvp,
 			struct ifla_vf_trust ivt;
 
 			NEXT_ARG();
-			ivt.setting = parse_on_off("trust", *argv, &ret);
+			ivt.setting = parse_on_off_deprecated("trust",
+							      *argv, &ret);
 			if (ret)
 				return ret;
 			ivt.vf = vf;
@@ -738,7 +741,8 @@  int iplink_parse(int argc, char **argv, struct iplink_req *req, char **type)
 			int carrier;
 
 			NEXT_ARG();
-			carrier = parse_on_off("carrier", *argv, &err);
+			carrier = parse_on_off_deprecated("carrier",
+							  *argv, &err);
 			if (err)
 				return err;
 
@@ -893,7 +897,8 @@  int iplink_parse(int argc, char **argv, struct iplink_req *req, char **type)
 			unsigned int proto_down;
 
 			NEXT_ARG();
-			proto_down = parse_on_off("protodown", *argv, &err);
+			proto_down = parse_on_off_deprecated("protodown",
+							     *argv, &err);
 			if (err)
 				return err;
 			addattr8(&req->n, sizeof(*req), IFLA_PROTO_DOWN,
diff --git a/ip/iplink_bridge_slave.c b/ip/iplink_bridge_slave.c
index 3821923b..361d5880 100644
--- a/ip/iplink_bridge_slave.c
+++ b/ip/iplink_bridge_slave.c
@@ -319,7 +319,7 @@  static void bridge_slave_parse_on_off(char *arg_name, char *arg_val,
 				      struct nlmsghdr *n, int type)
 {
 	int ret;
-	__u8 val = parse_on_off(arg_name, arg_val, &ret);
+	__u8 val = parse_on_off_deprecated(arg_name, arg_val, &ret);
 
 	if (ret)
 		exit(1);
diff --git a/ip/ipmacsec.c b/ip/ipmacsec.c
index 476a6d1d..e958a37b 100644
--- a/ip/ipmacsec.c
+++ b/ip/ipmacsec.c
@@ -593,7 +593,8 @@  static int do_offload(enum cmd c, int argc, char **argv)
 	if (argc == 0)
 		ipmacsec_usage();
 
-	offload = parse_one_of("offload", *argv, offload_str, ARRAY_SIZE(offload_str), &ret);
+	offload = parse_one_of_deprecated("offload", *argv, offload_str,
+					  ARRAY_SIZE(offload_str), &ret);
 	if (ret)
 		ipmacsec_usage();
 
@@ -1402,7 +1403,7 @@  static int macsec_parse_opt(struct link_util *lu, int argc, char **argv,
 			NEXT_ARG();
 			int i;
 
-			i = parse_on_off("encrypt", *argv, &ret);
+			i = parse_on_off_deprecated("encrypt", *argv, &ret);
 			if (ret != 0)
 				return ret;
 			addattr8(n, MACSEC_BUFLEN, IFLA_MACSEC_ENCRYPT, i);
@@ -1410,7 +1411,7 @@  static int macsec_parse_opt(struct link_util *lu, int argc, char **argv,
 			NEXT_ARG();
 			int i;
 
-			i = parse_on_off("send_sci", *argv, &ret);
+			i = parse_on_off_deprecated("send_sci", *argv, &ret);
 			if (ret != 0)
 				return ret;
 			send_sci = i;
@@ -1420,7 +1421,7 @@  static int macsec_parse_opt(struct link_util *lu, int argc, char **argv,
 			NEXT_ARG();
 			int i;
 
-			i = parse_on_off("end_station", *argv, &ret);
+			i = parse_on_off_deprecated("end_station", *argv, &ret);
 			if (ret != 0)
 				return ret;
 			es = i;
@@ -1429,7 +1430,7 @@  static int macsec_parse_opt(struct link_util *lu, int argc, char **argv,
 			NEXT_ARG();
 			int i;
 
-			i = parse_on_off("scb", *argv, &ret);
+			i = parse_on_off_deprecated("scb", *argv, &ret);
 			if (ret != 0)
 				return ret;
 			scb = i;
@@ -1438,7 +1439,7 @@  static int macsec_parse_opt(struct link_util *lu, int argc, char **argv,
 			NEXT_ARG();
 			int i;
 
-			i = parse_on_off("protect", *argv, &ret);
+			i = parse_on_off_deprecated("protect", *argv, &ret);
 			if (ret != 0)
 				return ret;
 			addattr8(n, MACSEC_BUFLEN, IFLA_MACSEC_PROTECT, i);
@@ -1446,7 +1447,7 @@  static int macsec_parse_opt(struct link_util *lu, int argc, char **argv,
 			NEXT_ARG();
 			int i;
 
-			i = parse_on_off("replay", *argv, &ret);
+			i = parse_on_off_deprecated("replay", *argv, &ret);
 			if (ret != 0)
 				return ret;
 			replay_protect = !!i;
@@ -1457,8 +1458,10 @@  static int macsec_parse_opt(struct link_util *lu, int argc, char **argv,
 				invarg("expected replay window size", *argv);
 		} else if (strcmp(*argv, "validate") == 0) {
 			NEXT_ARG();
-			validate = parse_one_of("validate", *argv, validate_str,
-						ARRAY_SIZE(validate_str), &ret);
+			validate = parse_one_of_deprecated("validate",
+							   *argv, validate_str,
+							   ARRAY_SIZE(validate_str),
+							   &ret);
 			if (ret != 0)
 				return ret;
 			addattr8(n, MACSEC_BUFLEN,
@@ -1472,8 +1475,10 @@  static int macsec_parse_opt(struct link_util *lu, int argc, char **argv,
 				invarg("expected an { 0..3 }", *argv);
 		} else if (strcmp(*argv, "offload") == 0) {
 			NEXT_ARG();
-			offload = parse_one_of("offload", *argv, offload_str,
-					       ARRAY_SIZE(offload_str), &ret);
+			offload = parse_one_of_deprecated("offload", *argv,
+							  offload_str,
+							  ARRAY_SIZE(offload_str),
+							  &ret);
 			if (ret != 0)
 				return ret;
 			addattr8(n, MACSEC_BUFLEN,
diff --git a/ip/ipstats.c b/ip/ipstats.c
index 3f94ff1e..ce6c8a0e 100644
--- a/ip/ipstats.c
+++ b/ip/ipstats.c
@@ -1282,7 +1282,8 @@  static int ipstats_set(int argc, char **argv)
 				return -EINVAL;
 			}
 			at = IFLA_STATS_SET_OFFLOAD_XSTATS_L3_STATS;
-			enable = parse_on_off("l3_stats", *argv, &err);
+			enable = parse_on_off_deprecated("l3_stats",
+							 *argv, &err);
 			if (err)
 				return err;
 		} else if (strcmp(*argv, "help") == 0) {
diff --git a/lib/utils.c b/lib/utils.c
index 7aa3409f..19a2b63d 100644
--- a/lib/utils.c
+++ b/lib/utils.c
@@ -1754,6 +1754,12 @@  __parse_one_of(const char *msg, const char *realval,
 
 int parse_one_of(const char *msg, const char *realval, const char * const *list,
 		 size_t len, int *p_err)
+{
+	return __parse_one_of(msg, realval, list, len, p_err, strcmp);
+}
+
+int parse_one_of_deprecated(const char *msg, const char *realval,
+			    const char * const *list, size_t len, int *p_err)
 {
 	return __parse_one_of(msg, realval, list, len, p_err, matches);
 }
@@ -1768,6 +1774,11 @@  static bool __parse_on_off(const char *msg, const char *realval, int *p_err,
 }
 
 bool parse_on_off(const char *msg, const char *realval, int *p_err)
+{
+	return __parse_on_off(msg, realval, p_err, strcmp);
+}
+
+bool parse_on_off_deprecated(const char *msg, const char *realval, int *p_err)
 {
 	return __parse_on_off(msg, realval, p_err, matches);
 }