[patch net-next] team: do not use -ENOENT

[patch net-next] team: do not use -ENOENT

[Jiri Pirko]

Since this error code means "No such file or directory", change this
value in team driver to ones which make more sense.

Signed-off-by: Jiri Pirko <jiri@resnulli.us>
---
 drivers/net/team/team.c                   | 4 ++--
 drivers/net/team/team_mode_activebackup.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
index 70d5d6b..3d7cf6e 100644
--- a/drivers/net/team/team.c
+++ b/drivers/net/team/team.c
@@ -1128,7 +1128,7 @@ static int team_port_del(struct team *team, struct net_device *port_dev)
 	if (!port || !team_port_find(team, port)) {
 		netdev_err(dev, "Device %s does not act as a port of this team\n",
 			   portname);
-		return -ENOENT;
+		return -ENODEV;
 	}
 
 	__team_option_inst_mark_removed_port(team, port);
@@ -2320,7 +2320,7 @@ static int team_nl_cmd_options_set(struct sk_buff *skb, struct genl_info *info)
 			list_add(&opt_inst->tmp_list, &opt_inst_list);
 		}
 		if (!opt_found) {
-			err = -ENOENT;
+			err = -EINVAL;
 			goto team_put;
 		}
 	}
diff --git a/drivers/net/team/team_mode_activebackup.c b/drivers/net/team/team_mode_activebackup.c
index 6262b4d..2792e13 100644
--- a/drivers/net/team/team_mode_activebackup.c
+++ b/drivers/net/team/team_mode_activebackup.c
@@ -81,7 +81,7 @@ static int ab_active_port_set(struct team *team, struct team_gsetter_ctx *ctx)
 			return 0;
 		}
 	}
-	return -ENOENT;
+	return -ENODEV;
 }
 
 static const struct team_option ab_options[] = {
-- 
1.8.1

Re: [patch net-next] team: do not use -ENOENT

[Stephen Hemminger]

On Thu, 17 Jan 2013 11:25:00 +0100
Jiri Pirko <jiri@resnulli.us> wrote:

> Since this error code means "No such file or directory", change this
> value in team driver to ones which make more sense.
> 
> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
> ---
>  drivers/net/team/team.c                   | 4 ++--
>  drivers/net/team/team_mode_activebackup.c | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
> index 70d5d6b..3d7cf6e 100644
> --- a/drivers/net/team/team.c
> +++ b/drivers/net/team/team.c
> @@ -1128,7 +1128,7 @@ static int team_port_del(struct team *team, struct net_device *port_dev)
>  	if (!port || !team_port_find(team, port)) {
>  		netdev_err(dev, "Device %s does not act as a port of this team\n",
>  			   portname);
> -		return -ENOENT;
> +		return -ENODEV;
>  	}
>  
>  	__team_option_inst_mark_removed_port(team, port);
> @@ -2320,7 +2320,7 @@ static int team_nl_cmd_options_set(struct sk_buff *skb, struct genl_info *info)
>  			list_add(&opt_inst->tmp_list, &opt_inst_list);
>  		}
>  		if (!opt_found) {
> -			err = -ENOENT;
> +			err = -EINVAL;
>  			goto team_put;
>  		}
>  	}
> diff --git a/drivers/net/team/team_mode_activebackup.c b/drivers/net/team/team_mode_activebackup.c
> index 6262b4d..2792e13 100644
> --- a/drivers/net/team/team_mode_activebackup.c
> +++ b/drivers/net/team/team_mode_activebackup.c
> @@ -81,7 +81,7 @@ static int ab_active_port_set(struct team *team, struct team_gsetter_ctx *ctx)
>  			return 0;
>  		}
>  	}
> -	return -ENOENT;
> +	return -ENODEV;
>  }
>  
>  static const struct team_option ab_options[] = {

To be pedantic.
Changing errno's means effectively changing the ABI.
Linus has already rejected similar patches in other areas.

Re: [patch net-next] team: do not use -ENOENT

[Jiri Pirko]

Thu, Jan 17, 2013 at 04:51:15PM CET, stephen@networkplumber.org wrote:
>On Thu, 17 Jan 2013 11:25:00 +0100
>Jiri Pirko <jiri@resnulli.us> wrote:
>
>> Since this error code means "No such file or directory", change this
>> value in team driver to ones which make more sense.
>> 
>> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>> ---
>>  drivers/net/team/team.c                   | 4 ++--
>>  drivers/net/team/team_mode_activebackup.c | 2 +-
>>  2 files changed, 3 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
>> index 70d5d6b..3d7cf6e 100644
>> --- a/drivers/net/team/team.c
>> +++ b/drivers/net/team/team.c
>> @@ -1128,7 +1128,7 @@ static int team_port_del(struct team *team, struct net_device *port_dev)
>>  	if (!port || !team_port_find(team, port)) {
>>  		netdev_err(dev, "Device %s does not act as a port of this team\n",
>>  			   portname);
>> -		return -ENOENT;
>> +		return -ENODEV;
>>  	}
>>  
>>  	__team_option_inst_mark_removed_port(team, port);
>> @@ -2320,7 +2320,7 @@ static int team_nl_cmd_options_set(struct sk_buff *skb, struct genl_info *info)
>>  			list_add(&opt_inst->tmp_list, &opt_inst_list);
>>  		}
>>  		if (!opt_found) {
>> -			err = -ENOENT;
>> +			err = -EINVAL;
>>  			goto team_put;
>>  		}
>>  	}
>> diff --git a/drivers/net/team/team_mode_activebackup.c b/drivers/net/team/team_mode_activebackup.c
>> index 6262b4d..2792e13 100644
>> --- a/drivers/net/team/team_mode_activebackup.c
>> +++ b/drivers/net/team/team_mode_activebackup.c
>> @@ -81,7 +81,7 @@ static int ab_active_port_set(struct team *team, struct team_gsetter_ctx *ctx)
>>  			return 0;
>>  		}
>>  	}
>> -	return -ENOENT;
>> +	return -ENODEV;
>>  }
>>  
>>  static const struct team_option ab_options[] = {
>
>To be pedantic.
>Changing errno's means effectively changing the ABI.
>Linus has already rejected similar patches in other areas.

I'm not really sure. But in this case, I do not think that is a problem.

1) I'm most probably the only one (libteam) who is using this api and
libteam does not mind about what err code is returned in these cases.

