Re: [PATCH] bitmap: fix n__ signess

[Elliott Mitchell <ehem+xen@xxxxxxx>]

On Fri, Sep 29, 2023 at 08:24:37AM +0100, Julien Grall wrote:
> Hi Stefano,
> 
> On 29/09/2023 00:19, Stefano Stabellini wrote:
> > All callers of the bitmap_switch macro (which are all within bitmap.h)
> > pass an int as first parameter. Do not assign it to an unsigned int
> > potentially introducing errors.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxx>
> > ---
> > 
> > We could also have gone the other way and change all the callers and the
> > callers' callers to use an unsigned int instead, but I went for the path
> > of least resistance.
> 
> I understand this will solve the issue right now because the callers are 
> all passing 'int'. However, all the callers will need to switch to 
> 'unsigned int' in order to solve violations in their callers.
> 
> That unless we decide to use 'int' everywhere, but I think this is a bad 
> idea because 'n__' is not supposed to be negative.
> 
> Overall, this may be an easy win right now, but this will need to be 
> reverted. So, I am not happy to ack it and would in fact be leaning 
> towards Nacking it.

I would have tended to review on the basis situations like this unsigned
is always the way to go.  n__ *wants* to be unsigned since such
structures nearly always have some level of implicit assemption of being
unsigned.

Turns out though the situation is worse since as proposed this also
significantly changes the logic.  The value of `(int)n__ <= BITS_PER_LONG`
may differ from `(unsigned int)n__ <= BITS_PER_LONG`.  I wouldn't expect
these functions to get negative values, but if they did a lurking issue
has been added.


-- 
(\___(\___(\______          --=> 8-) EHM <=--          ______/)___/)___/)
 \BS (    |         ehem+sigmsg@xxxxxxx  PGP 87145445         |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445