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