[v4] btrfs-progs: Fix disable backtrace assert error
diff mbox

Message ID 20170119040003.22773-1-quwenruo@cn.fujitsu.com
State Accepted
Headers show

Commit Message

Qu Wenruo Jan. 19, 2017, 4 a.m. UTC
Due to commit 00e769d04c2c83029d6c71(btrfs-progs: Correct value printed
by assertions/BUG_ON/WARN_ON), which changed the assert_trace()
parameter, the condition passed to assert/WARN_ON/BUG_ON are logical
notted for backtrace enabled and disabled case.

Such behavior makes us easier to pass value wrong, and in fact it did
cause us to pass wrong condition for ASSERT().

Instead of passing different conditions for ASSERT/WARN_ON/BUG_ON()
manually, this patch will use ASSERT() to implement the resting
ASSERT/WARN_ON/BUG() for disable backtrace case, and use assert_trace()
to implement ASSERT() and BUG_ON(), to allow them to print correct
value.

Also, move WARN_ON() out of the ifdef branch, as it's completely the
same for both branches.

Cc: Goldwyn Rodrigues <rgoldwyn@suse.de>
Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
v2:
  Keep ASSERT() outputing meaningful error string, use ASSERT() to
  implement BUG_ON() so only the abused BUG_ON() output is affected.
  Suggested by David.
v3:
  Update commit message, since we use ASSERT() instead of BUG_ON() as
  main assert function now.

v4:
  Make BUG_ON() to print correct condition.
---
 kerncompat.h | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

Comments

David Sterba Jan. 24, 2017, 11:55 a.m. UTC | #1
On Thu, Jan 19, 2017 at 12:00:03PM +0800, Qu Wenruo wrote:
> Due to commit 00e769d04c2c83029d6c71(btrfs-progs: Correct value printed
> by assertions/BUG_ON/WARN_ON), which changed the assert_trace()
> parameter, the condition passed to assert/WARN_ON/BUG_ON are logical
> notted for backtrace enabled and disabled case.
> 
> Such behavior makes us easier to pass value wrong, and in fact it did
> cause us to pass wrong condition for ASSERT().
> 
> Instead of passing different conditions for ASSERT/WARN_ON/BUG_ON()
> manually, this patch will use ASSERT() to implement the resting
> ASSERT/WARN_ON/BUG() for disable backtrace case, and use assert_trace()
> to implement ASSERT() and BUG_ON(), to allow them to print correct
> value.
> 
> Also, move WARN_ON() out of the ifdef branch, as it's completely the
> same for both branches.
> 
> Cc: Goldwyn Rodrigues <rgoldwyn@suse.de>
> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>

Applied, thanks.
--
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
David Sterba Jan. 24, 2017, 12:19 p.m. UTC | #2
On Tue, Jan 24, 2017 at 12:55:22PM +0100, David Sterba wrote:
> On Thu, Jan 19, 2017 at 12:00:03PM +0800, Qu Wenruo wrote:
> > Due to commit 00e769d04c2c83029d6c71(btrfs-progs: Correct value printed
> > by assertions/BUG_ON/WARN_ON), which changed the assert_trace()
> > parameter, the condition passed to assert/WARN_ON/BUG_ON are logical
> > notted for backtrace enabled and disabled case.
> > 
> > Such behavior makes us easier to pass value wrong, and in fact it did
> > cause us to pass wrong condition for ASSERT().
> > 
> > Instead of passing different conditions for ASSERT/WARN_ON/BUG_ON()
> > manually, this patch will use ASSERT() to implement the resting
> > ASSERT/WARN_ON/BUG() for disable backtrace case, and use assert_trace()
> > to implement ASSERT() and BUG_ON(), to allow them to print correct
> > value.
> > 
> > Also, move WARN_ON() out of the ifdef branch, as it's completely the
> > same for both branches.
> > 
> > Cc: Goldwyn Rodrigues <rgoldwyn@suse.de>
> > Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
> 
> Applied, thanks.

And FYI, I've added a trace dump for BUG_ON and removed the value
negation from ASSERT.
--
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

Patch
diff mbox

diff --git a/kerncompat.h b/kerncompat.h
index 19ed3fc0..b4070dac 100644
--- a/kerncompat.h
+++ b/kerncompat.h
@@ -291,18 +291,16 @@  static inline void assert_trace(const char *assertion, const char *filename,
 	abort();
 	exit(1);
 }
-
-#define BUG_ON(c) assert_trace(#c, __FILE__, __func__, __LINE__, (long)(c))
-#define WARN_ON(c) warning_trace(#c, __FILE__, __func__, __LINE__, (long)(c))
 #define	ASSERT(c) assert_trace(#c, __FILE__, __func__, __LINE__, (long)!(c))
-#define BUG() assert_trace(NULL, __FILE__, __func__, __LINE__, 1)
+#define BUG_ON(c) assert_trace(#c, __FILE__, __func__, __LINE__, (long)(c))
 #else
-#define BUG_ON(c) assert(!(c))
-#define WARN_ON(c) warning_trace(#c, __FILE__, __func__, __LINE__, (long)(c))
-#define ASSERT(c) assert(!(c))
-#define BUG() assert(0)
+#define ASSERT(c) assert(c)
+#define BUG_ON(c) ASSERT(!(c))
 #endif
 
+#define BUG() BUG_ON(1)
+#define WARN_ON(c) warning_trace(#c, __FILE__, __func__, __LINE__, (long)(c))
+
 #define container_of(ptr, type, member) ({                      \
         const typeof( ((type *)0)->member ) *__mptr = (ptr);    \
 	        (type *)( (char *)__mptr - offsetof(type,member) );})