netdev - Re: [PATCH 1/4] 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <6c1980f7-9cdb-4443-830d-1d76dc8e2dd6@wanadoo.fr>
Date: Thu, 24 Apr 2025 23:23:59 +0900
From: Vincent Mailhol <mailhol.vincent@...adoo.fr>
To: Felix Maurer <fmaurer@...hat.com>
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 1/4] selftests: can: Import tst-filter from can-tests

On 24/04/2025 at 23:02, Felix Maurer wrote:
> On 24.04.25 09:42, Vincent Mailhol wrote:
>> On Tue. 22 Apr. 2025 at 21:08, Felix Maurer <fmaurer@...hat.com> wrote:
> [...]
>>> +ALL_TESTS="
>>> +       test_raw_filter
>>> +"
>>> +
>>> +net_dir=$(dirname $0)/..
>>> +source $net_dir/lib.sh
>>> +
>>> +VCANIF="vcan0"
>>
>> Here, you are making the VCANIF variable configuration, but then, in
>> your test_raw_filter.c I see:
>>
>>   #define VCANIF "vcan0"
>>
>> This means that in order to modify the interface, one would have to
>> both modify the .sh script and the .c source. Wouldn't it be possible
>> to centralize this? For example by reading the environment variable in
>> the C file?
>>
>> Or maybe there is a smarter way to pass values in the kernel selftests
>> framework which I am not aware of?
> 
> Good point, I'll try to come up with something to avoid the duplication
> (either from the selftest framework or just for the CAN tests). I'd
> prefer an argument to the program though, as I find this the more usual
> way to pass info if one ever wants to run the test directly.

Passing an argument would be the best. I am not sure how to do this with the
selftests (but I did not investigate either).

>>> +setup()
>>> +{
>>> +       ip link add name $VCANIF type vcan || exit $ksft_skip
>>> +       ip link set dev $VCANIF up
>>> +       pwd
>>> +}

Speaking of which, if you allow the user to modify the interface, then you will
one additional check here to see whether it is a virtual can interface or not
(the ip link commands are not the same for the vcan and the physical can).

Something like:

  CANIF="${CANIF:-vcan}"
  BITRATE="${BITRATE:-500000}"

  setup()
  {
  	if [ $CANIF == vcan* ]; then
  		ip link add name $CANIF type vcan || exit $ksft_skip
  	else
  		ip link set dev $CANIF type can $BITRATE 500000
  	fi
  	ip link set dev $VCANIF up
  	pwd
  }

>>> +cleanup()
>>> +{
>>> +       ip link delete $VCANIF
>>> +}
>>
>> I guess that this setup() and this cleanup() is something that you
>> will also need in the other can tests. Would it make sense to declare
>> these in a common.sh file and just do a
>>
>>   source common.sh
>>
>> here?
> 
> I usually try to avoid making changes in anticipation of the future. I'm
> not sure if all the tests need a similar environment and would prefer to
> split this when we encounter that they do. Are you okay with that?

Yes, this works. Keep this idea in back of your mind and if there is a need to
reuse those in the future, then it will be a good timing to do the factorize the
code.


Yours sincerely,
Vincent Mailhol