* [PATCH ethtool 0/3] ethool: make --json reliable
@ 2021-08-13 17:19 Jakub Kicinski
2021-08-13 17:19 ` [PATCH ethtool 1/3] ethtool: remove questionable goto Jakub Kicinski
` (4 more replies)
0 siblings, 5 replies; 9+ messages in thread
From: Jakub Kicinski @ 2021-08-13 17:19 UTC (permalink / raw)
To: mkubecek, andrew; +Cc: netdev, dcavalca, filbranden, michel, Jakub Kicinski
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH ethtool 1/3] ethtool: remove questionable goto
2021-08-13 17:19 [PATCH ethtool 0/3] ethool: make --json reliable Jakub Kicinski
@ 2021-08-13 17:19 ` Jakub Kicinski
2021-08-13 17:19 ` [PATCH ethtool 2/3] ethtool: use dummy args[] entry for no-args case Jakub Kicinski
` (3 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: Jakub Kicinski @ 2021-08-13 17:19 UTC (permalink / raw)
To: mkubecek, andrew; +Cc: netdev, dcavalca, filbranden, michel, Jakub Kicinski
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH ethtool 2/3] ethtool: use dummy args[] entry for no-args case
2021-08-13 17:19 [PATCH ethtool 0/3] ethool: make --json reliable Jakub Kicinski
2021-08-13 17:19 ` [PATCH ethtool 1/3] ethtool: remove questionable goto Jakub Kicinski
@ 2021-08-13 17:19 ` Jakub Kicinski
2021-08-24 17:41 ` Michal Kubecek
2021-08-13 17:19 ` [PATCH ethtool 3/3] ethtool: return error if command does not support --json Jakub Kicinski
` (2 subsequent siblings)
4 siblings, 1 reply; 9+ messages in thread
From: Jakub Kicinski @ 2021-08-13 17:19 UTC (permalink / raw)
To: mkubecek, andrew; +Cc: netdev, dcavalca, filbranden, michel, Jakub Kicinski
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH ethtool 3/3] ethtool: return error if command does not support --json
2021-08-13 17:19 [PATCH ethtool 0/3] ethool: make --json reliable Jakub Kicinski
2021-08-13 17:19 ` [PATCH ethtool 1/3] ethtool: remove questionable goto Jakub Kicinski
2021-08-13 17:19 ` [PATCH ethtool 2/3] ethtool: use dummy args[] entry for no-args case Jakub Kicinski
@ 2021-08-13 17:19 ` Jakub Kicinski
2021-08-24 14:08 ` [PATCH ethtool 0/3] ethool: make --json reliable Jakub Kicinski
2021-08-24 17:30 ` patchwork-bot+netdevbpf
4 siblings, 0 replies; 9+ messages in thread
From: Jakub Kicinski @ 2021-08-13 17:19 UTC (permalink / raw)
To: mkubecek, andrew; +Cc: netdev, dcavalca, filbranden, michel, Jakub Kicinski
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH ethtool 0/3] ethool: make --json reliable
2021-08-13 17:19 [PATCH ethtool 0/3] ethool: make --json reliable Jakub Kicinski
` (2 preceding siblings ...)
2021-08-13 17:19 ` [PATCH ethtool 3/3] ethtool: return error if command does not support --json Jakub Kicinski
@ 2021-08-24 14:08 ` Jakub Kicinski
2021-08-24 17:30 ` patchwork-bot+netdevbpf
4 siblings, 0 replies; 9+ messages in thread
From: Jakub Kicinski @ 2021-08-24 14:08 UTC (permalink / raw)
To: mkubecek, andrew; +Cc: netdev
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?
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH ethtool 0/3] ethool: make --json reliable
2021-08-13 17:19 [PATCH ethtool 0/3] ethool: make --json reliable Jakub Kicinski
` (3 preceding siblings ...)
2021-08-24 14:08 ` [PATCH ethtool 0/3] ethool: make --json reliable Jakub Kicinski
@ 2021-08-24 17:30 ` patchwork-bot+netdevbpf
4 siblings, 0 replies; 9+ messages in thread
From: patchwork-bot+netdevbpf @ 2021-08-24 17:30 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: mkubecek, andrew, netdev, dcavalca, filbranden, michel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH ethtool 2/3] ethtool: use dummy args[] entry for no-args case
2021-08-13 17:19 ` [PATCH ethtool 2/3] ethtool: use dummy args[] entry for no-args case Jakub Kicinski
@ 2021-08-24 17:41 ` Michal Kubecek
2021-08-24 17:43 ` Jakub Kicinski
0 siblings, 1 reply; 9+ messages in thread
From: Michal Kubecek @ 2021-08-24 17:41 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: andrew, netdev, dcavalca, filbranden, michel
[-- Attachment #1: Type: text/plain, Size: 695 bytes --]
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH ethtool 2/3] ethtool: use dummy args[] entry for no-args case
2021-08-24 17:41 ` Michal Kubecek
@ 2021-08-24 17:43 ` Jakub Kicinski
2021-08-24 19:46 ` Michal Kubecek
0 siblings, 1 reply; 9+ messages in thread
From: Jakub Kicinski @ 2021-08-24 17:43 UTC (permalink / raw)
To: Michal Kubecek; +Cc: andrew, netdev, dcavalca, filbranden, michel
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:
> > 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?
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH ethtool 2/3] ethtool: use dummy args[] entry for no-args case
2021-08-24 17:43 ` Jakub Kicinski
@ 2021-08-24 19:46 ` Michal Kubecek
0 siblings, 0 replies; 9+ messages in thread
From: Michal Kubecek @ 2021-08-24 19:46 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: andrew, netdev, dcavalca, filbranden, michel
[-- Attachment #1: Type: text/plain, Size: 956 bytes --]
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:
> > 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.
>
> SGTM, will you commit a patch or should I send one?
I'll commit it.
Michal
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-08-24 19:47 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-13 17:19 [PATCH ethtool 0/3] ethool: make --json reliable Jakub Kicinski
2021-08-13 17:19 ` [PATCH ethtool 1/3] ethtool: remove questionable goto Jakub Kicinski
2021-08-13 17:19 ` [PATCH ethtool 2/3] ethtool: use dummy args[] entry for no-args case Jakub Kicinski
2021-08-24 17:41 ` Michal Kubecek
2021-08-24 17:43 ` Jakub Kicinski
2021-08-24 19:46 ` Michal Kubecek
2021-08-13 17:19 ` [PATCH ethtool 3/3] ethtool: return error if command does not support --json Jakub Kicinski
2021-08-24 14:08 ` [PATCH ethtool 0/3] ethool: make --json reliable Jakub Kicinski
2021-08-24 17:30 ` patchwork-bot+netdevbpf
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.