| Message ID | 20161027073606.6112-1-shmulik.ladkani@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | stephen hemminger |
| Headers | show |
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
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.
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
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?
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.
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; }
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(-)