Re: [PATCH net-next v6 05/15] ethtool: helper functions for netlink interface

[Jakub Kicinski]

On Tue, 2 Jul 2019 18:34:37 +0200, Michal Kubecek wrote:
> > >+	ret = nla_parse_nested(tb, ETHTOOL_A_HEADER_MAX, nest,
> > >+			       policy ?: dflt_header_policy, extack);
> > >+	if (ret < 0)  
> > 
> > if (ret)
> > 
> > Same remark goes to the rest of the code (also the rest of the patches),
> > in case called function cannot return positive values.  
> 
> The "if (ret < 0)" idiom for "on error do ..." is so ubiquitous through
> the whole kernel that I don't think it's worth it to carefully check
> which function can return a positive value and which cannot and risk
> that one day I overlook that some function. And yet another question is
> what exactly "cannot return" means: is it whenever the function does not
> return a positive value or only if it's explicitly documented not to?
> 
> Looking at existing networking code, e.g. net/netfilter (except ipvs),
> net/sched or net/core/rtnetlink.c are using "if (ret < 0)" rather
> uniformly. And (as you objected to the check of genl_register_family()
> previous patch) even genetlink itself has
> 
> 	err = genl_register_family(&genl_ctrl);
> 	if (err < 0)
> 		goto problem;
> 
> in genl_init().

I agree with Jiri, if a function only returns "0, or -errno" it's
easier to parse if the error check is not only for negative values.
At least to my eyes.

What I'm not sure about is whether we want to delay the merging of this
interface over this..