Re: [net-next PATCH iproute2 v2 1/1] tc: introduce IFE action

[Phil Sutter <phil@nwl.cc>]

On Thu, Mar 10, 2016 at 07:42:29AM -0500, Jamal Hadi Salim wrote:
> On 16-03-09 08:12 AM, Phil Sutter wrote:
> > On Wed, Mar 09, 2016 at 07:04:36AM -0500, Jamal Hadi Salim wrote:
> >> +static void ife_explain(void)
> >> +{
> >> +	fprintf(stderr,
> >> +		"Usage:... ife {decode|encode} {ALLOW|USE} [dst DMAC] [src SMAC] [type TYPE] [CONTROL] [index INDEX]\n");
> >
> > I'm just nitpicking here, but this syntax implies that ALLOW and USE are
> > mandatory and mutually exclusive. Looking at the code they're neither
> > (although specifying both might not make sense). OTOH you could probably
> > 'use mark' and 'allow prio'. So I'd suggest '[ALLOW] [USE]' instead.
> >
> 
> Maybe we need to come up with some consistent regex/bnf
> scheme. And then lets review all usage code and fix. Here are the rules
> for this case:
> Default: "allow" will be used for all metadata.
> You can specify zero or more "allows", one per metadata
> You can specify zero or more "use", one per metadata.
> 
> This is why my thinking was it was going to read
> [ALLOW|USE] which needed to read as [ALLOW|USE]*

Sure, this is still overly simplified. Using syntactically correct BNF
in help texts though is not worth the effort IMHO, as readability will
suffer tremendously.

> Example, I dont see "{..}" making good sense in any bnf/regex scheme.

They way I interpreted it so far (and is apparently used) is as a way of
having an XOR. E.g. 'bla { foo | bar }' is valid for 'bla foo' and 'bla
bar', but not just 'bla' or 'bla foo bar'.

Assuming that it's possible for allow/use statements to override
previous ones (the code permits that), correct BNF would probably look
like this:

| ife {decode|encode} [AULIST] [dst DMAC] [src SMAC] [type TYPE] [CONTROL] [index INDEX]
|
| AULIST := [AULIST] AU
| AU := {allow|use} {mark|prio}

Of course there are different BNF variants and AFAICT the syntax used in
iproute2 doesn't cleanly stick to any of them. So speaking of 'correct'
is somewhat nonsensical when I define the rules at the same time. :)

> > I'm missing a list of what actual keywords can be given, but after all
> > this is just a help text and a man page will provide much more detail
> > anyway (which you are about to submit too, are you? :).
> >
> 
> Phil, Phil - someday, yes ;->

I'll keep that in mind! But seriously, if we treated man page (updates)
as a substantial part of new features or code changes in general,
quality was higher - not only of documentation, but code as well.

> > Apart from that:
> >
> > Acked-by: Phil Sutter <phil@nwl.cc>
> >
> 
> Lets please get this in and we can then do a general check on all.

Famous last words? *SCNR*

Thanks, Phil