Re: [PATCHv2 net-next 3/8] net: sched: act: handle generic action errors

2018-02-15 Thread Alexander Aring 
Hi,

On Thu, Feb 15, 2018 at 6:14 AM, Davide Caratti  wrote:
> On Wed, 2018-02-14 at 17:13 -0500, Alexander Aring wrote:
>> This patch adds extack support for generic act handling. The extack
>> will be set deeper to each called function which is not part of netdev
>> core api.
>>
>> Based on work by David Ahern 
>
> hello Alexander,
>
> after looking at the code more closely, I think there is margin for some
> more improvement here (i.e make the message contents easier to grep from a
> log). Please let me know if the comments below make sense for you.
>

I will send v3 with your changes. But not with one error handling in
label, the most whole point is to get some messages when somebody
throw a -EINVAL to the userspace and TC subsystem has a lot of places
with that.

- Alex

Re: [PATCHv2 net-next 3/8] net: sched: act: handle generic action errors

2018-02-15 Thread Davide Caratti 
On Thu, 2018-02-15 at 10:43 -0500, Alexander Aring wrote:
> I will send v3 with your changes. But not with one error handling in
> label, the most whole point is to get some messages when somebody
> throw a -EINVAL to the userspace and TC subsystem has a lot of places
> with that.

that's ok for me.

thanks!
-- 
davide

Re: [PATCHv2 net-next 3/8] net: sched: act: handle generic action errors

2018-02-15 Thread Davide Caratti 
On Wed, 2018-02-14 at 17:13 -0500, Alexander Aring wrote:
> This patch adds extack support for generic act handling. The extack
> will be set deeper to each called function which is not part of netdev
> core api.
> 
> Based on work by David Ahern 

hello Alexander,

after looking at the code more closely, I think there is margin for some
more improvement here (i.e make the message contents easier to grep from a
log). Please let me know if the comments below make sense for you.

thank you in advance!

-- 
davide

