From mboxrd@z Thu Jan  1 00:00:00 1970
From: David Ahern <dsahern@gmail.com>
Subject: Re: [PATCH net-next 2/8] net: sched: cls_api: handle generic cls
 errors
Date: Tue, 16 Jan 2018 19:22:42 -0800
Message-ID: <4092eaf5-120b-488e-1378-c19d29f665c2@gmail.com>
References: <20180116172027.22128-1-aring@mojatatu.com>
 <20180116172027.22128-3-aring@mojatatu.com>
 <bfc1df84-0058-297b-7e06-f0c91a5b8e66@gmail.com>
 <ee8a4a80-8ab0-51de-57c7-9f3aab9326eb@mojatatu.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
Cc: xiyou.wangcong@gmail.com, jiri@resnulli.us, davem@davemloft.net,
        netdev@vger.kernel.org, kernel@mojatatu.com
To: Jamal Hadi Salim <jhs@mojatatu.com>,
        Alexander Aring <aring@mojatatu.com>
Return-path: <netdev-owner@vger.kernel.org>
Received: from mail-pf0-f179.google.com ([209.85.192.179]:40221 "EHLO
        mail-pf0-f179.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1750811AbeAQDXv (ORCPT
        <rfc822;netdev@vger.kernel.org>); Tue, 16 Jan 2018 22:23:51 -0500
Received: by mail-pf0-f179.google.com with SMTP id i66so10785677pfd.7
        for <netdev@vger.kernel.org>; Tue, 16 Jan 2018 19:23:51 -0800 (PST)
In-Reply-To: <ee8a4a80-8ab0-51de-57c7-9f3aab9326eb@mojatatu.com>
Content-Language: en-US
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

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.