[PATCH ethtool] ethtool: do_sset return correct value on fail

[PATCH ethtool] ethtool: do_sset return correct value on fail

[Tariq Toukan]

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)
-- 
2.21.0

Re: [PATCH ethtool] ethtool: do_sset return correct value on fail

[Michal Kubecek]

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