Re: [PATCH] Btrfs: add support for asserts

[Jeff Mahoney <jeffm@suse.com>]

[-- Attachment #1: Type: text/plain, Size: 1985 bytes --]

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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 841 bytes --]