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 <[email protected]> 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
