Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 2 Jul 2019 18:28:35 -0700
From:   Jakub Kicinski <jakub.kicinski@netronome.com>
To:     Michal Kubecek <mkubecek@suse.cz>
Cc:     netdev@vger.kernel.org, Jiri Pirko <jiri@resnulli.us>,
        David Miller <davem@davemloft.net>,
        Andrew Lunn <andrew@lunn.ch>,
        Florian Fainelli <f.fainelli@gmail.com>,
        John Linville <linville@tuxdriver.com>,
        Stephen Hemminger <stephen@networkplumber.org>,
        Johannes Berg <johannes@sipsolutions.net>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next v6 05/15] ethtool: helper functions for netlink
 interface
Message-ID: <20190702182835.69c9ac67@cakuba.netronome.com>
In-Reply-To: <20190702163437.GE20101@unicorn.suse.cz>
References: <cover.1562067622.git.mkubecek@suse.cz>
        <44957b13e8edbced71aca893908d184eb9e57341.1562067622.git.mkubecek@suse.cz>
        <20190702130515.GO2250@nanopsycho>
        <20190702163437.GE20101@unicorn.suse.cz>
Organization: Netronome Systems, Ltd.
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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