[PATCH iproute2-next v2 0/5] Change parsing in parse_one_of(), parse_on_off()

[PATCH iproute2-next v2 0/5] Change parsing in parse_one_of(), parse_on_off()

[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 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

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

[Petr Machata]

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.

Cc: Matteo Croce <mcroce@redhat.com>
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 v2 2/5] lib: utils: Generalize parse_one_of()

[Petr Machata]

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.

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

diff --git a/lib/utils.c b/lib/utils.c
index 1fc42a9a..5c91aaa9 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,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

[PATCH iproute2-next v2 3/5] lib: utils: Convert parse_on_off() to strcmp()

[Petr Machata]

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.

Signed-off-by: Petr Machata <petrm@nvidia.com>
---
 lib/utils.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/utils.c b/lib/utils.c
index 5c91aaa9..f1ca3852 100644
--- a/lib/utils.c
+++ b/lib/utils.c
@@ -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

[PATCH iproute2-next v2 4/5] lib: utils: Introduce parse_one_of_deprecated()

[Petr Machata]

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.

Signed-off-by: Petr Machata <petrm@nvidia.com>
---
 include/utils.h | 3 +++
 ip/ipmacsec.c   | 6 ++++--
 lib/utils.c     | 7 +++++++
 3 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/include/utils.h b/include/utils.h
index add55bfa..9ba129b8 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -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);
diff --git a/ip/ipmacsec.c b/ip/ipmacsec.c
index 476a6d1d..fc4c8631 100644
--- a/ip/ipmacsec.c
+++ b/ip/ipmacsec.c
@@ -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,
diff --git a/lib/utils.c b/lib/utils.c
index f1ca3852..9142dc1d 100644
--- a/lib/utils.c
+++ b/lib/utils.c
@@ -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

[PATCH iproute2-next v2 5/5] lib: utils: Have parse_one_of() warn about prefix matches

[Petr Machata]

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.

Signed-off-by: Petr Machata <petrm@nvidia.com>
---
 lib/utils.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/lib/utils.c b/lib/utils.c
index 9142dc1d..599e859e 100644
--- a/lib/utils.c
+++ b/lib/utils.c
@@ -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

Re: [PATCH iproute2-next v2 0/5] Change parsing in parse_one_of(), parse_on_off()

[patchwork-bot+netdevbpf]

Hello:

This series was applied to iproute2/iproute2-next.git (main)
by David Ahern <dsahern@kernel.org>:

On Wed, 22 Nov 2023 16:23:27 +0100 you wrote:
> 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.
> 
> [...]

Here is the summary with links:
  - [iproute2-next,v2,1/5] lib: utils: Switch matches() to returning int again
    https://git.kernel.org/pub/scm/network/iproute2/iproute2-next.git/commit/?id=60254925ccab
  - [iproute2-next,v2,2/5] lib: utils: Generalize parse_one_of()
    https://git.kernel.org/pub/scm/network/iproute2/iproute2-next.git/commit/?id=256e0ca4b84f
  - [iproute2-next,v2,3/5] lib: utils: Convert parse_on_off() to strcmp()
    https://git.kernel.org/pub/scm/network/iproute2/iproute2-next.git/commit/?id=5ba57152d27c
  - [iproute2-next,v2,4/5] lib: utils: Introduce parse_one_of_deprecated()
    https://git.kernel.org/pub/scm/network/iproute2/iproute2-next.git/commit/?id=2b8766663d3c
  - [iproute2-next,v2,5/5] lib: utils: Have parse_one_of() warn about prefix matches
    https://git.kernel.org/pub/scm/network/iproute2/iproute2-next.git/commit/?id=bd5226437a4c

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html