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

2018-01-17 Thread Jamal Hadi Salim 

On 18-01-16 10:22 PM, David Ahern wrote:


 tp = tcf_chain_tp_find(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

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

2018-01-16 Thread David Ahern 
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, _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.

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

2018-01-16 Thread Jamal Hadi Salim 

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

cheers,
jamal

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

2018-01-16 Thread David Ahern 
On 1/16/18 9:20 AM, Alexander Aring wrote:
> This patch adds extack support for generic cls handling. The extack
> will be set deeper to each called function which is not part of netdev
> core api.
> 
> Cc: David Ahern 
> Signed-off-by: Alexander Aring 
> ---
>  net/sched/cls_api.c | 55 
> +
>  1 file changed, 43 insertions(+), 12 deletions(-)
> 
> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index 01d09055707d..c25a9b4bcb4b 100644
> --- a/net/sched/cls_api.c
> +++ b/net/sched/cls_api.c
> @@ -122,7 +122,8 @@ static inline u32 tcf_auto_prio(struct tcf_proto *tp)
>  
>  static struct tcf_proto *tcf_proto_create(const char *kind, u32 protocol,
> u32 prio, u32 parent, struct Qdisc *q,
> -   struct tcf_chain *chain)
> +   struct tcf_chain *chain,
> +   struct netlink_ext_ack *extack)
>  {
>   struct tcf_proto *tp;
>   int err;
> @@ -148,6 +149,7 @@ static struct tcf_proto *tcf_proto_create(const char 
> *kind, u32 protocol,
>   module_put(tp->ops->owner);
>   err = -EAGAIN;
>   } else {
> + NL_SET_ERR_MSG(extack, "TC classifier not found");
>   err = -ENOENT;
>   }
>   goto errout;
> @@ -662,7 +664,8 @@ static int tfilter_notify(struct net *net, struct sk_buff 
> *oskb,
>  static int tfilter_del_notify(struct net *net, struct sk_buff *oskb,
> struct nlmsghdr *n, struct tcf_proto *tp,
> struct Qdisc *q, u32 parent,
> -   void *fh, bool unicast, bool *last)
> +   void *fh, bool unicast, bool *last,
> +   struct netlink_ext_ack *extack)
>  {
>   struct sk_buff *skb;
>   u32 portid = oskb ? NETLINK_CB(oskb).portid : 0;
> @@ -674,6 +677,7 @@ static int tfilter_del_notify(struct net *net, struct 
> sk_buff *oskb,
>  
>   if (tcf_fill_node(net, skb, tp, q, parent, fh, portid, n->nlmsg_seq,
> n->nlmsg_flags, RTM_DELTFILTER) <= 0) {
> + NL_SET_ERR_MSG(extack, "Failed to build del event 
> notification");
>   kfree_skb(skb);
>   return -EINVAL;
>   }
> @@ -687,8 +691,11 @@ static int tfilter_del_notify(struct net *net, struct 
> sk_buff *oskb,
>   if (unicast)
>   return netlink_unicast(net->rtnl, skb, portid, MSG_DONTWAIT);
>  
> - return rtnetlink_send(skb, net, portid, RTNLGRP_TC,
> -   n->nlmsg_flags & NLM_F_ECHO);
> + err = rtnetlink_send(skb, net, portid, RTNLGRP_TC,
> +  n->nlmsg_flags & NLM_F_ECHO);
> + if (err < 0)
> + NL_SET_ERR_MSG(extack, "Failed to send filter delete 
> notification");

not sure we want to do this -- extack for internal failures like this
one or below in tc_ctl_tfilter.


> + return err;
>  }
>  
>  static void tfilter_notify_chain(struct net *net, struct sk_buff *oskb,
> @@ -749,8 +756,10 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct 
> nlmsghdr *n,
>   if (prio == 0) {
>   switch (n->nlmsg_type) {
>   case RTM_DELTFILTER:
> - if (protocol || t->tcm_handle || tca[TCA_KIND])
> + if (protocol || t->tcm_handle || tca[TCA_KIND]) {
> + NL_SET_ERR_MSG(extack, "Cannot flush filters 
> with protocol, handle or kind set");
>   return -ENOENT;
> + }
>   break;
>   case RTM_NEWTFILTER:
>   /* If no priority is provided by the user,
> @@ -763,6 +772,7 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct 
> nlmsghdr *n,
>   }
>   /* fall-through */
>   default:
> + NL_SET_ERR_MSG(extack, "Invalid filter command with 
> priority of zero");
>   return -ENOENT;
>   }
>   }
> @@ -780,23 +790,31 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct 
> nlmsghdr *n,
>   parent = q->handle;
>   } else {
>   q = qdisc_lookup(dev, TC_H_MAJ(t->tcm_parent));
> - if (!q)
> + if (!q) {
> + NL_SET_ERR_MSG(extack, "Parent Qdisc doesn't exists");

Messages should avoid contractions; spell out 'does not'. Please check
all of the patches.

Also, it should be 'exist' (no 's' on the end).


>   return -EINVAL;
> + }
>   }
>  
>   /* Is it classful? */
>   cops = q->ops->cl_ops;
> - if (!cops)
> + if (!cops) {
> + NL_SET_ERR_MSG(extack, "Qdisc not classful");
>   return -EINVAL;
> + }
>  
> - if

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

2018-01-16 Thread Cong Wang 
On Tue, Jan 16, 2018 at 9:20 AM, Alexander Aring  wrote:
> @@ -1117,8 +1146,10 @@ int tcf_exts_validate(struct net *net, struct 
> tcf_proto *tp, struct nlattr **tb,
> }
>  #else
> if ((exts->action && tb[exts->action]) ||
> -   (exts->police && tb[exts->police]))
> +   (exts->police && tb[exts->police])) {
> +   NL_SET_ERR_MSG(extack, "Actions are not supported. Check 
> compile options");
> return -EOPNOTSUPP;
> +   }
>  #endif

"Check compile options" is confusing, it is clearer if we can just
say we need to enable CONFIG_NET_CLS_ACT here.

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

2018-01-16 Thread Alexander Aring 
This patch adds extack support for generic cls handling. The extack
will be set deeper to each called function which is not part of netdev
core api.

Cc: David Ahern 
Signed-off-by: Alexander Aring 
---
 net/sched/cls_api.c | 55 +
 1 file changed, 43 insertions(+), 12 deletions(-)

diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 01d09055707d..c25a9b4bcb4b 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -122,7 +122,8 @@ static inline u32 tcf_auto_prio(struct tcf_proto *tp)
 
 static struct tcf_proto *tcf_proto_create(const char *kind, u32 protocol,
  u32 prio, u32 parent, struct Qdisc *q,
- struct tcf_chain *chain)
+ struct tcf_chain *chain,
+ struct netlink_ext_ack *extack)
 {
struct tcf_proto *tp;
int err;
@@ -148,6 +149,7 @@ static struct tcf_proto *tcf_proto_create(const char *kind, 
u32 protocol,
module_put(tp->ops->owner);
err = -EAGAIN;
} else {
+   NL_SET_ERR_MSG(extack, "TC classifier not found");
err = -ENOENT;
}
goto errout;
@@ -662,7 +664,8 @@ static int tfilter_notify(struct net *net, struct sk_buff 
*oskb,
 static int tfilter_del_notify(struct net *net, struct sk_buff *oskb,
  struct nlmsghdr *n, struct tcf_proto *tp,
  struct Qdisc *q, u32 parent,
- void *fh, bool unicast, bool *last)
+ void *fh, bool unicast, bool *last,
+ struct netlink_ext_ack *extack)
 {
struct sk_buff *skb;
u32 portid = oskb ? NETLINK_CB(oskb).portid : 0;
@@ -674,6 +677,7 @@ static int tfilter_del_notify(struct net *net, struct 
sk_buff *oskb,
 
if (tcf_fill_node(net, skb, tp, q, parent, fh, portid, n->nlmsg_seq,
  n->nlmsg_flags, RTM_DELTFILTER) <= 0) {
+   NL_SET_ERR_MSG(extack, "Failed to build del event 
notification");
kfree_skb(skb);
return -EINVAL;
}
@@ -687,8 +691,11 @@ static int tfilter_del_notify(struct net *net, struct 
sk_buff *oskb,
if (unicast)
return netlink_unicast(net->rtnl, skb, portid, MSG_DONTWAIT);
 
-   return rtnetlink_send(skb, net, portid, RTNLGRP_TC,
- n->nlmsg_flags & NLM_F_ECHO);
+   err = rtnetlink_send(skb, net, portid, RTNLGRP_TC,
+n->nlmsg_flags & NLM_F_ECHO);
+   if (err < 0)
+   NL_SET_ERR_MSG(extack, "Failed to send filter delete 
notification");
+   return err;
 }
 
 static void tfilter_notify_chain(struct net *net, struct sk_buff *oskb,
@@ -749,8 +756,10 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct 
nlmsghdr *n,
if (prio == 0) {
switch (n->nlmsg_type) {
case RTM_DELTFILTER:
-   if (protocol || t->tcm_handle || tca[TCA_KIND])
+   if (protocol || t->tcm_handle || tca[TCA_KIND]) {
+   NL_SET_ERR_MSG(extack, "Cannot flush filters 
with protocol, handle or kind set");
return -ENOENT;
+   }
break;
case RTM_NEWTFILTER:
/* If no priority is provided by the user,
@@ -763,6 +772,7 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct 
nlmsghdr *n,
}
/* fall-through */
default:
+   NL_SET_ERR_MSG(extack, "Invalid filter command with 
priority of zero");
return -ENOENT;
}
}
@@ -780,23 +790,31 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct 
nlmsghdr *n,
parent = q->handle;
} else {
q = qdisc_lookup(dev, TC_H_MAJ(t->tcm_parent));
-   if (!q)
+   if (!q) {
+   NL_SET_ERR_MSG(extack, "Parent Qdisc doesn't exists");
return -EINVAL;
+   }
}
 
/* Is it classful? */
cops = q->ops->cl_ops;
-   if (!cops)
+   if (!cops) {
+   NL_SET_ERR_MSG(extack, "Qdisc not classful");
return -EINVAL;
+   }
 
-   if (!cops->tcf_block)
+   if (!cops->tcf_block) {
+   NL_SET_ERR_MSG(extack, "Class doesn't support blocks");
return -EOPNOTSUPP;
+   }
 
/* Do we search for filter, attached to class? */
if (TC_H_MIN(parent)) {
cl = cops->find(q, parent);
-   if (cl == 0)
+   if (cl == 0) {
+