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