[PATCH ethtool 0/3] ethool: make --json reliable

This series aims to make --json fail if the output is not
JSON formatted. This should make writing scripts around
ethtool less error prone and give us stronger signal when
produced JSON is invalid.

Jakub Kicinski (3):
  ethtool: remove questionable goto
  ethtool: use dummy args[] entry for no-args case
  ethtool: return error if command does not support --json

 ethtool.c | 52 ++++++++++++++++++++++++++++------------------------
 1 file changed, 28 insertions(+), 24 deletions(-)

-- 
2.31.1

goto opt_found can be trivially replaced by an else branch.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 ethtool.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/ethtool.c b/ethtool.c
index 33a0a492cb15..8cf1b13e4176 100644
--- a/ethtool.c
+++ b/ethtool.c

@@ -6352,15 +6352,14 @@ int main(int argc, char **argp)
 		nlfunc = args[k].nlfunc;
 		nlchk = args[k].nlchk;
 		no_dev = args[k].no_dev;
-		goto opt_found;
+	} else {
+		if ((*argp)[0] == '-')
+			exit_bad_args();
+		nlfunc = nl_gset;
+		func = do_gset;
+		no_dev = false;
 	}
-	if ((*argp)[0] == '-')
-		exit_bad_args();
-	nlfunc = nl_gset;
-	func = do_gset;
-	no_dev = false;
 
