diff mbox series

[iproute2-next,v2,02/11] lib: Add parse_one_of(), parse_on_off()

Message ID 194ae677df465086d6cd1d7962c07d790e6d049d.1604059429.git.me@pmachata.org
State Superseded
Headers show
Series None | expand

Commit Message

Petr Machata Oct. 30, 2020, 12:29 p.m. UTC 
Take from the macsec code parse_one_of() and adapt so that it passes the
primary result as the main return value, and error result through a
pointer. That is the simplest way to make the code reusable across data
types without introducing extra magic.

Also from macsec take the specialization of parse_one_of() for parsing
specifically the strings "off" and "on".

Convert the macsec code to the new helpers.

Signed-off-by: Petr Machata <me@pmachata.org>
---
 include/utils.h |  4 ++++
 ip/ipmacsec.c   | 52 +++++++++++--------------------------------------
 lib/utils.c     | 28 ++++++++++++++++++++++++++
 3 files changed, 43 insertions(+), 41 deletions(-)

Comments

David Ahern Oct. 31, 2020, 3:37 p.m. UTC | #1 
On 10/30/20 6:29 AM, Petr Machata wrote:
> diff --git a/lib/utils.c b/lib/utils.c

> index 9815e328c9e0..930877ae0f0d 100644

> --- a/lib/utils.c

> +++ b/lib/utils.c

> @@ -1735,3 +1735,31 @@ 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)

> +{

> +	int i;

> +

> +	for (i = 0; i < len; i++) {

> +		if (list[i] && matches(realval, list[i]) == 0) {

> +			*p_err = 0;

> +			return i;

> +		}

> +	}

> +

> +	fprintf(stderr, "Error: argument of \"%s\" must be one of ", msg);

> +	for (i = 0; i < len; i++)

> +		if (list[i])

> +			fprintf(stderr, "\"%s\", ", list[i]);

> +	fprintf(stderr, "not \"%s\"\n", realval);

> +	*p_err = -EINVAL;

> +	return 0;

> +}

> +

> +int 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);

> +}

> 


This has weird semantics to me. You have a buried array of strings and
returning the index of the one that matches. Let's use a 'bool' return
for parse_on_off that makes it clear that the string is 'off' = false or
'on' = true.
Petr Machata Oct. 31, 2020, 9:25 p.m. UTC | #2 
David Ahern <dsahern@gmail.com> writes:

> On 10/30/20 6:29 AM, Petr Machata wrote:

>> +int 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);

>> +}

>> 

>

> This has weird semantics to me. You have a buried array of strings and

> returning the index of the one that matches. Let's use a 'bool' return

> for parse_on_off that makes it clear that the string is 'off' = false or

> 'on' = true.


Agreed, it should return bool.
diff mbox series

Patch

diff --git a/include/utils.h b/include/utils.h
index 085b17b1f6e3..bd62cdcd7122 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -325,4 +325,8 @@  char *sprint_time64(__s64 time, char *buf);
 int do_batch(const char *name, bool force,
 	     int (*cmd)(int argc, char *argv[], void *user), void *user);
 
+int parse_one_of(const char *msg, const char *realval, const char * const *list,
+		 size_t len, int *p_err);
+int parse_on_off(const char *msg, const char *realval, int *p_err);
+
 #endif /* __UTILS_H__ */
diff --git a/ip/ipmacsec.c b/ip/ipmacsec.c
index 18289ecd6d9e..bf48e8b5d0b2 100644
--- a/ip/ipmacsec.c
+++ b/ip/ipmacsec.c
@@ -23,8 +23,6 @@ 
 #include "ll_map.h"
 #include "libgenl.h"
 
-static const char * const values_on_off[] = { "off", "on" };
-
 static const char * const validate_str[] = {
 	[MACSEC_VALIDATE_DISABLED] = "disabled",
 	[MACSEC_VALIDATE_CHECK] = "check",
@@ -108,25 +106,6 @@  static void ipmacsec_usage(void)
 	exit(-1);
 }
 
