Re: [RFC PATCH ethtool] ethtool: better syntax for combinations of FEC modes

[Michal Kubecek <mkubecek@suse.cz>]

On Wed, Sep 19, 2018 at 05:06:25PM +0100, Edward Cree wrote:
> Instead of commas, just have them as separate argvs.
> 
> The parsing state machine might look heavyweight, but it makes it easy to add
>  more parameters later and distinguish parameter names from encoding names.
> 
> Suggested-by: Michal Kubecek <mkubecek@suse.cz>
> Signed-off-by: Edward Cree <ecree@solarflare.com>
> ---

Looks good, thank you.

Reviewed-by: Michal Kubecek <mkubecek@suse.cz>


>  ethtool.8.in   |  6 +++---
>  ethtool.c      | 63 ++++++++++++++++------------------------------------------
>  test-cmdline.c | 10 +++++-----
>  3 files changed, 25 insertions(+), 54 deletions(-)
> 
> diff --git a/ethtool.8.in b/ethtool.8.in
> index 414eaa1..7ea2cc0 100644
> --- a/ethtool.8.in
> +++ b/ethtool.8.in
> @@ -390,7 +390,7 @@ ethtool \- query or control network driver and hardware settings
>  .B ethtool \-\-set\-fec
>  .I devname
>  .B encoding
> -.BR auto | off | rs | baser [ , ...]
> +.BR auto | off | rs | baser \ [...]
>  .
>  .\" Adjust lines (i.e. full justification) and hyphenate.
>  .ad
> @@ -1120,11 +1120,11 @@ current FEC mode, the driver or firmware must take the link down
>  administratively and report the problem in the system logs for users to correct.
>  .RS 4
>  .TP
> -.BR encoding\ auto | off | rs | baser [ , ...]
> +.BR encoding\ auto | off | rs | baser \ [...]
>  
>  Sets the FEC encoding for the device.  Combinations of options are specified as
>  e.g.
> -.B auto,rs
> +.B encoding auto rs
>  ; the semantics of such combinations vary between drivers.
>  .TS
>  nokeep;
> diff --git a/ethtool.c b/ethtool.c
> index 9997930..2f7e96b 100644
> --- a/ethtool.c
> +++ b/ethtool.c
> @@ -4979,39 +4979,6 @@ static int fecmode_str_to_type(const char *str)
>  	return 0;
>  }
>  
> -/* Takes a comma-separated list of FEC modes, returns the bitwise OR of their
> - * corresponding ETHTOOL_FEC_* constants.
> - * Accepts repetitions (e.g. 'auto,auto') and trailing comma (e.g. 'off,').
> - */
> -static int parse_fecmode(const char *str)
> -{
> -	int fecmode = 0;
> -	char buf[6];
> -
> -	if (!str)
> -		return 0;
> -	while (*str) {
> -		size_t next;
> -		int mode;
> -
> -		next = strcspn(str, ",");
> -		if (next >= 6) /* Bad mode, longest name is 5 chars */
> -			return 0;
> -		/* Copy into temp buffer and nul-terminate */
> -		memcpy(buf, str, next);
> -		buf[next] = 0;
> -		mode = fecmode_str_to_type(buf);
> -		if (!mode) /* Bad mode encountered */
> -			return 0;
> -		fecmode |= mode;
> -		str += next;
> -		/* Skip over ',' (but not nul) */
> -		if (*str)
> -			str++;
> -	}
> -	return fecmode;
> -}
> -
>  static int do_gfec(struct cmd_context *ctx)
>  {
>  	struct ethtool_fecparam feccmd = { 0 };
> @@ -5041,22 +5008,26 @@ static int do_gfec(struct cmd_context *ctx)
>  
>  static int do_sfec(struct cmd_context *ctx)
>  {
> -	char *fecmode_str = NULL;
> +	enum { ARG_NONE, ARG_ENCODING } state = ARG_NONE;
>  	struct ethtool_fecparam feccmd;
> -	struct cmdline_info cmdline_fec[] = {
> -		{ "encoding", CMDL_STR,  &fecmode_str,  &feccmd.fec},
> -	};
> -	int changed;
> -	int fecmode;
> -	int rv;
> +	int fecmode = 0, newmode;
> +	int rv, i;
>  
> -	parse_generic_cmdline(ctx, &changed, cmdline_fec,
> -			      ARRAY_SIZE(cmdline_fec));
> -
> -	if (!fecmode_str)
> +	for (i = 0; i < ctx->argc; i++) {
> +		if (!strcmp(ctx->argp[i], "encoding")) {
> +			state = ARG_ENCODING;
> +			continue;
> +		}
> +		if (state == ARG_ENCODING) {
> +			newmode = fecmode_str_to_type(ctx->argp[i]);
> +			if (!newmode)
> +				exit_bad_args();
> +			fecmode |= newmode;
> +			continue;
> +		}
>  		exit_bad_args();
> +	}
>  
> -	fecmode = parse_fecmode(fecmode_str);
>  	if (!fecmode)
>  		exit_bad_args();
>  
> @@ -5265,7 +5236,7 @@ static const struct option {
>  	  "		[ all ]\n"},
>  	{ "--show-fec", 1, do_gfec, "Show FEC settings"},
>  	{ "--set-fec", 1, do_sfec, "Set FEC settings",
> -	  "		[ encoding auto|off|rs|baser ]\n"},
> +	  "		[ encoding auto|off|rs|baser [...]]\n"},
>  	{ "-h|--help", 0, show_usage, "Show this help" },
>  	{ "--version", 0, do_version, "Show version number" },
>  	{}
> diff --git a/test-cmdline.c b/test-cmdline.c
> index 9c51cca..84630a5 100644
> --- a/test-cmdline.c
> +++ b/test-cmdline.c
> @@ -268,12 +268,12 @@ static struct test_case {
>  	{ 1, "--set-eee devname advertise foo" },
>  	{ 1, "--set-fec devname" },
>  	{ 0, "--set-fec devname encoding auto" },
> -	{ 0, "--set-fec devname encoding off," },
> -	{ 0, "--set-fec devname encoding baser,rs" },
> -	{ 0, "--set-fec devname encoding auto,auto," },
> +	{ 0, "--set-fec devname encoding off" },
> +	{ 0, "--set-fec devname encoding baser rs" },
> +	{ 0, "--set-fec devname encoding auto auto" },
>  	{ 1, "--set-fec devname encoding foo" },
> -	{ 1, "--set-fec devname encoding auto,foo" },
> -	{ 1, "--set-fec devname encoding auto,," },
> +	{ 1, "--set-fec devname encoding auto foo" },
> +	{ 1, "--set-fec devname encoding none" },
>  	{ 1, "--set-fec devname auto" },
>  	/* can't test --set-priv-flags yet */
>  	{ 0, "-h" },