-opt_found:
 	if (!no_dev) {
 		ctx.devname = *argp++;
 		argc--;

-- 
2.31.1

Simplify the code flow further by adding a struct option
entry for the no-args case (e.g. "ethtool eth0").

This leads to a slight change in the help output, there
will now be an extra space between FLAGS and DEVICE in
that case:

  ethtool [ FLAGS ]  DEVNAME	Display standard information about device

but hopefully that's okay.

Note that this patch adds a false-positive warning with GCC 11:

ethtool.c: In function ‘find_option’:
ethtool.c:6082:29: warning: offset ‘1’ outside bounds of constant string [-Warray-bounds]
 6082 |                         opt += len + 1;
      |                         ~~~~^~~~~~~~~~

we'll never get to that code if the string is empty.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 ethtool.c | 34 ++++++++++++++--------------------
 1 file changed, 14 insertions(+), 20 deletions(-)

diff --git a/ethtool.c b/ethtool.c
index 8cf1b13e4176..9e02fe4f09a5 100644
--- a/ethtool.c
+++ b/ethtool.c

@@ -5627,6 +5627,13 @@ struct option {
 };
 
 static const struct option args[] = {
+	{
+		/* "default" entry when no switch is used */
+		.opts	= "",
+		.func	= do_gset,
+		.nlfunc	= nl_gset,
+		.help	= "Display standard information about device",
+	},
 	{
 		.opts	= "-s|--change",
 		.func	= do_sset,

@@ -6041,10 +6048,7 @@ static int show_usage(struct cmd_context *ctx __maybe_unused)
 
 	/* ethtool -h */
 	fprintf(stdout, PACKAGE " version " VERSION "\n");
-	fprintf(stdout,
-		"Usage:\n"
-		"        ethtool [ FLAGS ] DEVNAME\t"
-		"Display standard information about device\n");
+	fprintf(stdout,	"Usage:\n");
 	for (i = 0; args[i].opts; i++) {
 		fputs("        ethtool [ FLAGS ] ", stdout);
 		fprintf(stdout, "%s %s\t%s\n",

@@ -6287,11 +6291,7 @@ static int ioctl_init(struct cmd_context *ctx, bool no_dev)
 
 int main(int argc, char **argp)
 {
-	int (*func)(struct cmd_context *);
 	struct cmd_context ctx = {};
-	nl_func_t nlfunc = NULL;
-	nl_chk_t nlchk = NULL;
-	bool no_dev;
 	int ret;
 	int k;
 

@@ -6345,22 +6345,16 @@ int main(int argc, char **argp)
 		exit_bad_args();
 
 	k = find_option(*argp);
-	if (k >= 0) {
+	if (k > 0) {
 		argp++;
 		argc--;
-		func = args[k].func;
-		nlfunc = args[k].nlfunc;
-		nlchk = args[k].nlchk;
-		no_dev = args[k].no_dev;
 	} else {
 		if ((*argp)[0] == '-')
 			exit_bad_args();
-		nlfunc = nl_gset;
-		func = do_gset;
-		no_dev = false;
+		k = 0;
 	}
 
-	if (!no_dev) {
+	if (!args[k].no_dev) {
 		ctx.devname = *argp++;
 		argc--;
 

@@ -6369,11 +6363,11 @@ int main(int argc, char **argp)
 	}
 	ctx.argc = argc;
 	ctx.argp = argp;
-	netlink_run_handler(&ctx, nlchk, nlfunc, !func);
+	netlink_run_handler(&ctx, args[k].nlchk, args[k].nlfunc, !args[k].func);
 
-	ret = ioctl_init(&ctx, no_dev);
+	ret = ioctl_init(&ctx, args[k].no_dev);
 	if (ret)
 		return ret;
 
-	return func(&ctx);
+	return args[k].func(&ctx);
 }

-- 
2.31.1

The --json switch is currently best effort, which is similar
to how other networking utilities treat it. Change to returning
an error if selected command does not support --json.

ethtool is more complex than other utilities because the JSON
support depends on both user space and kernel version. Older
kernel make ethtool use the IOCTL and none of the IOCTL handlers
support JSON.

The current behavior is counter-productive when trying to query
statistics - user has to check (1) if stats (-I) are supported,
(2) if json is supported (--json) and then (3) if underlying
device populates the statistic. Making --json fail if not supported
allows (1) and (2) to both be taken care of with simple check
of ethtool's exit code.

Link: https://pagure.io/centos-sig-hyperscale/package-bugs/issue/6
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 ethtool.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/ethtool.c b/ethtool.c
index 9e02fe4f09a5..bd6242ed383e 100644
--- a/ethtool.c
+++ b/ethtool.c

@@ -5619,6 +5619,7 @@ static int show_usage(struct cmd_context *ctx);
 struct option {
 	const char	*opts;
 	bool		no_dev;
+	bool		json;
 	int		(*func)(struct cmd_context *);
 	nl_chk_t	nlchk;
 	nl_func_t	nlfunc;

@@ -5655,6 +5656,7 @@ static const struct option args[] = {
 	},
 	{
 		.opts	= "-a|--show-pause",
+		.json	= true,
 		.func	= do_gpause,
 		.nlfunc	= nl_gpause,
 		.help	= "Show pause options"

@@ -5779,6 +5781,7 @@ static const struct option args[] = {
 	},
 	{
 		.opts	= "-S|--statistics",
+		.json	= true,
 		.func	= do_gnicstats,
 		.nlchk	= nl_gstats_chk,
 		.nlfunc	= nl_gstats,

@@ -5990,6 +5993,7 @@ static const struct option args[] = {
 	},
 	{
 		.opts	= "--show-fec",
+		.json	= true,
 		.func	= do_gfec,
 		.nlfunc	= nl_gfec,
 		.help	= "Show FEC settings",

@@ -6010,11 +6014,13 @@ static const struct option args[] = {
 	},
 	{
 		.opts	= "--cable-test",
+		.json	= true,
 		.nlfunc	= nl_cable_test,
 		.help	= "Perform a cable test",
 	},
 	{
 		.opts	= "--cable-test-tdr",
+		.json	= true,
 		.nlfunc	= nl_cable_test_tdr,
 		.help	= "Print cable test time domain reflectrometery data",
 		.xhelp	= "		[ first N ]\n"

@@ -6361,10 +6367,15 @@ int main(int argc, char **argp)
 		if (!ctx.devname)
 			exit_bad_args();
 	}
+	if (ctx.json && !args[k].json)
+		exit_bad_args();
 	ctx.argc = argc;
 	ctx.argp = argp;
 	netlink_run_handler(&ctx, args[k].nlchk, args[k].nlfunc, !args[k].func);
 
+	if (ctx.json) /* no IOCTL command supports JSON output */
+		exit_bad_args();
+
 	ret = ioctl_init(&ctx, args[k].no_dev);
 	if (ret)
 		return ret;

-- 
2.31.1

On Fri, 13 Aug 2021 10:19:35 -0700 Jakub Kicinski wrote:

This series aims to make --json fail if the output is not
JSON formatted. This should make writing scripts around
ethtool less error prone and give us stronger signal when
produced JSON is invalid.

Seems like it's been over a week, any comments anyone?

Hello:

This series was applied to ethtool/ethtool.git (refs/heads/master):

On Fri, 13 Aug 2021 10:19:35 -0700 you wrote:

This series aims to make --json fail if the output is not
JSON formatted. This should make writing scripts around
ethtool less error prone and give us stronger signal when
produced JSON is invalid.

Jakub Kicinski (3):
  ethtool: remove questionable goto
  ethtool: use dummy args[] entry for no-args case
  ethtool: return error if command does not support --json

[...]

Here is the summary with links:
  - [ethtool,1/3] ethtool: remove questionable goto
    https://git.kernel.org/pub/scm/network/ethtool/ethtool.git/commit/?id=6cf8d25c5be4
  - [ethtool,2/3] ethtool: use dummy args[] entry for no-args case
    https://git.kernel.org/pub/scm/network/ethtool/ethtool.git/commit/?id=444f36546f53
  - [ethtool,3/3] ethtool: return error if command does not support --json
    https://git.kernel.org/pub/scm/network/ethtool/ethtool.git/commit/?id=9a935085ec1c

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

On Fri, Aug 13, 2021 at 10:19:37AM -0700, Jakub Kicinski wrote:

Note that this patch adds a false-positive warning with GCC 11:

ethtool.c: In function ‘find_option’:
ethtool.c:6082:29: warning: offset ‘1’ outside bounds of constant string [-Warray-bounds]
 6082 |                         opt += len + 1;
      |                         ~~~~^~~~~~~~~~

we'll never get to that code if the string is empty.

Unless I missed something, an easy workaround should be starting the
loop in find_option() from 1 rather than from 0. It would IMHO even make
more sense as there is little point comparing the first argument against
the dummy args[0] entry.

Michal

On Tue, 24 Aug 2021 19:41:23 +0200 Michal Kubecek wrote:

On Fri, Aug 13, 2021 at 10:19:37AM -0700, Jakub Kicinski wrote:

quoted

Note that this patch adds a false-positive warning with GCC 11:

ethtool.c: In function ‘find_option’:
ethtool.c:6082:29: warning: offset ‘1’ outside bounds of constant string [-Warray-bounds]
 6082 |                         opt += len + 1;
      |                         ~~~~^~~~~~~~~~

we'll never get to that code if the string is empty.  

Unless I missed something, an easy workaround should be starting the
loop in find_option() from 1 rather than from 0. It would IMHO even make
more sense as there is little point comparing the first argument against
the dummy args[0] entry.

SGTM, will you commit a patch or should I send one?

On Tue, Aug 24, 2021 at 10:43:23AM -0700, Jakub Kicinski wrote:

On Tue, 24 Aug 2021 19:41:23 +0200 Michal Kubecek wrote:

quoted

On Fri, Aug 13, 2021 at 10:19:37AM -0700, Jakub Kicinski wrote:

quoted

Note that this patch adds a false-positive warning with GCC 11:

ethtool.c: In function ‘find_option’:
ethtool.c:6082:29: warning: offset ‘1’ outside bounds of constant string [-Warray-bounds]
 6082 |                         opt += len + 1;
      |                         ~~~~^~~~~~~~~~

we'll never get to that code if the string is empty.  

Unless I missed something, an easy workaround should be starting the
loop in find_option() from 1 rather than from 0. It would IMHO even make
more sense as there is little point comparing the first argument against
the dummy args[0] entry.

SGTM, will you commit a patch or should I send one?

I'll commit it.

Michal