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

[Phil Sutter <phil@nwl.cc>]

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