| Message ID | 1358418300-2346-1-git-send-email-jiri@resnulli.us |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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()". -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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! -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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[] = {
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(-)