This is the initial import of a CAN selftest from can-tests[1] into the tree. For now, it is just a single test but when agreed on the structure, we intend to import more tests from can-tests and add additional test cases.
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.
I have cc'ed netdev and its reviewers and maintainers to make sure they are okay with the location of the tests and the changes to the paths in MAINTAINERS. The changes should be merged through linux-can-next and subsequent changes will not go to netdev anymore.
[1]: https://github.com/linux-can/can-tests
Felix Maurer (4): selftests: can: Import tst-filter from can-tests selftests: can: use kselftest harness in test_raw_filter selftests: can: Use fixtures in test_raw_filter selftests: can: Document test_raw_filter test cases
MAINTAINERS | 2 + tools/testing/selftests/Makefile | 1 + tools/testing/selftests/net/can/.gitignore | 2 + tools/testing/selftests/net/can/Makefile | 11 + .../selftests/net/can/test_raw_filter.c | 338 ++++++++++++++++++ .../selftests/net/can/test_raw_filter.sh | 37 ++ 6 files changed, 391 insertions(+) create mode 100644 tools/testing/selftests/net/can/.gitignore create mode 100644 tools/testing/selftests/net/can/Makefile create mode 100644 tools/testing/selftests/net/can/test_raw_filter.c create mode 100755 tools/testing/selftests/net/can/test_raw_filter.sh
Tests for the can subsytem have been in the can-tests repository[1] so far. Start moving the tests to kernel selftests by importing the current tst-filter test. Subsequent patches will update the test to be more aligned with the kernel selftests and follow the coding style.
The imported test verifies that the single filters on raw CAN sockets work as expected.
[1]: https://github.com/linux-can/can-tests
Signed-off-by: Felix Maurer fmaurer@redhat.com ---
Notes: I have removed the long form of the licenses in the beginning of the file during the import, as that is covered by the SPDX line anyways. The copyright is left as it was originally.
MAINTAINERS | 2 + tools/testing/selftests/Makefile | 1 + tools/testing/selftests/net/can/.gitignore | 2 + tools/testing/selftests/net/can/Makefile | 9 + .../selftests/net/can/test_raw_filter.c | 204 ++++++++++++++++++ .../selftests/net/can/test_raw_filter.sh | 37 ++++ 6 files changed, 255 insertions(+) create mode 100644 tools/testing/selftests/net/can/.gitignore create mode 100644 tools/testing/selftests/net/can/Makefile create mode 100644 tools/testing/selftests/net/can/test_raw_filter.c create mode 100755 tools/testing/selftests/net/can/test_raw_filter.sh
diff --git a/MAINTAINERS b/MAINTAINERS index 241ca9e260a2..55749b492ebb 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -5155,6 +5155,7 @@ F: include/uapi/linux/can/isotp.h F: include/uapi/linux/can/raw.h F: net/can/ F: net/sched/em_canid.c +F: tools/testing/selftests/net/can/
CAN-J1939 NETWORK LAYER M: Robin van der Gracht robin@protonic.nl @@ -16577,6 +16578,7 @@ X: net/ceph/ X: net/mac80211/ X: net/rfkill/ X: net/wireless/ +X: tools/testing/selftests/net/can/
NETWORKING [IPSEC] M: Steffen Klassert steffen.klassert@secunet.com diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile index 8daac70c2f9d..e5c9ecd52b73 100644 --- a/tools/testing/selftests/Makefile +++ b/tools/testing/selftests/Makefile @@ -64,6 +64,7 @@ TARGETS += mqueue TARGETS += nci TARGETS += net TARGETS += net/af_unix +TARGETS += net/can TARGETS += net/forwarding TARGETS += net/hsr TARGETS += net/mptcp diff --git a/tools/testing/selftests/net/can/.gitignore b/tools/testing/selftests/net/can/.gitignore new file mode 100644 index 000000000000..764a53fc837f --- /dev/null +++ b/tools/testing/selftests/net/can/.gitignore @@ -0,0 +1,2 @@ +# SPDX-License-Identifier: GPL-2.0-only +test_raw_filter diff --git a/tools/testing/selftests/net/can/Makefile b/tools/testing/selftests/net/can/Makefile new file mode 100644 index 000000000000..44ef37f064ad --- /dev/null +++ b/tools/testing/selftests/net/can/Makefile @@ -0,0 +1,9 @@ +# SPDX-License-Identifier: GPL-2.0 + +top_srcdir = ../../../../.. + +TEST_PROGS := test_raw_filter.sh + +TEST_GEN_FILES := test_raw_filter + +include ../../lib.mk diff --git a/tools/testing/selftests/net/can/test_raw_filter.c b/tools/testing/selftests/net/can/test_raw_filter.c new file mode 100644 index 000000000000..c84260f36565 --- /dev/null +++ b/tools/testing/selftests/net/can/test_raw_filter.c @@ -0,0 +1,204 @@ +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-3-Clause) +/* + * Copyright (c) 2011 Volkswagen Group Electronic Research + * All rights reserved. + */ + +#include <stdio.h> +#include <stdlib.h> +#include <unistd.h> +#include <string.h> + +#include <sys/types.h> +#include <sys/socket.h> +#include <sys/ioctl.h> +#include <sys/time.h> +#include <net/if.h> + +#include <linux/can.h> +#include <linux/can/raw.h> + +#define ID 0x123 +#define TC 18 /* # of testcases */ + +const int rx_res[TC] = {4, 4, 4, 4, 2, 2, 2, 2, 2, 2, 2, 2, 1, 1, 1, 1, 1, 1}; +const int rxbits_res[TC] = {4369, 4369, 4369, 4369, 17, 4352, 17, 4352, 257, 257, 4112, 4112, 1, 256, 16, 4096, 1, 256}; + +#define VCANIF "vcan0" + +canid_t calc_id(int testcase) +{ + canid_t id = ID; + + if (testcase & 1) + id |= CAN_EFF_FLAG; + if (testcase & 2) + id |= CAN_RTR_FLAG; + + return id; +} + +canid_t calc_mask(int testcase) +{ + canid_t mask = CAN_SFF_MASK; + + if (testcase > 15) + return (CAN_EFF_MASK | CAN_EFF_FLAG | CAN_RTR_FLAG); + + if (testcase & 4) + mask |= CAN_EFF_FLAG; + if (testcase & 8) + mask |= CAN_RTR_FLAG; + + return mask; +} + +int main(int argc, char **argv) +{ + fd_set rdfs; + struct timeval tv; + int s; + struct sockaddr_can addr; + struct can_filter rfilter; + struct can_frame frame; + int testcase; + int have_rx; + int rx; + int rxbits, rxbitval; + int ret; + int recv_own_msgs = 1; + int err = 0; + struct ifreq ifr; + + if ((s = socket(PF_CAN, SOCK_RAW, CAN_RAW)) < 0) { + perror("socket"); + err = 1; + goto out; + } + + strcpy(ifr.ifr_name, VCANIF); + if (ioctl(s, SIOCGIFINDEX, &ifr) < 0) { + perror("SIOCGIFINDEX"); + err = 1; + goto out_socket; + } + addr.can_family = AF_CAN; + addr.can_ifindex = ifr.ifr_ifindex; + + setsockopt(s, SOL_CAN_RAW, CAN_RAW_RECV_OWN_MSGS, + &recv_own_msgs, sizeof(recv_own_msgs)); + + if (bind(s, (struct sockaddr *)&addr, sizeof(addr)) < 0) { + perror("bind"); + err = 1; + goto out_socket; + } + + printf("---\n"); + + for (testcase = 0; testcase < TC; testcase++) { + + rfilter.can_id = calc_id(testcase); + rfilter.can_mask = calc_mask(testcase); + setsockopt(s, SOL_CAN_RAW, CAN_RAW_FILTER, + &rfilter, sizeof(rfilter)); + + printf("testcase %2d filters : can_id = 0x%08X can_mask = 0x%08X\n", + testcase, rfilter.can_id, rfilter.can_mask); + + printf("testcase %2d sending patterns ... ", testcase); + + frame.can_dlc = 1; + frame.data[0] = testcase; + + frame.can_id = ID; + if (write(s, &frame, sizeof(frame)) < 0) { + perror("write"); + exit(1); + } + frame.can_id = (ID | CAN_RTR_FLAG); + if (write(s, &frame, sizeof(frame)) < 0) { + perror("write"); + exit(1); + } + frame.can_id = (ID | CAN_EFF_FLAG); + if (write(s, &frame, sizeof(frame)) < 0) { + perror("write"); + exit(1); + } + frame.can_id = (ID | CAN_EFF_FLAG | CAN_RTR_FLAG); + if (write(s, &frame, sizeof(frame)) < 0) { + perror("write"); + exit(1); + } + + printf("ok\n"); + + have_rx = 1; + rx = 0; + rxbits = 0; + + while (have_rx) { + + have_rx = 0; + FD_ZERO(&rdfs); + FD_SET(s, &rdfs); + tv.tv_sec = 0; + tv.tv_usec = 50000; /* 50ms timeout */ + + ret = select(s+1, &rdfs, NULL, NULL, &tv); + if (ret < 0) { + perror("select"); + exit(1); + } + + if (FD_ISSET(s, &rdfs)) { + have_rx = 1; + ret = read(s, &frame, sizeof(struct can_frame)); + if (ret < 0) { + perror("read"); + exit(1); + } + if ((frame.can_id & CAN_SFF_MASK) != ID) { + fprintf(stderr, "received wrong can_id!\n"); + exit(1); + } + if (frame.data[0] != testcase) { + fprintf(stderr, "received wrong testcase!\n"); + exit(1); + } + + /* test & calc rxbits */ + rxbitval = 1 << ((frame.can_id & (CAN_EFF_FLAG|CAN_RTR_FLAG|CAN_ERR_FLAG)) >> 28); + + /* only receive a rxbitval once */ + if ((rxbits & rxbitval) == rxbitval) { + fprintf(stderr, "received rxbitval %d twice!\n", rxbitval); + exit(1); + } + rxbits |= rxbitval; + rx++; + + printf("testcase %2d rx : can_id = 0x%08X rx = %d rxbits = %d\n", + testcase, frame.can_id, rx, rxbits); + } + } + /* rx timed out -> check the received results */ + if (rx_res[testcase] != rx) { + fprintf(stderr, "wrong rx value in testcase %d : %d (expected %d)\n", + testcase, rx, rx_res[testcase]); + exit(1); + } + if (rxbits_res[testcase] != rxbits) { + fprintf(stderr, "wrong rxbits value in testcase %d : %d (expected %d)\n", + testcase, rxbits, rxbits_res[testcase]); + exit(1); + } + printf("testcase %2d ok\n---\n", testcase); + } + +out_socket: + close(s); +out: + return err; +} diff --git a/tools/testing/selftests/net/can/test_raw_filter.sh b/tools/testing/selftests/net/can/test_raw_filter.sh new file mode 100755 index 000000000000..e5f175c8b27b --- /dev/null +++ b/tools/testing/selftests/net/can/test_raw_filter.sh @@ -0,0 +1,37 @@ +#!/bin/bash +# SPDX-License-Identifier: GPL-2.0 + +#set -x + +ALL_TESTS=" + test_raw_filter +" + +net_dir=$(dirname $0)/.. +source $net_dir/lib.sh + +VCANIF="vcan0" + +setup() +{ + ip link add name $VCANIF type vcan || exit $ksft_skip + ip link set dev $VCANIF up + pwd +} + +cleanup() +{ + ip link delete $VCANIF +} + +test_raw_filter() +{ + ./test_raw_filter +} + +trap cleanup EXIT +setup + +tests_run + +exit $EXIT_STATUS
On Tue. 22 Apr. 2025 at 21:08, Felix Maurer fmaurer@redhat.com wrote:
diff --git a/tools/testing/selftests/net/can/test_raw_filter.sh b/tools/testing/selftests/net/can/test_raw_filter.sh new file mode 100755 index 000000000000..e5f175c8b27b --- /dev/null +++ b/tools/testing/selftests/net/can/test_raw_filter.sh @@ -0,0 +1,37 @@ +#!/bin/bash +# SPDX-License-Identifier: GPL-2.0
+#set -x
Leftover from the debug? Can you remove this line?
+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?
+setup() +{
ip link add name $VCANIF type vcan || exit $ksft_skipip link set dev $VCANIF uppwd+}
+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?
+test_raw_filter() +{
./test_raw_filter+}
+trap cleanup EXIT +setup
+tests_run
+exit $EXIT_STATUS
Yours sincerely, Vincent Mailhol
On 24.04.25 09:42, Vincent Mailhol wrote:
On Tue. 22 Apr. 2025 at 21:08, Felix Maurer fmaurer@redhat.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.
+setup() +{
ip link add name $VCANIF type vcan || exit $ksft_skipip link set dev $VCANIF uppwd+}
+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?
Thanks, Felix
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@redhat.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_skipip link set dev $VCANIF uppwd+}
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
Update test_raw_filter to use the kselftest harness to make the test output conform with TAP. Use the logging and assertations from the harness.
Signed-off-by: Felix Maurer fmaurer@redhat.com --- tools/testing/selftests/net/can/Makefile | 2 + .../selftests/net/can/test_raw_filter.c | 152 ++++++++---------- 2 files changed, 73 insertions(+), 81 deletions(-)
diff --git a/tools/testing/selftests/net/can/Makefile b/tools/testing/selftests/net/can/Makefile index 44ef37f064ad..5b82e60a03e7 100644 --- a/tools/testing/selftests/net/can/Makefile +++ b/tools/testing/selftests/net/can/Makefile @@ -2,6 +2,8 @@
top_srcdir = ../../../../..
+CFLAGS += -Wall -Wl,--no-as-needed -O2 -g -I$(top_srcdir)/usr/include $(KHDR_INCLUDES) + TEST_PROGS := test_raw_filter.sh
TEST_GEN_FILES := test_raw_filter diff --git a/tools/testing/selftests/net/can/test_raw_filter.c b/tools/testing/selftests/net/can/test_raw_filter.c index c84260f36565..7414b9aef823 100644 --- a/tools/testing/selftests/net/can/test_raw_filter.c +++ b/tools/testing/selftests/net/can/test_raw_filter.c @@ -18,6 +18,8 @@ #include <linux/can.h> #include <linux/can/raw.h>
+#include "../../kselftest_harness.h" + #define ID 0x123 #define TC 18 /* # of testcases */
@@ -53,7 +55,38 @@ canid_t calc_mask(int testcase) return mask; }
-int main(int argc, char **argv) +int send_can_frames(int sock, int testcase) +{ + struct can_frame frame; + + frame.can_dlc = 1; + frame.data[0] = testcase; + + frame.can_id = ID; + if (write(sock, &frame, sizeof(frame)) < 0) { + perror("write"); + return 1; + } + frame.can_id = (ID | CAN_RTR_FLAG); + if (write(sock, &frame, sizeof(frame)) < 0) { + perror("write"); + return 1; + } + frame.can_id = (ID | CAN_EFF_FLAG); + if (write(sock, &frame, sizeof(frame)) < 0) { + perror("write"); + return 1; + } + frame.can_id = (ID | CAN_EFF_FLAG | CAN_RTR_FLAG); + if (write(sock, &frame, sizeof(frame)) < 0) { + perror("write"); + return 1; + } + + return 0; +} + +TEST(can_filter) { fd_set rdfs; struct timeval tv; @@ -67,34 +100,26 @@ int main(int argc, char **argv) int rxbits, rxbitval; int ret; int recv_own_msgs = 1; - int err = 0; struct ifreq ifr;
- if ((s = socket(PF_CAN, SOCK_RAW, CAN_RAW)) < 0) { - perror("socket"); - err = 1; - goto out; - } + s = socket(PF_CAN, SOCK_RAW, CAN_RAW); + ASSERT_LT(0, s) + TH_LOG("failed to create CAN_RAW socket (%d)", errno);
strcpy(ifr.ifr_name, VCANIF); - if (ioctl(s, SIOCGIFINDEX, &ifr) < 0) { - perror("SIOCGIFINDEX"); - err = 1; - goto out_socket; - } + ret = ioctl(s, SIOCGIFINDEX, &ifr); + ASSERT_LE(0, ret) + TH_LOG("failed SIOCGIFINDEX (%d)", errno); + addr.can_family = AF_CAN; addr.can_ifindex = ifr.ifr_ifindex;
setsockopt(s, SOL_CAN_RAW, CAN_RAW_RECV_OWN_MSGS, &recv_own_msgs, sizeof(recv_own_msgs));
- if (bind(s, (struct sockaddr *)&addr, sizeof(addr)) < 0) { - perror("bind"); - err = 1; - goto out_socket; - } - - printf("---\n"); + ret = bind(s, (struct sockaddr *)&addr, sizeof(addr)); + ASSERT_EQ(0, ret) + TH_LOG("failed bind socket (%d)", errno);
for (testcase = 0; testcase < TC; testcase++) {
@@ -103,36 +128,14 @@ int main(int argc, char **argv) setsockopt(s, SOL_CAN_RAW, CAN_RAW_FILTER, &rfilter, sizeof(rfilter));
- printf("testcase %2d filters : can_id = 0x%08X can_mask = 0x%08X\n", + TH_LOG("testcase %2d filters : can_id = 0x%08X can_mask = 0x%08X", testcase, rfilter.can_id, rfilter.can_mask);
- printf("testcase %2d sending patterns ... ", testcase); - - frame.can_dlc = 1; - frame.data[0] = testcase; - - frame.can_id = ID; - if (write(s, &frame, sizeof(frame)) < 0) { - perror("write"); - exit(1); - } - frame.can_id = (ID | CAN_RTR_FLAG); - if (write(s, &frame, sizeof(frame)) < 0) { - perror("write"); - exit(1); - } - frame.can_id = (ID | CAN_EFF_FLAG); - if (write(s, &frame, sizeof(frame)) < 0) { - perror("write"); - exit(1); - } - frame.can_id = (ID | CAN_EFF_FLAG | CAN_RTR_FLAG); - if (write(s, &frame, sizeof(frame)) < 0) { - perror("write"); - exit(1); - } + TH_LOG("testcase %2d sending patterns...", testcase);
- printf("ok\n"); + ret = send_can_frames(s, testcase); + ASSERT_EQ(0, ret) + TH_LOG("failed to send CAN frames");
have_rx = 1; rx = 0; @@ -147,58 +150,45 @@ int main(int argc, char **argv) tv.tv_usec = 50000; /* 50ms timeout */
ret = select(s+1, &rdfs, NULL, NULL, &tv); - if (ret < 0) { - perror("select"); - exit(1); - } + ASSERT_LE(0, ret) + TH_LOG("failed select for frame %d (%d)", rx, errno);
if (FD_ISSET(s, &rdfs)) { have_rx = 1; ret = read(s, &frame, sizeof(struct can_frame)); - if (ret < 0) { - perror("read"); - exit(1); - } - if ((frame.can_id & CAN_SFF_MASK) != ID) { - fprintf(stderr, "received wrong can_id!\n"); - exit(1); - } - if (frame.data[0] != testcase) { - fprintf(stderr, "received wrong testcase!\n"); - exit(1); - } + ASSERT_LE(0, ret) + TH_LOG("failed to read frame %d (%d)", rx, errno); + + ASSERT_EQ(ID, frame.can_id & CAN_SFF_MASK) + TH_LOG("received wrong can_id"); + ASSERT_EQ(testcase, frame.data[0]) + TH_LOG("received wrong test case");
/* test & calc rxbits */ rxbitval = 1 << ((frame.can_id & (CAN_EFF_FLAG|CAN_RTR_FLAG|CAN_ERR_FLAG)) >> 28);
/* only receive a rxbitval once */ - if ((rxbits & rxbitval) == rxbitval) { - fprintf(stderr, "received rxbitval %d twice!\n", rxbitval); - exit(1); - } + ASSERT_NE(rxbitval, rxbits & rxbitval) + TH_LOG("received rxbitval %d twice", rxbitval); rxbits |= rxbitval; rx++;
- printf("testcase %2d rx : can_id = 0x%08X rx = %d rxbits = %d\n", + TH_LOG("testcase %2d rx : can_id = 0x%08X rx = %d rxbits = %d", testcase, frame.can_id, rx, rxbits); } } /* rx timed out -> check the received results */ - if (rx_res[testcase] != rx) { - fprintf(stderr, "wrong rx value in testcase %d : %d (expected %d)\n", - testcase, rx, rx_res[testcase]); - exit(1); - } - if (rxbits_res[testcase] != rxbits) { - fprintf(stderr, "wrong rxbits value in testcase %d : %d (expected %d)\n", - testcase, rxbits, rxbits_res[testcase]); - exit(1); - } - printf("testcase %2d ok\n---\n", testcase); + ASSERT_EQ(rx_res[testcase], rx) + TH_LOG("wrong number of received frames %d", testcase); + ASSERT_EQ(rxbits_res[testcase], rxbits) + TH_LOG("wrong rxbits value in testcase %d", testcase); + + TH_LOG("testcase %2d ok", testcase); + TH_LOG("---"); }
-out_socket: close(s); -out: - return err; + return; } + +TEST_HARNESS_MAIN
On Tue. 22 Apr. 2025 at 21:04, Felix Maurer fmaurer@redhat.com wrote:
Update test_raw_filter to use the kselftest harness to make the test output conform with TAP. Use the logging and assertations from the harness.
^^^^^^^^^^^^ assertions?
Signed-off-by: Felix Maurer fmaurer@redhat.com
tools/testing/selftests/net/can/Makefile | 2 + .../selftests/net/can/test_raw_filter.c | 152 ++++++++---------- 2 files changed, 73 insertions(+), 81 deletions(-)
diff --git a/tools/testing/selftests/net/can/Makefile b/tools/testing/selftests/net/can/Makefile index 44ef37f064ad..5b82e60a03e7 100644 --- a/tools/testing/selftests/net/can/Makefile +++ b/tools/testing/selftests/net/can/Makefile @@ -2,6 +2,8 @@
top_srcdir = ../../../../..
+CFLAGS += -Wall -Wl,--no-as-needed -O2 -g -I$(top_srcdir)/usr/include $(KHDR_INCLUDES)
TEST_PROGS := test_raw_filter.sh
TEST_GEN_FILES := test_raw_filter diff --git a/tools/testing/selftests/net/can/test_raw_filter.c b/tools/testing/selftests/net/can/test_raw_filter.c index c84260f36565..7414b9aef823 100644 --- a/tools/testing/selftests/net/can/test_raw_filter.c +++ b/tools/testing/selftests/net/can/test_raw_filter.c @@ -18,6 +18,8 @@ #include <linux/can.h> #include <linux/can/raw.h>
+#include "../../kselftest_harness.h"
#define ID 0x123 #define TC 18 /* # of testcases */
@@ -53,7 +55,38 @@ canid_t calc_mask(int testcase) return mask; }
-int main(int argc, char **argv) +int send_can_frames(int sock, int testcase) +{
struct can_frame frame;frame.can_dlc = 1;frame.data[0] = testcase;frame.can_id = ID;if (write(sock, &frame, sizeof(frame)) < 0) {perror("write");return 1;}frame.can_id = (ID | CAN_RTR_FLAG);if (write(sock, &frame, sizeof(frame)) < 0) {perror("write");return 1;}frame.can_id = (ID | CAN_EFF_FLAG);if (write(sock, &frame, sizeof(frame)) < 0) {perror("write");return 1;}frame.can_id = (ID | CAN_EFF_FLAG | CAN_RTR_FLAG);if (write(sock, &frame, sizeof(frame)) < 0) {perror("write");return 1;}return 0;
Nitpick: can you centralize the error handling under a goto label?
write_err: perror("write"); return 1;
+}
+TEST(can_filter) { fd_set rdfs; struct timeval tv; @@ -67,34 +100,26 @@ int main(int argc, char **argv) int rxbits, rxbitval; int ret; int recv_own_msgs = 1;
int err = 0; struct ifreq ifr;if ((s = socket(PF_CAN, SOCK_RAW, CAN_RAW)) < 0) {perror("socket");err = 1;goto out;}
s = socket(PF_CAN, SOCK_RAW, CAN_RAW);ASSERT_LT(0, s)TH_LOG("failed to create CAN_RAW socket (%d)", errno); strcpy(ifr.ifr_name, VCANIF);
if (ioctl(s, SIOCGIFINDEX, &ifr) < 0) {perror("SIOCGIFINDEX");err = 1;goto out_socket;}
ret = ioctl(s, SIOCGIFINDEX, &ifr);ASSERT_LE(0, ret)TH_LOG("failed SIOCGIFINDEX (%d)", errno);addr.can_family = AF_CAN; addr.can_ifindex = ifr.ifr_ifindex; setsockopt(s, SOL_CAN_RAW, CAN_RAW_RECV_OWN_MSGS, &recv_own_msgs, sizeof(recv_own_msgs));
if (bind(s, (struct sockaddr *)&addr, sizeof(addr)) < 0) {perror("bind");err = 1;goto out_socket;}printf("---\n");
ret = bind(s, (struct sockaddr *)&addr, sizeof(addr));ASSERT_EQ(0, ret)TH_LOG("failed bind socket (%d)", errno); for (testcase = 0; testcase < TC; testcase++) {@@ -103,36 +128,14 @@ int main(int argc, char **argv) setsockopt(s, SOL_CAN_RAW, CAN_RAW_FILTER, &rfilter, sizeof(rfilter));
printf("testcase %2d filters : can_id = 0x%08X can_mask = 0x%08X\n",
TH_LOG("testcase %2d filters : can_id = 0x%08X can_mask = 0x%08X", testcase, rfilter.can_id, rfilter.can_mask);
printf("testcase %2d sending patterns ... ", testcase);frame.can_dlc = 1;frame.data[0] = testcase;frame.can_id = ID;if (write(s, &frame, sizeof(frame)) < 0) {perror("write");exit(1);}frame.can_id = (ID | CAN_RTR_FLAG);if (write(s, &frame, sizeof(frame)) < 0) {perror("write");exit(1);}frame.can_id = (ID | CAN_EFF_FLAG);if (write(s, &frame, sizeof(frame)) < 0) {perror("write");exit(1);}frame.can_id = (ID | CAN_EFF_FLAG | CAN_RTR_FLAG);if (write(s, &frame, sizeof(frame)) < 0) {perror("write");exit(1);}
TH_LOG("testcase %2d sending patterns...", testcase);
printf("ok\n");
ret = send_can_frames(s, testcase);ASSERT_EQ(0, ret)TH_LOG("failed to send CAN frames"); have_rx = 1; rx = 0;@@ -147,58 +150,45 @@ int main(int argc, char **argv) tv.tv_usec = 50000; /* 50ms timeout */
ret = select(s+1, &rdfs, NULL, NULL, &tv);
if (ret < 0) {perror("select");exit(1);}
ASSERT_LE(0, ret)TH_LOG("failed select for frame %d (%d)", rx, errno);
^^^^ Nitpick: the Linux coding style suggests not printing numbers in parentheses (%d).
https://www.kernel.org/doc/html/latest/process/coding-style.html#printing-ke...
Suggestion:
TH_LOG("failed select for frame %d, err: %d", rx, errno);
if (FD_ISSET(s, &rdfs)) { have_rx = 1; ret = read(s, &frame, sizeof(struct can_frame));
if (ret < 0) {perror("read");exit(1);}if ((frame.can_id & CAN_SFF_MASK) != ID) {fprintf(stderr, "received wrong can_id!\n");exit(1);}if (frame.data[0] != testcase) {fprintf(stderr, "received wrong testcase!\n");exit(1);}
ASSERT_LE(0, ret)TH_LOG("failed to read frame %d (%d)", rx, errno);ASSERT_EQ(ID, frame.can_id & CAN_SFF_MASK)TH_LOG("received wrong can_id");ASSERT_EQ(testcase, frame.data[0])TH_LOG("received wrong test case"); /* test & calc rxbits */ rxbitval = 1 << ((frame.can_id & (CAN_EFF_FLAG|CAN_RTR_FLAG|CAN_ERR_FLAG)) >> 28); /* only receive a rxbitval once */
if ((rxbits & rxbitval) == rxbitval) {fprintf(stderr, "received rxbitval %d twice!\n", rxbitval);exit(1);}
ASSERT_NE(rxbitval, rxbits & rxbitval)TH_LOG("received rxbitval %d twice", rxbitval); rxbits |= rxbitval; rx++;
printf("testcase %2d rx : can_id = 0x%08X rx = %d rxbits = %d\n",
TH_LOG("testcase %2d rx : can_id = 0x%08X rx = %d rxbits = %d", testcase, frame.can_id, rx, rxbits); } } /* rx timed out -> check the received results */
if (rx_res[testcase] != rx) {fprintf(stderr, "wrong rx value in testcase %d : %d (expected %d)\n",testcase, rx, rx_res[testcase]);exit(1);}if (rxbits_res[testcase] != rxbits) {fprintf(stderr, "wrong rxbits value in testcase %d : %d (expected %d)\n",testcase, rxbits, rxbits_res[testcase]);exit(1);}printf("testcase %2d ok\n---\n", testcase);
ASSERT_EQ(rx_res[testcase], rx)TH_LOG("wrong number of received frames %d", testcase);ASSERT_EQ(rxbits_res[testcase], rxbits)TH_LOG("wrong rxbits value in testcase %d", testcase);TH_LOG("testcase %2d ok", testcase);TH_LOG("---"); }-out_socket: close(s); -out:
return err;
return;}
+TEST_HARNESS_MAIN
Yours sincerely, Vincent Mailhol
Use fixtures in test_raw_filter instead of generating the test inputs during execution. This should make tests easier to follow and extend.
Signed-off-by: Felix Maurer fmaurer@redhat.com --- .../selftests/net/can/test_raw_filter.c | 311 ++++++++++++------ 1 file changed, 211 insertions(+), 100 deletions(-)
diff --git a/tools/testing/selftests/net/can/test_raw_filter.c b/tools/testing/selftests/net/can/test_raw_filter.c index 7414b9aef823..7fe11e020a1c 100644 --- a/tools/testing/selftests/net/can/test_raw_filter.c +++ b/tools/testing/selftests/net/can/test_raw_filter.c @@ -18,43 +18,13 @@ #include <linux/can.h> #include <linux/can/raw.h>
+#define TH_LOG_ENABLED 0 #include "../../kselftest_harness.h"
#define ID 0x123 -#define TC 18 /* # of testcases */ - -const int rx_res[TC] = {4, 4, 4, 4, 2, 2, 2, 2, 2, 2, 2, 2, 1, 1, 1, 1, 1, 1}; -const int rxbits_res[TC] = {4369, 4369, 4369, 4369, 17, 4352, 17, 4352, 257, 257, 4112, 4112, 1, 256, 16, 4096, 1, 256};
#define VCANIF "vcan0"
-canid_t calc_id(int testcase) -{ - canid_t id = ID; - - if (testcase & 1) - id |= CAN_EFF_FLAG; - if (testcase & 2) - id |= CAN_RTR_FLAG; - - return id; -} - -canid_t calc_mask(int testcase) -{ - canid_t mask = CAN_SFF_MASK; - - if (testcase > 15) - return (CAN_EFF_MASK | CAN_EFF_FLAG | CAN_RTR_FLAG); - - if (testcase & 4) - mask |= CAN_EFF_FLAG; - if (testcase & 8) - mask |= CAN_RTR_FLAG; - - return mask; -} - int send_can_frames(int sock, int testcase) { struct can_frame frame; @@ -86,21 +56,16 @@ int send_can_frames(int sock, int testcase) return 0; }
-TEST(can_filter) +FIXTURE(can_filters) { + int sock; +}; + +FIXTURE_SETUP(can_filters) { - fd_set rdfs; - struct timeval tv; - int s; struct sockaddr_can addr; - struct can_filter rfilter; - struct can_frame frame; - int testcase; - int have_rx; - int rx; - int rxbits, rxbitval; - int ret; - int recv_own_msgs = 1; struct ifreq ifr; + int recv_own_msgs = 1; + int s, ret;
s = socket(PF_CAN, SOCK_RAW, CAN_RAW); ASSERT_LT(0, s) @@ -121,74 +86,220 @@ TEST(can_filter) ASSERT_EQ(0, ret) TH_LOG("failed bind socket (%d)", errno);
- for (testcase = 0; testcase < TC; testcase++) { + self->sock = s; +} + +FIXTURE_TEARDOWN(can_filters) +{ + close(self->sock); +}
- rfilter.can_id = calc_id(testcase); - rfilter.can_mask = calc_mask(testcase); - setsockopt(s, SOL_CAN_RAW, CAN_RAW_FILTER, - &rfilter, sizeof(rfilter)); +FIXTURE_VARIANT(can_filters) { + int testcase; + canid_t id; + canid_t mask; + int exp_num_rx; + int exp_rxbits; +}; + +FIXTURE_VARIANT_ADD(can_filters, base) { + .testcase = 1, + .id = ID, + .mask = CAN_SFF_MASK, + .exp_num_rx = 4, + .exp_rxbits = 4369, +}; +FIXTURE_VARIANT_ADD(can_filters, base_eff) { + .testcase = 2, + .id = ID | CAN_EFF_FLAG, + .mask = CAN_SFF_MASK, + .exp_num_rx = 4, + .exp_rxbits = 4369, +}; +FIXTURE_VARIANT_ADD(can_filters, base_rtr) { + .testcase = 3, + .id = ID | CAN_RTR_FLAG, + .mask = CAN_SFF_MASK, + .exp_num_rx = 4, + .exp_rxbits = 4369, +}; +FIXTURE_VARIANT_ADD(can_filters, base_effrtr) { + .testcase = 4, + .id = ID | CAN_EFF_FLAG | CAN_RTR_FLAG, + .mask = CAN_SFF_MASK, + .exp_num_rx = 4, + .exp_rxbits = 4369, +}; + +FIXTURE_VARIANT_ADD(can_filters, filter_eff) { + .testcase = 5, + .id = ID, + .mask = CAN_SFF_MASK | CAN_EFF_FLAG, + .exp_num_rx = 2, + .exp_rxbits = 17, +}; +FIXTURE_VARIANT_ADD(can_filters, filter_eff_eff) { + .testcase = 6, + .id = ID | CAN_EFF_FLAG, + .mask = CAN_SFF_MASK | CAN_EFF_FLAG, + .exp_num_rx = 2, + .exp_rxbits = 4352, +}; +FIXTURE_VARIANT_ADD(can_filters, filter_eff_rtr) { + .testcase = 7, + .id = ID | CAN_RTR_FLAG, + .mask = CAN_SFF_MASK | CAN_EFF_FLAG, + .exp_num_rx = 2, + .exp_rxbits = 17, +}; +FIXTURE_VARIANT_ADD(can_filters, filter_eff_effrtr) { + .testcase = 8, + .id = ID | CAN_EFF_FLAG | CAN_RTR_FLAG, + .mask = CAN_SFF_MASK | CAN_EFF_FLAG, + .exp_num_rx = 2, + .exp_rxbits = 4352, +}; + +FIXTURE_VARIANT_ADD(can_filters, filter_rtr) { + .testcase = 9, + .id = ID, + .mask = CAN_SFF_MASK | CAN_RTR_FLAG, + .exp_num_rx = 2, + .exp_rxbits = 257, +}; +FIXTURE_VARIANT_ADD(can_filters, filter_rtr_eff) { + .testcase = 10, + .id = ID | CAN_EFF_FLAG, + .mask = CAN_SFF_MASK | CAN_RTR_FLAG, + .exp_num_rx = 2, + .exp_rxbits = 257, +}; +FIXTURE_VARIANT_ADD(can_filters, filter_rtr_rtr) { + .testcase = 11, + .id = ID | CAN_RTR_FLAG, + .mask = CAN_SFF_MASK | CAN_RTR_FLAG, + .exp_num_rx = 2, + .exp_rxbits = 4112, +}; +FIXTURE_VARIANT_ADD(can_filters, filter_rtr_effrtr) { + .testcase = 12, + .id = ID | CAN_EFF_FLAG | CAN_RTR_FLAG, + .mask = CAN_SFF_MASK | CAN_RTR_FLAG, + .exp_num_rx = 2, + .exp_rxbits = 4112, +}; + +FIXTURE_VARIANT_ADD(can_filters, filter_effrtr) { + .testcase = 13, + .id = ID, + .mask = CAN_SFF_MASK | CAN_EFF_FLAG | CAN_RTR_FLAG, + .exp_num_rx = 1, + .exp_rxbits = 1, +}; +FIXTURE_VARIANT_ADD(can_filters, filter_effrtr_eff) { + .testcase = 14, + .id = ID | CAN_EFF_FLAG, + .mask = CAN_SFF_MASK | CAN_EFF_FLAG | CAN_RTR_FLAG, + .exp_num_rx = 1, + .exp_rxbits = 256, +}; +FIXTURE_VARIANT_ADD(can_filters, filter_effrtr_rtr) { + .testcase = 15, + .id = ID | CAN_RTR_FLAG, + .mask = CAN_SFF_MASK | CAN_EFF_FLAG | CAN_RTR_FLAG, + .exp_num_rx = 1, + .exp_rxbits = 16, +}; +FIXTURE_VARIANT_ADD(can_filters, filter_effrtr_effrtr) { + .testcase = 16, + .id = ID | CAN_EFF_FLAG | CAN_RTR_FLAG, + .mask = CAN_SFF_MASK | CAN_EFF_FLAG | CAN_RTR_FLAG, + .exp_num_rx = 1, + .exp_rxbits = 4096, +}; + +FIXTURE_VARIANT_ADD(can_filters, eff) { + .testcase = 17, + .id = ID, + .mask = CAN_EFF_MASK | CAN_EFF_FLAG | CAN_RTR_FLAG, + .exp_num_rx = 1, + .exp_rxbits = 1, +}; +FIXTURE_VARIANT_ADD(can_filters, eff_eff) { + .testcase = 18, + .id = ID | CAN_EFF_FLAG, + .mask = CAN_EFF_MASK | CAN_EFF_FLAG | CAN_RTR_FLAG, + .exp_num_rx = 1, + .exp_rxbits = 256, +}; + +TEST_F(can_filters, test_filter) +{ + fd_set rdfs; + struct timeval tv; + struct can_filter rfilter; + struct can_frame frame; + int have_rx; + int rx; + int rxbits, rxbitval; + int ret;
- TH_LOG("testcase %2d filters : can_id = 0x%08X can_mask = 0x%08X", - testcase, rfilter.can_id, rfilter.can_mask); + rfilter.can_id = variant->id; + rfilter.can_mask = variant->mask; + setsockopt(self->sock, SOL_CAN_RAW, CAN_RAW_FILTER, + &rfilter, sizeof(rfilter));
- TH_LOG("testcase %2d sending patterns...", testcase); + TH_LOG("filters: can_id = 0x%08X can_mask = 0x%08X", + rfilter.can_id, rfilter.can_mask);
- ret = send_can_frames(s, testcase); - ASSERT_EQ(0, ret) - TH_LOG("failed to send CAN frames"); + ret = send_can_frames(self->sock, variant->testcase); + ASSERT_EQ(0, ret) + TH_LOG("failed to send CAN frames");
- have_rx = 1; - rx = 0; - rxbits = 0; + rx = 0; + rxbits = 0;
- while (have_rx) { + do { + have_rx = 0; + FD_ZERO(&rdfs); + FD_SET(self->sock, &rdfs); + tv.tv_sec = 0; + tv.tv_usec = 50000; /* 50ms timeout */
- have_rx = 0; - FD_ZERO(&rdfs); - FD_SET(s, &rdfs); - tv.tv_sec = 0; - tv.tv_usec = 50000; /* 50ms timeout */ + ret = select(self->sock + 1, &rdfs, NULL, NULL, &tv); + ASSERT_LE(0, ret) + TH_LOG("failed select for frame %d (%d)", rx, errno);
- ret = select(s+1, &rdfs, NULL, NULL, &tv); + if (FD_ISSET(self->sock, &rdfs)) { + have_rx = 1; + ret = read(self->sock, &frame, sizeof(struct can_frame)); ASSERT_LE(0, ret) - TH_LOG("failed select for frame %d (%d)", rx, errno); - - if (FD_ISSET(s, &rdfs)) { - have_rx = 1; - ret = read(s, &frame, sizeof(struct can_frame)); - ASSERT_LE(0, ret) - TH_LOG("failed to read frame %d (%d)", rx, errno); - - ASSERT_EQ(ID, frame.can_id & CAN_SFF_MASK) - TH_LOG("received wrong can_id"); - ASSERT_EQ(testcase, frame.data[0]) - TH_LOG("received wrong test case"); - - /* test & calc rxbits */ - rxbitval = 1 << ((frame.can_id & (CAN_EFF_FLAG|CAN_RTR_FLAG|CAN_ERR_FLAG)) >> 28); - - /* only receive a rxbitval once */ - ASSERT_NE(rxbitval, rxbits & rxbitval) - TH_LOG("received rxbitval %d twice", rxbitval); - rxbits |= rxbitval; - rx++; - - TH_LOG("testcase %2d rx : can_id = 0x%08X rx = %d rxbits = %d", - testcase, frame.can_id, rx, rxbits); - } + TH_LOG("failed to read frame %d (%d)", rx, errno); + + ASSERT_EQ(ID, frame.can_id & CAN_SFF_MASK) + TH_LOG("received wrong can_id"); + ASSERT_EQ(variant->testcase, frame.data[0]) + TH_LOG("received wrong test case"); + + /* test & calc rxbits */ + rxbitval = 1 << ((frame.can_id & (CAN_EFF_FLAG|CAN_RTR_FLAG|CAN_ERR_FLAG)) >> 28); + + /* only receive a rxbitval once */ + ASSERT_NE(rxbitval, rxbits & rxbitval) + TH_LOG("received rxbitval %d twice", rxbitval); + rxbits |= rxbitval; + rx++; + + TH_LOG("rx: can_id = 0x%08X rx = %d rxbits = %d", + frame.can_id, rx, rxbits); } - /* rx timed out -> check the received results */ - ASSERT_EQ(rx_res[testcase], rx) - TH_LOG("wrong number of received frames %d", testcase); - ASSERT_EQ(rxbits_res[testcase], rxbits) - TH_LOG("wrong rxbits value in testcase %d", testcase); - - TH_LOG("testcase %2d ok", testcase); - TH_LOG("---"); - } + } while (have_rx);
- close(s); - return; + /* rx timed out -> check the received results */ + ASSERT_EQ(variant->exp_num_rx, rx) + TH_LOG("wrong number of received frames"); + ASSERT_EQ(variant->exp_rxbits, rxbits) + TH_LOG("wrong rxbits value"); }
TEST_HARNESS_MAIN
On Tue. 22 Apr. 2025 at 21:04, Felix Maurer fmaurer@redhat.com wrote:
Use fixtures in test_raw_filter instead of generating the test inputs during execution. This should make tests easier to follow and extend.
Signed-off-by: Felix Maurer fmaurer@redhat.com
.../selftests/net/can/test_raw_filter.c | 311 ++++++++++++------ 1 file changed, 211 insertions(+), 100 deletions(-)
diff --git a/tools/testing/selftests/net/can/test_raw_filter.c b/tools/testing/selftests/net/can/test_raw_filter.c index 7414b9aef823..7fe11e020a1c 100644 --- a/tools/testing/selftests/net/can/test_raw_filter.c +++ b/tools/testing/selftests/net/can/test_raw_filter.c @@ -18,43 +18,13 @@
(...)
+TEST_F(can_filters, test_filter) +{
fd_set rdfs;struct timeval tv;struct can_filter rfilter;struct can_frame frame;int have_rx;int rx;int rxbits, rxbitval;int ret;
Can you reduce the scope of the variables? More precisely, can you move those declarations
fd_set rdfs; struct timeval tv;
to the do {} while(); loop and move those declarations
struct can_frame frame; int rxbitval;
to the if (FD_ISSET(self->sock, &rdfs)) block?
TH_LOG("testcase %2d filters : can_id = 0x%08X can_mask = 0x%08X",testcase, rfilter.can_id, rfilter.can_mask);
rfilter.can_id = variant->id;rfilter.can_mask = variant->mask;setsockopt(self->sock, SOL_CAN_RAW, CAN_RAW_FILTER,&rfilter, sizeof(rfilter));
TH_LOG("testcase %2d sending patterns...", testcase);
TH_LOG("filters: can_id = 0x%08X can_mask = 0x%08X",rfilter.can_id, rfilter.can_mask);
ret = send_can_frames(s, testcase);ASSERT_EQ(0, ret)TH_LOG("failed to send CAN frames");
ret = send_can_frames(self->sock, variant->testcase);ASSERT_EQ(0, ret)TH_LOG("failed to send CAN frames");
have_rx = 1;rx = 0;rxbits = 0;
rx = 0;rxbits = 0;
while (have_rx) {
do {have_rx = 0;FD_ZERO(&rdfs);FD_SET(self->sock, &rdfs);tv.tv_sec = 0;tv.tv_usec = 50000; /* 50ms timeout */
have_rx = 0;FD_ZERO(&rdfs);FD_SET(s, &rdfs);tv.tv_sec = 0;tv.tv_usec = 50000; /* 50ms timeout */
ret = select(self->sock + 1, &rdfs, NULL, NULL, &tv);ASSERT_LE(0, ret)TH_LOG("failed select for frame %d (%d)", rx, errno);
ret = select(s+1, &rdfs, NULL, NULL, &tv);
if (FD_ISSET(self->sock, &rdfs)) {have_rx = 1;ret = read(self->sock, &frame, sizeof(struct can_frame)); ASSERT_LE(0, ret)
TH_LOG("failed select for frame %d (%d)", rx, errno);if (FD_ISSET(s, &rdfs)) {have_rx = 1;ret = read(s, &frame, sizeof(struct can_frame));
^^^^^^^^^^^^^^^^ sizeof(frame)
ASSERT_LE(0, ret)TH_LOG("failed to read frame %d (%d)", rx, errno);ASSERT_EQ(ID, frame.can_id & CAN_SFF_MASK)TH_LOG("received wrong can_id");ASSERT_EQ(testcase, frame.data[0])TH_LOG("received wrong test case");/* test & calc rxbits */rxbitval = 1 << ((frame.can_id & (CAN_EFF_FLAG|CAN_RTR_FLAG|CAN_ERR_FLAG)) >> 28);/* only receive a rxbitval once */ASSERT_NE(rxbitval, rxbits & rxbitval)TH_LOG("received rxbitval %d twice", rxbitval);rxbits |= rxbitval;rx++;TH_LOG("testcase %2d rx : can_id = 0x%08X rx = %d rxbits = %d",testcase, frame.can_id, rx, rxbits);}
TH_LOG("failed to read frame %d (%d)", rx, errno);ASSERT_EQ(ID, frame.can_id & CAN_SFF_MASK)TH_LOG("received wrong can_id");ASSERT_EQ(variant->testcase, frame.data[0])TH_LOG("received wrong test case");/* test & calc rxbits */rxbitval = 1 << ((frame.can_id & (CAN_EFF_FLAG|CAN_RTR_FLAG|CAN_ERR_FLAG)) >> 28);/* only receive a rxbitval once */ASSERT_NE(rxbitval, rxbits & rxbitval)TH_LOG("received rxbitval %d twice", rxbitval);rxbits |= rxbitval;rx++;TH_LOG("rx: can_id = 0x%08X rx = %d rxbits = %d",frame.can_id, rx, rxbits); }
/* rx timed out -> check the received results */ASSERT_EQ(rx_res[testcase], rx)TH_LOG("wrong number of received frames %d", testcase);ASSERT_EQ(rxbits_res[testcase], rxbits)TH_LOG("wrong rxbits value in testcase %d", testcase);TH_LOG("testcase %2d ok", testcase);TH_LOG("---");}
} while (have_rx);
close(s);return;
/* rx timed out -> check the received results */ASSERT_EQ(variant->exp_num_rx, rx)TH_LOG("wrong number of received frames");ASSERT_EQ(variant->exp_rxbits, rxbits)TH_LOG("wrong rxbits value");}
Yours sincerely, Vincent Mailhol
The expected results did not explain very well what was really tested. Make the expectations more clear by writing out the flags that should be set in the received frames and add a short explanation for each test case. Also, document the overall test design.
Signed-off-by: Felix Maurer fmaurer@redhat.com --- .../selftests/net/can/test_raw_filter.c | 65 ++++++++++++++----- 1 file changed, 49 insertions(+), 16 deletions(-)
diff --git a/tools/testing/selftests/net/can/test_raw_filter.c b/tools/testing/selftests/net/can/test_raw_filter.c index 7fe11e020a1c..8d43053824d2 100644 --- a/tools/testing/selftests/net/can/test_raw_filter.c +++ b/tools/testing/selftests/net/can/test_raw_filter.c @@ -101,94 +101,113 @@ FIXTURE_VARIANT(can_filters) { int exp_num_rx; int exp_rxbits; }; +#define T_EFF (CAN_EFF_FLAG >> 28) +#define T_RTR (CAN_RTR_FLAG >> 28)
+/* Receive all frames when filtering for the ID in standard frame format */ FIXTURE_VARIANT_ADD(can_filters, base) { .testcase = 1, .id = ID, .mask = CAN_SFF_MASK, .exp_num_rx = 4, - .exp_rxbits = 4369, + .exp_rxbits = (1 | 1 << (T_EFF) | 1 << (T_RTR) | 1 << (T_EFF | T_RTR)), }; +/* Ignore EFF flag in filter ID if not covered by filter mask */ FIXTURE_VARIANT_ADD(can_filters, base_eff) { .testcase = 2, .id = ID | CAN_EFF_FLAG, .mask = CAN_SFF_MASK, .exp_num_rx = 4, - .exp_rxbits = 4369, + .exp_rxbits = (1 | 1 << (T_EFF) | 1 << (T_RTR) | 1 << (T_EFF | T_RTR)), }; +/* Ignore RTR flag in filter ID if not covered by filter mask */ FIXTURE_VARIANT_ADD(can_filters, base_rtr) { .testcase = 3, .id = ID | CAN_RTR_FLAG, .mask = CAN_SFF_MASK, .exp_num_rx = 4, - .exp_rxbits = 4369, + .exp_rxbits = (1 | 1 << (T_EFF) | 1 << (T_RTR) | 1 << (T_EFF | T_RTR)), }; +/* Ignore EFF and RTR flags in filter ID if not covered by filter mask */ FIXTURE_VARIANT_ADD(can_filters, base_effrtr) { .testcase = 4, .id = ID | CAN_EFF_FLAG | CAN_RTR_FLAG, .mask = CAN_SFF_MASK, .exp_num_rx = 4, - .exp_rxbits = 4369, + .exp_rxbits = (1 | 1 << (T_EFF) | 1 << (T_RTR) | 1 << (T_EFF | T_RTR)), };
+/* Receive only SFF frames when expecting no EFF flag */ FIXTURE_VARIANT_ADD(can_filters, filter_eff) { .testcase = 5, .id = ID, .mask = CAN_SFF_MASK | CAN_EFF_FLAG, .exp_num_rx = 2, - .exp_rxbits = 17, + .exp_rxbits = (1 | 1 << (T_RTR)), }; +/* Receive only EFF frames when filter id and filter mask include EFF flag */ FIXTURE_VARIANT_ADD(can_filters, filter_eff_eff) { .testcase = 6, .id = ID | CAN_EFF_FLAG, .mask = CAN_SFF_MASK | CAN_EFF_FLAG, .exp_num_rx = 2, - .exp_rxbits = 4352, + .exp_rxbits = (1 << (T_EFF) | 1 << (T_EFF | T_RTR)), }; +/* Receive only SFF frames when expecting no EFF flag, ignoring RTR flag */ FIXTURE_VARIANT_ADD(can_filters, filter_eff_rtr) { .testcase = 7, .id = ID | CAN_RTR_FLAG, .mask = CAN_SFF_MASK | CAN_EFF_FLAG, .exp_num_rx = 2, - .exp_rxbits = 17, + .exp_rxbits = (1 | 1 << (T_RTR)), }; +/* Receive only EFF frames when filter id and filter mask include EFF flag, + * ignoring RTR flag + */ FIXTURE_VARIANT_ADD(can_filters, filter_eff_effrtr) { .testcase = 8, .id = ID | CAN_EFF_FLAG | CAN_RTR_FLAG, .mask = CAN_SFF_MASK | CAN_EFF_FLAG, .exp_num_rx = 2, - .exp_rxbits = 4352, + .exp_rxbits = (1 << (T_EFF) | 1 << (T_EFF | T_RTR)), };
+/* Receive no remote frames when filtering for no RTR flag */ FIXTURE_VARIANT_ADD(can_filters, filter_rtr) { .testcase = 9, .id = ID, .mask = CAN_SFF_MASK | CAN_RTR_FLAG, .exp_num_rx = 2, - .exp_rxbits = 257, + .exp_rxbits = (1 | 1 << (T_EFF)), }; +/* Receive no remote frames when filtering for no RTR flag, ignoring EFF flag */ FIXTURE_VARIANT_ADD(can_filters, filter_rtr_eff) { .testcase = 10, .id = ID | CAN_EFF_FLAG, .mask = CAN_SFF_MASK | CAN_RTR_FLAG, .exp_num_rx = 2, - .exp_rxbits = 257, + .exp_rxbits = (1 | 1 << (T_EFF)), }; +/* Receive only remote frames when filter includes RTR flag */ FIXTURE_VARIANT_ADD(can_filters, filter_rtr_rtr) { .testcase = 11, .id = ID | CAN_RTR_FLAG, .mask = CAN_SFF_MASK | CAN_RTR_FLAG, .exp_num_rx = 2, - .exp_rxbits = 4112, + .exp_rxbits = (1 << (T_RTR) | 1 << (T_EFF | T_RTR)), }; +/* Receive only remote frames when filter includes RTR flag, ignoring EFF + * flag + */ FIXTURE_VARIANT_ADD(can_filters, filter_rtr_effrtr) { .testcase = 12, .id = ID | CAN_EFF_FLAG | CAN_RTR_FLAG, .mask = CAN_SFF_MASK | CAN_RTR_FLAG, .exp_num_rx = 2, - .exp_rxbits = 4112, + .exp_rxbits = (1 << (T_RTR) | 1 << (T_EFF | T_RTR)), };
+/* Receive only SFF data frame when filtering for no flags */ FIXTURE_VARIANT_ADD(can_filters, filter_effrtr) { .testcase = 13, .id = ID, @@ -196,28 +215,34 @@ FIXTURE_VARIANT_ADD(can_filters, filter_effrtr) { .exp_num_rx = 1, .exp_rxbits = 1, }; +/* Receive only EFF data frame when filtering for EFF but no RTR flag */ FIXTURE_VARIANT_ADD(can_filters, filter_effrtr_eff) { .testcase = 14, .id = ID | CAN_EFF_FLAG, .mask = CAN_SFF_MASK | CAN_EFF_FLAG | CAN_RTR_FLAG, .exp_num_rx = 1, - .exp_rxbits = 256, + .exp_rxbits = (1 << (T_EFF)), }; +/* Receive only SFF remote frame when filtering for RTR but no EFF flag */ FIXTURE_VARIANT_ADD(can_filters, filter_effrtr_rtr) { .testcase = 15, .id = ID | CAN_RTR_FLAG, .mask = CAN_SFF_MASK | CAN_EFF_FLAG | CAN_RTR_FLAG, .exp_num_rx = 1, - .exp_rxbits = 16, + .exp_rxbits = (1 << (T_RTR)), }; +/* Receive only EFF remote frame when filtering for EFF and RTR flag */ FIXTURE_VARIANT_ADD(can_filters, filter_effrtr_effrtr) { .testcase = 16, .id = ID | CAN_EFF_FLAG | CAN_RTR_FLAG, .mask = CAN_SFF_MASK | CAN_EFF_FLAG | CAN_RTR_FLAG, .exp_num_rx = 1, - .exp_rxbits = 4096, + .exp_rxbits = (1 << (T_EFF | T_RTR)), };
+/* Receive only SFF data frame when filtering for no EFF flag and no RTR flag + * but based on EFF mask + */ FIXTURE_VARIANT_ADD(can_filters, eff) { .testcase = 17, .id = ID, @@ -225,14 +250,22 @@ FIXTURE_VARIANT_ADD(can_filters, eff) { .exp_num_rx = 1, .exp_rxbits = 1, }; +/* Receive only EFF data frame when filtering for EFF flag and no RTR flag but + * based on EFF mask + */ FIXTURE_VARIANT_ADD(can_filters, eff_eff) { .testcase = 18, .id = ID | CAN_EFF_FLAG, .mask = CAN_EFF_MASK | CAN_EFF_FLAG | CAN_RTR_FLAG, .exp_num_rx = 1, - .exp_rxbits = 256, + .exp_rxbits = (1 << (T_EFF)), };
+/* This test verifies that the raw CAN filters work, by checking if only frames + * with the expected set of flags are received. For each test case, the given + * filter (id and mask) is added and four CAN frames are sent with every + * combination of set/unset EFF/RTR flags. + */ TEST_F(can_filters, test_filter) { fd_set rdfs;
On Tue. 22 Apr. 2025 at 21:03, Felix Maurer fmaurer@redhat.com wrote:
The expected results did not explain very well what was really tested. Make the expectations more clear by writing out the flags that should be set in the received frames and add a short explanation for each test case. Also, document the overall test design.
Signed-off-by: Felix Maurer fmaurer@redhat.com
.../selftests/net/can/test_raw_filter.c | 65 ++++++++++++++----- 1 file changed, 49 insertions(+), 16 deletions(-)
diff --git a/tools/testing/selftests/net/can/test_raw_filter.c b/tools/testing/selftests/net/can/test_raw_filter.c index 7fe11e020a1c..8d43053824d2 100644 --- a/tools/testing/selftests/net/can/test_raw_filter.c +++ b/tools/testing/selftests/net/can/test_raw_filter.c @@ -101,94 +101,113 @@ FIXTURE_VARIANT(can_filters) { int exp_num_rx; int exp_rxbits; }; +#define T_EFF (CAN_EFF_FLAG >> 28) +#define T_RTR (CAN_RTR_FLAG >> 28)
I do not like this
28
shift. I understand that it is part of the original design, but for me, this is just obfuscation.
Why just not using CAN_EFF_FLAG and CAN_RTR_FLAG as-is for the expected values? What benefit does this shift add?
+/* Receive all frames when filtering for the ID in standard frame format */ FIXTURE_VARIANT_ADD(can_filters, base) { .testcase = 1, .id = ID, .mask = CAN_SFF_MASK, .exp_num_rx = 4,
.exp_rxbits = 4369,
.exp_rxbits = (1 | 1 << (T_EFF) | 1 << (T_RTR) | 1 << (T_EFF | T_RTR)),
^ ^ Nitpick: those outermost parentheses are not needed.
This took me time to process. Isn't your expression redundant? What about
.exp_rxbits = 1 | 1 << (T_EFF | T_RTR),
?
This gives me the same result:
https://godbolt.org/z/cr3q5vjMr
}; +/* Ignore EFF flag in filter ID if not covered by filter mask */ FIXTURE_VARIANT_ADD(can_filters, base_eff) { .testcase = 2, .id = ID | CAN_EFF_FLAG, .mask = CAN_SFF_MASK, .exp_num_rx = 4,
.exp_rxbits = 4369,
.exp_rxbits = (1 | 1 << (T_EFF) | 1 << (T_RTR) | 1 << (T_EFF | T_RTR)),
^ What is the meaning of this 1?
};
(...)
Yours sincerely, Vincent Mailhol
On Thu. 24 Apr. 2025 at 16:44, Vincent Mailhol mailhol.vincent@wanadoo.fr wrote:
On Tue. 22 Apr. 2025 at 21:03, Felix Maurer fmaurer@redhat.com wrote:
(...)
.exp_rxbits = (1 | 1 << (T_EFF) | 1 << (T_RTR) | 1 << (T_EFF | T_RTR)),^ ^Nitpick: those outermost parentheses are not needed.
This took me time to process. Isn't your expression redundant? What about
.exp_rxbits = 1 | 1 << (T_EFF | T_RTR),
?
This gives me the same result:
Never mind. This was a silly comment. I messed up the operator precedence in the above example, these are obviously different.
Please disregard my comment and sorry for the noise.
Yours sincerely, Vincent Mailhol
On 24.04.25 09:44, Vincent Mailhol wrote:
On Tue. 22 Apr. 2025 at 21:03, Felix Maurer fmaurer@redhat.com wrote:
[...]
diff --git a/tools/testing/selftests/net/can/test_raw_filter.c b/tools/testing/selftests/net/can/test_raw_filter.c index 7fe11e020a1c..8d43053824d2 100644 --- a/tools/testing/selftests/net/can/test_raw_filter.c +++ b/tools/testing/selftests/net/can/test_raw_filter.c @@ -101,94 +101,113 @@ FIXTURE_VARIANT(can_filters) { int exp_num_rx; int exp_rxbits; }; +#define T_EFF (CAN_EFF_FLAG >> 28) +#define T_RTR (CAN_RTR_FLAG >> 28)
I do not like this
28
shift. I understand that it is part of the original design, but for me, this is just obfuscation.
Why just not using CAN_EFF_FLAG and CAN_RTR_FLAG as-is for the expected values? What benefit does this shift add?
I agree, that looks like magic numbers and the original design is not very nice here. The main reason for the >>28 is that later on values are shifted by T_EFF and/or T_RTR, so they shouldn't be too large (with the
28, the shift value later is in the range 0-14). See below for a
slightly different idea.
+/* Ignore EFF flag in filter ID if not covered by filter mask */ FIXTURE_VARIANT_ADD(can_filters, base_eff) { .testcase = 2, .id = ID | CAN_EFF_FLAG, .mask = CAN_SFF_MASK, .exp_num_rx = 4,
.exp_rxbits = 4369,
.exp_rxbits = (1 | 1 << (T_EFF) | 1 << (T_RTR) | 1 << (T_EFF | T_RTR)),^What is the meaning of this 1?
The 1 means that a packet will be received with no flags set.
The whole rxbit thing took me a while to understand and the result now is not straightforward either. Let's see if we can come up with something better.
The exp_rxbits is basically a bitfield that describes which flags should be set on the received frames. Maybe this could be made more explicit with something like this:
.exp_rxbits = FRAME_NOFLAGS | FRAME_EFF | FRAME_RTR | FRAME_EFFRTR,
And in the receive loop something like this:
rxbits |= FRAME_RCVD(frame.can_id);
Of course, the definitions of these macros would still have the >>28, but at a central point, with better explanation. Do you think that's more understandable? Or do you have a different idea?
Thanks, Felix
On 24/04/2025 at 23:02, Felix Maurer wrote:
On 24.04.25 09:44, Vincent Mailhol wrote:
On Tue. 22 Apr. 2025 at 21:03, Felix Maurer fmaurer@redhat.com wrote:
[...]
diff --git a/tools/testing/selftests/net/can/test_raw_filter.c b/tools/testing/selftests/net/can/test_raw_filter.c index 7fe11e020a1c..8d43053824d2 100644 --- a/tools/testing/selftests/net/can/test_raw_filter.c +++ b/tools/testing/selftests/net/can/test_raw_filter.c @@ -101,94 +101,113 @@ FIXTURE_VARIANT(can_filters) { int exp_num_rx; int exp_rxbits; }; +#define T_EFF (CAN_EFF_FLAG >> 28) +#define T_RTR (CAN_RTR_FLAG >> 28)
I do not like this
28
shift. I understand that it is part of the original design, but for me, this is just obfuscation.
Why just not using CAN_EFF_FLAG and CAN_RTR_FLAG as-is for the expected values? What benefit does this shift add?
I agree, that looks like magic numbers and the original design is not very nice here. The main reason for the >>28 is that later on values are shifted by T_EFF and/or T_RTR, so they shouldn't be too large (with the
28, the shift value later is in the range 0-14). See below for a
slightly different idea.
+/* Ignore EFF flag in filter ID if not covered by filter mask */ FIXTURE_VARIANT_ADD(can_filters, base_eff) { .testcase = 2, .id = ID | CAN_EFF_FLAG, .mask = CAN_SFF_MASK, .exp_num_rx = 4,
.exp_rxbits = 4369,
.exp_rxbits = (1 | 1 << (T_EFF) | 1 << (T_RTR) | 1 << (T_EFF | T_RTR)),^What is the meaning of this 1?
The 1 means that a packet will be received with no flags set.
OK. Now I understand.
The whole rxbit thing took me a while to understand and the result now is not straightforward either. Let's see if we can come up with something better.
The exp_rxbits is basically a bitfield that describes which flags should be set on the received frames. Maybe this could be made more explicit with something like this:
.exp_rxbits = FRAME_NOFLAGS | FRAME_EFF | FRAME_RTR | FRAME_EFFRTR,
This is better. But yet, how would this scale in the future if we introduce the CAN FD? For n flags, you have n combinations.
And in the receive loop something like this:
rxbits |= FRAME_RCVD(frame.can_id);
Of course, the definitions of these macros would still have the >>28, but at a central point, with better explanation. Do you think that's more understandable? Or do you have a different idea?
The
28
trick just allows to save a couple line but by doing so, adds a ton of complexity. What is wrong in writing this:
FIXTURE_VARIANT(can_filters) { int testcase; canid_t id; canid_t mask; int exp_num_rx; canid_t exp_flags[]; };
/* Receive all frames when filtering for the ID in standard frame format */ FIXTURE_VARIANT_ADD(can_filters, base) { .testcase = 1, .id = ID, .mask = CAN_SFF_MASK, .exp_num_rx = 4, .exp_flags = { 0, CAN_EFF_FLAG, CAN_RTR_FLAG, CAN_EFF_FLAG | CAN_RTR_FLAG, }, };
And then, in your TEST_F(), the do {} while loops becomes a:
for (int i = 0; i <= variant->exp_num_rx; i++) { /* FD logic here */ ret = FD_ISSET(self->sock, &rdfs); if (i == variant->exp_num_rx) { ASSERT_EQ(ret == 0); } else (i < variant->exp_num_rx) /* other relevant checks */ ASSERT_EQ(frame.can_id & ~CAN_ERR_MASK == variant->exp_flags[i]); } }
Here, you even check that the frames are received in order.
OK, the bitmap saved some memory, but here, we are speaking of selftests. The priority is readability. I will happily get rid of the bitmap and just simplify the logic.
Yours sincerely, Vincent Mailhol
On 24.04.25 17:08, Vincent Mailhol wrote:
On 24/04/2025 at 23:02, Felix Maurer wrote:
On 24.04.25 09:44, Vincent Mailhol wrote:
On Tue. 22 Apr. 2025 at 21:03, Felix Maurer fmaurer@redhat.com wrote:
[...]
diff --git a/tools/testing/selftests/net/can/test_raw_filter.c b/tools/testing/selftests/net/can/test_raw_filter.c index 7fe11e020a1c..8d43053824d2 100644 --- a/tools/testing/selftests/net/can/test_raw_filter.c +++ b/tools/testing/selftests/net/can/test_raw_filter.c @@ -101,94 +101,113 @@ FIXTURE_VARIANT(can_filters) { int exp_num_rx; int exp_rxbits; }; +#define T_EFF (CAN_EFF_FLAG >> 28) +#define T_RTR (CAN_RTR_FLAG >> 28)
I do not like this
28
shift. I understand that it is part of the original design, but for me, this is just obfuscation.
Why just not using CAN_EFF_FLAG and CAN_RTR_FLAG as-is for the expected values? What benefit does this shift add?
I agree, that looks like magic numbers and the original design is not very nice here. The main reason for the >>28 is that later on values are shifted by T_EFF and/or T_RTR, so they shouldn't be too large (with the
28, the shift value later is in the range 0-14). See below for a
slightly different idea.
+/* Ignore EFF flag in filter ID if not covered by filter mask */ FIXTURE_VARIANT_ADD(can_filters, base_eff) { .testcase = 2, .id = ID | CAN_EFF_FLAG, .mask = CAN_SFF_MASK, .exp_num_rx = 4,
.exp_rxbits = 4369,
.exp_rxbits = (1 | 1 << (T_EFF) | 1 << (T_RTR) | 1 << (T_EFF | T_RTR)),^What is the meaning of this 1?
The 1 means that a packet will be received with no flags set.
OK. Now I understand.
The whole rxbit thing took me a while to understand and the result now is not straightforward either. Let's see if we can come up with something better.
The exp_rxbits is basically a bitfield that describes which flags should be set on the received frames. Maybe this could be made more explicit with something like this:
.exp_rxbits = FRAME_NOFLAGS | FRAME_EFF | FRAME_RTR | FRAME_EFFRTR,
This is better. But yet, how would this scale in the future if we introduce the CAN FD? For n flags, you have n combinations.
And in the receive loop something like this:
rxbits |= FRAME_RCVD(frame.can_id);
Of course, the definitions of these macros would still have the >>28, but at a central point, with better explanation. Do you think that's more understandable? Or do you have a different idea?
The
28
trick just allows to save a couple line but by doing so, adds a ton of complexity. What is wrong in writing this:
I don't see anything wrong with it, I like it :) I'll send an updated version of the patches soon (probably squashed as well).
FIXTURE_VARIANT(can_filters) { int testcase; canid_t id; canid_t mask; int exp_num_rx; canid_t exp_flags[]; };
/* Receive all frames when filtering for the ID in standard frame format */ FIXTURE_VARIANT_ADD(can_filters, base) { .testcase = 1, .id = ID, .mask = CAN_SFF_MASK, .exp_num_rx = 4, .exp_flags = { 0, CAN_EFF_FLAG, CAN_RTR_FLAG, CAN_EFF_FLAG | CAN_RTR_FLAG, }, };
And then, in your TEST_F(), the do {} while loops becomes a:
for (int i = 0; i <= variant->exp_num_rx; i++) { /* FD logic here */ ret = FD_ISSET(self->sock, &rdfs); if (i == variant->exp_num_rx) { ASSERT_EQ(ret == 0); } else (i < variant->exp_num_rx) /* other relevant checks */ ASSERT_EQ(frame.can_id & ~CAN_ERR_MASK == variant->exp_flags[i]); } }
Here, you even check that the frames are received in order.
OK, the bitmap saved some memory, but here, we are speaking of selftests. The priority is readability. I will happily get rid of the bitmap and just simplify the logic.
I fully agree, thank you! Felix
Hi Felix,
On Tue. 22 Apr. 2025 at 21:08, Felix Maurer fmaurer@redhat.com wrote:
This is the initial import of a CAN selftest from can-tests[1] into the tree. For now, it is just a single test but when agreed on the structure, we intend to import more tests from can-tests and add additional test cases.
Excellent initiative!
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.
I have cc'ed netdev and its reviewers and maintainers to make sure they are okay with the location of the tests and the changes to the paths in MAINTAINERS. The changes should be merged through linux-can-next and subsequent changes will not go to netdev anymore.
I am not a netdev maintainer, just a /drivers/net/can maintainer, but you have my blessing on this. As far the location goes, your proposal makes perfect sense to me. Actually, I can not think of any other places than
tools/testing/selftests/net/can
for this kind of thing.
Felix Maurer (4): selftests: can: Import tst-filter from can-tests selftests: can: use kselftest harness in test_raw_filter selftests: can: Use fixtures in test_raw_filter selftests: can: Document test_raw_filter test cases
You are doing a lot of change to the original to the point that this is more a full rewrite. I have no intent of reviewing the first patch which is just the copy paste from the original. If no one else has a strong opinion on this, I would rather prefer if you just squash everything and send a single patch with the final result. This will also save you some effort when migrating the other tests.
I have a few comments on the individual patches, but overall very good. Thanks a lot!
Yours sincerely, Vincent Mailhol
On 24.04.25 09:45, Vincent Mailhol wrote: [...]
Felix Maurer (4): selftests: can: Import tst-filter from can-tests selftests: can: use kselftest harness in test_raw_filter selftests: can: Use fixtures in test_raw_filter selftests: can: Document test_raw_filter test cases
You are doing a lot of change to the original to the point that this is more a full rewrite. I have no intent of reviewing the first patch which is just the copy paste from the original. If no one else has a strong opinion on this, I would rather prefer if you just squash everything and send a single patch with the final result. This will also save you some effort when migrating the other tests.
I have a few comments on the individual patches, but overall very good. Thanks a lot!
Thank you very much for your feedback! I'll silently include most of it and will only reply where I think discussions are necessary.
For squashing / keeping this as individual patches: I usually like to have some kind of history available, but here it might not provide a lot of value. I would be fine with squashing as well. If there are any stronger opinions on this, keep them coming.
Thanks, Felix
On 24/04/2025 at 23:01, Felix Maurer wrote:
On 24.04.25 09:45, Vincent Mailhol wrote: [...]
Felix Maurer (4): selftests: can: Import tst-filter from can-tests selftests: can: use kselftest harness in test_raw_filter selftests: can: Use fixtures in test_raw_filter selftests: can: Document test_raw_filter test cases
You are doing a lot of change to the original to the point that this is more a full rewrite. I have no intent of reviewing the first patch which is just the copy paste from the original. If no one else has a strong opinion on this, I would rather prefer if you just squash everything and send a single patch with the final result. This will also save you some effort when migrating the other tests.
I have a few comments on the individual patches, but overall very good. Thanks a lot!
Thank you very much for your feedback! I'll silently include most of it and will only reply where I think discussions are necessary.
For squashing / keeping this as individual patches: I usually like to have some kind of history available, but here it might not provide a lot of value. I would be fine with squashing as well. If there are any stronger opinions on this, keep them coming.
I would normally agree, but here, I did a git diff between the first patch and the final result, and there is just fives lines of code which you did not touch.
If I want to review each patch (which I normally always do), this becomes double work. Splitting a series in small patches should simplify the review process, unfortunately, here, it had the opposite effect for me.
Yours sincerely, Vincent Mailhol
On Tue, 22 Apr 2025 14:02:33 +0200 Felix Maurer wrote:
I have cc'ed netdev and its reviewers and maintainers to make sure they are okay with the location of the tests and the changes to the paths in MAINTAINERS. The changes should be merged through linux-can-next and subsequent changes will not go to netdev anymore.
Makes perfect sense to me as well, FWIW.
linux-kselftest-mirror@lists.linaro.org
-
Felix Maurer -
Jakub Kicinski -
Vincent Mailhol