Message-ID: <CAMZ6RqKmPD+BZkVC1C-vn7hcAVdQr8Qhd6PW8bASZiQkD6MV-A@mail.gmail.com>
Date: Wed, 14 May 2025 18:47:47 +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 v2] selftests: can: Import tst-filter from can-tests

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.

> ---
>
> Notes:
>     I keep netdev and its reviewers and maintainers in CC because of the
>     changes to their paths in MAINTAINERS, even though Jakub acked them on
>     v1. The change should be merged through linux-can-next and subsequent
>     changes will not go to netdev anymore.
>
>     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.

Ack.

>     Changes to v1:
>     - link: https://lore.kernel.org/linux-can/cover.1745323279.git.fmaurer@redhat.com/
>     - Squashed import and rewrite into a single commit
>     - Simplified checking of the received flags
>     - Pass the interface name through env (easier with the selftest
>       framework than adding an argument)
>
>     I have not updated test_raw_filter.sh to work with physical CAN
>     interfaces so far because I don't have one to test this right now and
>     don't think it's a priority for selftests for now.

OK but can you just:

  s/VCANIF/CANIF/g

in the .sh and in the .c files? This way, when the test gets updated
to also support the physical interfaces, the patch diff will be
smaller.

I have the hardware, so it is something I can easily contribute.

>  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       | 395 ++++++++++++++++++
>  .../selftests/net/can/test_raw_filter.sh      |  37 ++
>  6 files changed, 448 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@...tonic.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@...unet.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..5b82e60a03e7
> --- /dev/null
> +++ b/tools/testing/selftests/net/can/Makefile
> @@ -0,0 +1,11 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +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
> +
> +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..3c0e43cab1e8
> --- /dev/null
> +++ b/tools/testing/selftests/net/can/test_raw_filter.c
> @@ -0,0 +1,395 @@
> +// 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/if.h>
> +
> +#include <linux/can.h>
> +#include <linux/can/raw.h>
> +
> +#include "../../kselftest_harness.h"
> +
> +#define ID 0x123
> +
> +char VCANIF[IFNAMSIZ];
> +
> +static 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)
> +               goto write_err;
> +
> +       frame.can_id = (ID | CAN_RTR_FLAG);
> +       if (write(sock, &frame, sizeof(frame)) < 0)
> +               goto write_err;
> +
> +       frame.can_id = (ID | CAN_EFF_FLAG);
> +       if (write(sock, &frame, sizeof(frame)) < 0)
> +               goto write_err;
> +
> +       frame.can_id = (ID | CAN_EFF_FLAG | CAN_RTR_FLAG);
> +       if (write(sock, &frame, sizeof(frame)) < 0)
> +               goto write_err;
> +
> +       return 0;
> +
> +write_err:
> +       perror("write");
> +       return 1;
> +

Nitpick: remove the empty line.

> +}
> +
> +FIXTURE(can_filters) {
> +       int sock;
> +};
> +
> +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)

> +               TH_LOG("failed to create CAN_RAW socket: %d", errno);
> +
> +       strncpy(ifr.ifr_name, VCANIF, sizeof(ifr.ifr_name));
> +       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));
> +
> +       ret = bind(s, (struct sockaddr *)&addr, sizeof(addr));
> +       ASSERT_EQ(0, ret)
> +               TH_LOG("failed bind socket: %d", errno);
> +
> +       self->sock = s;
> +}
> +
> +FIXTURE_TEARDOWN(can_filters)
> +{
> +       close(self->sock);
> +}
> +
> +FIXTURE_VARIANT(can_filters) {
> +       int testcase;
> +       canid_t id;
> +       canid_t mask;
> +       int exp_num_rx;
> +       canid_t exp_flags[];
> +};
> +#define T_EFF (CAN_EFF_FLAG >> 28)
> +#define T_RTR (CAN_RTR_FLAG >> 28)

These two macros are not needed anymore and can be removed.

