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

[Fernando Fernandez Mancera <ffmancera@riseup.net>]

On 3/13/19 12:27 PM, Phil Sutter wrote:
> 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?
> 

Yes that is correct. We can do that if you think it is more suitable. Do
we all agree then?

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

Yes, we can use it in osf_expr_json(). I am going to work in a v3 patch
series if it looks fine to you. Thanks!

> Cheers, Phil
>