Re: [GIT PULL] first round of SCSI updates for the 4.14+ merge window

James Bottomley
2017-11-15T09:36:28Z

On Tue, 2017-11-14 at 16:33 -0800, Linus Torvalds wrote:
> On Tue, Nov 14, 2017 at 8:36 AM, James Bottomley
> <James.Bottomley@hansenpartnership.com> wrote:
> >
> >
> > Hannes Reinecke (14):
> >       scsi: scsi_devinfo: Reformat blacklist flags
>
> Ugh, that's just really ugly, but it's also wrong.
>
> Just having long lines would probably have been much preferable, and
> would mean that the commit that explains the bit would show up when
> you grep for the bit.
>
> Having a small helper macro like
>
>    #define BLIST_n(x) ((__force __u32 __bitwise)(1 << (n)))
>
> woiuld also likely have made it more legible.
>
> But that only takes care of the ugliness and the greppability.
>
> It's not right for sparse even _with_ those changes.
>
> Why? Because "__bitwise" actually creates a new type. So what those
> BLIST defines should do is to use a special type something like
>
>     typedef unsigned int __bitwise blist_flags_t;
>
> and now you have _one_ type thanks to that typedef, that is different
> from all the other bitwise types. Then you force all the constants
> and the field that implements to have that type, and you have type-
> safety: you can use those constants together, and you can assign the
> result to the blist flags, but you can't mix it with other __bitwise
> types.
>
> That's why things like this work:
>
>     typedef __u16 __bitwise __le16;
>     typedef __u16 __bitwise __be16;
>
> where __le16 and __be16 are actually different types, even though
> their underlying _storage_ is the same (a 16-bit unsigned).
>
> Anyway, I've pulled, because clearly this only matters for sparse,
> but I would hope that this gets fixed up, ok?

It will, boss; I'll make sure of it.

James