2) In this case, it is only about different number. And one number or
another, it does not imply userspace to behave differently. In other words,
userspace should not take different actions in case for example -ENOENT
or -ENODEV is returned.

But as I said, I'm not sure.

Thanks

Jiri

Re: [patch net-next] team: do not use -ENOENT

[David Miller]

From: Jiri Pirko <jiri@resnulli.us>
Date: Thu, 17 Jan 2013 21:33:47 +0100

>>> @@ -2320,7 +2320,7 @@ static int team_nl_cmd_options_set(struct sk_buff *skb, struct genl_info *info)
>>>  			list_add(&opt_inst->tmp_list, &opt_inst_list);
>>>  		}
>>>  		if (!opt_found) {
>>> -			err = -ENOENT;
>>> +			err = -EINVAL;
>>>  			goto team_put;
>>>  		}
>>>  	}


> I'm not really sure. But in this case, I do not think that is a problem.
> 
> 1) I'm most probably the only one (libteam) who is using this api and
> libteam does not mind about what err code is returned in these cases.
> 
> 2) In this case, it is only about different number. And one number or
> another, it does not imply userspace to behave differently. In other words,
> userspace should not take different actions in case for example -ENOENT
> or -ENODEV is returned.

I agree with this analysis.

But for the team_nl_cmd_options_set() case, I would strongly advise
that you use some error code more descriptive than -EINVAL.  In fact
the existing -ENOENT I feel is better, because it tells the caller
what kind of problem there was.

Even if you don't like the fact that -ENOENT is oriented towards file
existence, it does convey a ton more information than -EINVAL does.

Re: [patch net-next] team: do not use -ENOENT

[Jiri Pirko]

Thu, Jan 17, 2013 at 09:42:40PM CET, davem@davemloft.net wrote:
>From: Jiri Pirko <jiri@resnulli.us>
>Date: Thu, 17 Jan 2013 21:33:47 +0100
>
>>>> @@ -2320,7 +2320,7 @@ static int team_nl_cmd_options_set(struct sk_buff *skb, struct genl_info *info)
>>>>  			list_add(&opt_inst->tmp_list, &opt_inst_list);
>>>>  		}
>>>>  		if (!opt_found) {
>>>> -			err = -ENOENT;
>>>> +			err = -EINVAL;
>>>>  			goto team_put;
>>>>  		}
>>>>  	}
>
>
>> I'm not really sure. But in this case, I do not think that is a problem.
>> 
>> 1) I'm most probably the only one (libteam) who is using this api and
>> libteam does not mind about what err code is returned in these cases.
>> 
>> 2) In this case, it is only about different number. And one number or
>> another, it does not imply userspace to behave differently. In other words,
>> userspace should not take different actions in case for example -ENOENT
>> or -ENODEV is returned.
>
>I agree with this analysis.
>
>But for the team_nl_cmd_options_set() case, I would strongly advise
>that you use some error code more descriptive than -EINVAL.  In fact
>the existing -ENOENT I feel is better, because it tells the caller
>what kind of problem there was.
>
>Even if you don't like the fact that -ENOENT is oriented towards file
>existence, it does convey a ton more information than -EINVAL does.

I understand your feeling, because I have the same one :)
But looking all over the code and on possible err codes as well, I did
not find any suitable err code to indicate some object was not found.
And since I recently saw email from Linus about the fact that -ENOENT
should be used only in relation to files, -EINVAL the "default:" in my
"switch()".

Re: [patch net-next] team: do not use -ENOENT

[David Miller]

From: Jiri Pirko <jiri@resnulli.us>
Date: Thu, 17 Jan 2013 21:52:12 +0100

