From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jamal Hadi Salim Subject: Re: [PATCH net-next 2/8] net: sched: cls_api: handle generic cls errors Date: Wed, 17 Jan 2018 09:32:47 -0500 Message-ID: <1886f57b-d9b1-02d7-6c94-721ee5f34e59@mojatatu.com> References: <20180116172027.22128-1-aring@mojatatu.com> <20180116172027.22128-3-aring@mojatatu.com> <4092eaf5-120b-488e-1378-c19d29f665c2@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Cc: xiyou.wangcong@gmail.com, jiri@resnulli.us, davem@davemloft.net, netdev@vger.kernel.org, kernel@mojatatu.com To: David Ahern , Alexander Aring Return-path: Received: from mail-io0-f175.google.com ([209.85.223.175]:36744 "EHLO mail-io0-f175.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752491AbeAQOcu (ORCPT ); Wed, 17 Jan 2018 09:32:50 -0500 Received: by mail-io0-f175.google.com with SMTP id l17so11166354ioc.3 for ; Wed, 17 Jan 2018 06:32:49 -0800 (PST) In-Reply-To: <4092eaf5-120b-488e-1378-c19d29f665c2@gmail.com> Content-Language: en-GB Sender: netdev-owner@vger.kernel.org List-ID: On 18-01-16 10:22 PM, David Ahern wrote: > tp = tcf_chain_tp_find(chain, &chain_info, protocol, > prio, prio_allocate); > if (IS_ERR(tp)) { > err = PTR_ERR(tp); > goto errout; > } > > > if (tp == NULL) { > /* Proto-tcf does not exist, create new one */ > > > if (tca[TCA_KIND] == NULL || !protocol) { > err = -EINVAL; > goto errout; > } > > if (n->nlmsg_type != RTM_NEWTFILTER || > !(n->nlmsg_flags & NLM_F_CREATE)) { > err = -ENOENT; > goto errout; > } > The assumption is if we got that far in the code a create path(per the comment) is the sane thing to do. A create requires tca[TCA_KIND], protocol, RTM_NEWTFILTER and NLM_F_CREATE > Seems like that code path is run for other than RTM_NEWTFILTER. Even the > check there says != is ok -- just error out with an ENOENT. > To be honest, I am not fond of those checks either, I find myself thinking sometimes about what would happen given a specific message. A lot of these control paths in tc kernel parts sometimes are too clever. That code needs refactoring to separate all 3 operations for readability/maintainability. I would say the same for sch_api We could fix it up (but those are separate patches); then we can have much better error messages as well. >> Generally true, but should this rule really be scripture? >> The main user here is tc in  user space and it doesnt make mistakes >> in this case i.e we will  never see this error with tc because a >> create will always have those two set correctly; OTOH, a developer >> writing some new app is more likely to make this mistake (in which >> case this message is very helpful). > > argumentative. We are having a discussion. >I have focused on adding specific error messages that > help a user understand why a command failed. It can be done with > referencing API names. That is one use case which is sensible. It is not like the message is saying "consult IBM manual X for error code 1234" IMO: The kernel should be helpful to the user - but that user is not just the user of an app within iproute2. It could be a robot, a human using another CLI app, a developer etc. I think we need to look at specific cases and make those calls. My view is that we should never fail from tc for this specific case unless someone is fixing up tc. If you want to say it is always a human that needs to interpret and react - then we need rules well stated somewhere (eg you stated ENOMEM should not have extack, it was sufficiently expressive etc). This will help reduce the time spent reviewing patches or for people to respin. The kernel should - when it makes sense - even return an extack for a _successful message_ (I can give you several use cases in tc today) or binary data via the cookie; and hopefully we can avoid collission when we start sending such patches by having the discussion now. cheers, jamal