On 8/27/13 9:47 AM, Josef Bacik wrote:
> On Mon, Aug 26, 2013 at 02:53:26PM -0700, Zach Brown wrote:
>>> With this we can
>>> go through and convert any BUG_ON()'s that we have to catch actual programming
>>> mistakes to the new ASSERT() and then fix everybody else to return errors.
>>
>> I like the sound of that!
>>
>>> --- a/fs/btrfs/ctree.h
>>> +++ b/fs/btrfs/ctree.h
>>> @@ -3814,6 +3814,22 @@ void btrfs_printk(const struct btrfs_fs_info *fs_info, const char *fmt, ...)
>>> #define btrfs_debug(fs_info, fmt, args...) \
>>> btrfs_printk(fs_info, KERN_DEBUG fmt, ##args)
>>>
>>> +#ifdef BTRFS_ASSERT
>>> +
>>> +static inline void assfail(char *expr, char *file, int lin)
>>> +{
>>> + printk(KERN_ERR "BTRFS assertion failed: %s, file: %s, line: %d",
>>> + expr, file, line);
>>> + BUG();
>>> +}
>>
>> I'm not sure why this is needed.
>>
>>> +#define ASSERT(expr) \
>>> + (unlikely(expr) ? (void)0 : assfail(#expr, __FILE__, __LINE__))
>>
>> (Passing the assertion is unlikely()? I know, this is from xfs...
>> still.)
>>
>
> Yeah I copy+pasted and then thought about it after I sent it. I will fix it up.
>
>>> +#else
>>> +#define ASSERT(expr) ((void)0)
>>> +#endif
>>
>> Anyway, if you're going to do it this way, why not:
>>
>> #ifdef BTRFS_ASSERT
>> #define btrfs_assert(cond) BUG_ON(!(cond))
>> #else
>> #define btrfs_assert(cond) do { if (cond) ; } while (0)
>> #endif
>>
>
> I like the verbosity, especially with random kernel versions and such, it will
> help me figure out where we BUG_ON()'ed without having to checkout a particular
> version and go hunting. Thanks,
Agreed. One of the positives of the obnoxious reiserfs warning IDs is
that it uniquely identifies a call site across kernel versions. You can
tell at a glance that it's the same failure you may have been chasing
for a while. Anything to make the ID-at-a-glance easy is worth it.
-Jeff
--
Jeff Mahoney
SUSE Labs
Attachment:
signature.asc
Description: OpenPGP digital signature
