On 24.04.25 17:08, Vincent Mailhol wrote:
> 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:
I don't see anything wrong with it, I like it :) I'll send an updated
version of the patches soon (probably squashed as well).
> 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.
I fully agree, thank you!
Felix