-static int one_of(const char *msg, const char *realval, const char * const *list,
-		  size_t len, int *index)
-{
-	int i;
-
-	for (i = 0; i < len; i++) {
-		if (matches(realval, list[i]) == 0) {
-			*index = i;
-			return 0;
-		}
-	}
-
-	fprintf(stderr, "Error: argument of \"%s\" must be one of ", msg);
-	for (i = 0; i < len; i++)
-		fprintf(stderr, "\"%s\", ", list[i]);
-	fprintf(stderr, "not \"%s\"\n", realval);
-	return -1;
-}
-
 static int get_an(__u8 *val, const char *arg)
 {
 	int ret = get_u8(val, arg, 0);
@@ -559,8 +538,7 @@  static int do_offload(enum cmd c, int argc, char **argv)
 	if (argc == 0)
 		ipmacsec_usage();
 
-	ret = one_of("offload", *argv, offload_str, ARRAY_SIZE(offload_str),
-		     (int *)&offload);
+	offload = parse_one_of("offload", *argv, offload_str, ARRAY_SIZE(offload_str), &ret);
 	if (ret)
 		ipmacsec_usage();
 
@@ -1334,8 +1312,7 @@  static int macsec_parse_opt(struct link_util *lu, int argc, char **argv,
 			NEXT_ARG();
 			int i;
 
-			ret = one_of("encrypt", *argv, values_on_off,
-				     ARRAY_SIZE(values_on_off), &i);
+			i = parse_on_off("encrypt", *argv, &ret);
 			if (ret != 0)
 				return ret;
 			addattr8(n, MACSEC_BUFLEN, IFLA_MACSEC_ENCRYPT, i);
@@ -1343,8 +1320,7 @@  static int macsec_parse_opt(struct link_util *lu, int argc, char **argv,
 			NEXT_ARG();
 			int i;
 
-			ret = one_of("send_sci", *argv, values_on_off,
-				     ARRAY_SIZE(values_on_off), &i);
+			i = parse_on_off("send_sci", *argv, &ret);
 			if (ret != 0)
 				return ret;
 			send_sci = i;
@@ -1354,8 +1330,7 @@  static int macsec_parse_opt(struct link_util *lu, int argc, char **argv,
 			NEXT_ARG();
 			int i;
 
-			ret = one_of("end_station", *argv, values_on_off,
-				     ARRAY_SIZE(values_on_off), &i);
+			i = parse_on_off("end_station", *argv, &ret);
 			if (ret != 0)
 				return ret;
 			es = i;
@@ -1364,8 +1339,7 @@  static int macsec_parse_opt(struct link_util *lu, int argc, char **argv,
 			NEXT_ARG();
 			int i;
 
-			ret = one_of("scb", *argv, values_on_off,
-				     ARRAY_SIZE(values_on_off), &i);
+			i = parse_on_off("scb", *argv, &ret);
 			if (ret != 0)
 				return ret;
 			scb = i;
@@ -1374,8 +1348,7 @@  static int macsec_parse_opt(struct link_util *lu, int argc, char **argv,
 			NEXT_ARG();
 			int i;
 
-			ret = one_of("protect", *argv, values_on_off,
-				     ARRAY_SIZE(values_on_off), &i);
+			i = parse_on_off("protect", *argv, &ret);
 			if (ret != 0)
 				return ret;
 			addattr8(n, MACSEC_BUFLEN, IFLA_MACSEC_PROTECT, i);
@@ -1383,8 +1356,7 @@  static int macsec_parse_opt(struct link_util *lu, int argc, char **argv,
 			NEXT_ARG();
 			int i;
 
-			ret = one_of("replay", *argv, values_on_off,
-				     ARRAY_SIZE(values_on_off), &i);
+			i = parse_on_off("replay", *argv, &ret);
 			if (ret != 0)
 				return ret;
 			replay_protect = !!i;
@@ -1395,9 +1367,8 @@  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();
-			ret = one_of("validate", *argv,
-				     validate_str, ARRAY_SIZE(validate_str),
-				     (int *)&validate);
+			validate = parse_one_of("validate", *argv, validate_str,
+						ARRAY_SIZE(validate_str), &ret);
 			if (ret != 0)
 				return ret;
 			addattr8(n, MACSEC_BUFLEN,
@@ -1411,9 +1382,8 @@  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();
-			ret = one_of("offload", *argv,
-				     offload_str, ARRAY_SIZE(offload_str),
-				     (int *)&offload);
+			offload = parse_one_of("offload", *argv, offload_str,
+					       ARRAY_SIZE(offload_str), &ret);
 			if (ret != 0)
 				return ret;
 			addattr8(n, MACSEC_BUFLEN,
diff --git a/lib/utils.c b/lib/utils.c
index 9815e328c9e0..930877ae0f0d 100644
--- a/lib/utils.c
+++ b/lib/utils.c
@@ -1735,3 +1735,31 @@  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)
+{
+	int i;
+
+	for (i = 0; i < len; i++) {
+		if (list[i] && matches(realval, list[i]) == 0) {
+			*p_err = 0;
+			return i;
+		}
+	}
+
+	fprintf(stderr, "Error: argument of \"%s\" must be one of ", msg);
+	for (i = 0; i < len; i++)
+		if (list[i])
+			fprintf(stderr, "\"%s\", ", list[i]);
+	fprintf(stderr, "not \"%s\"\n", realval);
+	*p_err = -EINVAL;
+	return 0;
+}
+
+int 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);
+}