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

[David Ahern <dsahern@gmail.com>]

On 1/16/18 4:19 PM, Jamal Hadi Salim wrote:
> On 18-01-16 06:58 PM, David Ahern wrote:
>> On 1/16/18 9:20 AM, Alexander Aring wrote:
> 
> 
>>>           }
>>>             if (n->nlmsg_type != RTM_NEWTFILTER ||
>>>               !(n->nlmsg_flags & NLM_F_CREATE)) {
>>> +            NL_SET_ERR_MSG(extack, "Need both RTM_NEWTFILTER and
>>> NLM_F_CREATE to create a new filter");
>>
>> that does not seem the right message. tc_ctl_tfilter is overloaded for
>> new, delete and get so the response here needs to reflect that. I
>> believe in this case the user did not specify a valid chain.
>>
> 
> Are you sure you are looking at the correct code?

        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;
                }

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.


> It is a create message that is at stake here.
> A create has to have RTM_NEWTFILTER and NLM_F_CREATE
> 
>> Also, the messages are targeted at users not developers, so no code
>> jargon / API references.
> 
> 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. 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.