[PATCH net-next 0/2] net/sched: cleanup parsing prints in htb and qfq

[PATCH net-next 0/2] net/sched: cleanup parsing prints in htb and qfq

[Pedro Tammela]

These two qdiscs are still using prints on dmesg to report parsing
errors. Since the parsing code has access to extack, convert these error
messages to extack.

Pedro Tammela (2):
  net/sched: sch_htb: use extack on errors messages
  net/sched: sch_qfq: use extack on errors messages

 net/sched/sch_htb.c | 19 +++++++++++--------
 net/sched/sch_qfq.c | 15 +++++++++------
 2 files changed, 20 insertions(+), 14 deletions(-)

-- 
2.34.1

[PATCH net-next 1/2] net/sched: sch_htb: use extack on errors messages

[Pedro Tammela]

Some error messages are still being printed to dmesg.
Since extack is available, provide error messages there.

Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
---
 net/sched/sch_htb.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c
index 92f2975b6a82..bc2da8027650 100644
--- a/net/sched/sch_htb.c
+++ b/net/sched/sch_htb.c
@@ -1786,7 +1786,7 @@ static int htb_change_class(struct Qdisc *sch, u32 classid,
 		goto failure;
 
 	err = nla_parse_nested_deprecated(tb, TCA_HTB_MAX, opt, htb_policy,
-					  NULL);
+					  extack);
 	if (err < 0)
 		goto failure;
 
