if(function()) BUG(); still bad?

if(function()) BUG(); still bad?

[Roel Kluin]

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:

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

Re: if(function()) BUG(); still bad?

[Matthew Wilcox]

On Mon, Dec 03, 2007 at 01:20:20AM +0100, Roel Kluin wrote:
> for instance in a patch I saw this:
> 
> > +		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.

-- 
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."

Re: if(function()) BUG(); still bad?

[Roel Kluin]

Matthew Wilcox wrote:
> On Mon, Dec 03, 2007 at 01:20:20AM +0100, Roel Kluin wrote:
>> for instance in a patch I saw this:
>>
>>> +		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.
> 

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.

Re: if(function()) BUG(); still bad?

[Adrian Bunk]

On Sun, Dec 02, 2007 at 05:32:22PM -0700, Matthew Wilcox wrote:
> On Mon, Dec 03, 2007 at 01:20:20AM +0100, Roel Kluin wrote:
> > for instance in a patch I saw this:
> > 
> > > +		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.

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

-- 

       "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 Seed

Re: if(function()) BUG(); still bad?

[Matthew Wilcox]

On Mon, Dec 03, 2007 at 02:18:35AM +0100, Adrian Bunk wrote:
> On Sun, Dec 02, 2007 at 05:32:22PM -0700, Matthew Wilcox wrote:
> > On Mon, Dec 03, 2007 at 01:20:20AM +0100, Roel Kluin wrote:
> > > for instance in a patch I saw this:
> > > 
> > > > +		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 have CONFIG_BUG or 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)

Yes, but it gives the wrong impression.

-- 
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."