On Fri. 15 Jan 2021 at 21:38, Oliver Hartkopp <socketcan@xxxxxxxxxxxx> wrote:
>
> -DaveM
> -Jakub
> -netdev
>
> On 15.01.21 02:57, Vincent MAILHOL wrote:
>
> >> 48 */
> >> + /* 49 - 64 is checked in can_fd_len2dlc() */
> >
> > Ack
> >
> >>
> >> /* map the sanitized data length to an appropriate data length code */
> >> u8 can_fd_len2dlc(u8 len)
> >> {
> >> - if (unlikely(len > 64))
> >> + if (len > 48)
> >
> > I personally prefer the use of macros instead of hardcoded values. 48 is the
> > last index of the table, i.e. it is ARRAY_SIZE(len2dlc) - 1.
>
> In general I'm with you here.
>
> > For me, it is like doing this:
> > for (i = 0; i <= harcoded_value_representing_last_index_of_array; i++)
> > instead of this:
> > for (i = 0; i < ARRAY_SIZE(array); i++)
> >
> > Definitely prefer the later and (len >= ARRAY_SIZE(len2dlc)) is nothing less
> > than the negation of the i < ARRAY_SIZE(array) that we usually see in a for
> > loop.
> >
> > I recognize below patterns to be correct:
> > i < ARRAY_SIZE(array): check that variable is inbound.
> > i >= ARRAY_SIZE(array): check that variable is outbound.
> >
> > Anything which deviates from those patterns is fishy and it is actually how
> > I spotted the bug.
> >
> > If we don’t use ARRAY_SIZE() we lose that recognizable pattern and we need
> > to be aware of the actual content of len2dlc[] to understand the code.
> > (And I know that the table is just above the function and that this makes my
> > argument weaker).
> >
> > So IMO, checks done against the array size should use the ARRAY_SIZE() macro
> > in order 1/ to make it a recognizable pattern and 2/ to make it work regardless
> > of the actual size of the table (i.e. no hardcoded value).
>
> The problem is NOT that we make sure to access this array correctly.
>
> This particular array is no set of arbitrary values that may be extended
> later on BUT it is a 'translation map' for defined length values which
> will never change.
>
> Therefore ARRAY_SIZE(array) hides the fact that every length value
> "greater than 48" results to a DLC of 15.
>
> For that reason my former code was very clear:
>
> 1. It had a table that mapped 0 .. 64 to a DLC
> 2. It had a test for '> 64' as sanity test.
>
> Now the sanity test is gone and mixed up with the mapping of length
> values - and finally with ARRAY_SIZE(whatever) which doesn't give a hint
> why this is checked.
>
> We are writing code to be understandable for the reader and the
> suggested 'improvement' which saves 16 bytes does exactly the opposite.
You did not convince me but I will not argue more on the use of
ARRAY_SIZE(). I do not want to trigger an endless discussion :)
If we take the code as being understandable as top priority, then
I would suggest to remove the len2dlc[] array and use the GNU
case range extension (which is supported in the kernel):
/* map the data length to an appropriate data length code */
u8 can_fd_len2dlc(u8 len)
{
switch(len) {
case 0 ... 8:
return len;
case 9 ... 12:
return 9;
case 13 ... 16:
return 10;
case 17 ... 20:
return 11;
case 21 ... 24:
return 12;
case 25 ... 32:
return 13;
case 33 ... 48:
return 14;
case 49 ... 64:
default:
return CANFD_MAX_DLC;
}
}
And we will just leave the optimizations in the hand of the compiler.
> >
> >> return 0xF;
> >
> > I would also prefer to use the CANFD_MAX_DLC macro here.
>
> Ack. Me too. Just seen that after pressing the 'send' button :-/
No problem :)
This also happens to me too often.
Yours sincerely,
Vincent
