| Message ID | 1386665340-16820-1-git-send-email-yangyingliang@huawei.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Yang Yingliang <yangyingliang@huawei.com> Date: Tue, 10 Dec 2013 16:49:00 +0800 > When tc_modify_qdisc return EINVAL, user cannot distinguish > errors easliy. Add some messages to make it easier. > > Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> There is no chance I am ever going to apply a patch like this one, sorry. If the indication to userspace is not fine grained enough, fix that. Don't spam the kernel log with random messages triggerable by the user. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2013/12/11 2:47, David Miller wrote: > From: Yang Yingliang <yangyingliang@huawei.com> > Date: Tue, 10 Dec 2013 16:49:00 +0800 > >> When tc_modify_qdisc return EINVAL, user cannot distinguish >> errors easliy. Add some messages to make it easier. >> >> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> > > There is no chance I am ever going to apply a patch like this one, > sorry. > > If the indication to userspace is not fine grained enough, fix that. > Don't spam the kernel log with random messages triggerable by the > user. > > Ok, thanks! Then how about change the errno, is that ok ? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Yang Yingliang <yangyingliang@huawei.com> Date: Wed, 11 Dec 2013 10:37:01 +0800 > Then how about change the errno, is that ok ? No, it will break existing code. This is really an old topic which has been discussed before. The problem is that signalling a single integer for errors is not sufficient enough to transmit the necessary information to the user. You have to therefore think more largely about this problem than trying to fix it easily with some error code adjustments. The problem is fundamentally that we only return error codes to userspace, rather than something more sophisticated. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c index cd81505..6b0e53c 100644 --- a/net/sched/sch_api.c +++ b/net/sched/sch_api.c @@ -995,8 +995,11 @@ static int qdisc_change(struct Qdisc *sch, struct nlattr **tca) int err = 0; if (tca[TCA_OPTIONS]) { - if (sch->ops->change == NULL) + if (sch->ops->change == NULL) { + pr_err_ratelimited("Can't change %s qdisc parameters.\n", + sch->ops->id); return -EINVAL; + } err = sch->ops->change(sch, tca[TCA_OPTIONS]); if (err) return err; @@ -1186,15 +1189,20 @@ replay: if (tcm->tcm_handle) { if (q && !(n->nlmsg_flags & NLM_F_REPLACE)) return -EEXIST; - if (TC_H_MIN(tcm->tcm_handle)) + if (TC_H_MIN(tcm->tcm_handle)) { + pr_err_ratelimited("Wrong minor handle, it must be 0.\n"); return -EINVAL; + } q = qdisc_lookup(dev, tcm->tcm_handle); if (!q) goto create_n_graft; if (n->nlmsg_flags & NLM_F_EXCL) return -EEXIST; - if (tca[TCA_KIND] && nla_strcmp(tca[TCA_KIND], q->ops->id)) + if (tca[TCA_KIND] && nla_strcmp(tca[TCA_KIND], q->ops->id)) { + pr_err_ratelimited("A different qdisc(%s) with handle %d: already exists.\n", + q->ops->id, TC_H_MAJ(tcm->tcm_handle) >> 16); return -EINVAL; + } if (q == p || (p && check_loop(q, p, 0))) return -ELOOP; @@ -1232,8 +1240,10 @@ replay: } } } else { - if (!tcm->tcm_handle) + if (!tcm->tcm_handle) { + pr_err_ratelimited("Wrong handle, it can't be 0.\n"); return -EINVAL; + } q = qdisc_lookup(dev, tcm->tcm_handle); } @@ -1242,8 +1252,13 @@ replay: return -ENOENT; if (n->nlmsg_flags & NLM_F_EXCL) return -EEXIST; - if (tca[TCA_KIND] && nla_strcmp(tca[TCA_KIND], q->ops->id)) + if (tca[TCA_KIND] && nla_strcmp(tca[TCA_KIND], q->ops->id)) { + char name[IFNAMSIZ]; + nla_strlcpy(name, tca[TCA_KIND], IFNAMSIZ); + pr_err_ratelimited("A %s qdisc already exists, can't change its parameters to %s's.\n", + q->ops->id, name); return -EINVAL; + } err = qdisc_change(q, tca); if (err == 0) qdisc_notify(net, skb, n, clid, NULL, q);
When tc_modify_qdisc return EINVAL, user cannot distinguish errors easliy. Add some messages to make it easier. Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> --- net/sched/sch_api.c | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-)