Re: [PATCH 3 of 3] xen/debug: Introduce ASSERT_RUN()

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 15/10/12 10:23, Jan Beulich wrote:
>>>> On 08.10.12 at 20:16, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> This is a variant of ASSERT() which takes a predicate, a function an
>> argument for the function.  It is designed for debugging in situations
>> where ASSERT_PRINTK() is perhaps not powerful enough.
>>
>> It will run the given function with the given argument before the BUG()
>> which kills Xen.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>
>> diff -r 477ccdb9870e -r 0a1a3f35f56a xen/include/xen/lib.h
>> --- a/xen/include/xen/lib.h
>> +++ b/xen/include/xen/lib.h
>> @@ -48,6 +48,16 @@ do {                                    
>>  } while (0)
>>  #endif /* assert_printk_failed */
>>  
>> +#ifndef assert_run_failed
>> +#define assert_run_failed(p, func, arg)                         \
>> +do {                                                            \
>> +    printk("Assertion '%s' failed, line %d, file %s\n", p ,     \
>> +                   __LINE__, __FILE__);                         \
>> +    (func)((arg));                                              \
> Quite a few pointless parentheses here.

Ok

>
>> +    BUG();                                                      \
>> +} while (0)
>> +#endif /* assert_run_failed */
>> +
>>  #ifdef CONFIG_ASSERTS
>>  #define ASSERT(p) \
>>      do { if ( unlikely(!(p)) ) assert_failed(#p); } while (0)
>> @@ -55,9 +65,15 @@ do {                                    
>>  #define ASSERT_PRINTK(p, ...)                                   \
>>      do { if ( unlikely(!(p)) )                                  \
>>              assert_printk_failed(#p, __VA_ARGS__); } while (0)
>> +
>> +/* func expected to be void, taking a single void * argument */
>> +#define ASSERT_RUN(p, func, arg)                                \
> Since the user of the construct specifies both func and arg, I don't
> see the need to specify what type they are. Nor is it meaningful
> here whether the function returns void (it would at most need to
> be stated - I'd consider this mostly obvious - that an eventual
> return value doesn't get used).

Good point

>
>> +    do { if ( unlikely(!(p)) )                                  \
>> +            assert_run_failed(#p, func, arg); } while (0)
>>  #else
>>  #define ASSERT(p) do { if ( 0 && (p) ); } while (0)
>>  #define ASSERT_PRINTK(p, ...) do { if ( 0 && (p) ); } while (0)
>> +#define ASSERT_RUN(p, func, arg) do { if ( 0 && (p) ); } while (0)
> You shall evaluate func and arg (i.e. invoke func(arg)) in the
> (dead) if() body, to both avoid the need for #ifdef-s at use
> sites and check type compatibility even in non-debug (or non-
> assert, following your earlier patch) builds.
>
> Jan

I presume that you mean I should?  Why would that prevent the need for
#ifdefs? I can see the argument for type compatibility.

~Andrew

>
>>  #endif
>>  
>>  #define ABS(_x) ({                              \
>
>

-- 
Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer
T: +44 (0)1223 225 900, http://www.citrix.com