subject:"\[PATCH 1\/2\] btrfs\-progs\: Correct value printed by assertions\/BUG_ON\/WARN

Re: [PATCH 1/2] btrfs-progs: Correct value printed by assertions/BUG_ON/WARN_ON

2016-12-06 Thread Qu Wenruo




At 12/06/2016 08:44 PM, Goldwyn Rodrigues wrote:



On 12/05/2016 09:08 PM, Qu Wenruo wrote:



At 12/06/2016 10:51 AM, Goldwyn Rodrigues wrote:



On 12/05/2016 08:03 PM, Qu Wenruo wrote:

BTW, the DISABLE_BACKTRACE branch seems quite different from
backtrace one.

#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)
#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)

Condition of BUG_ON/ASSERT/BUG are all logical notted for
DISABLE_BACKTRACE.
While WARN_ON() of both branch are the same condition.


WARN_ON is using warning_trace as opposed to assert, and that is the
reason it is not notted.



This seems quite confusing to me.

Any idea to make it more straightforward?



I just kept it the same as before. warning_trace was using an extra
variable, trace, which was not needed because the print_trace was
already in ifndefs.


I mean, better make the condition the same for both BUG/BUG_ON/ASSERT.
So that we don't need to manually logical not the condition.


First of all, ASSERT and BUG_ON have opposite meanings. ASSERT() checks
if the condition is true and continues (halts if false). BUG_ON() "bugs"
if condition is true and halts (continues if false). So you would have
to use opposite conditions.


I know, I mean, for both backtrace disabled and enabled case, the 
condition should be the same.


If not the same condition, it means assert_trace() has different meaning 
than original assert.






For example:
#define ASSERT(c) assert_trace(#c, __FILE__, __func__, __LINE__,（long）
(c))
and
#define ASSERT(c) assert((c))

This looks much more straightforward, and easier to expose bug at review
time.


Could you explain with a patch? You idea seems to add more code than
reduce it.


Sure, will submit one soon.

Thanks,
Qu


--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] btrfs-progs: Correct value printed by assertions/BUG_ON/WARN_ON

2016-12-06 Thread Goldwyn Rodrigues



On 12/05/2016 09:08 PM, Qu Wenruo wrote:
> 
> 
> At 12/06/2016 10:51 AM, Goldwyn Rodrigues wrote:
>>
>>
>> On 12/05/2016 08:03 PM, Qu Wenruo wrote:
>>> BTW, the DISABLE_BACKTRACE branch seems quite different from
>>> backtrace one.
>>>
>>> #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)
>>> #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)
>>>
>>> Condition of BUG_ON/ASSERT/BUG are all logical notted for
>>> DISABLE_BACKTRACE.
>>> While WARN_ON() of both branch are the same condition.
>>
>> WARN_ON is using warning_trace as opposed to assert, and that is the
>> reason it is not notted.
>>
>>>
>>> This seems quite confusing to me.
>>>
>>> Any idea to make it more straightforward?
>>>
>>
>> I just kept it the same as before. warning_trace was using an extra
>> variable, trace, which was not needed because the print_trace was
>> already in ifndefs.
> 
> I mean, better make the condition the same for both BUG/BUG_ON/ASSERT.
> So that we don't need to manually logical not the condition.

First of all, ASSERT and BUG_ON have opposite meanings. ASSERT() checks
if the condition is true and continues (halts if false). BUG_ON() "bugs"
if condition is true and halts (continues if false). So you would have
to use opposite conditions.

> 
> For example:
> #define ASSERT(c) assert_trace(#c, __FILE__, __func__, __LINE__,（long）
> (c))
> and
> #define ASSERT(c) assert((c))
> 
> This looks much more straightforward, and easier to expose bug at review
> time.

Could you explain with a patch? You idea seems to add more code than
reduce it.


-- 
Goldwyn
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] btrfs-progs: Correct value printed by assertions/BUG_ON/WARN_ON

2016-12-05 Thread Qu Wenruo




At 12/06/2016 10:51 AM, Goldwyn Rodrigues wrote:



