From mboxrd@z Thu Jan  1 00:00:00 1970
From: Dmitry Safonov <0x7f454c46@gmail.com>
Subject: Re: [GIT] Networking
Date: Tue, 7 Aug 2018 18:56:00 +0100
Message-ID: <CAJwJo6a7-p_WO4oUTC3bGMB_APZR2dEYV+TvgAcXeFjYgfLhqQ@mail.gmail.com>
References: <20180805.004744.1043412275329854518.davem@davemloft.net> <CA+55aFx1vbQWnQUVoieFgKrNECLKJs-RQi8z8cpe29g0aAiq3A@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="UTF-8"
Cc: David Miller <davem@davemloft.net>,
        Andrew Morton <akpm@linux-foundation.org>,
        Network Development <netdev@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
To: Linus Torvalds <torvalds@linux-foundation.org>
Return-path: <netdev-owner@vger.kernel.org>
Received: from mail-io0-f196.google.com ([209.85.223.196]:39287 "EHLO
        mail-io0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1727605AbeHGULu (ORCPT
        <rfc822;netdev@vger.kernel.org>); Tue, 7 Aug 2018 16:11:50 -0400
In-Reply-To: <CA+55aFx1vbQWnQUVoieFgKrNECLKJs-RQi8z8cpe29g0aAiq3A@mail.gmail.com>
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

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