> Thu, Jan 17, 2013 at 09:42:40PM CET, davem@davemloft.net wrote:
>>From: Jiri Pirko <jiri@resnulli.us>
>>Date: Thu, 17 Jan 2013 21:33:47 +0100
>>
>>>>> @@ -2320,7 +2320,7 @@ static int team_nl_cmd_options_set(struct sk_buff *skb, struct genl_info *info)
>>>>>  			list_add(&opt_inst->tmp_list, &opt_inst_list);
>>>>>  		}
>>>>>  		if (!opt_found) {
>>>>> -			err = -ENOENT;
>>>>> +			err = -EINVAL;
>>>>>  			goto team_put;
>>>>>  		}
>>>>>  	}
>>
>>
>>> I'm not really sure. But in this case, I do not think that is a problem.
>>> 
>>> 1) I'm most probably the only one (libteam) who is using this api and
>>> libteam does not mind about what err code is returned in these cases.
>>> 
>>> 2) In this case, it is only about different number. And one number or
>>> another, it does not imply userspace to behave differently. In other words,
>>> userspace should not take different actions in case for example -ENOENT
>>> or -ENODEV is returned.
>>
>>I agree with this analysis.
>>
>>But for the team_nl_cmd_options_set() case, I would strongly advise
>>that you use some error code more descriptive than -EINVAL.  In fact
>>the existing -ENOENT I feel is better, because it tells the caller
>>what kind of problem there was.
>>
>>Even if you don't like the fact that -ENOENT is oriented towards file
>>existence, it does convey a ton more information than -EINVAL does.
> 
> I understand your feeling, because I have the same one :)
> But looking all over the code and on possible err codes as well, I did
> not find any suitable err code to indicate some object was not found.
> And since I recently saw email from Linus about the fact that -ENOENT
> should be used only in relation to files, -EINVAL the "default:" in my
> "switch()".

Look in the packet scheduler API for how much we use -ENOENT in this
kind of situation where the requested object to operate on could not
be found.

I think it is entirely appropriate to use -ENOENT, if not completely
consistent with the rest of the networking.

Re: [patch net-next] team: do not use -ENOENT

[Jiri Pirko]

Thu, Jan 17, 2013 at 10:07:42PM CET, davem@davemloft.net wrote:
>From: Jiri Pirko <jiri@resnulli.us>
>Date: Thu, 17 Jan 2013 21:52:12 +0100
>
>> Thu, Jan 17, 2013 at 09:42:40PM CET, davem@davemloft.net wrote:
>>>From: Jiri Pirko <jiri@resnulli.us>
>>>Date: Thu, 17 Jan 2013 21:33:47 +0100
>>>
>>>>>> @@ -2320,7 +2320,7 @@ static int team_nl_cmd_options_set(struct sk_buff *skb, struct genl_info *info)
>>>>>>  			list_add(&opt_inst->tmp_list, &opt_inst_list);
>>>>>>  		}
>>>>>>  		if (!opt_found) {
>>>>>> -			err = -ENOENT;
>>>>>> +			err = -EINVAL;
>>>>>>  			goto team_put;
>>>>>>  		}
>>>>>>  	}
>>>
>>>
>>>> I'm not really sure. But in this case, I do not think that is a problem.
>>>> 
>>>> 1) I'm most probably the only one (libteam) who is using this api and
>>>> libteam does not mind about what err code is returned in these cases.
>>>> 
>>>> 2) In this case, it is only about different number. And one number or
>>>> another, it does not imply userspace to behave differently. In other words,
>>>> userspace should not take different actions in case for example -ENOENT
>>>> or -ENODEV is returned.
>>>
>>>I agree with this analysis.
>>>
>>>But for the team_nl_cmd_options_set() case, I would strongly advise
>>>that you use some error code more descriptive than -EINVAL.  In fact
>>>the existing -ENOENT I feel is better, because it tells the caller
>>>what kind of problem there was.
>>>
>>>Even if you don't like the fact that -ENOENT is oriented towards file
>>>existence, it does convey a ton more information than -EINVAL does.
>> 
>> I understand your feeling, because I have the same one :)
>> But looking all over the code and on possible err codes as well, I did
>> not find any suitable err code to indicate some object was not found.
>> And since I recently saw email from Linus about the fact that -ENOENT
>> should be used only in relation to files, -EINVAL the "default:" in my
>> "switch()".
>
>Look in the packet scheduler API for how much we use -ENOENT in this
>kind of situation where the requested object to operate on could not
>be found.
>
>I think it is entirely appropriate to use -ENOENT, if not completely
>consistent with the rest of the networking.

Okay, fair enough. In that case, I believe that also the other 2
occurrences of -ENOENT in team driver are ok as well. Please scratch this
patch.

Thanks!

Re: [patch net-next] team: do not use -ENOENT

[David Miller]

From: Jiri Pirko <jiri@resnulli.us>
Date: Thu, 17 Jan 2013 22:15:32 +0100

> Okay, fair enough. In that case, I believe that also the other 2
> occurrences of -ENOENT in team driver are ok as well. Please scratch this
> patch.

Ok.

> Thanks!

No problem.