lkml.org 
[lkml]   [2018]   [Aug]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [GIT] Networking
Hi Linus,

2018-08-05 16:52 GMT+01:00 Linus Torvalds <torvalds@linux-foundation.org>:
> On Sun, Aug 5, 2018 at 12:47 AM David Miller <davem@davemloft.net> wrote:
>>
>> 4) Fix regression in netlink bind handling of multicast
>> gruops, from Dmitry Safonov.
>
> This one is written kind of stupidly.
>
> The code went from the original
>
> groups &= (1UL << nlk->ngroups) - 1;
>
> (which is incorrect for large values of nlk->ngroups) to the fixed
>
> if (nlk->ngroups == 0)
> groups = 0;
> else if (nlk->ngroups < 8*sizeof(groups))
> groups &= (1UL << nlk->ngroups) - 1;
>
> which isn't technically incorrect...
>
> But it is stupid.
>
> Why stupid? Because the test for 0 is pointless.

Heh, I've been stupid enough at that moment to think that
(1 << 0 == 1) and forgetting that I'm subtracting 1 for mask.

> Just doing
>
> if (nlk->ngroups < 8*sizeof(groups))
> groups &= (1UL << nlk->ngroups) - 1;
>
> would have been fine and more understandable, since the "mask by shift
> count" already does the right thing for a ngroups value of 0. Now that
> test for zero makes me go "what's special about zero?". It turns out
> that the answer to that is "nothing".
>
> I certainly didn't care enough to fix it up, and maybe the compiler is
> even smart enough to remove the unnecessary test for zero, but it's
> kind of sad to see this kind of "people didn't think the code through"
> patch this late in the rc.

Yes, sorry.

> I'll be making an rc8 today anyway, but I did want to just point to it
> and say "hey guys, the code is unnecessarily stupid and overly
> complicated".
>
> The type of "groups" is kind of silly too.
>
> Yeah, "long unsigned int" isn't _technically_ wrong. But we normally
> call that type "unsigned long".
>
> And comparing against "8*sizeof(groups)" is misleading too, when the
> actual masking expression works and is correct in "unsigned long"
> because that's the type of the actual mask we're computing (due to the
> "1UL").
>
> So _regardless_ of the type of "groups" itself, the mask is actually
> correct in unsigned long. I personally think it would have been more
> legible as just
>
> unsigned long groups;
> ...
> if (nlk->ngroups < BITS_PER_LONG)
> groups &= (1UL << nlk->ngroups) - 1;
>
> but by now I'm just nitpicking.

I'll prepare the cleanup for linux-next.

Sorry about the stupid code,
Dmitry

\
 
 \ /
  Last update: 2018-08-07 19:57    [W:0.076 / U:15.816 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site