> +/* 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_RTR_FLAG,
> +               CAN_EFF_FLAG,
> +               CAN_EFF_FLAG | CAN_RTR_FLAG,
> +       },
> +};
> +/* 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_flags = {
> +               0,
> +               CAN_RTR_FLAG,
> +               CAN_EFF_FLAG,
> +               CAN_EFF_FLAG | CAN_RTR_FLAG,
> +       },
> +};
> +/* 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_flags = {
> +               0,
> +               CAN_RTR_FLAG,
> +               CAN_EFF_FLAG,
> +               CAN_EFF_FLAG | CAN_RTR_FLAG,
> +       },
> +};
> +/* 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_flags = {
> +               0,
> +               CAN_RTR_FLAG,
> +               CAN_EFF_FLAG,
> +               CAN_EFF_FLAG | CAN_RTR_FLAG,
> +       },
> +};
> +
> +/* 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_flags = {
> +               0,
> +               CAN_RTR_FLAG,
> +       },
> +};
> +/* 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_flags = {
> +               CAN_EFF_FLAG,
> +               CAN_EFF_FLAG | CAN_RTR_FLAG,
> +       },
> +};
> +/* 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_flags = {
> +               0,
> +               CAN_RTR_FLAG,
> +       },
> +};
> +/* 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_flags = {
> +               CAN_EFF_FLAG,
> +               CAN_EFF_FLAG | CAN_RTR_FLAG,
> +       },
> +};
> +
> +/* 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_flags = {
> +               0,
> +               CAN_EFF_FLAG,
> +       },
> +};

Nitpick: sometimes you have an empty line between the fixtures,
sometimes you don't. Can you rationalize this and always have an empty
new line between the fixtures?

> +/* 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_flags = {
> +               0,
> +               CAN_EFF_FLAG,
> +       },
> +};
> +/* 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_flags = {
> +               CAN_RTR_FLAG,
> +               CAN_EFF_FLAG | CAN_RTR_FLAG,
> +       },
> +};
> +/* 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_flags = {
> +               CAN_RTR_FLAG,
> +               CAN_EFF_FLAG | CAN_RTR_FLAG,
> +       },
> +};
> +
> +/* Receive only SFF data frame when filtering for no flags */
> +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_flags = {
> +               0,
> +       },
> +};
> +/* 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_flags = {
> +               CAN_EFF_FLAG,
> +       },
> +};
> +/* 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_flags = {
> +               CAN_RTR_FLAG,
> +       },
> +};
> +/* 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_flags = {
> +               CAN_EFF_FLAG | CAN_RTR_FLAG,
> +       },
> +};
> +
> +/* 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,
> +       .mask = CAN_EFF_MASK | CAN_EFF_FLAG | CAN_RTR_FLAG,
> +       .exp_num_rx = 1,
> +       .exp_flags = {
> +               0,
> +       },
> +};
> +/* 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_flags = {
> +               CAN_EFF_FLAG,
> +       },
> +};
> +
> +/* 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)
> +{
> +       struct can_filter rfilter;
> +       int ret;
> +
> +       rfilter.can_id = variant->id;
> +       rfilter.can_mask = variant->mask;
> +       setsockopt(self->sock, SOL_CAN_RAW, CAN_RAW_FILTER,
> +                  &rfilter, sizeof(rfilter));
> +
> +       TH_LOG("filters: can_id = 0x%08X can_mask = 0x%08X",
> +               rfilter.can_id, rfilter.can_mask);
> +
> +       ret = send_can_frames(self->sock, variant->testcase);
> +       ASSERT_EQ(0, ret)
> +               TH_LOG("failed to send CAN frames");
> +
> +       for (int i = 0; i <= variant->exp_num_rx; i++) {
> +               struct can_frame frame;
> +               struct timeval tv;

Nitpick: you can directly initialize this variable:

        struct timeval tv = {
                .tv_sec = 0,
                .tv_usec = 50000, /* 50ms timeout */
        };

> +               fd_set rdfs;
> +
> +               FD_ZERO(&rdfs);
> +               FD_SET(self->sock, &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, err: %d)", i, errno);
> +
> +               ret = FD_ISSET(self->sock, &rdfs);
> +               if (i == variant->exp_num_rx) {
> +                       ASSERT_EQ(0, ret)
> +                               TH_LOG("too many frames received");
> +               } else {
> +                       ASSERT_NE(0, ret)
> +                               TH_LOG("too few frames received");
> +
> +                       ret = read(self->sock, &frame, sizeof(frame));
> +                       ASSERT_LE(0, ret)
> +                               TH_LOG("failed to read frame %d, err: %d", i, errno);
> +
> +                       TH_LOG("rx: can_id = 0x%08X rx = %d", frame.can_id, i);
> +
> +                       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");
> +
> +                       ASSERT_EQ(frame.can_id & ~CAN_ERR_MASK,
> +                                 variant->exp_flags[i])
> +                               TH_LOG("received unexpected flags");
> +

Remove this empty line.

> +               }
> +       }
> +}
> +
> +int main(int argc, char **argv)
> +{
> +       char *ifname = getenv("VCANIF");
> +
> +       if (ifname) {
> +               strncpy(VCANIF, ifname, sizeof(VCANIF) - 1);
> +       } else {
> +               printf("VCANIF environment variable must contain the test interface\n");
> +               return KSFT_FAIL;
> +       }

Nitpick: test the error condition first:

        if (!ifname) {
                printf("VCANIF environment variable must contain the
test interface\n");
                return KSFT_FAIL;
        }

        strncpy(VCANIF, ifname, sizeof(VCANIF) - 1);

> +       return test_harness_run(argc, argv);
> +}
> 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..95f45c3c824b
> --- /dev/null
> +++ b/tools/testing/selftests/net/can/test_raw_filter.sh
> @@ -0,0 +1,37 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +
> +ALL_TESTS="
> +       test_raw_filter
> +"
> +
> +net_dir=$(dirname $0)/..
> +source $net_dir/lib.sh
> +
> +export VCANIF="vcan0"

Could you make it so that if the environment variable is already set,
you do not override it?

  : ${CANIF:=vcan0}
  export CANIF

This way, users can easily test other interfaces without having to
edit the script.

> +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
> +       check_err $?
> +       log_test "test_raw_filter"
> +}
> +
> +trap cleanup EXIT
> +setup
> +
> +tests_run
> +
> +exit $EXIT_STATUS
> --
> 2.49.0
>
>

Powered by blists - more mailing lists