* [PATCHv2 iproute2 net-next] tc: m_mirred: Fix parsing of 'index' optional argument
@ 2016-10-27 7:36 Shmulik Ladkani
2016-10-27 9:46 ` Phil Sutter
0 siblings, 1 reply; 6+ messages in thread
From: Shmulik Ladkani @ 2016-10-27 7:36 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: Jamal Hadi Salim, netdev, Shmulik Ladkani
Code in parse_mirred() suggests "index" argument can be placed either
after the egress/ingress clause, or as the first argument (after
"action mirred").
However, parse_direction() fails to correctly parse "index" if it's the
first argument.
For example:
# tc filter add ... action mirred index 5
RTNETLINK answers: Invalid argument
(unnecessary RTNETLINK issued, should have been parse error)
# tc filter add ... action mirred index 5 egress redirect dev eth0
bad action type egress
(should have been parsed successfully)
Fix parse_direction as follows:
- continue parsing after valid "index" is seen
- don't issue the RTNETLINK unless valid "egress"/"ingress" is seen
Signed-off-by: Shmulik Ladkani <shmulik.ladkani@gmail.com>
---
v2: rebased to recent tip of net-next, amended log message
An alternative solution: banning "index" as 1st argument in parse_mirred
tc/m_mirred.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/tc/m_mirred.c b/tc/m_mirred.c
index 01f916d..64d870a 100644
--- a/tc/m_mirred.c
+++ b/tc/m_mirred.c
@@ -105,9 +105,8 @@ parse_direction(struct action_util *a, int *argc_p, char ***argv_p,
}
iok++;
if (!ok) {
- argc--;
- argv++;
- break;
+ NEXT_ARG();
+ continue;
}
} else if (!ok) {
fprintf(stderr, "was expecting egress or ingress (%s)\n", *argv);
@@ -150,7 +149,7 @@ parse_direction(struct action_util *a, int *argc_p, char ***argv_p,
NEXT_ARG();
}
- if (!ok && !iok) {
+ if (!ok) {
return -1;
}
--
2.10.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCHv2 iproute2 net-next] tc: m_mirred: Fix parsing of 'index' optional argument
2016-10-27 7:36 [PATCHv2 iproute2 net-next] tc: m_mirred: Fix parsing of 'index' optional argument Shmulik Ladkani
@ 2016-10-27 9:46 ` Phil Sutter
2016-10-27 14:22 ` Shmulik Ladkani
0 siblings, 1 reply; 6+ messages in thread
From: Phil Sutter @ 2016-10-27 9:46 UTC (permalink / raw)
To: Shmulik Ladkani; +Cc: Stephen Hemminger, Jamal Hadi Salim, netdev
Hi,
On Thu, Oct 27, 2016 at 10:36:06AM +0300, Shmulik Ladkani wrote:
> Code in parse_mirred() suggests "index" argument can be placed either
> after the egress/ingress clause, or as the first argument (after
> "action mirred").
>
> However, parse_direction() fails to correctly parse "index" if it's the
> first argument.
>
> For example:
>
> # tc filter add ... action mirred index 5
> RTNETLINK answers: Invalid argument
> (unnecessary RTNETLINK issued, should have been parse error)
>
> # tc filter add ... action mirred index 5 egress redirect dev eth0
> bad action type egress
> (should have been parsed successfully)
According to the action's help text (and the man page which is based
upon that), this behaviour is perfectly fine:
| Usage: mirred <DIRECTION> <ACTION> [index INDEX] <dev DEVICENAME>
So first argument *must* be the direction, second one *must* be the
action, then an optional index and the last one *must* be the interface.
While I don't see a problem with changing that (apart from that I don't
think it's necessary), if you change it you should adjust help text and
man page accordingly.
But is all this really worth it? And if so, why not fix the parser for
good by delaying the whole validation (apart from double arg detection)
until after the parsing itself?
Cheers, Phil
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCHv2 iproute2 net-next] tc: m_mirred: Fix parsing of 'index' optional argument
2016-10-27 9:46 ` Phil Sutter
@ 2016-10-27 14:22 ` Shmulik Ladkani
2016-10-27 14:56 ` Phil Sutter
0 siblings, 1 reply; 6+ messages in thread
From: Shmulik Ladkani @ 2016-10-27 14:22 UTC (permalink / raw)
To: Phil Sutter; +Cc: Stephen Hemminger, Jamal Hadi Salim, netdev
Hi Phil,
On Thu, 27 Oct 2016 11:46:33 +0200, phil@nwl.cc wrote:
> According to the action's help text (and the man page which is based
> upon that), this behaviour is perfectly fine:
>
> | Usage: mirred <DIRECTION> <ACTION> [index INDEX] <dev DEVICENAME>
>
> So first argument *must* be the direction, second one *must* be the
> action, then an optional index and the last one *must* be the interface.
There is an inconsistency betweem man/help and code.
Actual code, since first committed, attempts to parse "index" as 1st
argument (without success), see parse_mirred():
if (matches(*argv, "egress") == 0 || matches(*argv, "index") == 0) {
int ret = parse_egress(a, &argc, &argv, tca_id, n);
> While I don't see a problem with changing that (apart from that I don't
> think it's necessary)
Not "changing" per-se, but rather "fixing", at least according code
author's intention :)
As I suggested in the notes part of the commit log,
>> An alternative solution: banning "index" as 1st argument in parse_mirred
I ok with removing the code trying to support "index" as 1st argument as
well.
I only assumed one might want this behaviour due to intention expressed
by original code.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCHv2 iproute2 net-next] tc: m_mirred: Fix parsing of 'index' optional argument
2016-10-27 14:22 ` Shmulik Ladkani
@ 2016-10-27 14:56 ` Phil Sutter
2016-10-27 19:00 ` Shmulik Ladkani
0 siblings, 1 reply; 6+ messages in thread
From: Phil Sutter @ 2016-10-27 14:56 UTC (permalink / raw)
To: Shmulik Ladkani; +Cc: Stephen Hemminger, Jamal Hadi Salim, netdev
On Thu, Oct 27, 2016 at 05:22:39PM +0300, Shmulik Ladkani wrote:
> Hi Phil,
>
> On Thu, 27 Oct 2016 11:46:33 +0200, phil@nwl.cc wrote:
> > According to the action's help text (and the man page which is based
> > upon that), this behaviour is perfectly fine:
> >
> > | Usage: mirred <DIRECTION> <ACTION> [index INDEX] <dev DEVICENAME>
> >
> > So first argument *must* be the direction, second one *must* be the
> > action, then an optional index and the last one *must* be the interface.
>
> There is an inconsistency betweem man/help and code.
>
> Actual code, since first committed, attempts to parse "index" as 1st
> argument (without success), see parse_mirred():
>
> if (matches(*argv, "egress") == 0 || matches(*argv, "index") == 0) {
> int ret = parse_egress(a, &argc, &argv, tca_id, n);
Oh, I missed that! But to me this looks like the author wanted to avoid
erroring out with "mirred option not supported index" in case of missing
'egress' keyword.
>From that perspective, I think parse_direction() really should be
removed and it's content made part of parse_mirred() itself.
> > While I don't see a problem with changing that (apart from that I don't
> > think it's necessary)
>
> Not "changing" per-se, but rather "fixing", at least according code
> author's intention :)
I still think we don't fully know the author's intention. :)
> As I suggested in the notes part of the commit log,
>
> >> An alternative solution: banning "index" as 1st argument in parse_mirred
>
> I ok with removing the code trying to support "index" as 1st argument as
> well.
> I only assumed one might want this behaviour due to intention expressed
> by original code.
Yeah, I'd go with least effort approach, i.e. not adding any additional
flexibility in arg parsing. Since the docs never stated otherwise, I
don't think it was a real issue for users.
Cheers, Phil
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCHv2 iproute2 net-next] tc: m_mirred: Fix parsing of 'index' optional argument
2016-10-27 14:56 ` Phil Sutter
@ 2016-10-27 19:00 ` Shmulik Ladkani
2016-11-29 20:34 ` Stephen Hemminger
0 siblings, 1 reply; 6+ messages in thread
From: Shmulik Ladkani @ 2016-10-27 19:00 UTC (permalink / raw)
To: Phil Sutter, Stephen Hemminger, Jamal Hadi Salim; +Cc: netdev
Hi,
On Thu, 27 Oct 2016 16:56:43 +0200 Phil Sutter <phil@nwl.cc> wrote:
> > Actual code, since first committed, attempts to parse "index" as 1st
> > argument (without success), see parse_mirred():
> >
> > if (matches(*argv, "egress") == 0 || matches(*argv, "index") == 0) {
> > int ret = parse_egress(a, &argc, &argv, tca_id, n);
>
> Oh, I missed that! But to me this looks like the author wanted to avoid
> erroring out with "mirred option not supported index" in case of missing
> 'egress' keyword.
Could be; Not that it matters much, but evidence in parse_egress() shows
that it DOES try to parse and store "index" EVEN if no "egress" seen YET.
> Yeah, I'd go with least effort approach, i.e. not adding any additional
> flexibility in arg parsing. Since the docs never stated otherwise, I
> don't think it was a real issue for users.
Sure. It never really worked ;)
If we go that way, then some code in parse_direction needs to be
eliminated/restructured; So we'll end up with:
- less code in parse_direction
- not adding any additional flexibility in arg parsing
- bigger diff than suggested
Stephen, Jamal, do you have any preference here?
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCHv2 iproute2 net-next] tc: m_mirred: Fix parsing of 'index' optional argument
2016-10-27 19:00 ` Shmulik Ladkani
@ 2016-11-29 20:34 ` Stephen Hemminger
0 siblings, 0 replies; 6+ messages in thread
From: Stephen Hemminger @ 2016-11-29 20:34 UTC (permalink / raw)
To: Shmulik Ladkani; +Cc: Phil Sutter, Jamal Hadi Salim, netdev
On Thu, 27 Oct 2016 22:00:40 +0300
Shmulik Ladkani <shmulik.ladkani@gmail.com> wrote:
> Hi,
>
> On Thu, 27 Oct 2016 16:56:43 +0200 Phil Sutter <phil@nwl.cc> wrote:
> > > Actual code, since first committed, attempts to parse "index" as 1st
> > > argument (without success), see parse_mirred():
> > >
> > > if (matches(*argv, "egress") == 0 || matches(*argv, "index") == 0) {
> > > int ret = parse_egress(a, &argc, &argv, tca_id, n);
> >
> > Oh, I missed that! But to me this looks like the author wanted to avoid
> > erroring out with "mirred option not supported index" in case of missing
> > 'egress' keyword.
>
> Could be; Not that it matters much, but evidence in parse_egress() shows
> that it DOES try to parse and store "index" EVEN if no "egress" seen YET.
>
> > Yeah, I'd go with least effort approach, i.e. not adding any additional
> > flexibility in arg parsing. Since the docs never stated otherwise, I
> > don't think it was a real issue for users.
>
> Sure. It never really worked ;)
>
> If we go that way, then some code in parse_direction needs to be
> eliminated/restructured; So we'll end up with:
> - less code in parse_direction
> - not adding any additional flexibility in arg parsing
> - bigger diff than suggested
>
> Stephen, Jamal, do you have any preference here?
My only demand is not to break existing users.
Jamal is the original developer of mirred so I would like his feedback.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-11-29 20:34 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-10-27 7:36 [PATCHv2 iproute2 net-next] tc: m_mirred: Fix parsing of 'index' optional argument Shmulik Ladkani
2016-10-27 9:46 ` Phil Sutter
2016-10-27 14:22 ` Shmulik Ladkani
2016-10-27 14:56 ` Phil Sutter
2016-10-27 19:00 ` Shmulik Ladkani
2016-11-29 20:34 ` Stephen Hemminger
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).