Re: [GIT] Networking

[Dmitry Safonov]

Hi Linus,

2018-08-05 16:52 GMT+01:00 Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>:
> On Sun, Aug 5, 2018 at 12:47 AM David Miller <davem@xxxxxxxxxxxxx> 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