Re: [PATCHv2 iproute2 net-next] tc: m_mirred: Fix parsing of 'index' optional argument

[Stephen Hemminger]

On Thu, 27 Oct 2016 22:00:40 +0300
Shmulik Ladkani  wrote:

> Hi,
> 
> On Thu, 27 Oct 2016 16:56:43 +0200 Phil Sutter  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, , , 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.

Re: [PATCHv2 iproute2 net-next] tc: m_mirred: Fix parsing of 'index' optional argument

[Shmulik Ladkani]

Hi,

On Thu, 27 Oct 2016 16:56:43 +0200 Phil Sutter  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, , , 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?

Re: [PATCHv2 iproute2 net-next] tc: m_mirred: Fix parsing of 'index' optional argument

[Phil Sutter]

On Thu, Oct 27, 2016 at 05:22:39PM +0300, Shmulik Ladkani wrote:
> Hi Phil,
> 
> On Thu, 27 Oct 2016 11:46:33 +0200, p...@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   [index INDEX] 
> > 
> > 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, , , 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

Re: [PATCHv2 iproute2 net-next] tc: m_mirred: Fix parsing of 'index' optional argument

[Phil Sutter]

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   [index INDEX] 

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

Re: [PATCHv2 iproute2 net-next] tc: m_mirred: Fix parsing of 'index' optional argument

[Shmulik Ladkani]

Hi Phil,

On Thu, 27 Oct 2016 11:46:33 +0200, p...@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   [index INDEX] 
> 
> 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, , , 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.

[PATCHv2 iproute2 net-next] tc: m_mirred: Fix parsing of 'index' optional argument

[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 
---
 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