Re: block: new gcc-5.1 warnings..

[Jens Axboe <axboe@kernel.dk>]

On 05/27/2015 04:32 PM, Linus Torvalds wrote:
> So gcc-5.1 seems to have a few new warnings, most of which seem of
> dubious value, but whatever.
>
> One of them
>
> drivers/block/hd.c: In function ‘hd_request’:
> drivers/block/hd.c:630:11: warning: switch condition has boolean value
> [-Wswitch-bool]
>     switch (rq_data_dir(req)) {
>             ^
>
> just made me go "what?" since doing a switch on a boolean is perfectly
> fine, and there can be various valid reasons to do so (using "break"
> and fall-through etc can make the structure of the true/false cases
> nicer).
>
> So the compiler warning is just silly and stupid.
>
> The warning would make more sense (and still trigger for this kernel
> case) if the case statements then didn't use boolean values.
>
> So despite the warning in general just being insane, in this case it
> happens to show an oddity of the kernel source code: rq_data_dir()
> returns a boolean, and that actually makes little sense, since it's
> normally compared to READ/WRITE. Which *happen* to be 0/1, and integer
> promotion does the right thing, but if you actually look at what
> READ/WRITE are, it really is 0/1, not false/true.
>
> This odd boolean came in through commit 5953316dbf90 ("block: make
> rq->cmd_flags be 64-bit") and I think that change really was
> questionable. What happened was that "cmd_flags" got turned into
> "u64", and that commit wants to avoid making rq_data_dir() return a
> u64, because that screws up printk() and friends.
>
> But I think it might be better off as (I didn't test this):
>
>   #define rq_data_dir(rq)                ((int)((rq)->cmd_flags & 1))

That'd work just fine.

> instead, to match the type of READ/WRITE. That would also get rid of
> the (bogus) warning introduced by gcc-5.1.1.
>
> And maybe somebody could then convince the gcc people that
>
>     switch (boolean) {
>     case true:
>         ...
>     case false:
>     }
>
> is actually perfectly fine.  It could still complain about the truly
> odd cases (which the kernel use really arguably is).
>
> Hmm? Jens?

The case you quoted is arguably crap, since the default case can never 
be hit. I'm assuming this code dates back a long time, from when we 
checked the actual command type instead of a bit being set or not. Now, 
I don't have gcc-5.1 here, and the warning does make it seem like this 
is not what it's complaining about. So I'd be fine with just making 
rq_data_dir() return the right type and get rid of the != 0.

-- 
Jens Axboe