if(function()) BUG(); still bad?
On the kernel janitors site it is stated:
make sure BUG() is used correctly (i.e. if(function()) BUG(); is evil)
* i.e. even when no-op-ing BUG we still have an if (See also: BUG_ON)
Is this still true? A recent patch added such entries, see for instance
http://lkml.org/lkml/2007/11/30/298 There are also several occurances in the kernel, which can be spotted with:
[begin bash]
s="[[:space:]]*";
V="[[:alpha:]_]\+[[:alnum:]_]*"
# a bit of magic to cope with comments, chars and strings cendl="$s\(\/[\*\/].*\)\?$" ccode="\([^\/\"']*\|\/[^\*\/]\|\/\*.*\*\/\|'[^']*'\|\"[^\"]*\"\)*"
q="${s}if$s($ccode$s$V$s($ccode)$s$ccode$s)"
git-grep -n -B1 "^\($q\)\?${s}BUG($s)$s;$cendl" | grep -A1 "$q$cendl" | less
[end bash]
There are a few false positives due to sizeof() and MACROS(), but I can think of a way to filter those as well.
for instance in a patch I saw this:
Review
Quoted 3 lines
(quoted diff) +2
-0+ if (__blk_end_request(rq, 0, 0)) + BUG(); spin_unlock(q->queue_lock);
If it's still bad, what should the fix be? (I'll write a script and post patches)
Roel
Quoted 8 lines · Roel Kluin
for instance in a patch I saw this:
Review
Quoted 3 lines
(quoted diff) +2
-0+ if (__blk_end_request(rq, 0, 0)) + BUG(); spin_unlock(q->queue_lock);If it's still bad, what should the fix be? (I'll write a script and post patches)
This one should not be converted to a BUG_ON. We always want to call __blk_end_request(), whether we have CONFIG_BUG or not. This case just wants to test the return value.
Signature · 4 lines
Intel are signing my paycheques ... these opinions are still mine "Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step."
Quoted 9 lines · Matthew Wilcox
Quoted 7 lines · Roel Kluin
for instance in a patch I saw this:
Review
Quoted 3 lines
(quoted diff) +2
-0+ if (__blk_end_request(rq, 0, 0)) + BUG(); spin_unlock(q->queue_lock);If it's still bad, what should the fix be? (I'll write a script and post patches)
Quoted 4 lines
This one should not be converted to a
BUG_ON. We always want to call__blk_end_request(), whether we haveCONFIG_BUGor not. This case just wants to test the return value.
Ok, from your reply I can make up it's bad if the function in the if() assigns a value, or has pointers as arguments, which it modifies.
Quoted 10 lines · Matthew Wilcox
Quoted 8 lines · Roel Kluin
for instance in a patch I saw this:
Review
Quoted 3 lines
(quoted diff) +2
-0+ if (__blk_end_request(rq, 0, 0)) + BUG(); spin_unlock(q->queue_lock);If it's still bad, what should the fix be? (I'll write a script and post patches)
Quoted 3 lines
This one should not be converted to a
BUG_ON. We always want to call__blk_end_request(), whether we haveCONFIG_BUGor not. This case just wants to test the return value.
It can be converted, the empty BUG_ON() in include/asm-generic/bug.h handles it with
#define BUG_ON(condition) do { if (condition) ; } while(0)cu
Adrian
Signature · 4 lines
"Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
"Only a promise," Lao Er said.
Pearl S. Buck - Dragon SeedQuoted 11 lines · Adrian Bunk
Quoted 9 lines · Matthew Wilcox
Quoted 4 lines · Roel Kluin
for instance in a patch I saw this:
Review
Quoted 2 lines
(quoted diff) +2
-0+ if (__blk_end_request(rq, 0, 0)) + BUG();This one should not be converted to a
BUG_ON. We always want to call__blk_end_request(), whether we haveCONFIG_BUGor not. This case just wants to test the return value.
Quoted 4 lines
It can be converted, the empty
BUG_ON()ininclude/asm-generic/bug.hhandles it with#define BUG_ON(condition) do { if (condition) ; } while(0)
Yes, but it gives the wrong impression.
Signature · 4 lines
Intel are signing my paycheques ... these opinions are still mine "Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step."