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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160310131240.GA19221@orbyte.nwl.cc>
Date:	Thu, 10 Mar 2016 14:12:40 +0100
From:	Phil Sutter <phil@....cc>
To:	Jamal Hadi Salim <jhs@...atatu.com>
Cc:	shemming@...cade.com, netdev@...r.kernel.org
Subject: Re: [net-next PATCH iproute2 v2 1/1] tc: introduce IFE action

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@....cc>
> >
> 
> Lets please get this in and we can then do a general check on all.

Famous last words? *SCNR*

Thanks, Phil