| Message ID | 20240201034653.450138-2-stephen@networkplumber.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net/sched: netem cleanups | expand |
Thu, Feb 01, 2024 at 04:45:58AM CET, stephen@networkplumber.org wrote: >The error handling in netem predates introduction of extack, >and was mostly using pr_info(). Use extack to put errors in >result rather than console log. > [...] >@@ -1068,18 +1073,16 @@ static int netem_init(struct Qdisc *sch, struct nlattr *opt, > struct netlink_ext_ack *extack) > { > struct netem_sched_data *q = qdisc_priv(sch); >- int ret; > > qdisc_watchdog_init(&q->watchdog, sch); > >- if (!opt) >+ if (!opt) { >+ NL_SET_ERR_MSG_MOD(extack, "Netem missing required parameters"); Drop "Netem " here. Otherwise, this looks fine. > return -EINVAL; >+ } > > q->loss_model = CLG_RANDOM; >- ret = netem_change(sch, opt, extack); >- if (ret) >- pr_info("netem: change failed\n"); >- return ret; >+ return netem_change(sch, opt, extack); > } > > static void netem_destroy(struct Qdisc *sch) >-- >2.43.0 > >
On Thu, 1 Feb 2024 10:30:27 +0100 Jiri Pirko wrote: > Thu, Feb 01, 2024 at 04:45:58AM CET, stephen@networkplumber.org wrote: > >- if (!opt) > >+ if (!opt) { > >+ NL_SET_ERR_MSG_MOD(extack, "Netem missing required parameters"); > > Drop "Netem " here. > > Otherwise, this looks fine. Looks like most sch's require opt. Would it be a bad idea to pull the check out to the caller? Minor simplification, plus the caller has the outer message so they can use NL_SET_ERR_ATTR_MISS() and friends. $ git grep -A1 'if (!opt)' -- net/sched/ net/sched/cls_fw.c: if (!opt) net/sched/cls_fw.c- return handle ? -EINVAL : 0; /* Succeed if it is old method. */ -- net/sched/cls_u32.c: if (!opt) { net/sched/cls_u32.c- if (handle) { -- net/sched/sch_cbs.c: if (!opt) { net/sched/sch_cbs.c- NL_SET_ERR_MSG(extack, "Missing CBS qdisc options which are mandatory"); -- net/sched/sch_drr.c: if (!opt) { net/sched/sch_drr.c- NL_SET_ERR_MSG(extack, "DRR options are required for this operation"); -- net/sched/sch_etf.c: if (!opt) { net/sched/sch_etf.c- NL_SET_ERR_MSG(extack, -- net/sched/sch_ets.c: if (!opt) { net/sched/sch_ets.c- NL_SET_ERR_MSG(extack, "ETS options are required for this operation"); -- net/sched/sch_ets.c: if (!opt) net/sched/sch_ets.c- return -EINVAL; -- net/sched/sch_gred.c: if (!opt) net/sched/sch_gred.c- return -EINVAL; -- net/sched/sch_htb.c: if (!opt) net/sched/sch_htb.c- return -EINVAL; -- net/sched/sch_htb.c: if (!opt) net/sched/sch_htb.c- goto failure; -- net/sched/sch_multiq.c: if (!opt) net/sched/sch_multiq.c- return -EINVAL; -- net/sched/sch_netem.c: if (!opt) net/sched/sch_netem.c- return -EINVAL; -- net/sched/sch_prio.c: if (!opt) net/sched/sch_prio.c- return -EINVAL; -- net/sched/sch_red.c: if (!opt) net/sched/sch_red.c- return -EINVAL; -- net/sched/sch_skbprio.c: if (!opt) net/sched/sch_skbprio.c- return 0; -- net/sched/sch_taprio.c: if (!opt) net/sched/sch_taprio.c- return -EINVAL; -- net/sched/sch_tbf.c: if (!opt) net/sched/sch_tbf.c- return -EINVAL;
Jakub Kicinski <kuba@kernel.org> writes: > On Thu, 1 Feb 2024 10:30:27 +0100 Jiri Pirko wrote: >> Thu, Feb 01, 2024 at 04:45:58AM CET, stephen@networkplumber.org wrote: >> >- if (!opt) >> >+ if (!opt) { >> >+ NL_SET_ERR_MSG_MOD(extack, "Netem missing required parameters"); >> >> Drop "Netem " here. >> >> Otherwise, this looks fine. > > Looks like most sch's require opt. Would it be a bad idea to pull > the check out to the caller? Minor simplification, plus the caller > has the outer message so they can use NL_SET_ERR_ATTR_MISS() and > friends. There's also these which maybe complicates things: $ git grep -A1 'if (opt == NULL)' -- net/sched/ net/sched/cls_flow.c: if (opt == NULL) net/sched/cls_flow.c- return -EINVAL; -- net/sched/sch_choke.c: if (opt == NULL) net/sched/sch_choke.c- return -EINVAL; -- net/sched/sch_fifo.c: if (opt == NULL) { net/sched/sch_fifo.c- u32 limit = qdisc_dev(sch)->tx_queue_len; -- net/sched/sch_hfsc.c: if (opt == NULL) net/sched/sch_hfsc.c- return -EINVAL; -- net/sched/sch_plug.c: if (opt == NULL) { net/sched/sch_plug.c- q->limit = qdisc_dev(sch)->tx_queue_len I'm in favour of qdisc specific extack messages. > $ git grep -A1 'if (!opt)' -- net/sched/ > net/sched/cls_fw.c: if (!opt) > net/sched/cls_fw.c- return handle ? -EINVAL : 0; /* Succeed if it is old method. */ > -- > net/sched/cls_u32.c: if (!opt) { > net/sched/cls_u32.c- if (handle) { > -- > net/sched/sch_cbs.c: if (!opt) { > net/sched/sch_cbs.c- NL_SET_ERR_MSG(extack, "Missing CBS qdisc options which are mandatory"); > -- > net/sched/sch_drr.c: if (!opt) { > net/sched/sch_drr.c- NL_SET_ERR_MSG(extack, "DRR options are required for this operation"); > -- > net/sched/sch_etf.c: if (!opt) { > net/sched/sch_etf.c- NL_SET_ERR_MSG(extack, > -- > net/sched/sch_ets.c: if (!opt) { > net/sched/sch_ets.c- NL_SET_ERR_MSG(extack, "ETS options are required for this operation"); > -- > net/sched/sch_ets.c: if (!opt) > net/sched/sch_ets.c- return -EINVAL; > -- > net/sched/sch_gred.c: if (!opt) > net/sched/sch_gred.c- return -EINVAL; > -- > net/sched/sch_htb.c: if (!opt) > net/sched/sch_htb.c- return -EINVAL; > -- > net/sched/sch_htb.c: if (!opt) > net/sched/sch_htb.c- goto failure; > -- > net/sched/sch_multiq.c: if (!opt) > net/sched/sch_multiq.c- return -EINVAL; > -- > net/sched/sch_netem.c: if (!opt) > net/sched/sch_netem.c- return -EINVAL; > -- > net/sched/sch_prio.c: if (!opt) > net/sched/sch_prio.c- return -EINVAL; > -- > net/sched/sch_red.c: if (!opt) > net/sched/sch_red.c- return -EINVAL; > -- > net/sched/sch_skbprio.c: if (!opt) > net/sched/sch_skbprio.c- return 0; > -- > net/sched/sch_taprio.c: if (!opt) > net/sched/sch_taprio.c- return -EINVAL; > -- > net/sched/sch_tbf.c: if (!opt) > net/sched/sch_tbf.c- return -EINVAL;
On Fri, 02 Feb 2024 11:53:04 +0000 Donald Hunter wrote: > > Looks like most sch's require opt. Would it be a bad idea to pull > > the check out to the caller? Minor simplification, plus the caller > > has the outer message so they can use NL_SET_ERR_ATTR_MISS() and > > friends. > > There's also these which maybe complicates things: > > $ git grep -A1 'if (opt == NULL)' -- net/sched/ > net/sched/cls_flow.c: if (opt == NULL) > net/sched/cls_flow.c- return -EINVAL; > -- > net/sched/sch_choke.c: if (opt == NULL) > net/sched/sch_choke.c- return -EINVAL; > -- > net/sched/sch_fifo.c: if (opt == NULL) { > net/sched/sch_fifo.c- u32 limit = qdisc_dev(sch)->tx_queue_len; > -- > net/sched/sch_hfsc.c: if (opt == NULL) > net/sched/sch_hfsc.c- return -EINVAL; > -- > net/sched/sch_plug.c: if (opt == NULL) { > net/sched/sch_plug.c- q->limit = qdisc_dev(sch)->tx_queue_len That's fine, I was thinking opt-in. Add a bit to ops that says "init_requires_opts" or whatnot. > I'm in favour of qdisc specific extack messages. Most of them just say "$name requires options" in a more or less concise and more or less well spelled form :( Even if we don't want to depend purely on ATTR_MISS - extack messages support printf now, and we have the qdisc name in the ops (ops->id), so we can printf the same string in the core. Just an idea, if you all prefer to keep things as they are, that's fine. But we've been sprinkling the damn string messages throughout TC for years now, and still they keep coming and still if you step one foot away from the most actively developed actions and classifiers, you're back in the 90s :(
On Fri, 2 Feb 2024 at 16:24, Jakub Kicinski <kuba@kernel.org> wrote: > > That's fine, I was thinking opt-in. Add a bit to ops that says > "init_requires_opts" or whatnot. > > > I'm in favour of qdisc specific extack messages. > > Most of them just say "$name requires options" in a more or less concise > and more or less well spelled form :( Even if we don't want to depend > purely on ATTR_MISS - extack messages support printf now, and we have > the qdisc name in the ops (ops->id), so we can printf the same string > in the core. Fair point, it's not easy to steer people to the right options attrs. > Just an idea, if you all prefer to keep things as they are, that's fine. > But we've been sprinkling the damn string messages throughout TC for > years now, and still they keep coming and still if you step one foot > away from the most actively developed actions and classifiers, you're > back in the 90s :( So true. FWIW I was thinking of putting some effort into smoothing off some of the sharp edges in all of the qdiscs, along the lines of this: diff --git a/net/sched/sch_choke.c b/net/sched/sch_choke.c index ae1da08e268f..8c16fcbaad71 100644 --- a/net/sched/sch_choke.c +++ b/net/sched/sch_choke.c @@ -352,19 +352,28 @@ static int choke_change(struct Qdisc *sch, struct nlattr *opt, if (err < 0) return err; - if (tb[TCA_CHOKE_PARMS] == NULL || - tb[TCA_CHOKE_STAB] == NULL) + if (tb[TCA_CHOKE_PARMS] == NULL) { + NL_SET_ERR_MSG(extack, + "Missing TCA_CHOKE_PARMS of type 'struct tc_red_qopt'"); return -EINVAL; + } + if (tb[TCA_CHOKE_STAB] == NULL) { + NL_SET_ERR_MSG(extack, "Missing TCA_CHOKE_STAB"); + return -EINVAL; + } max_P = tb[TCA_CHOKE_MAX_P] ? nla_get_u32(tb[TCA_CHOKE_MAX_P]) : 0; ctl = nla_data(tb[TCA_CHOKE_PARMS]); stab = nla_data(tb[TCA_CHOKE_STAB]); - if (!red_check_params(ctl->qth_min, ctl->qth_max, ctl->Wlog, ctl->Scell_log, stab)) + if (!red_check_params(ctl->qth_min, ctl->qth_max, ctl->Wlog, ctl->Scell_log, stab)) { + NL_SET_ERR_MSG(extack, "TCA_CHOKE_PARMS has invalid red parameters"); return -EINVAL; - - if (ctl->limit > CHOKE_MAX_QUEUE) + } + if (ctl->limit > CHOKE_MAX_QUEUE) { + NL_SET_ERR_MSG(extack, "TCA_CHOKE_PARMS.limit exceeds CHOKE_MAX_QUEUE"); return -EINVAL; + } mask = roundup_pow_of_two(ctl->limit + 1) - 1; if (mask != q->tab_mask) {
diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c index fa678eb88528..7c37a69aba0e 100644 --- a/net/sched/sch_netem.c +++ b/net/sched/sch_netem.c @@ -782,15 +782,18 @@ static void dist_free(struct disttable *d) * signed 16 bit values. */ -static int get_dist_table(struct disttable **tbl, const struct nlattr *attr) +static int get_dist_table(struct disttable **tbl, const struct nlattr *attr, + struct netlink_ext_ack *extack) { size_t n = nla_len(attr)/sizeof(__s16); const __s16 *data = nla_data(attr); struct disttable *d; int i; - if (!n || n > NETEM_DIST_MAX) + if (!n || n > NETEM_DIST_MAX) { + NL_SET_ERR_MSG_FMT_MOD(extack, "invalid distribution table size: %zu", n); return -EINVAL; + } d = kvmalloc(struct_size(d, table, n), GFP_KERNEL); if (!d) @@ -865,7 +868,8 @@ static void get_rate(struct netem_sched_data *q, const struct nlattr *attr) q->cell_size_reciprocal = (struct reciprocal_value) { 0 }; } -static int get_loss_clg(struct netem_sched_data *q, const struct nlattr *attr) +static int get_loss_clg(struct netem_sched_data *q, const struct nlattr *attr, + struct netlink_ext_ack *extack) { const struct nlattr *la; int rem; @@ -878,7 +882,7 @@ static int get_loss_clg(struct netem_sched_data *q, const struct nlattr *attr) const struct tc_netem_gimodel *gi = nla_data(la); if (nla_len(la) < sizeof(struct tc_netem_gimodel)) { - pr_info("netem: incorrect gi model size\n"); + NL_SET_ERR_MSG_MOD(extack, "incorrect GI model size"); return -EINVAL; } @@ -897,7 +901,7 @@ static int get_loss_clg(struct netem_sched_data *q, const struct nlattr *attr) const struct tc_netem_gemodel *ge = nla_data(la); if (nla_len(la) < sizeof(struct tc_netem_gemodel)) { - pr_info("netem: incorrect ge model size\n"); + NL_SET_ERR_MSG_MOD(extack, "incorrect GE model size"); return -EINVAL; } @@ -911,7 +915,7 @@ static int get_loss_clg(struct netem_sched_data *q, const struct nlattr *attr) } default: - pr_info("netem: unknown loss type %u\n", type); + NL_SET_ERR_MSG_FMT_MOD(extack, "unknown loss type: %u", type); return -EINVAL; } } @@ -934,12 +938,13 @@ static const struct nla_policy netem_policy[TCA_NETEM_MAX + 1] = { }; static int parse_attr(struct nlattr *tb[], int maxtype, struct nlattr *nla, - const struct nla_policy *policy, int len) + const struct nla_policy *policy, int len, + struct netlink_ext_ack *extack) { int nested_len = nla_len(nla) - NLA_ALIGN(len); if (nested_len < 0) { - pr_info("netem: invalid attributes len %d\n", nested_len); + NL_SET_ERR_MSG_MOD(extack, "invalid nested attribute length"); return -EINVAL; } @@ -966,18 +971,18 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt, int ret; qopt = nla_data(opt); - ret = parse_attr(tb, TCA_NETEM_MAX, opt, netem_policy, sizeof(*qopt)); + ret = parse_attr(tb, TCA_NETEM_MAX, opt, netem_policy, sizeof(*qopt), extack); if (ret < 0) return ret; if (tb[TCA_NETEM_DELAY_DIST]) { - ret = get_dist_table(&delay_dist, tb[TCA_NETEM_DELAY_DIST]); + ret = get_dist_table(&delay_dist, tb[TCA_NETEM_DELAY_DIST], extack); if (ret) goto table_free; } if (tb[TCA_NETEM_SLOT_DIST]) { - ret = get_dist_table(&slot_dist, tb[TCA_NETEM_SLOT_DIST]); + ret = get_dist_table(&slot_dist, tb[TCA_NETEM_SLOT_DIST], extack); if (ret) goto table_free; } @@ -988,7 +993,7 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt, old_loss_model = q->loss_model; if (tb[TCA_NETEM_LOSS]) { - ret = get_loss_clg(q, tb[TCA_NETEM_LOSS]); + ret = get_loss_clg(q, tb[TCA_NETEM_LOSS], extack); if (ret) { q->loss_model = old_loss_model; q->clg = old_clg; @@ -1068,18 +1073,16 @@ static int netem_init(struct Qdisc *sch, struct nlattr *opt, struct netlink_ext_ack *extack) { struct netem_sched_data *q = qdisc_priv(sch); - int ret; qdisc_watchdog_init(&q->watchdog, sch); - if (!opt) + if (!opt) { + NL_SET_ERR_MSG_MOD(extack, "Netem missing required parameters"); return -EINVAL; + } q->loss_model = CLG_RANDOM; - ret = netem_change(sch, opt, extack); - if (ret) - pr_info("netem: change failed\n"); - return ret; + return netem_change(sch, opt, extack); } static void netem_destroy(struct Qdisc *sch)
The error handling in netem predates introduction of extack, and was mostly using pr_info(). Use extack to put errors in result rather than console log. Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> --- net/sched/sch_netem.c | 39 +++++++++++++++++++++------------------ 1 file changed, 21 insertions(+), 18 deletions(-)