* [PATCH] btrfs-progs: Fix disable backtrace assert error
@ 2016-12-07 1:29 Qu Wenruo
2016-12-07 15:06 ` Goldwyn Rodrigues
0 siblings, 1 reply; 5+ messages in thread
From: Qu Wenruo @ 2016-12-07 1:29 UTC (permalink / raw)
To: linux-btrfs; +Cc: dsterba, Goldwyn Rodrigues
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 BUG_ON() to implement the resting
ASSERT/WARN_ON/BUG(), so we don't need to pass 3 different conditions
but only one.
And to further info the review for the fact that the condition should be
different, rename "assert_trace" to "bugon_trace", as unlike assert, we
will only trigger the bug when condition is true.
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>
---
kerncompat.h | 19 +++++++++++--------
1 file changed, 11 insertions(+), 8 deletions(-)
diff --git a/kerncompat.h b/kerncompat.h
index e374614..be77608 100644
--- a/kerncompat.h
+++ b/kerncompat.h
@@ -277,7 +277,7 @@ static inline long IS_ERR(const void *ptr)
#define vfree(x) free(x)
#ifndef BTRFS_DISABLE_BACKTRACE
-static inline void assert_trace(const char *assertion, const char *filename,
+static inline void bugon_trace(const char *assertion, const char *filename,
const char *func, unsigned line, long val)
{
if (!val)
@@ -287,17 +287,20 @@ static inline void assert_trace(const char *assertion, const char *filename,
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) bugon_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)
#endif
+#define WARN_ON(c) warning_trace(#c, __FILE__, __func__, __LINE__, (long)(c))
+/*
+ * TODO: ASSERT() should be depercated. In case like ASSERT(ret == 0), it
+ * won't output any useful value for ret.
+ * Should be replaced by BUG_ON(ret);
+ */
+#define ASSERT(c) BUG_ON(!(c))
+#define BUG() BUG_ON(1)
+
#define container_of(ptr, type, member) ({ \
const typeof( ((type *)0)->member ) *__mptr = (ptr); \
(type *)( (char *)__mptr - offsetof(type,member) );})
--
2.10.2
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] btrfs-progs: Fix disable backtrace assert error
2016-12-07 1:29 [PATCH] btrfs-progs: Fix disable backtrace assert error Qu Wenruo
@ 2016-12-07 15:06 ` Goldwyn Rodrigues
2016-12-08 0:18 ` Qu Wenruo
0 siblings, 1 reply; 5+ messages in thread
From: Goldwyn Rodrigues @ 2016-12-07 15:06 UTC (permalink / raw)
To: Qu Wenruo, linux-btrfs; +Cc: dsterba
On 12/06/2016 07:29 PM, 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 BUG_ON() to implement the resting
> ASSERT/WARN_ON/BUG(), so we don't need to pass 3 different conditions
> but only one.
>
> And to further info the review for the fact that the condition should be
> different, rename "assert_trace" to "bugon_trace", as unlike assert, we
> will only trigger the bug when condition is true.
>
> 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>
> ---
> kerncompat.h | 19 +++++++++++--------
> 1 file changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/kerncompat.h b/kerncompat.h
> index e374614..be77608 100644
> --- a/kerncompat.h
> +++ b/kerncompat.h
> @@ -277,7 +277,7 @@ static inline long IS_ERR(const void *ptr)
> #define vfree(x) free(x)
>
> #ifndef BTRFS_DISABLE_BACKTRACE
> -static inline void assert_trace(const char *assertion, const char *filename,
> +static inline void bugon_trace(const char *assertion, const char *filename,
> const char *func, unsigned line, long val)
> {
> if (!val)
To keep confusion to the minimum, you can call this *condition instead
of *assertion.
> @@ -287,17 +287,20 @@ static inline void assert_trace(const char *assertion, const char *filename,
> 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) bugon_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)
> #endif
>
> +#define WARN_ON(c) warning_trace(#c, __FILE__, __func__, __LINE__, (long)(c))
> +/*
> + * TODO: ASSERT() should be depercated. In case like ASSERT(ret == 0), it
> + * won't output any useful value for ret.
> + * Should be replaced by BUG_ON(ret);
> + */
> +#define ASSERT(c) BUG_ON(!(c))
I am not sure of this. As you are stating, this (double negation) will
kill the value of the condition. Won't it be better to remove all
ASSERTs first instead of putting this TODO?
> +#define BUG() BUG_ON(1)
> +
> #define container_of(ptr, type, member) ({ \
> const typeof( ((type *)0)->member ) *__mptr = (ptr); \
> (type *)( (char *)__mptr - offsetof(type,member) );})
>
--
Goldwyn
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] btrfs-progs: Fix disable backtrace assert error
2016-12-07 15:06 ` Goldwyn Rodrigues
@ 2016-12-08 0:18 ` Qu Wenruo
2016-12-12 18:00 ` David Sterba
0 siblings, 1 reply; 5+ messages in thread
From: Qu Wenruo @ 2016-12-08 0:18 UTC (permalink / raw)
To: Goldwyn Rodrigues, linux-btrfs; +Cc: dsterba
At 12/07/2016 11:06 PM, Goldwyn Rodrigues wrote:
>
>
> On 12/06/2016 07:29 PM, 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 BUG_ON() to implement the resting
>> ASSERT/WARN_ON/BUG(), so we don't need to pass 3 different conditions
>> but only one.
>>
>> And to further info the review for the fact that the condition should be
>> different, rename "assert_trace" to "bugon_trace", as unlike assert, we
>> will only trigger the bug when condition is true.
>>
>> 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>
>> ---
>> kerncompat.h | 19 +++++++++++--------
>> 1 file changed, 11 insertions(+), 8 deletions(-)
>>
>> diff --git a/kerncompat.h b/kerncompat.h
>> index e374614..be77608 100644
>> --- a/kerncompat.h
>> +++ b/kerncompat.h
>> @@ -277,7 +277,7 @@ static inline long IS_ERR(const void *ptr)
>> #define vfree(x) free(x)
>>
>> #ifndef BTRFS_DISABLE_BACKTRACE
>> -static inline void assert_trace(const char *assertion, const char *filename,
>> +static inline void bugon_trace(const char *assertion, const char *filename,
>> const char *func, unsigned line, long val)
>> {
>> if (!val)
>
> To keep confusion to the minimum, you can call this *condition instead
> of *assertion.
Right, I'll update it.
>
>> @@ -287,17 +287,20 @@ static inline void assert_trace(const char *assertion, const char *filename,
>> 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) bugon_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)
>> #endif
>>
>> +#define WARN_ON(c) warning_trace(#c, __FILE__, __func__, __LINE__, (long)(c))
>> +/*
>> + * TODO: ASSERT() should be depercated. In case like ASSERT(ret == 0), it
>> + * won't output any useful value for ret.
>> + * Should be replaced by BUG_ON(ret);
>> + */
>> +#define ASSERT(c) BUG_ON(!(c))
>
> I am not sure of this. As you are stating, this (double negation) will
> kill the value of the condition. Won't it be better to remove all
> ASSERTs first instead of putting this TODO?
IIRC the ASSERT/BUG_ON will be removed step by step.
And we have about 60+ ASSERT in current code base, not an easy thing to
fix soon.
So I prefer to mark ASSERT() deprecated and remove them in later cleanups.
Thanks,
Qu
>
>
>> +#define BUG() BUG_ON(1)
>> +
>> #define container_of(ptr, type, member) ({ \
>> const typeof( ((type *)0)->member ) *__mptr = (ptr); \
>> (type *)( (char *)__mptr - offsetof(type,member) );})
>>
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] btrfs-progs: Fix disable backtrace assert error
2016-12-08 0:18 ` Qu Wenruo
@ 2016-12-12 18:00 ` David Sterba
2016-12-13 0:59 ` Qu Wenruo
0 siblings, 1 reply; 5+ messages in thread
From: David Sterba @ 2016-12-12 18:00 UTC (permalink / raw)
To: Qu Wenruo; +Cc: Goldwyn Rodrigues, linux-btrfs, dsterba
On Thu, Dec 08, 2016 at 08:18:44AM +0800, Qu Wenruo wrote:
> > On 12/06/2016 07:29 PM, 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 BUG_ON() to implement the resting
> >> ASSERT/WARN_ON/BUG(), so we don't need to pass 3 different conditions
> >> but only one.
> >>
> >> And to further info the review for the fact that the condition should be
> >> different, rename "assert_trace" to "bugon_trace", as unlike assert, we
> >> will only trigger the bug when condition is true.
> >>
> >> 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>
> >> ---
> >> kerncompat.h | 19 +++++++++++--------
> >> 1 file changed, 11 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/kerncompat.h b/kerncompat.h
> >> index e374614..be77608 100644
> >> --- a/kerncompat.h
> >> +++ b/kerncompat.h
> >> @@ -277,7 +277,7 @@ static inline long IS_ERR(const void *ptr)
> >> #define vfree(x) free(x)
> >>
> >> #ifndef BTRFS_DISABLE_BACKTRACE
> >> -static inline void assert_trace(const char *assertion, const char *filename,
> >> +static inline void bugon_trace(const char *assertion, const char *filename,
> >> const char *func, unsigned line, long val)
> >> {
> >> if (!val)
> >
> > To keep confusion to the minimum, you can call this *condition instead
> > of *assertion.
>
> Right, I'll update it.
>
> >
> >> @@ -287,17 +287,20 @@ static inline void assert_trace(const char *assertion, const char *filename,
> >> 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) bugon_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)
> >> #endif
> >>
> >> +#define WARN_ON(c) warning_trace(#c, __FILE__, __func__, __LINE__, (long)(c))
> >> +/*
> >> + * TODO: ASSERT() should be depercated. In case like ASSERT(ret == 0), it
> >> + * won't output any useful value for ret.
> >> + * Should be replaced by BUG_ON(ret);
> >> + */
> >> +#define ASSERT(c) BUG_ON(!(c))
> >
> > I am not sure of this. As you are stating, this (double negation) will
> > kill the value of the condition. Won't it be better to remove all
> > ASSERTs first instead of putting this TODO?
>
> IIRC the ASSERT/BUG_ON will be removed step by step.
> And we have about 60+ ASSERT in current code base, not an easy thing to
> fix soon.
>
> So I prefer to mark ASSERT() deprecated and remove them in later cleanups.
But asserts have their meaning. We want to get rid of BUG_ON that are
abused for error not-handling. The asserts eg. catch programmer errors
like incorrect combinations of function parameters, "explicit checks for
implicit assumptions", and other "never happens" conditions. Anything
that depends on external output must not be hadled via assert/bug_on.
I have some write ups in progress about the coding conventions, will be
put to git once it's more or less complete, so we're all on the same
page.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] btrfs-progs: Fix disable backtrace assert error
2016-12-12 18:00 ` David Sterba
@ 2016-12-13 0:59 ` Qu Wenruo
0 siblings, 0 replies; 5+ messages in thread
From: Qu Wenruo @ 2016-12-13 0:59 UTC (permalink / raw)
To: dsterba, Goldwyn Rodrigues, linux-btrfs
At 12/13/2016 02:00 AM, David Sterba wrote:
> On Thu, Dec 08, 2016 at 08:18:44AM +0800, Qu Wenruo wrote:
>>> On 12/06/2016 07:29 PM, 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 BUG_ON() to implement the resting
>>>> ASSERT/WARN_ON/BUG(), so we don't need to pass 3 different conditions
>>>> but only one.
>>>>
>>>> And to further info the review for the fact that the condition should be
>>>> different, rename "assert_trace" to "bugon_trace", as unlike assert, we
>>>> will only trigger the bug when condition is true.
>>>>
>>>> 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>
>>>> ---
>>>> kerncompat.h | 19 +++++++++++--------
>>>> 1 file changed, 11 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/kerncompat.h b/kerncompat.h
>>>> index e374614..be77608 100644
>>>> --- a/kerncompat.h
>>>> +++ b/kerncompat.h
>>>> @@ -277,7 +277,7 @@ static inline long IS_ERR(const void *ptr)
>>>> #define vfree(x) free(x)
>>>>
>>>> #ifndef BTRFS_DISABLE_BACKTRACE
>>>> -static inline void assert_trace(const char *assertion, const char *filename,
>>>> +static inline void bugon_trace(const char *assertion, const char *filename,
>>>> const char *func, unsigned line, long val)
>>>> {
>>>> if (!val)
>>>
>>> To keep confusion to the minimum, you can call this *condition instead
>>> of *assertion.
>>
>> Right, I'll update it.
>>
>>>
>>>> @@ -287,17 +287,20 @@ static inline void assert_trace(const char *assertion, const char *filename,
>>>> 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) bugon_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)
>>>> #endif
>>>>
>>>> +#define WARN_ON(c) warning_trace(#c, __FILE__, __func__, __LINE__, (long)(c))
>>>> +/*
>>>> + * TODO: ASSERT() should be depercated. In case like ASSERT(ret == 0), it
>>>> + * won't output any useful value for ret.
>>>> + * Should be replaced by BUG_ON(ret);
>>>> + */
>>>> +#define ASSERT(c) BUG_ON(!(c))
>>>
>>> I am not sure of this. As you are stating, this (double negation) will
>>> kill the value of the condition. Won't it be better to remove all
>>> ASSERTs first instead of putting this TODO?
>>
>> IIRC the ASSERT/BUG_ON will be removed step by step.
>> And we have about 60+ ASSERT in current code base, not an easy thing to
>> fix soon.
>>
>> So I prefer to mark ASSERT() deprecated and remove them in later cleanups.
>
> But asserts have their meaning. We want to get rid of BUG_ON that are
> abused for error not-handling. The asserts eg. catch programmer errors
> like incorrect combinations of function parameters, "explicit checks for
> implicit assumptions", and other "never happens" conditions. Anything
> that depends on external output must not be hadled via assert/bug_on.
> I have some write ups in progress about the coding conventions, will be
> put to git once it's more or less complete, so we're all on the same
> page.
>
>
I'm OK to follow the newly established coding standard.
A lot of BUG_ON and ASSERT are abused.
But we still need to fix the behavior first.
Or a lot of normal operation of btrfsck will fail in backtrace disabled
case.
So what about the the patch without the comment?
At least it's easier to maintain.
Thanks,
Qu
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-12-13 0:59 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-12-07 1:29 [PATCH] btrfs-progs: Fix disable backtrace assert error Qu Wenruo
2016-12-07 15:06 ` Goldwyn Rodrigues
2016-12-08 0:18 ` Qu Wenruo
2016-12-12 18:00 ` David Sterba
2016-12-13 0:59 ` Qu Wenruo
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).