From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Ahern Subject: Re: [PATCH net-next 2/8] net: sched: cls_api: handle generic cls errors Date: Tue, 16 Jan 2018 15:58:38 -0800 Message-ID: References: <20180116172027.22128-1-aring@mojatatu.com> <20180116172027.22128-3-aring@mojatatu.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: xiyou.wangcong@gmail.com, jiri@resnulli.us, davem@davemloft.net, netdev@vger.kernel.org, kernel@mojatatu.com To: Alexander Aring , jhs@mojatatu.com Return-path: Received: from mail-pg0-f65.google.com ([74.125.83.65]:38501 "EHLO mail-pg0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750816AbeAPX6l (ORCPT ); Tue, 16 Jan 2018 18:58:41 -0500 Received: by mail-pg0-f65.google.com with SMTP id y27so4320908pgc.5 for ; Tue, 16 Jan 2018 15:58:40 -0800 (PST) In-Reply-To: <20180116172027.22128-3-aring@mojatatu.com> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: 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 (!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) { > + NL_SET_ERR_MSG(extack, "Specified Class doesn't exist"); s/Class/class/ > return -ENOENT; > + } > } > > /* And the last stroke */ > @@ -808,12 +826,14 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n, > > chain_index = tca[TCA_CHAIN] ? nla_get_u32(tca[TCA_CHAIN]) : 0; > if (chain_index > TC_ACT_EXT_VAL_MASK) { > + NL_SET_ERR_MSG(extack, "Specified chain index exceeds upper limit"); > err = -EINVAL; > goto errout; > } > chain = tcf_chain_get(block, chain_index, > n->nlmsg_type == RTM_NEWTFILTER); > if (!chain) { > + NL_SET_ERR_MSG(extack, "Cannot find specified filter chain"); > err = n->nlmsg_type == RTM_NEWTFILTER ? -ENOMEM : -EINVAL; > goto errout; > } > @@ -829,6 +849,7 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n, > tp = tcf_chain_tp_find(chain, &chain_info, protocol, > prio, prio_allocate); > if (IS_ERR(tp)) { > + NL_SET_ERR_MSG(extack, "Filter with specified priority/protocol not found"); > err = PTR_ERR(tp); > goto errout; > } > @@ -837,12 +858,14 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n, > /* Proto-tcf does not exist, create new one */ > > if (tca[TCA_KIND] == NULL || !protocol) { > + NL_SET_ERR_MSG(extack, "Filter kind and protocol must be specified"); > err = -EINVAL; > goto errout; > } > > 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. Also, the messages are targeted at users not developers, so no code jargon / API references. > err = -ENOENT; > goto errout; > } > @@ -851,13 +874,14 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n, > prio = tcf_auto_prio(tcf_chain_tp_prev(&chain_info)); > > tp = tcf_proto_create(nla_data(tca[TCA_KIND]), > - protocol, prio, parent, q, chain); > + protocol, prio, parent, q, chain, extack); > if (IS_ERR(tp)) { > err = PTR_ERR(tp); > goto errout; > } > tp_created = 1; > } else if (tca[TCA_KIND] && nla_strcmp(tca[TCA_KIND], tp->ops->kind)) { > + NL_SET_ERR_MSG(extack, "Specified filter kind does not match existing one"); > err = -EINVAL; > goto errout; > } > @@ -876,6 +900,7 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n, > > 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"); same here. > err = -ENOENT; > goto errout; > } > @@ -887,13 +912,14 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n, > if (n->nlmsg_flags & NLM_F_EXCL) { > if (tp_created) > tcf_proto_destroy(tp); > + NL_SET_ERR_MSG(extack, "Exclusivity check success. Filter already exists"); Just "Filter already exists" > err = -EEXIST; > goto errout; > } > break; > case RTM_DELTFILTER: > err = tfilter_del_notify(net, skb, n, tp, q, parent, > - fh, false, &last); > + fh, false, &last, extack); > if (err) > goto errout; > if (last) { > @@ -904,8 +930,11 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n, > case RTM_GETTFILTER: > err = tfilter_notify(net, skb, n, tp, q, parent, fh, > RTM_NEWTFILTER, true); > + if (err < 0) > + NL_SET_ERR_MSG(extack, "Failed to send filter notify message"); > goto errout; > default: > + NL_SET_ERR_MSG(extack, "Invalid netlink message type"); > err = -EINVAL; > goto errout; > } > @@ -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"); To Cong's comment, perhaps "Classifier actions are not supported per compile options" > return -EOPNOTSUPP; > + } > #endif > > return 0; >