Messages in this thread | | | From | Linus Torvalds <> | Date | Sun, 5 Aug 2018 08:52:50 -0700 | Subject | Re: [GIT] Networking |
| |
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.
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.
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.
Linus
| |