Change parsing in parse_one_of(), parse_on_off()

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 keywords. 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.

Patch #1 is a cleanup. Patch #2 is shaping the code for the next patches.

Patch #3 converts parse_on_off() to strcmp(). See the commit message for the rationale of why the change should be considered acceptable.

We'd ideally do parse_one_of() likewise. But the strings this function parses tend to be longer, which means more opportunities for typos and more of a reason to abbreviate things.

So instead, patch #4 adds a function parse_one_of_deprecated() for ip macsec to use in one place, where these typos are to be expected, and converts that site to the new function.

Then patch #5 changes the behavior of parse_one_of() to accept prefixes like it has so far, but to warn that they are deprecated:

    # dcb ets set dev swp1 tc-tsa 0:s
    WARNING: 's' matches 'strict' by prefix.
    Matching by prefix is deprecated in this context, please use the full string.

The idea is that several releases down the line, we might consider switching over to strcmp(), as presumably enough advance warning will have been given.

v2:
- Ditch parse_on_off_deprecated() and just use strcmp outright.
- Only use parse_one_of_deprecated() for one call site. Change parse_one_of() to warn on prefix matches.

Petr Machata (5):
lib: utils: Switch matches() to returning int again
lib: utils: Generalize parse_one_of()
lib: utils: Convert parse_on_off() to strcmp()
lib: utils: Introduce parse_one_of_deprecated()
lib: utils: Have parse_one_of() warn about prefix matches

 include/utils.h |  5 ++++-
 ip/ipmacsec.c   |  6 ++++--
 lib/utils.c     | 49 +++++++++++++++++++++++++++++++++++++++++--------
 3 files changed, 49 insertions(+), 11 deletions(-)

2.41.0

Since commit 1f420318bda3 ("utils: don't match empty strings as prefixes") 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(). Even the function itself actually 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.

---
 include/utils.h | 2 +-
 lib/utils.c     | 8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)

index f26ed822..add55bfa 100644
@@ -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);
 

index 99ba7a23..1fc42a9a 100644
@@ -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

The following patch will change the way parse_one_of() and parse_on_off() parse the strings they are given. To prepare for this change, extract from parse_one_of() the functional core, which express in terms of a configurable matcher, a pointer to a function that does the string comparison. Then rewrite parse_one_of() and parse_on_off() as wrappers that just pass matches() as the matcher, thereby maintaining the same behavior as they currently have.

---
 lib/utils.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

index 1fc42a9a..5c91aaa9 100644
@@ -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,18 @@ int parse_one_of(const char *msg, const char *realval, const char * const *list,
 	return 0;
 }
 
+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);
+}
+
 bool parse_on_off(const char *msg, const char *realval, int *p_err)
 {
 	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, matches);
 }
 
 int parse_mapping_gen(int *argcp, char ***argvp,

2.41.0

The function parse_on_off() currently uses 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. In this patch, change parsing to strcmp(). This is a breaking change. The following paragraphs give arguments for why it should be considered acceptable.

First and foremost: on/off are very short strings that it makes practically no sense to shorten. Since "o" is the universal prefix, the only unambiguous shortening is "of" for "off". It is doubtful that anyone would intentionally decide to save typing of the second "f" when they already typed the first. It also seems unlikely that the typo of "of" for "off" would not be caught immediately, as missing a third of the word length would likely be noticed. In other words, it seems improbable that the abbreviated variants are used, intentionally or by mistake.

Commit 9262ccc3ed32 ("bridge: link: Port over to parse_on_off()") and commit 3e0d2a73ba06 ("ip: iplink_bridge_slave: Port over to parse_on_off()") converted several sites from open-coding strcmp()-based on/off parsing to parse_on_off(), which is itself based on matches(). This made the list of permissible strings more generic, but the behavior was exact match to begin with, and this patch restores it.

Commit 5f685d064b03 ("ip: iplink: Convert to use parse_on_off()") has changed from matches()-based parsing, which however had branches in the other order, and "o" would parse to mean on. This indicates that at least in this context, people were not using the shorthand of "o" or the commit would have broken their use case. This supports the thesis that the abbreviations are not really used for on/off parsing.

For completeness, commit 82604d28525a ("lib: Add parse_one_of(), parse_on_off()") introduced parse_on_off(), converting several users in the ip link macsec code in the process. Those users have always used matches(), and had branches in the same order as the newly-introduced parse_on_off().

A survey of selftests and documentation of Linux kernel (by way of git grep), has not discovered any cases of the involved options getting arguments other than the exact strings on and off.

---
 lib/utils.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 5c91aaa9..f1ca3852 100644
@@ -1763,7 +1763,7 @@ bool parse_on_off(const char *msg, const char *realval, int *p_err)
 	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, matches);
