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

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

Hi Phil,

On 3/13/19 4:06 PM, Phil Sutter wrote:
> Hi Fernando,
> 
> On Wed, Mar 13, 2019 at 03:15:51PM +0100, Fernando Fernandez Mancera wrote:
>> 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?
> 
> I think reducing redundancy is always a good thing. Only having to
> specify the string and extracting the required info from it would make
> it easier for users I guess.
> 
> That whole string is sent to the kernel, right? So it wouldn't make
> sense to split the fields it is made up from into separate properties in
> JSON, correct?
> 
> Thanks, Phil
> 

Yes, that makes sense. In this case, we don't need flags support anymore
so it reduces the patch series. Should we continue with the
implementation of the flags support or just forget about it until needed
again?