Re: [PATCH net-next] selftests: fib offload: use sensible tos values

[Shuah Khan <skhan@linuxfoundation.org>]

On 2/2/22 1:16 PM, Guillaume Nault wrote:
> On Wed, Feb 02, 2022 at 12:46:10PM -0700, Shuah Khan wrote:
>> On 2/2/22 11:30 AM, Guillaume Nault wrote:
>>> Although both iproute2 and the kernel accept 1 and 2 as tos values for
>>> new routes, those are invalid. These values only set ECN bits, which
>>> are ignored during IPv4 fib lookups. Therefore, no packet can actually
>>> match such routes. This selftest therefore only succeeds because it
>>> doesn't verify that the new routes do actually work in practice (it
>>> just checks if the routes are offloaded or not).
>>>
>>> It makes more sense to use tos values that don't conflict with ECN.
>>> This way, the selftest won't be affected if we later decide to warn or
>>> even reject invalid tos configurations for new routes.
>>
>> Wouldn't it make sense to leave these invalid values in the test though.
>> Removing these makes this test out of sync withe kernel.
> 
> Do you mean keeping the test as is and only modify it when (if) we
> decide to reject such invalid values?

This is for sure. Remove the invalid values in sync with the kernel code.

> Or to write two versions of the
> test, one with invalid values, the other with correct ones?
> 

This one makes sense if it adds value in testing to make sure we continue
to reject invalid values.

> I don't get what keeping a test with the invalid values could bring us.
> It's confusing for the reader, and might break in the future. This
> patch makes the test future proof, without altering its intent and code
> coverage. It still works on current (and past) kernels, so I don't see
> what this patch could make out of sync.
> 

If kernel still accepts these values, then the test is valid as long as
kernel still doesn't flag these values as invalid.

I might be missing something. Don't you want to test with invalid values
so make sure they are indeed rejected?

thanks,
-- Shuah