From mboxrd@z Thu Jan 1 00:00:00 1970 From: Linus Torvalds Subject: Re: [GIT] Networking Date: Sun, 5 Aug 2018 08:52:50 -0700 Message-ID: References: <20180805.004744.1043412275329854518.davem@davemloft.net> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Cc: Andrew Morton , Network Development , Linux Kernel Mailing List To: David Miller Return-path: In-Reply-To: <20180805.004744.1043412275329854518.davem@davemloft.net> Sender: linux-kernel-owner@vger.kernel.org List-Id: netdev.vger.kernel.org On Sun, Aug 5, 2018 at 12:47 AM David Miller 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