+			      ARRAY_SIZE(values_on_off), p_err, strcmp);
 }
 
 int parse_mapping_gen(int *argcp, char ***argvp,

2.41.0

The function parse_one_of() currently uses matches() for string comparison under the hood. Extending matches()-based parsers is tricky, because newly added matches might change the way strings are parsed, if the newly-added string shares a prefix with a string that is matched later in the code.

In this patch, introduce a new function, parse_one_of_deprecated(). This will be currently synonymous with parse_one_of(), however the latter will change behavior in the next patch.

Use the new function for parsing of the macsec "validate" option. The reason is that the valid strings for that option are "disabled", "check" and "strict". It is not hard to see how "disabled" could be misspelled as "disable", and be baked in some script in this form.

---
 include/utils.h | 3 +++
 ip/ipmacsec.c   | 6 ++++--
 lib/utils.c     | 7 +++++++
 3 files changed, 14 insertions(+), 2 deletions(-)

index add55bfa..9ba129b8 100644
@@ -342,6 +342,9 @@ 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);
 
 int parse_mapping_num_all(__u32 *keyp, const char *key);

index 476a6d1d..fc4c8631 100644
@@ -1457,8 +1457,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,

index f1ca3852..9142dc1d 100644
@@ -1758,6 +1758,13 @@ int parse_one_of(const char *msg, const char *realval, const char * const *list,
 	return __parse_one_of(msg, realval, list, len, p_err, matches);
 }
 
+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);
+}
+
 bool parse_on_off(const char *msg, const char *realval, int *p_err)
 {
 	static const char * const values_on_off[] = { "off", "on" };

2.41.0

The function parse_one_of() currently uses matches() for string comparison under the hood. Extending matches()-based parsers is tricky, because newly added matches might change the way strings are parsed, if the newly-added string shares a prefix with a string that is matched later in the code.

Therefore in this patch, add a twist to parse_one_of() that partial prefix matches yield a warning. This will not disturb standard output or the overall behavior, but will make it obvious that the usage is discouraged and prompt users to update their scripts and habits.

An example of output:

    # dcb ets set dev swp1 tc-tsa 0:s
    WARNING: 's' matches 'strict' by prefix.
    Matching by prefix is deprecated in this context, please use the full string.

---
 lib/utils.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

index 9142dc1d..599e859e 100644
@@ -887,6 +887,23 @@ int matches(const char *prefix, const char *string)
 	return *prefix;
 }
 
+static int matches_warn(const char *prefix, const char *string)
+{
+	int rc;
+
+	rc = matches(prefix, string);
+	if (rc)
+		return rc;
+
+	if (strlen(prefix) != strlen(string))
+		fprintf(stderr,
+			"WARNING: '%s' matches '%s' by prefix.\n"
+			"Matching by prefix is deprecated in this context, please use the full string.\n",
+			prefix, string);
+
+	return 0;
+}
+
 int inet_addr_match(const inet_prefix *a, const inet_prefix *b, int bits)
 {
 	const __u32 *a1 = a->data;

@@ -1755,7 +1772,7 @@ __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, matches);
+	return __parse_one_of(msg, realval, list, len, p_err, matches_warn);
 }
 
 int parse_one_of_deprecated(const char *msg, const char *realval,

2.41.0