On 12/05/2016 08:03 PM, Qu Wenruo wrote:

BTW, the DISABLE_BACKTRACE branch seems quite different from backtrace one.

#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)
#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)

Condition of BUG_ON/ASSERT/BUG are all logical notted for
DISABLE_BACKTRACE.
While WARN_ON() of both branch are the same condition.


WARN_ON is using warning_trace as opposed to assert, and that is the
reason it is not notted.



This seems quite confusing to me.

Any idea to make it more straightforward?



I just kept it the same as before. warning_trace was using an extra
variable, trace, which was not needed because the print_trace was
already in ifndefs.


I mean, better make the condition the same for both BUG/BUG_ON/ASSERT.
So that we don't need to manually logical not the condition.

For example:
#define ASSERT(c) assert_trace(#c, __FILE__, __func__, __LINE__,（long）(c))
and
#define ASSERT(c) assert((c))

This looks much more straightforward, and easier to expose bug at review 
time.


Thanks,
Qu




If you are talking about keeping WARN_ON outside of ifndef, yes, that
will reduce the code further by another line.




--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] btrfs-progs: Correct value printed by assertions/BUG_ON/WARN_ON

2016-12-05 Thread Goldwyn Rodrigues



On 12/05/2016 08:03 PM, Qu Wenruo wrote:
> BTW, the DISABLE_BACKTRACE branch seems quite different from backtrace one.
> 
> #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)
> #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)
> 
> Condition of BUG_ON/ASSERT/BUG are all logical notted for
> DISABLE_BACKTRACE.
> While WARN_ON() of both branch are the same condition.

WARN_ON is using warning_trace as opposed to assert, and that is the
reason it is not notted.

> 
> This seems quite confusing to me.
> 
> Any idea to make it more straightforward?
> 

I just kept it the same as before. warning_trace was using an extra
variable, trace, which was not needed because the print_trace was
already in ifndefs.

If you are talking about keeping WARN_ON outside of ifndef, yes, that
will reduce the code further by another line.

-- 
Goldwyn
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] btrfs-progs: Correct value printed by assertions/BUG_ON/WARN_ON

2016-12-05 Thread Qu Wenruo


BTW, the DISABLE_BACKTRACE branch seems quite different from backtrace one.

#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)
#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)

Condition of BUG_ON/ASSERT/BUG are all logical notted for DISABLE_BACKTRACE.
While WARN_ON() of both branch are the same condition.

This seems quite confusing to me.

Any idea to make it more straightforward?

Thanks,
Qu

At 12/05/2016 07:38 PM, Goldwyn Rodrigues wrote:

Hi Qu,

Yes, the assert for ifdef BTRFS_DIABLE_BACKTRACE is not correct. The
condition should not have a not(!).

Thanks for reporting.

On 12/05/2016 01:10 AM, Qu Wenruo wrote:

Hi, Goldwyn and David,

This patch seems to cause btrfs test case 023 to fail.

Bisect leads me to this patch.


