* [PATCH] Btrfs: use BUG_ON @ 2014-07-08 22:21 Himangi Saraogi 2014-07-10 0:05 ` Satoru Takeuchi 0 siblings, 1 reply; 3+ messages in thread From: Himangi Saraogi @ 2014-07-08 22:21 UTC (permalink / raw) To: Chris Mason, Josef Bacik, linux-btrfs, linux-kernel; +Cc: julia.lawall 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> 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) -- 1.9.1 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] Btrfs: use BUG_ON 2014-07-08 22:21 [PATCH] Btrfs: use BUG_ON Himangi Saraogi @ 2014-07-10 0:05 ` Satoru Takeuchi 2014-07-10 5:56 ` Julia Lawall 0 siblings, 1 reply; 3+ messages in thread From: Satoru Takeuchi @ 2014-07-10 0:05 UTC (permalink / raw) To: Himangi Saraogi, Chris Mason, Josef Bacik, linux-btrfs, linux-kernel Cc: julia.lawall 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) > ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] Btrfs: use BUG_ON 2014-07-10 0:05 ` Satoru Takeuchi @ 2014-07-10 5:56 ` Julia Lawall 0 siblings, 0 replies; 3+ messages in thread From: Julia Lawall @ 2014-07-10 5:56 UTC (permalink / raw) To: Satoru Takeuchi Cc: Himangi Saraogi, Chris Mason, Josef Bacik, linux-btrfs, linux-kernel 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) > > > > ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2014-07-10 5:56 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-07-08 22:21 [PATCH] Btrfs: use BUG_ON Himangi Saraogi 2014-07-10 0:05 ` Satoru Takeuchi 2014-07-10 5:56 ` Julia Lawall
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).