@@ -1858,7 +1858,7 @@ static int htb_change_class(struct Qdisc *sch, u32 classid,
 
 		/* check maximal depth */
 		if (parent && parent->parent && parent->parent->level < 2) {
-			pr_err("htb: tree is too deep\n");
+			NL_SET_ERR_MSG_MOD(extack, "tree is too deep");
 			goto failure;
 		}
 		err = -ENOBUFS;
@@ -1917,8 +1917,9 @@ static int htb_change_class(struct Qdisc *sch, u32 classid,
 			};
 			err = htb_offload(dev, &offload_opt);
 			if (err) {
-				pr_err("htb: TC_HTB_LEAF_ALLOC_QUEUE failed with err = %d\n",
-				       err);
+				NL_SET_ERR_MSG_FMT_MOD(extack,
+						       "TC_HTB_LEAF_ALLOC_QUEUE failed with err = %d\n",
+						       err);
 				goto err_kill_estimator;
 			}
 			dev_queue = netdev_get_tx_queue(dev, offload_opt.qid);
@@ -1937,8 +1938,9 @@ static int htb_change_class(struct Qdisc *sch, u32 classid,
 			};
 			err = htb_offload(dev, &offload_opt);
 			if (err) {
-				pr_err("htb: TC_HTB_LEAF_TO_INNER failed with err = %d\n",
-				       err);
+				NL_SET_ERR_MSG_FMT_MOD(extack,
+						       "TC_HTB_LEAF_TO_INNER failed with err = %d",
+						       err);
 				htb_graft_helper(dev_queue, old_q);
 				goto err_kill_estimator;
 			}
@@ -2067,8 +2069,9 @@ static int htb_change_class(struct Qdisc *sch, u32 classid,
 	qdisc_put(parent_qdisc);
 
 	if (warn)
-		pr_warn("HTB: quantum of class %X is %s. Consider r2q change.\n",
-			    cl->common.classid, (warn == -1 ? "small" : "big"));
+		NL_SET_ERR_MSG_FMT_MOD(extack,
+				       "quantum of class %X is %s. Consider r2q change.",
+				       cl->common.classid, (warn == -1 ? "small" : "big"));
 
 	qdisc_class_hash_grow(sch, &q->clhash);
 
-- 
2.34.1

Re: [PATCH net-next 1/2] net/sched: sch_htb: use extack on errors messages

[Jakub Kicinski]

On Fri, 14 Apr 2023 15:53:09 -0300 Pedro Tammela wrote:
> @@ -1917,8 +1917,9 @@ static int htb_change_class(struct Qdisc *sch, u32 classid,
>  			};
>  			err = htb_offload(dev, &offload_opt);
>  			if (err) {
> -				pr_err("htb: TC_HTB_LEAF_ALLOC_QUEUE failed with err = %d\n",
> -				       err);
> +				NL_SET_ERR_MSG_FMT_MOD(extack,

What's the ruling on using _MOD() in qdiscs ?
There are some extacks already in this file without _MOD().

> +						       "TC_HTB_LEAF_ALLOC_QUEUE failed with err = %d\n",
> +						       err);

The formatting of an error into the message is unnecessary duplication.
The error value does not make it to dmesg so we need to print it there,
but it's already present at the netlink level.

Re: [PATCH net-next 1/2] net/sched: sch_htb: use extack on errors messages

[Jamal Hadi Salim]

On Fri, Apr 14, 2023 at 9:13 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Fri, 14 Apr 2023 15:53:09 -0300 Pedro Tammela wrote:
> > @@ -1917,8 +1917,9 @@ static int htb_change_class(struct Qdisc *sch, u32 classid,
> >                       };
> >                       err = htb_offload(dev, &offload_opt);
> >                       if (err) {
> > -                             pr_err("htb: TC_HTB_LEAF_ALLOC_QUEUE failed with err = %d\n",
> > -                                    err);
> > +                             NL_SET_ERR_MSG_FMT_MOD(extack,
>
> What's the ruling on using _MOD() in qdiscs ?
> There are some extacks already in this file without _MOD().

There is no "rule" other than the LinuxWay(tm) i.e. people cutnpaste.
It's not just on qdiscs that this inconsistency exists but also on
filters and actions.
Do we need a rule to prefer one over the other? _MOD() seems to
provide more information - which is always useful.

cheers,
jamal

Re: [PATCH net-next 1/2] net/sched: sch_htb: use extack on errors messages

[Jakub Kicinski]

On Sat, 15 Apr 2023 10:06:36 -0400 Jamal Hadi Salim wrote:
> There is no "rule" other than the LinuxWay(tm) i.e. people cutnpaste.

:-)

> It's not just on qdiscs that this inconsistency exists but also on
> filters and actions.
> Do we need a rule to prefer one over the other? _MOD() seems to
> provide more information - which is always useful.

People started adding _MOD() on every extack so I was pushing back 
a little lately. It will bloat the strings and makes the output between
parsing and hand coded checks different. Plus if we really want the
mod name, we should just make it the default and remove the non-mod
version.

The rule of thumb I had was that if the message comes from "core" of 
a family then _MOD() is unnecessary. In this case HTB is a one-of-many
implementations so _MOD() seems fine. Then again errors about parsing
TCA_HTB_* attrs are unlikely to come from something else than HTB..

Re: [PATCH net-next 1/2] net/sched: sch_htb: use extack on errors messages

[Pedro Tammela]

On 17/04/2023 12:52, Jakub Kicinski wrote:
> [...]
> 
> The rule of thumb I had was that if the message comes from "core" of
> a family then _MOD() is unnecessary. In this case HTB is a one-of-many
> implementations so _MOD() seems fine. Then again errors about parsing
> TCA_HTB_* attrs are unlikely to come from something else than HTB..

Agreed, will re-spin with this in mind.

Pedro

[PATCH net-next 2/2] net/sched: sch_qfq: use extack on errors messages

[Pedro Tammela]

Some error messages are still being printed to dmesg.
Since extack is available, report error messages there instead.

Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
---
 net/sched/sch_qfq.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/net/sched/sch_qfq.c b/net/sched/sch_qfq.c
index cf5ebe43b3b4..b2a4cf01766c 100644
--- a/net/sched/sch_qfq.c
+++ b/net/sched/sch_qfq.c
@@ -403,19 +403,20 @@ static int qfq_change_class(struct Qdisc *sch, u32 classid, u32 parentid,
 	int delta_w;
 
 	if (tca[TCA_OPTIONS] == NULL) {
-		pr_notice("qfq: no options\n");
+		NL_SET_ERR_MSG_MOD(extack, "missing options");
 		return -EINVAL;
 	}
 
 	err = nla_parse_nested_deprecated(tb, TCA_QFQ_MAX, tca[TCA_OPTIONS],
-					  qfq_policy, NULL);
+					  qfq_policy, extack);
 	if (err < 0)
 		return err;
 
 	if (tb[TCA_QFQ_WEIGHT]) {
 		weight = nla_get_u32(tb[TCA_QFQ_WEIGHT]);
 		if (!weight || weight > (1UL << QFQ_MAX_WSHIFT)) {
-			pr_notice("qfq: invalid weight %u\n", weight);
+			NL_SET_ERR_MSG_FMT_MOD(extack, "invalid weight %u\n",
+					       weight);
 			return -EINVAL;
 		}
 	} else
@@ -424,7 +425,8 @@ static int qfq_change_class(struct Qdisc *sch, u32 classid, u32 parentid,
 	if (tb[TCA_QFQ_LMAX]) {
 		lmax = nla_get_u32(tb[TCA_QFQ_LMAX]);
 		if (lmax < QFQ_MIN_LMAX || lmax > (1UL << QFQ_MTU_SHIFT)) {
-			pr_notice("qfq: invalid max length %u\n", lmax);
+			NL_SET_ERR_MSG_FMT_MOD(extack,
+					       "invalid max length %u\n", lmax);
 			return -EINVAL;
 		}
 	} else
@@ -441,8 +443,9 @@ static int qfq_change_class(struct Qdisc *sch, u32 classid, u32 parentid,
 	delta_w = weight - (cl ? cl->agg->class_weight : 0);
 
 	if (q->wsum + delta_w > QFQ_MAX_WSUM) {
-		pr_notice("qfq: total weight out of range (%d + %u)\n",
-			  delta_w, q->wsum);
+		NL_SET_ERR_MSG_FMT_MOD(extack,
+				       "qfq: total weight out of range (%d + %u)\n",
+				       delta_w, q->wsum);
 		return -EINVAL;
 	}
 
-- 
2.34.1

Re: [PATCH net-next 2/2] net/sched: sch_qfq: use extack on errors messages

[Jakub Kicinski]

On Fri, 14 Apr 2023 15:53:10 -0300 Pedro Tammela wrote:
>  	if (tca[TCA_OPTIONS] == NULL) {
> -		pr_notice("qfq: no options\n");
> +		NL_SET_ERR_MSG_MOD(extack, "missing options");

NL_REQ_ATTR_CHECK() (probably in addition to the string message)
since it's legacy netlink.

>  		return -EINVAL;
>  	}
>  
>  	err = nla_parse_nested_deprecated(tb, TCA_QFQ_MAX, tca[TCA_OPTIONS],
> -					  qfq_policy, NULL);
> +					  qfq_policy, extack);
>  	if (err < 0)
>  		return err;
>  
>  	if (tb[TCA_QFQ_WEIGHT]) {
>  		weight = nla_get_u32(tb[TCA_QFQ_WEIGHT]);
>  		if (!weight || weight > (1UL << QFQ_MAX_WSHIFT)) {
> -			pr_notice("qfq: invalid weight %u\n", weight);
> +			NL_SET_ERR_MSG_FMT_MOD(extack, "invalid weight %u\n",
> +					       weight);

The checks should be expressed as part of the policy and parsing will
take care of the extack

>  			return -EINVAL;
>  		}
>  	} else
> @@ -424,7 +425,8 @@ static int qfq_change_class(struct Qdisc *sch, u32 classid, u32 parentid,
>  	if (tb[TCA_QFQ_LMAX]) {
>  		lmax = nla_get_u32(tb[TCA_QFQ_LMAX]);
>  		if (lmax < QFQ_MIN_LMAX || lmax > (1UL << QFQ_MTU_SHIFT)) {
> -			pr_notice("qfq: invalid max length %u\n", lmax);
> +			NL_SET_ERR_MSG_FMT_MOD(extack,
> +					       "invalid max length %u\n", lmax);
>  			return -EINVAL;

ditto