| Message ID | 20140708222141.GA11854@himangi-Dell (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
Hi Himangi, (2014/07/09 7:21), Himangi Saraogi wrote: > Use BUG_ON(x) rather than if(x) BUG(); > > The semantic patch that fixes this problem is as follows: > > // <smpl> > @@ identifier x; @@ > -if (x) BUG(); > +BUG_ON(x); > // </smpl> Strictly speaking, BUG_ON() is "if (unlikely(x)) BUG". Anyway, I consider that put this condition in unlikely() in this case is good. BTW, there are many "if BUG()" case under fs/btrfs. How about fix all of them? =============================================================================== $ grep -rnH -B 1 "BUG()" fs/btrfs | grep -A 1 'if.*(' fs/btrfs/inode.c-6318- } else if (create && PageUptodate(page)) { fs/btrfs/inode.c:6319: BUG(); -- fs/btrfs/volumes.c-2626- else if (ret) fs/btrfs/volumes.c:2627: BUG(); # <- your patch fixes it. -- fs/btrfs/volumes.c-3092- if (ret == 0) fs/btrfs/volumes.c:3093: BUG(); /* FIXME break ? */ -- fs/btrfs/raid56.c-2048- if (rbio->faila == -1) { fs/btrfs/raid56.c:2049: BUG(); =============================================================================== Thanks, Satoru > > Signed-off-by: Himangi Saraogi <himangi774@gmail.com> > Acked-by: Julia Lawall <julia.lawall@lip6.fr> > --- > fs/btrfs/volumes.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c > index 6104676..63e746e 100644 > --- a/fs/btrfs/volumes.c > +++ b/fs/btrfs/volumes.c > @@ -2623,8 +2623,8 @@ again: > found_key.offset); > if (ret == -ENOSPC) > failed++; > - else if (ret) > - BUG(); > + else > + BUG_ON(ret); > } > > if (found_key.offset == 0) > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 10 Jul 2014, Satoru Takeuchi wrote: > Hi Himangi, > > (2014/07/09 7:21), Himangi Saraogi wrote: > > Use BUG_ON(x) rather than if(x) BUG(); > > > > The semantic patch that fixes this problem is as follows: > > > > // <smpl> > > @@ identifier x; @@ > > -if (x) BUG(); > > +BUG_ON(x); > > // </smpl> > > Strictly speaking, BUG_ON() is "if (unlikely(x)) BUG". > Anyway, I consider that put this condition in unlikely() > in this case is good. > > BTW, there are many "if BUG()" case under fs/btrfs. > How about fix all of them? > > =============================================================================== > $ grep -rnH -B 1 "BUG()" fs/btrfs | grep -A 1 'if.*(' > fs/btrfs/inode.c-6318- } else if (create && PageUptodate(page)) { In the past, BUG_ON (and BUG) could have a definition that would just do nothing. In that case, the call to PageUptodate would go away. The function may do a read barrier, so that could be undesirable. But now I don't see any definition of BUG_ON that discards the condition. So Himangi, you can discard the requirement that the tested expression be an identifier in the semantic patch, and just allow any expression: -if(e) BUG(); +BUG_ON(e); julia > fs/btrfs/inode.c:6319: BUG(); > -- > fs/btrfs/volumes.c-2626- else if (ret) > fs/btrfs/volumes.c:2627: BUG(); # <- your patch > fixes it. > -- > fs/btrfs/volumes.c-3092- if (ret == 0) > fs/btrfs/volumes.c:3093: BUG(); /* FIXME break ? */ > -- > fs/btrfs/raid56.c-2048- if (rbio->faila == -1) { > fs/btrfs/raid56.c:2049: BUG(); > =============================================================================== > > Thanks, > Satoru > > > > > Signed-off-by: Himangi Saraogi <himangi774@gmail.com> > > Acked-by: Julia Lawall <julia.lawall@lip6.fr> > > --- > > fs/btrfs/volumes.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c > > index 6104676..63e746e 100644 > > --- a/fs/btrfs/volumes.c > > +++ b/fs/btrfs/volumes.c > > @@ -2623,8 +2623,8 @@ again: > > found_key.offset); > > if (ret == -ENOSPC) > > failed++; > > - else if (ret) > > - BUG(); > > + else > > + BUG_ON(ret); > > } > > > > if (found_key.offset == 0) > > > > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index 6104676..63e746e 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -2623,8 +2623,8 @@ again: found_key.offset); if (ret == -ENOSPC) failed++; - else if (ret) - BUG(); + else + BUG_ON(ret); } if (found_key.offset == 0)