Re: [PATCH iproute2 -next] ingress, clsact: don't add TCA_OPTIONS to nl msg
On 05/17/2016 01:54 PM, Jamal Hadi Salim wrote: On 16-05-15 12:36 PM, Daniel Borkmann wrote: In ingress and clsact qdisc TCA_OPTIONS are ignored, since it's parameterless. In tc, we add an empty addattr_l(... TCA_OPTIONS, NULL, 0) to the netlink message nevertheless. This has the side effect that when someone tries a 'tc qdisc replace' and already an existing such qdisc is present, tc fails with EINVAL here. Reason is that in the kernel, this invokes qdisc_change() when such requested qdisc is already present. When TCA_OPTIONS are passed to modify parameters, it looks whether qdisc implements .change() callback, and if not present (like in both cases here) it returns with error. Rather than adding an empty stub to the kernel that ignores TCA_OPTIONS again, just don't add TCA_OPTIONS to the netlink message in the first place. Before: # tc qdisc replace dev foo clsact# first try # tc qdisc replace dev foo clsact# second one RTNETLINK answers: Invalid argument After: # tc qdisc replace dev foo clsact # tc qdisc replace dev foo clsact # tc qdisc replace dev foo clsact Signed-off-by: Daniel BorkmannAcked-by: Jamal Hadi Salim I see need for correctness but was curious of use case that made you even look at this ;-> Sure, wanted to use this in a script that is called when some event arrives and the error 'Invalid argument' looked quite weird to me (... would have understood -ENOTSUPP ;)), which then led me looking into whether clsact code is correct or not, but it eventually turned out that the api code throws that error when TCA_OPTIONS are present, but no .change() callback, so motivation was to make sure clsact is fine and to improve usability there for the replace command. Thanks, Daniel
Re: [PATCH iproute2 -next] ingress, clsact: don't add TCA_OPTIONS to nl msg
On 16-05-15 12:36 PM, Daniel Borkmann wrote: In ingress and clsact qdisc TCA_OPTIONS are ignored, since it's parameterless. In tc, we add an empty addattr_l(... TCA_OPTIONS, NULL, 0) to the netlink message nevertheless. This has the side effect that when someone tries a 'tc qdisc replace' and already an existing such qdisc is present, tc fails with EINVAL here. Reason is that in the kernel, this invokes qdisc_change() when such requested qdisc is already present. When TCA_OPTIONS are passed to modify parameters, it looks whether qdisc implements .change() callback, and if not present (like in both cases here) it returns with error. Rather than adding an empty stub to the kernel that ignores TCA_OPTIONS again, just don't add TCA_OPTIONS to the netlink message in the first place. Before: # tc qdisc replace dev foo clsact# first try # tc qdisc replace dev foo clsact# second one RTNETLINK answers: Invalid argument After: # tc qdisc replace dev foo clsact # tc qdisc replace dev foo clsact # tc qdisc replace dev foo clsact Signed-off-by: Daniel BorkmannAcked-by: Jamal Hadi Salim I see need for correctness but was curious of use case that made you even look at this ;-> cheers, jamal
Re: [PATCH iproute2 -next] ingress, clsact: don't add TCA_OPTIONS to nl msg
On Sun, 15 May 2016 18:36:03 +0200 Daniel Borkmannwrote: > In ingress and clsact qdisc TCA_OPTIONS are ignored, since it's > parameterless. In tc, we add an empty addattr_l(... TCA_OPTIONS, > NULL, 0) to the netlink message nevertheless. This has the > side effect that when someone tries a 'tc qdisc replace' and > already an existing such qdisc is present, tc fails with > EINVAL here. > > Reason is that in the kernel, this invokes qdisc_change() when > such requested qdisc is already present. When TCA_OPTIONS are > passed to modify parameters, it looks whether qdisc implements > .change() callback, and if not present (like in both cases here) > it returns with error. Rather than adding an empty stub to the > kernel that ignores TCA_OPTIONS again, just don't add TCA_OPTIONS > to the netlink message in the first place. > > Before: > > # tc qdisc replace dev foo clsact# first try > # tc qdisc replace dev foo clsact# second one > RTNETLINK answers: Invalid argument > > After: > > # tc qdisc replace dev foo clsact > # tc qdisc replace dev foo clsact > # tc qdisc replace dev foo clsact > > Signed-off-by: Daniel Borkmann > --- > tc/q_clsact.c | 1 - > tc/q_ingress.c | 1 - > 2 files changed, 2 deletions(-) Applied to net-next
[PATCH iproute2 -next] ingress, clsact: don't add TCA_OPTIONS to nl msg
In ingress and clsact qdisc TCA_OPTIONS are ignored, since it's parameterless. In tc, we add an empty addattr_l(... TCA_OPTIONS, NULL, 0) to the netlink message nevertheless. This has the side effect that when someone tries a 'tc qdisc replace' and already an existing such qdisc is present, tc fails with EINVAL here. Reason is that in the kernel, this invokes qdisc_change() when such requested qdisc is already present. When TCA_OPTIONS are passed to modify parameters, it looks whether qdisc implements .change() callback, and if not present (like in both cases here) it returns with error. Rather than adding an empty stub to the kernel that ignores TCA_OPTIONS again, just don't add TCA_OPTIONS to the netlink message in the first place. Before: # tc qdisc replace dev foo clsact# first try # tc qdisc replace dev foo clsact# second one RTNETLINK answers: Invalid argument After: # tc qdisc replace dev foo clsact # tc qdisc replace dev foo clsact # tc qdisc replace dev foo clsact Signed-off-by: Daniel Borkmann--- tc/q_clsact.c | 1 - tc/q_ingress.c | 1 - 2 files changed, 2 deletions(-) diff --git a/tc/q_clsact.c b/tc/q_clsact.c index 0c05dbd..e2a1a71 100644 --- a/tc/q_clsact.c +++ b/tc/q_clsact.c @@ -18,7 +18,6 @@ static int clsact_parse_opt(struct qdisc_util *qu, int argc, char **argv, return -1; } - addattr_l(n, 1024, TCA_OPTIONS, NULL, 0); return 0; } diff --git a/tc/q_ingress.c b/tc/q_ingress.c index c3c9b40..31699a8 100644 --- a/tc/q_ingress.c +++ b/tc/q_ingress.c @@ -34,7 +34,6 @@ static int ingress_parse_opt(struct qdisc_util *qu, int argc, char **argv, } } - addattr_l(n, 1024, TCA_OPTIONS, NULL, 0); return 0; } -- 1.9.3