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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 2 Jul 2019 18:28:35 -0700
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     Michal Kubecek <mkubecek@...e.cz>
Cc:     netdev@...r.kernel.org, Jiri Pirko <jiri@...nulli.us>,
        David Miller <davem@...emloft.net>,
        Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        John Linville <linville@...driver.com>,
        Stephen Hemminger <stephen@...workplumber.org>,
        Johannes Berg <johannes@...solutions.net>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v6 05/15] ethtool: helper functions for netlink
 interface

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..