Re: [PATCH nft v2 1/6] osf: add version fingerprint support

From: Phil Sutter <phil@nwl.cc>
To: Fernando Fernandez Mancera <ffmancera@riseup.net>
Cc: netfilter-devel@vger.kernel.org
Subject: Re: [PATCH nft v2 1/6] osf: add version fingerprint support
Date: Wed, 13 Mar 2019 12:27:33 +0100	[thread overview]
Message-ID: <20190313112733.GL4851@orbyte.nwl.cc> (raw)
In-Reply-To: <b1d98d09-3cf7-3de1-1fb3-479244da560b@riseup.net>

On Wed, Mar 13, 2019 at 11:14:04AM +0100, Fernando Fernandez Mancera wrote:
> Hi Phil,
> 
> On 3/13/19 10:44 AM, Phil Sutter wrote:
> > Hi Fernando,
> > 
> > On Mon, Mar 11, 2019 at 04:14:12PM +0100, Fernando Fernandez Mancera wrote:
> >> Add support for version fingerprint in "osf" expression. Example:
> >>
> >> table ip foo {
> >> 	chain bar {
> >> 		type filter hook input priority filter; policy accept;
> >> 		osf ttl skip name "Linux"
> >> 		osf ttl skip name version "Linux:4.20"
> >> 	}
> >> }
> > 
> > The syntax seems overly complicated to me, although I'm not really
> > familiar with OSF so may lack background knowledge. Any reason why you
> > didn't go with 'osf ttl skip name "Linux" version "4.20"' instead?
> > 
> 
> You are right, 'osf ttl skip name "Linux" version "4.20"' was my first
> thought but in compilation time the parser applies shift-reduce to the
> expression.. I decided 'osf ttl skip name version "Linux:4.20"' to avoid
> a complex workaround in the parser.

Shift/reduce warnings often require voodoo to fix, but it's not
impossible. :)

Regarding my suggestion, I see that this string is actually the
right-hand-side of a relational expression. To implement what I had in
mind you would have to turn osf expression into a statement.

> The fingerprints database syntax is "genre:version:subtype:details" so
> the nft 'osf' expression syntax is like the original one.

Can we deduce required flags from the given string on RHS? I.e. by
looking at the amount of semi-colons and the number of characters
between them? I'm assuming the syntax works like "genre::subtype" and
"genre:::details" to omit certain parts, is that correct?

> > Also with regards to your patch to json_parser, I guess you should
> > introduce an enum for flag values, something like:
> > 
> > | enum osf_flags {
> > | 	OSF_FLAG_INVALID = 0x0,
> > | 	OSF_FLAG_VERSION = 0x1
> > | };
> > | 
> > | const char *osf_flag_names[] = {
> > | 	[OSF_VERSION] = "version"
> > | };
> > 
> > What do you think?
> > 
> 
> This patch already introduces an enum for flags values, you can find it
> below. Do you think we need another one? Sorry if I am misunderstanding
> you. Thanks!

Oh, I missed that one. My concern is how the index of values in
osf_flags array defined in osf_expr_json() and osf_flag_parse() is
relevant although it doesn't seem so when only looking at the array
definition.

Cheers, Phil