On 12/6/17 9:08 AM, Alexander Aring wrote:
> diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
> index a48ca41b7ecf..7b52a16d2fea 100644
> --- a/net/sched/sch_api.c
> +++ b/net/sched/sch_api.c
> @@ -449,7 +449,8 @@ static const struct nla_policy stab_policy[TCA_STAB_MAX +
> 1] = {
> [TCA_STAB_DATA] = { .type = NLA_BINARY },
> };
>
> -static struct qdisc_size_table *qdisc_get_stab(struct nlattr *opt)
> +static struct qdisc_size_table *qdisc_get_stab(struct nlattr *opt,
> + struct netlink_ext_ack *extack)
> {
> struct nlattr *tb[TCA_STAB_MAX + 1];
> struct qdisc_size_table *stab;
> @@ -458,23 +459,31 @@ static struct qdisc_size_table *qdisc_get_stab(struct
> nlattr *opt)
> u16 *tab = NULL;
> int err;
>
> - err = nla_parse_nested(tb, TCA_STAB_MAX, opt, stab_policy, NULL);
> - if (err < 0)
> + err = nla_parse_nested(tb, TCA_STAB_MAX, opt, stab_policy, extack);
> + if (err < 0) {
> + NL_SET_ERR_MSG(extack, "Failed to parse stab netlink msg");
you don't want to set extack here; it should be set in nla_parse_nested
since it is passed as an arg.
> return ERR_PTR(err);
> - if (!tb[TCA_STAB_BASE])
> + }
> + if (!tb[TCA_STAB_BASE]) {
> + NL_SET_ERR_MSG(extack, "Stab base is missing");
"stab base" is the name of the netlink attribute, but it does not
describe *what* is missing. The key here is returning a message that
tells an average user what they did wrong.
> return ERR_PTR(-EINVAL);
> + }
>
> s = nla_data(tb[TCA_STAB_BASE]);
>
> if (s->tsize > 0) {
> - if (!tb[TCA_STAB_DATA])
> + if (!tb[TCA_STAB_DATA]) {
> + NL_SET_ERR_MSG(extack, "Stab data is missing");
ditto here.
Looking at the code for the tc command, stab appears to be table size;
perhaps a tc author can elaborate on what STAB really means.
> return ERR_PTR(-EINVAL);
> + }
> tab = nla_data(tb[TCA_STAB_DATA]);
> tsize = nla_len(tb[TCA_STAB_DATA]) / sizeof(u16);
> }
>
> - if (tsize != s->tsize || (!tab && tsize > 0))
> + if (tsize != s->tsize || (!tab && tsize > 0)) {
> + NL_SET_ERR_MSG(extack, "Invalid data length of stab data");
> return ERR_PTR(-EINVAL);
> + }
>
> list_for_each_entry(stab, &qdisc_stab_list, list) {
> if (memcmp(&stab->szopts, s, sizeof(*s)))
> @@ -486,8 +495,10 @@ static struct qdisc_size_table *qdisc_get_stab(struct
> nlattr *opt)
> }
>
> stab = kmalloc(sizeof(*stab) + tsize * sizeof(u16), GFP_KERNEL);
> - if (!stab)
> + if (!stab) {
> + NL_SET_ERR_MSG(extack, "Failed to allocate memory for stab
> data");
ENOMEM does not need a text message. Which memory allocation failed is
not really relevant.
> return ERR_PTR(-ENOMEM);
> + }
>
> stab->refcnt = 1;
> stab->szopts = *s;
> @@ -896,7 +907,8 @@ static void notify_and_destroy(struct net *net, struct
> sk_buff *skb,
>
> static int qdisc_graft(struct net_device *dev, struct Qdisc *parent,
> struct sk_buff *skb, struct nlmsghdr *n, u32 classid,
> - struct Qdisc *new, struct Qdisc *old)
> + struct Qdisc *new, struct Qdisc *old,
> + struct netlink_ext_ack *extack)
> {
> struct Qdisc *q = old;
> struct net *net = dev_net(dev);
> @@ -911,8 +923,10 @@ static int qdisc_graft(struct net_device *dev, struct
> Qdisc *parent,
> (new && new->flags & TCQ_F_INGRESS)) {
> num_q = 1;
> ingress = 1;
> - if (!dev_ingress_queue(dev))
> + if (!dev_ingress_queue(dev)) {
> + NL_SET_ERR_MSG(extack, "Cannot find ingress
> queue for specified netdev");
netdev is a kernel term. 'device' is in the tc man page so a relevant
term for the message.
I think the above gives you an idea. Apply those comments to the rest of
this patch and the next ones.
