| Message ID | 20201213142503.25509-1-tariqt@nvidia.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Michal Kubecek |
| Headers | show |
| Series | [ethtool] ethtool: do_sset return correct value on fail | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Sun, Dec 13, 2020 at 04:25:03PM +0200, Tariq Toukan wrote: > From: Roy Novich <royno@nvidia.com> > > The return value for do_sset was constant and returned 0. > This value is misleading when returned on operation failure. > Changed return value to the correct function err status. > > Fixes: 32c8037055f5 ("Initial import of ethtool version 3 + a few patches.") > Signed-off-by: Roy Novich <royno@nvidia.com> > Signed-off-by: Tariq Toukan <tariqt@nvidia.com> > --- > ethtool.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/ethtool.c b/ethtool.c > index 1d9067e774af..5cc875c64591 100644 > --- a/ethtool.c > +++ b/ethtool.c > @@ -3287,7 +3287,7 @@ static int do_sset(struct cmd_context *ctx) > } > } > > - return 0; > + return err; > } > > static int do_gregs(struct cmd_context *ctx) I'm afraid it's not as easy as this. The problem with -s/--set subcommand is that its parameters are in fact implemented by three separate requests (for ioctl, four requests for netlink). Each of them may fail or succeed independently of others. Currently do_sset() always returns 0 which is indeed unfortunate but it's not clear what should we return if some requests fail and some succeed. With your patch, do_sset() would always return the result of last request it sent to kernel which is inconsistent; consider e.g. ethtool -s eth0 mdix on wol g If the ETHTOOL_SLINKSETTINGS request setting MDI-X succeeds and ETHTOOL_SWOL setting WoL fails, the result would be a failure. But if setting MDI-X fails and setting WoL succeeds, do_sset() would report success. So if we really want to change the behaviour after so many years, it should be consistent: either return non-zero exit code if any request fails or if all fail. (Personally, I would prefer the latter.) And we should probably modify nl_sset() to behave the same way as it currently bails out on first failed request. Michal
diff --git a/ethtool.c b/ethtool.c index 1d9067e774af..5cc875c64591 100644 --- a/ethtool.c +++ b/ethtool.c @@ -3287,7 +3287,7 @@ static int do_sset(struct cmd_context *ctx) } } - return 0; + return err; } static int do_gregs(struct cmd_context *ctx)