[PATCH iproute2-next 0/3] Change parse_one_of(), parse_on_off() to strcmp()

[PATCH iproute2-next 0/3] Change parse_one_of(), parse_on_off() to strcmp()

[Petr Machata]

Library functions parse_one_of() and parse_on_off() were added about three
years ago to unify all the disparate reimplementations of the same basic
idea. It used the matches() function to determine whether a string under
consideration corresponds to one of the patterns. This reflected many,
though not all cases of on/off parsing at the time.

This decision 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 either 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().

Hence this patchset, which renames the existing functions to
parse_one_of_deprecated() and parse_on_off_deprecated(), respectively,
and introduces the new functions in their place, parse_one_of() and
parse_on_off(), which use strcmp() under the hood.

Petr Machata (3):
  lib: utils: Switch matches() to returning int again
  lib: utils: Generalize code of parse_one_of(), parse_on_off()
  lib: utils: Add parse_one_of_deprecated(), parse_on_off_deprecated()

 bridge/link.c            | 48 ++++++++++++++++++++++++++--------------
 bridge/vlan.c            |  4 ++--
 dcb/dcb_ets.c            |  6 +++--
 dcb/dcb_pfc.c            |  5 +++--
 include/utils.h          |  6 ++++-
 ip/iplink.c              | 15 ++++++++-----
 ip/iplink_bridge_slave.c |  2 +-
 ip/ipmacsec.c            | 27 +++++++++++++---------
 ip/ipstats.c             |  3 ++-
 lib/utils.c              | 44 ++++++++++++++++++++++++++++--------
 10 files changed, 109 insertions(+), 51 deletions(-)

-- 
2.41.0

[PATCH iproute2-next 1/3] lib: utils: Switch matches() to returning int again

[Petr Machata]

Since the commit cited below, the function has pretended to return a
boolean. But every user expects it to return zero on success and a non-zero
value on failure, like strcmp(). The function actually even returns "true"
to mean "no match". This only makes sense if one considers a boolean to be
a one-bit unsigned integer with no inherent meaning, which I do not think
is reasonable.

Switch the prototype back to int, and return 1 instead of true.

Cc: Matteo Croce <mcroce@redhat.com>
Fixes: 1f420318bda3 ("utils: don't match empty strings as prefixes")
Signed-off-by: Petr Machata <petrm@nvidia.com>
---
 include/utils.h | 2 +-
 lib/utils.c     | 8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/utils.h b/include/utils.h
index f26ed822..add55bfa 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -198,7 +198,7 @@ int check_ifname(const char *);
 int check_altifname(const char *name);
 int get_ifname(char *, const char *);
 const char *get_ifname_rta(int ifindex, const struct rtattr *rta);
-bool matches(const char *prefix, const char *string);
+int matches(const char *prefix, const char *string);
 int inet_addr_match(const inet_prefix *a, const inet_prefix *b, int bits);
 int inet_addr_match_rta(const inet_prefix *m, const struct rtattr *rta);
 
diff --git a/lib/utils.c b/lib/utils.c
index 99ba7a23..1fc42a9a 100644
--- a/lib/utils.c
+++ b/lib/utils.c
@@ -873,18 +873,18 @@ const char *get_ifname_rta(int ifindex, const struct rtattr *rta)
 	return name;
 }
 
-/* Returns false if 'prefix' is a not empty prefix of 'string'.
+/* Returns 0 if 'prefix' is a not empty prefix of 'string', != 0 otherwise.
  */
-bool matches(const char *prefix, const char *string)
+int matches(const char *prefix, const char *string)
 {
 	if (!*prefix)
-		return true;
+		return 1;
 	while (*string && *prefix == *string) {
 		prefix++;
 		string++;
 	}
 
-	return !!*prefix;
+	return *prefix;
 }
 
 int inet_addr_match(const inet_prefix *a, const inet_prefix *b, int bits)
-- 
2.41.0

[PATCH iproute2-next 2/3] lib: utils: Generalize code of parse_one_of(), parse_on_off()

[Petr Machata]

The following patch will split these two functions into their deprecated
and vanilla versions. Their behavior should be identical, except the
deprecated version will keep using matches() under the hood, wherease the
vanilla will use strcmp().

To prepare for this change, extract from each function a core, which
express in terms of a configurable matcher. Then rewrite parse_one_of() and
parse_on_off() as wrappers that pass matches() as the matcher to keep the
current behavior.

Signed-off-by: Petr Machata <petrm@nvidia.com>
---
 lib/utils.c | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/lib/utils.c b/lib/utils.c
index 1fc42a9a..7aa3409f 100644
--- a/lib/utils.c
+++ b/lib/utils.c
@@ -1729,13 +1729,15 @@ int do_batch(const char *name, bool force,
 	return ret;
 }
 
-int parse_one_of(const char *msg, const char *realval, const char * const *list,
-		 size_t len, int *p_err)
+static int
+__parse_one_of(const char *msg, const char *realval,
+	       const char * const *list, size_t len, int *p_err,
+	       int (*matcher)(const char *, const char *))
 {
 	int i;
 
 	for (i = 0; i < len; i++) {
-		if (list[i] && matches(realval, list[i]) == 0) {
+		if (list[i] && matcher(realval, list[i]) == 0) {
 			*p_err = 0;
 			return i;
 		}
@@ -1750,11 +1752,24 @@ int parse_one_of(const char *msg, const char *realval, const char * const *list,
 	return 0;
 }
 
-bool parse_on_off(const char *msg, const char *realval, int *p_err)
+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, matches);
+}
+
+static bool __parse_on_off(const char *msg, const char *realval, int *p_err,
+			   int (*matcher)(const char *, const char *))
 {
 	static const char * const values_on_off[] = { "off", "on" };
 
-	return parse_one_of(msg, realval, values_on_off, ARRAY_SIZE(values_on_off), p_err);
+	return __parse_one_of(msg, realval, values_on_off,
+			      ARRAY_SIZE(values_on_off), p_err, matcher);
+}
+
+bool parse_on_off(const char *msg, const char *realval, int *p_err)
+{
+	return __parse_on_off(msg, realval, p_err, matches);
 }
 
 int parse_mapping_gen(int *argcp, char ***argvp,
-- 
2.41.0

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

[Petr Machata]

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 <petrm@nvidia.com>
---
 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(-)

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);
 }
-- 
2.41.0

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

[Stephen Hemminger]

On Wed, 15 Nov 2023 16:31:59 +0100
Petr Machata <petrm@nvidia.com> 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.

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

[Stephen Hemminger]

On Wed, 15 Nov 2023 16:31:59 +0100
Petr Machata <petrm@nvidia.com> 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.

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

[Petr Machata]

Stephen Hemminger <stephen@networkplumber.org> writes:

> On Wed, 15 Nov 2023 16:31:59 +0100
> Petr Machata <petrm@nvidia.com> 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?

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

[David Ahern]

On 11/15/23 8:57 AM, Petr Machata wrote:
> 
> Stephen Hemminger <stephen@networkplumber.org> writes:
> 
>> On Wed, 15 Nov 2023 16:31:59 +0100
>> Petr Machata <petrm@nvidia.com> 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.