netdev - Re: [PATCH 4/4] selftests: can: Document test_raw

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: <CAMZ6RqKcp=JNcbZjX6xSGo9Hyw=1nXbpS9Nc36xuDkbGG+=wtA@mail.gmail.com>
Date: Thu, 24 Apr 2025 16:44:26 +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 4/4] selftests: can: Document test_raw_filter test cases

On Tue. 22 Apr. 2025 at 21:03, Felix Maurer <fmaurer@...hat.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@...hat.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