> Cc: David Ahern 
> Signed-off-by: Alexander Aring 
> ---
>  net/sched/act_api.c | 93 
> +++--
>  1 file changed, 61 insertions(+), 32 deletions(-)
> 
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index 8d89b026414f..a5138ae026a1 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -617,31 +617,40 @@ struct tc_action *tcf_action_init_1(struct net *net, 
> struct tcf_proto *tp,
>   int err;
>  
>   if (name == NULL) {
> - err = nla_parse_nested(tb, TCA_ACT_MAX, nla, NULL, NULL);
> + err = nla_parse_nested(tb, TCA_ACT_MAX, nla, NULL, extack);
>   if (err < 0)
>   goto err_out;
>   err = -EINVAL;
>   kind = tb[TCA_ACT_KIND];
> - if (!kind)
> + if (!kind) {
> + NL_SET_ERR_MSG(extack, "TC action kind must be 
> specified");
>   goto err_out;
> - if (nla_strlcpy(act_name, kind, IFNAMSIZ) >= IFNAMSIZ)
> + }
> + if (nla_strlcpy(act_name, kind, IFNAMSIZ) >= IFNAMSIZ) {
> + NL_SET_ERR_MSG(extack, "TC action name too long");
>   goto err_out;
> + }
>   if (tb[TCA_ACT_COOKIE]) {
>   int cklen = nla_len(tb[TCA_ACT_COOKIE]);
>  
> - if (cklen > TC_COOKIE_MAX_SIZE)
> + if (cklen > TC_COOKIE_MAX_SIZE) {
> + NL_SET_ERR_MSG(extack, "TC cookie size above 
> the maximum");
>   goto err_out;
> + }
>  
>   cookie = nla_memdup_cookie(tb);
>   if (!cookie) {
> + NL_SET_ERR_MSG(extack, "No memory to generate 
> TC cookie");
>   err = -ENOMEM;
>   goto err_out;
>   }
>   }
>   } else {
> - err = -EINVAL;
> - if (strlcpy(act_name, name, IFNAMSIZ) >= IFNAMSIZ)
> + if (strlcpy(act_name, name, IFNAMSIZ) >= IFNAMSIZ) {
> + NL_SET_ERR_MSG(extack, "TC action name too long");
> + err = -EINVAL;
>   goto err_out;
> + }
>   }
>  
>   a_o = tc_lookup_action_n(act_name);
> @@ -664,6 +673,7 @@ struct tc_action *tcf_action_init_1(struct net *net, 
> struct tcf_proto *tp,
>   goto err_mod;
>   }
>  #endif
> + NL_SET_ERR_MSG(extack, "Failed to load TC action module");
>   err = -ENOENT;
>   goto err_out;
>   }
> @@ -698,6 +708,7 @@ struct tc_action *tcf_action_init_1(struct net *net, 
> struct tcf_proto *tp,
>  
>   list_add_tail(>list, );
>   tcf_action_destroy(, bind);
> + NL_SET_ERR_MSG(extack, "Failed to init action chain");

most of the times, the word 'action' is prepended by the word 'TC'.
Proposal:

"Failed to init TC action chain" 

>   return ERR_PTR(err);
>   }
>   }
> @@ -734,7 +745,7 @@ int tcf_action_init(struct net *net, struct tcf_proto 
> *tp, struct nlattr *nla,
>   int err;
>   int i;
>  
> - err = nla_parse_nested(tb, TCA_ACT_MAX_PRIO, nla, NULL, NULL);
> + err = nla_parse_nested(tb, TCA_ACT_MAX_PRIO, nla, NULL, extack);
>   if (err < 0)
>   return err;
>  
> @@ -842,7 +853,8 @@ static int tca_get_fill(struct sk_buff *skb, struct 
> list_head *actions,
>  
>  static int
>  tcf_get_notify(struct net *net, u32 portid, struct nlmsghdr *n,
> -struct list_head *actions, int event)
> +struct list_head *actions, int event,
> +struct netlink_ext_ack *extack)
>  {
>   struct sk_buff *skb;
>  
> @@ -851,6 +863,7 @@ tcf_get_notify(struct net *net, u32 portid, struct 
> nlmsghdr *n,
>   return -ENOBUFS;
>   if (tca_get_fill(skb, actions, portid, n->nlmsg_seq, 0, event,
>0, 0) <= 0) {
> + NL_SET_ERR_MSG(extack, "Failed to fill netlink tc action 
> attributes");

see also @@ -975,6 +1000,7 @@ and @@ -975,6 +1000,7 @@:

this is the same message you get in case tcf_add_notify() fails, and it's
very similar to what you get when tcf_del_notify() fails: the only

[PATCHv2 net-next 3/8] net: sched: act: handle generic action errors

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

Based on work by David Ahern 

Cc: David Ahern 
Signed-off-by: Alexander Aring 
---
 net/sched/act_api.c | 93 +++--
 1 file changed, 61 insertions(+), 32 deletions(-)

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 8d89b026414f..a5138ae026a1 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -617,31 +617,40 @@ struct tc_action *tcf_action_init_1(struct net *net, 
struct tcf_proto *tp,
int err;
 
if (name == NULL) {
-   err = nla_parse_nested(tb, TCA_ACT_MAX, nla, NULL, NULL);
+   err = nla_parse_nested(tb, TCA_ACT_MAX, nla, NULL, extack);
if (err < 0)
goto err_out;
err = -EINVAL;
kind = tb[TCA_ACT_KIND];
-   if (!kind)
+   if (!kind) {
+   NL_SET_ERR_MSG(extack, "TC action kind must be 
specified");
goto err_out;
-   if (nla_strlcpy(act_name, kind, IFNAMSIZ) >= IFNAMSIZ)
+   }
+   if (nla_strlcpy(act_name, kind, IFNAMSIZ) >= IFNAMSIZ) {
+   NL_SET_ERR_MSG(extack, "TC action name too long");
goto err_out;
+   }
if (tb[TCA_ACT_COOKIE]) {
int cklen = nla_len(tb[TCA_ACT_COOKIE]);
 
-   if (cklen > TC_COOKIE_MAX_SIZE)
+   if (cklen > TC_COOKIE_MAX_SIZE) {
+   NL_SET_ERR_MSG(extack, "TC cookie size above 
the maximum");
goto err_out;
+   }
 
cookie = nla_memdup_cookie(tb);
if (!cookie) {
+   NL_SET_ERR_MSG(extack, "No memory to generate 
TC cookie");
err = -ENOMEM;
goto err_out;
}
}
} else {
-   err = -EINVAL;
-   if (strlcpy(act_name, name, IFNAMSIZ) >= IFNAMSIZ)
+   if (strlcpy(act_name, name, IFNAMSIZ) >= IFNAMSIZ) {
+   NL_SET_ERR_MSG(extack, "TC action name too long");
+   err = -EINVAL;
goto err_out;
+   }
}
 
a_o = tc_lookup_action_n(act_name);
@@ -664,6 +673,7 @@ struct tc_action *tcf_action_init_1(struct net *net, struct 
tcf_proto *tp,
goto err_mod;
}
 #endif
+   NL_SET_ERR_MSG(extack, "Failed to load TC action module");
err = -ENOENT;
goto err_out;
}
@@ -698,6 +708,7 @@ struct tc_action *tcf_action_init_1(struct net *net, struct 
tcf_proto *tp,
 
list_add_tail(>list, );
tcf_action_destroy(, bind);
+   NL_SET_ERR_MSG(extack, "Failed to init action chain");
return ERR_PTR(err);
}
}
@@ -734,7 +745,7 @@ int tcf_action_init(struct net *net, struct tcf_proto *tp, 
struct nlattr *nla,
int err;
int i;
 
-   err = nla_parse_nested(tb, TCA_ACT_MAX_PRIO, nla, NULL, NULL);
+   err = nla_parse_nested(tb, TCA_ACT_MAX_PRIO, nla, NULL, extack);
if (err < 0)
return err;
 
@@ -842,7 +853,8 @@ static int tca_get_fill(struct sk_buff *skb, struct 
list_head *actions,
 
 static int
 tcf_get_notify(struct net *net, u32 portid, struct nlmsghdr *n,
-  struct list_head *actions, int event)
+  struct list_head *actions, int event,
+  struct netlink_ext_ack *extack)
 {
struct sk_buff *skb;
 
@@ -851,6 +863,7 @@ tcf_get_notify(struct net *net, u32 portid, struct nlmsghdr 
*n,
return -ENOBUFS;
if (tca_get_fill(skb, actions, portid, n->nlmsg_seq, 0, event,
 0, 0) <= 0) {
+   NL_SET_ERR_MSG(extack, "Failed to fill netlink tc action 
attributes");
kfree_skb(skb);
return -EINVAL;
}
@@ -859,7 +872,8 @@ tcf_get_notify(struct net *net, u32 portid, struct nlmsghdr 
*n,
 }
 
 static struct tc_action *tcf_action_get_1(struct net *net, struct nlattr *nla,
- struct nlmsghdr *n, u32 portid)
+ struct nlmsghdr *n, u32 portid,
+ struct netlink_ext_ack *extack)
 {
struct nlattr *tb[TCA_ACT_MAX + 1];
const struct tc_action_ops *ops;
@@ -867,20 +881,24 @@ static struct tc_action *tcf_action_get_1(struct net 
*net, struct nlattr *nla,
int index;
int err;
 
-   err = nla_parse_nested(tb,