* [patch net-next] team: do not use -ENOENT
@ 2013-01-17 10:25 Jiri Pirko
2013-01-17 15:51 ` Stephen Hemminger
0 siblings, 1 reply; 8+ messages in thread
From: Jiri Pirko @ 2013-01-17 10:25 UTC (permalink / raw)
To: netdev; +Cc: davem
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
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [patch net-next] team: do not use -ENOENT
2013-01-17 10:25 [patch net-next] team: do not use -ENOENT Jiri Pirko
@ 2013-01-17 15:51 ` Stephen Hemminger
2013-01-17 20:33 ` Jiri Pirko
0 siblings, 1 reply; 8+ messages in thread
From: Stephen Hemminger @ 2013-01-17 15:51 UTC (permalink / raw)
To: Jiri Pirko; +Cc: netdev, davem
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.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [patch net-next] team: do not use -ENOENT
2013-01-17 15:51 ` Stephen Hemminger
@ 2013-01-17 20:33 ` Jiri Pirko
2013-01-17 20:42 ` David Miller
0 siblings, 1 reply; 8+ messages in thread
From: Jiri Pirko @ 2013-01-17 20:33 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev, davem
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [patch net-next] team: do not use -ENOENT
2013-01-17 20:33 ` Jiri Pirko
@ 2013-01-17 20:42 ` David Miller
2013-01-17 20:52 ` Jiri Pirko
0 siblings, 1 reply; 8+ messages in thread
From: David Miller @ 2013-01-17 20:42 UTC (permalink / raw)
To: jiri; +Cc: stephen, netdev
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.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [patch net-next] team: do not use -ENOENT
2013-01-17 20:42 ` David Miller
@ 2013-01-17 20:52 ` Jiri Pirko
2013-01-17 21:07 ` David Miller
0 siblings, 1 reply; 8+ messages in thread
From: Jiri Pirko @ 2013-01-17 20:52 UTC (permalink / raw)
To: David Miller; +Cc: stephen, netdev
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()".
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [patch net-next] team: do not use -ENOENT
2013-01-17 20:52 ` Jiri Pirko
@ 2013-01-17 21:07 ` David Miller
2013-01-17 21:15 ` Jiri Pirko
0 siblings, 1 reply; 8+ messages in thread
From: David Miller @ 2013-01-17 21:07 UTC (permalink / raw)
To: jiri; +Cc: stephen, netdev
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.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [patch net-next] team: do not use -ENOENT
2013-01-17 21:07 ` David Miller
@ 2013-01-17 21:15 ` Jiri Pirko
2013-01-17 21:18 ` David Miller
0 siblings, 1 reply; 8+ messages in thread
From: Jiri Pirko @ 2013-01-17 21:15 UTC (permalink / raw)
To: David Miller; +Cc: stephen, netdev
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!
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2013-01-17 21:18 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-01-17 10:25 [patch net-next] team: do not use -ENOENT Jiri Pirko
2013-01-17 15:51 ` Stephen Hemminger
2013-01-17 20:33 ` Jiri Pirko
2013-01-17 20:42 ` David Miller
2013-01-17 20:52 ` Jiri Pirko
2013-01-17 21:07 ` David Miller
2013-01-17 21:15 ` Jiri Pirko
2013-01-17 21:18 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).