linux-kernel - block: new gcc-5.1 warnings..

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 27 May 2015 15:32:15 -0700
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Jens Axboe <axboe@...nel.dk>
Cc:	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: block: new gcc-5.1 warnings..

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))

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?

               Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/