Re: [PATCH net-next 2/8] net: sched: cls_api: handle generic cls errors

[Jamal Hadi Salim <jhs@mojatatu.com>]

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