netdev - Re: [PATCH v2] selftests: can: Import tst-filter from can-tests

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] [day] [month] [year] [list]

Message-ID: <96837efb-63ac-4191-8e2a-4785672c8d7a@redhat.com>
Date: Wed, 21 May 2025 13:39:15 +0200
From: Felix Maurer <fmaurer@...hat.com>
To: Vincent Mailhol <mailhol.vincent@...adoo.fr>
Cc: socketcan@...tkopp.net, mkl@...gutronix.de, shuah@...nel.org,
 davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
 pabeni@...hat.com, horms@...nel.org, linux-can@...r.kernel.org,
 netdev@...r.kernel.org, linux-kselftest@...r.kernel.org,
 dcaratti@...hat.com, fstornio@...hat.com
Subject: Re: [PATCH v2] selftests: can: Import tst-filter from can-tests

Hi Vincent,

On 14.05.25 11:47, Vincent Mailhol wrote:
> Hi Felix,
> 
> On Sat. 10 May 2025 at 00:07, Felix Maurer <fmaurer@...hat.com> wrote:
>> Tests for the can subsystem have been in the can-tests repository[1] so
>> far. Start moving the tests to kernel selftests by importing the current
>> tst-filter test. The test is now named test_raw_filter and is substantially
>> updated to be more aligned with the kernel selftests, follow the coding
>> style, and simplify the validation of received CAN frames. We also include
>> documentation of the test design. The test verifies that the single filters
>> on raw CAN sockets work as expected.
>>
>> We intend to import more tests from can-tests and add additional test cases
>> in the future. The goal of moving the CAN selftests into the tree is to
>> align the tests more closely with the kernel, improve testing of CAN in
>> general, and to simplify running the tests automatically in the various
>> kernel CI systems.
>>
>> [1]: https://github.com/linux-can/can-tests
>>
>> Signed-off-by: Felix Maurer <fmaurer@...hat.com>
> 
> Thanks again.
> 
> I left a set of nitpicks, I expect to give my reviewed-by tag on the
> next version.

Thank you for your feedback. I'll post a new version with the changes
included soon.

[...]
>> +FIXTURE_SETUP(can_filters)
>> +{
>> +       struct sockaddr_can addr;
>> +       struct ifreq ifr;
>> +       int recv_own_msgs = 1;
>> +       int s, ret;
>> +
>> +       s = socket(PF_CAN, SOCK_RAW, CAN_RAW);
>> +       ASSERT_LT(0, s)
> 
> 0 is a valid fd (OK it is used for the stout, so your code will work,
> but the comparison still looks unnatural).
> 
> What about:
> 
>   ASSERT_NE(s, -1)
> 
> or:
> 
>   ASSERT_GE(s, 0)
> 
> ?
> 
> (same comment for the other ASSERT_LE)

I was a bit hesitant to change the order of expected and seen value for
the the assertions because it's documented as ASSERT_*(expected, seen).
But it seems to be common in the selftest to not follow this order where
assertions are used for error checking and failure message doesn't
explicitly say what was expected/seen. I'll take a look at the error
checking in the whole file where the more familiar form is with reversed
arguments.

Thanks,
   Felix