$ ./btrfs check ~/quota_balance_loop_backref.raw.restored
Checking filesystem on /home/adam/quota_balance_loop_backref.raw.restored
UUID: c33c5ce3-3ad9-4320-9201-c337c04e0051
checking extents
btrfs: cmds-check.c:12284: build_roots_info_cache: Assertion `!(ret ==
0)' failed.
Aborted (core dumped)


And gdb backref:
#0  0x76fd204f in raise () from /usr/lib/libc.so.6
#1  0x76fd347a in abort () from /usr/lib/libc.so.6
#2  0x76fcaea7 in __assert_fail_base () from /usr/lib/libc.so.6
#3  0x76fcaf52 in __assert_fail () from /usr/lib/libc.so.6
#4  0x00440426 in build_roots_info_cache (info=0x6f43c0) at
cmds-check.c:12284
#5  0x00440945 in repair_root_items (info=0x6f43c0) at
cmds-check.c:12412
#6  0x004418c3 in cmd_check (argc=2, argv=0x7fffe100) at
cmds-check.c:12892
#7  0x0040a74c in main (argc=2, argv=0x7fffe100) at btrfs.c:301


For frame 4:
(gdb) frame 4
#4  0x00440426 in build_roots_info_cache (info=0x6f43c0) at
cmds-check.c:12284
12284ASSERT(ret == 0);
(gdb) list
12279rii->cache_extent.start = root_id;
12280rii->cache_extent.size = 1;
12281rii->level = (u8)-1;
12282entry = >cache_extent;
12283ret = insert_cache_extent(roots_info_cache, entry);
12284ASSERT(ret == 0);
12285} else {
12286rii = container_of(entry, struct root_item_info,
12287   cache_extent);
12288}
(gdb) print ret
$1 = 0

For me, ASSERT(ret == 0) seems quite safe and common here.
Doesn't the patch changed the ASSERT() behavior?

Thanks,
Qu

At 11/30/2016 12:24 AM, Goldwyn Rodrigues wrote:

From: Goldwyn Rodrigues 

The values passed to BUG_ON/WARN_ON are negated(!) and printed, which
results in printing the value zero for each bug/warning. For example:
volumes.c:988: btrfs_alloc_chunk: Assertion `ret` failed, value 0

This is not useful. Instead changed to print the value of the parameter
passed to BUG_ON()/WARN_ON(). The value needed to be changed to long
to accomodate pointers being passed.

Also, consolidated assert() and BUG() into ifndef.

Signed-off-by: Goldwyn Rodrigues 
---
 kerncompat.h | 35 +++
 1 file changed, 15 insertions(+), 20 deletions(-)

diff --git a/kerncompat.h b/kerncompat.h
index ed9a042..9bd25bd 100644
--- a/kerncompat.h
+++ b/kerncompat.h
@@ -88,39 +88,36 @@ static inline void print_trace(void)
 }

 static inline void assert_trace(const char *assertion, const char
*filename,
-  const char *func, unsigned line, int val)
+  const char *func, unsigned line, long val)
 {
-if (val)
+if (!val)
 return;
 if (assertion)
-fprintf(stderr, "%s:%d: %s: Assertion `%s` failed, value %d\n",
+fprintf(stderr, "%s:%d: %s: Assertion `%s` failed, value %ld\n",
 filename, line, func, assertion, val);
 else
-fprintf(stderr, "%s:%d: %s: Assertion failed, value %d.\n",
+fprintf(stderr, "%s:%d: %s: Assertion failed, value %ld.\n",
 filename, line, func, val);
 print_trace();
 abort();
 exit(1);
 }

-#define BUG() assert_trace(NULL, __FILE__, __func__, __LINE__, 0)
-#else
-#define BUG() assert(0)
 #endif

 static inline void warning_trace(const char *assertion, const char
*filename,
-  const char *func, unsigned line, int val,
+  const char *func, unsigned line, long val,
   int trace)
 {
-if (val)
+if (!val)
 return;
 if (assertion)
 fprintf(stderr,
-"%s:%d: %s: Warning: assertion `%s` failed, value %d\n",
+"%s:%d: %s: Warning: assertion `%s` failed, value %ld\n",
 filename, line, func, assertion,

Re: [PATCH 1/2] btrfs-progs: Correct value printed by assertions/BUG_ON/WARN_ON

2016-12-05 Thread Goldwyn Rodrigues

Hi Qu,

Yes, the assert for ifdef BTRFS_DIABLE_BACKTRACE is not correct. The
condition should not have a not(!).

Thanks for reporting.

On 12/05/2016 01:10 AM, Qu Wenruo wrote:
> Hi, Goldwyn and David,
> 
> This patch seems to cause btrfs test case 023 to fail.
> 
> Bisect leads me to this patch.
> 
> 
> $ ./btrfs check ~/quota_balance_loop_backref.raw.restored
> Checking filesystem on /home/adam/quota_balance_loop_backref.raw.restored
> UUID: c33c5ce3-3ad9-4320-9201-c337c04e0051
> checking extents
> btrfs: cmds-check.c:12284: build_roots_info_cache: Assertion `!(ret ==
> 0)' failed.
> Aborted (core dumped)
> 
> 
> And gdb backref:
> #0  0x76fd204f in raise () from /usr/lib/libc.so.6
> #1  0x76fd347a in abort () from /usr/lib/libc.so.6
> #2  0x76fcaea7 in __assert_fail_base () from /usr/lib/libc.so.6
> #3  0x76fcaf52 in __assert_fail () from /usr/lib/libc.so.6
> #4  0x00440426 in build_roots_info_cache (info=0x6f43c0) at
> cmds-check.c:12284
> #5  0x00440945 in repair_root_items (info=0x6f43c0) at
> cmds-check.c:12412
> #6  0x004418c3 in cmd_check (argc=2, argv=0x7fffe100) at
> cmds-check.c:12892
> #7  0x0040a74c in main (argc=2, argv=0x7fffe100) at btrfs.c:301
> 
> 
> For frame 4:
> (gdb) frame 4
> #4  0x00440426 in build_roots_info_cache (info=0x6f43c0) at
> cmds-check.c:12284
> 12284ASSERT(ret == 0);
> (gdb) list
> 12279rii->cache_extent.start = root_id;
> 12280rii->cache_extent.size = 1;
> 12281rii->level = (u8)-1;
> 12282entry = >cache_extent;
> 12283ret = insert_cache_extent(roots_info_cache, entry);
> 12284ASSERT(ret == 0);
> 12285} else {
> 12286rii = container_of(entry, struct root_item_info,
> 12287   cache_extent);
> 12288}
> (gdb) print ret
> $1 = 0
> 
> For me, ASSERT(ret == 0) seems quite safe and common here.
> Doesn't the patch changed the ASSERT() behavior?
> 
> Thanks,
> Qu
> 
> At 11/30/2016 12:24 AM, Goldwyn Rodrigues wrote:
>> From: Goldwyn Rodrigues 
>>
>> The values passed to BUG_ON/WARN_ON are negated(!) and printed, which
>> results in printing the value zero for each bug/warning. For example:
>> volumes.c:988: btrfs_alloc_chunk: Assertion `ret` failed, value 0
>>
>> This is not useful. Instead changed to print the value of the parameter
>> passed to BUG_ON()/WARN_ON(). The value needed to be changed to long
>> to accomodate pointers being passed.
>>
>> Also, consolidated assert() and BUG() into ifndef.
>>
>> Signed-off-by: Goldwyn Rodrigues 
>> ---
>>  kerncompat.h | 35 +++
>>  1 file changed, 15 insertions(+), 20 deletions(-)
>>
>> diff --git a/kerncompat.h b/kerncompat.h
>> index ed9a042..9bd25bd 100644
>> --- a/kerncompat.h
>> +++ b/kerncompat.h
>> @@ -88,39 +88,36 @@ static inline void print_trace(void)
>>  }
>>
>>  static inline void assert_trace(const char *assertion, const char
>> *filename,
>> -  const char *func, unsigned line, int val)
>> +  const char *func, unsigned line, long val)
>>  {
>> -if (val)
>> +if (!val)
>>  return;
>>  if (assertion)
>> -fprintf(stderr, "%s:%d: %s: Assertion `%s` failed, value %d\n",
>> +fprintf(stderr, "%s:%d: %s: Assertion `%s` failed, value %ld\n",
>>  filename, line, func, assertion, val);
>>  else
>> -fprintf(stderr, "%s:%d: %s: Assertion failed, value %d.\n",
>> +fprintf(stderr, "%s:%d: %s: Assertion failed, value %ld.\n",
>>  filename, line, func, val);
>>  print_trace();
>>  abort();
>>  exit(1);
>>  }
>>
>> -#define BUG() assert_trace(NULL, __FILE__, __func__, __LINE__, 0)
>> -#else
>> -#define BUG() assert(0)
>>  #endif
>>
>>  static inline void warning_trace(const char *assertion, const char
>> *filename,
>> -  const char *func, unsigned line, int val,
>> +  const char *func, unsigned line, long val,
>>int trace)
>>  {
>> -if (val)
>> +if (!val)
>>  return;
>>  if (assertion)
>>  fprintf(stderr,
>> -"%s:%d: %s: Warning: assertion `%s` failed, value %d\n",
>> +"%s:%d: %s: Warning: assertion `%s` failed, value %ld\n",
>>  filename, line, func, assertion, val);
>>  else
>>  fprintf(stderr,
>> -"%s:%d: %s: Warning: assertion failed, value %d.\n",
>> +"%s:%d: %s: Warning: assertion failed, value %ld.\n",
>>  filename, line, func, val);
>>  #ifndef BTRFS_DISABLE_BACKTRACE
>>  if (trace)
>> @@ -299,17 +296,15 @@ static inline long IS_ERR(const void *ptr)
>>  #define vfree(x) free(x)
>>
>>  #ifndef BTRFS_DISABLE_BACKTRACE
>> -#define BUG_ON(c) assert_trace(#c, __FILE__, __func__, __LINE__, !(c))
>> -#define WARN_ON(c)

Re: [PATCH 1/2] btrfs-progs: Correct value printed by assertions/BUG_ON/WARN_ON

2016-12-04 Thread Qu Wenruo


Hi, Goldwyn and David,

This patch seems to cause btrfs test case 023 to fail.

Bisect leads me to this patch.


$ ./btrfs check ~/quota_balance_loop_backref.raw.restored
Checking filesystem on /home/adam/quota_balance_loop_backref.raw.restored
UUID: c33c5ce3-3ad9-4320-9201-c337c04e0051
checking extents
btrfs: cmds-check.c:12284: build_roots_info_cache: Assertion `!(ret == 
0)' failed.

Aborted (core dumped)


And gdb backref:
#0  0x76fd204f in raise () from /usr/lib/libc.so.6
#1  0x76fd347a in abort () from /usr/lib/libc.so.6
#2  0x76fcaea7 in __assert_fail_base () from /usr/lib/libc.so.6
#3  0x76fcaf52 in __assert_fail () from /usr/lib/libc.so.6
#4  0x00440426 in build_roots_info_cache (info=0x6f43c0) at 
cmds-check.c:12284
#5  0x00440945 in repair_root_items (info=0x6f43c0) at 
cmds-check.c:12412
#6  0x004418c3 in cmd_check (argc=2, argv=0x7fffe100) at 
cmds-check.c:12892

#7  0x0040a74c in main (argc=2, argv=0x7fffe100) at btrfs.c:301


For frame 4:
(gdb) frame 4
#4  0x00440426 in build_roots_info_cache (info=0x6f43c0) at 
cmds-check.c:12284

12284   ASSERT(ret == 0);
(gdb) list
12279   rii->cache_extent.start = root_id;
12280   rii->cache_extent.size = 1;
12281   rii->level = (u8)-1;
12282   entry = >cache_extent;
12283   ret = insert_cache_extent(roots_info_cache, 
entry);
12284   ASSERT(ret == 0);
12285   } else {
12286   rii = container_of(entry, struct root_item_info,
12287  cache_extent);
12288   }
(gdb) print ret
$1 = 0

For me, ASSERT(ret == 0) seems quite safe and common here.
Doesn't the patch changed the ASSERT() behavior?

Thanks,
Qu

At 11/30/2016 12:24 AM, Goldwyn Rodrigues wrote:

From: Goldwyn Rodrigues 

The values passed to BUG_ON/WARN_ON are negated(!) and printed, which
results in printing the value zero for each bug/warning. For example:
volumes.c:988: btrfs_alloc_chunk: Assertion `ret` failed, value 0

This is not useful. Instead changed to print the value of the parameter
passed to BUG_ON()/WARN_ON(). The value needed to be changed to long
to accomodate pointers being passed.

Also, consolidated assert() and BUG() into ifndef.

Signed-off-by: Goldwyn Rodrigues 
---
 kerncompat.h | 35 +++
 1 file changed, 15 insertions(+), 20 deletions(-)

diff --git a/kerncompat.h b/kerncompat.h
index ed9a042..9bd25bd 100644
--- a/kerncompat.h
+++ b/kerncompat.h
@@ -88,39 +88,36 @@ static inline void print_trace(void)
 }

 static inline void assert_trace(const char *assertion, const char *filename,
- const char *func, unsigned line, int val)
+ const char *func, unsigned line, long val)
 {
-   if (val)
+   if (!val)
return;
if (assertion)
-   fprintf(stderr, "%s:%d: %s: Assertion `%s` failed, value %d\n",
+   fprintf(stderr, "%s:%d: %s: Assertion `%s` failed, value %ld\n",
filename, line, func, assertion, val);
else
-   fprintf(stderr, "%s:%d: %s: Assertion failed, value %d.\n",
+   fprintf(stderr, "%s:%d: %s: Assertion failed, value %ld.\n",
filename, line, func, val);
print_trace();
abort();
exit(1);
 }

-#define BUG() assert_trace(NULL, __FILE__, __func__, __LINE__, 0)
-#else
-#define BUG() assert(0)
 #endif

 static inline void warning_trace(const char *assertion, const char *filename,
- const char *func, unsigned line, int val,
+ const char *func, unsigned line, long val,
  int trace)
 {
-   if (val)
+   if (!val)
return;
if (assertion)
fprintf(stderr,
-   "%s:%d: %s: Warning: assertion `%s` failed, value %d\n",
+   "%s:%d: %s: Warning: assertion `%s` failed, value 
%ld\n",
filename, line, func, assertion, val);
else
fprintf(stderr,
-   "%s:%d: %s: Warning: assertion failed, value %d.\n",
+   "%s:%d: %s: Warning: assertion failed, value %ld.\n",
filename, line, func, val);
 #ifndef BTRFS_DISABLE_BACKTRACE
if (trace)
@@ -299,17 +296,15 @@ static inline long IS_ERR(const void *ptr)
 #define vfree(x) free(x)

 #ifndef BTRFS_DISABLE_BACKTRACE
-#define BUG_ON(c) assert_trace(#c, __FILE__, __func__, __LINE__, !(c))
-#define WARN_ON(c) warning_trace(#c, __FILE__, __func__, __LINE__, !(c), 1)
+#define BUG_ON(c) assert_trace(#c, __FILE__, __func__, __LINE__, (long)(c))
+#define WARN_ON(c)

Re: [PATCH 1/2] btrfs-progs: Correct value printed by assertions/BUG_ON/WARN_ON

2016-11-30 Thread David Sterba

On Tue, Nov 29, 2016 at 10:24:52AM -0600, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues 
> 
> The values passed to BUG_ON/WARN_ON are negated(!) and printed, which
> results in printing the value zero for each bug/warning. For example:
> volumes.c:988: btrfs_alloc_chunk: Assertion `ret` failed, value 0
> 
> This is not useful. Instead changed to print the value of the parameter
> passed to BUG_ON()/WARN_ON(). The value needed to be changed to long
> to accomodate pointers being passed.
> 
> Also, consolidated assert() and BUG() into ifndef.
> 
> Signed-off-by: Goldwyn Rodrigues 

Applied, thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] btrfs-progs: Correct value printed by assertions/BUG_ON/WARN_ON

2016-11-29 Thread Goldwyn Rodrigues

From: Goldwyn Rodrigues 

The values passed to BUG_ON/WARN_ON are negated(!) and printed, which
results in printing the value zero for each bug/warning. For example:
volumes.c:988: btrfs_alloc_chunk: Assertion `ret` failed, value 0

This is not useful. Instead changed to print the value of the parameter
passed to BUG_ON()/WARN_ON(). The value needed to be changed to long
to accomodate pointers being passed.

Also, consolidated assert() and BUG() into ifndef.

Signed-off-by: Goldwyn Rodrigues 
---
 kerncompat.h | 35 +++
 1 file changed, 15 insertions(+), 20 deletions(-)

diff --git a/kerncompat.h b/kerncompat.h
index ed9a042..9bd25bd 100644
--- a/kerncompat.h
+++ b/kerncompat.h
@@ -88,39 +88,36 @@ static inline void print_trace(void)
 }
 
 static inline void assert_trace(const char *assertion, const char *filename,
- const char *func, unsigned line, int val)
+ const char *func, unsigned line, long val)
 {
-   if (val)
+   if (!val)
return;
if (assertion)
-   fprintf(stderr, "%s:%d: %s: Assertion `%s` failed, value %d\n",
+   fprintf(stderr, "%s:%d: %s: Assertion `%s` failed, value %ld\n",
filename, line, func, assertion, val);
else
-   fprintf(stderr, "%s:%d: %s: Assertion failed, value %d.\n",
+   fprintf(stderr, "%s:%d: %s: Assertion failed, value %ld.\n",
filename, line, func, val);
print_trace();
abort();
exit(1);
 }
 
-#define BUG() assert_trace(NULL, __FILE__, __func__, __LINE__, 0)
-#else
-#define BUG() assert(0)
 #endif
 
 static inline void warning_trace(const char *assertion, const char *filename,
- const char *func, unsigned line, int val,
+ const char *func, unsigned line, long val,
  int trace)
 {
-   if (val)
+   if (!val)
return;
if (assertion)
fprintf(stderr,
-   "%s:%d: %s: Warning: assertion `%s` failed, value %d\n",
+   "%s:%d: %s: Warning: assertion `%s` failed, value 
%ld\n",
filename, line, func, assertion, val);
else
fprintf(stderr,
-   "%s:%d: %s: Warning: assertion failed, value %d.\n",
+   "%s:%d: %s: Warning: assertion failed, value %ld.\n",
filename, line, func, val);
 #ifndef BTRFS_DISABLE_BACKTRACE
if (trace)
@@ -299,17 +296,15 @@ static inline long IS_ERR(const void *ptr)
 #define vfree(x) free(x)
 
 #ifndef BTRFS_DISABLE_BACKTRACE
-#define BUG_ON(c) assert_trace(#c, __FILE__, __func__, __LINE__, !(c))
-#define WARN_ON(c) warning_trace(#c, __FILE__, __func__, __LINE__, !(c), 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), 
1)
+#defineASSERT(c) assert_trace(#c, __FILE__, __func__, __LINE__, 
(long)!(c))
+#define BUG() assert_trace(NULL, __FILE__, __func__, __LINE__, 1)
 #else
 #define BUG_ON(c) assert(!(c))
-#define WARN_ON(c) warning_trace(#c, __FILE__, __func__, __LINE__, !(c), 0)
-#endif
-
-#ifndef BTRFS_DISABLE_BACKTRACE
-#defineASSERT(c) assert_trace(#c, __FILE__, __func__, __LINE__, (c))
-#else
-#define ASSERT(c) assert(c)
+#define WARN_ON(c) warning_trace(#c, __FILE__, __func__, __LINE__, (long)(c), 
0)
+#define ASSERT(c) assert(!(c))
+#define BUG() assert(0)
 #endif
 
 #define container_of(ptr, type, member) ({  \
-- 
2.10.0

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] btrfs-progs: Correct value printed by assertions/BUG_ON/WARN_ON

Re: [PATCH 1/2] btrfs-progs: Correct value printed by assertions/BUG_ON/WARN_ON

Re: [PATCH 1/2] btrfs-progs: Correct value printed by assertions/BUG_ON/WARN_ON

Re: [PATCH 1/2] btrfs-progs: Correct value printed by assertions/BUG_ON/WARN_ON

Re: [PATCH 1/2] btrfs-progs: Correct value printed by assertions/BUG_ON/WARN_ON

Re: [PATCH 1/2] btrfs-progs: Correct value printed by assertions/BUG_ON/WARN_ON

Re: [PATCH 1/2] btrfs-progs: Correct value printed by assertions/BUG_ON/WARN_ON

Re: [PATCH 1/2] btrfs-progs: Correct value printed by assertions/BUG_ON/WARN_ON

[PATCH 1/2] btrfs-progs: Correct value printed by assertions/BUG_ON/WARN_ON

9 matches

Site Navigation

Mail list logo

Footer information