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

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

On 15/10/12 10:17, 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, and a variable
>> number of arguments which get fed to prink() before the BUG().
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>
>> --
>> This does use C99 varadic macros, but given that we use other C99
>> features without #ifdef guards, I felt it not necessary to guard this as
>> well.
>>
>> diff -r 2927e18e9a7c -r 477ccdb9870e xen/include/xen/lib.h
>> --- a/xen/include/xen/lib.h
>> +++ b/xen/include/xen/lib.h
>> @@ -38,11 +38,26 @@ do {                                    
>>  } while (0)
>>  #endif
>>  
>> +#ifndef assert_printk_failed
>> +#define assert_printk_failed(p, ...)                            \
>> +do {                                                            \
>> +    printk("Assertion '%s' failed, line %d, file %s\n", p ,     \
>> +                   __LINE__, __FILE__);                         \
>> +    printk(__VA_ARGS__);                                        \
> The first argument here necessarily is a format string, so it
> should also be enforced that way.

Except for the trailing comma issue present in C99 varadic macros, which
is why it is specified this way.

#define COMMA(fmt, ...) printf(fmt, _VA_ARGS__);

Calling COMMA("foobar") will expand to 'printf("foobar",);' leading to a
syntax error.  There is a GCCism which fixes this issue, but it is not
portable.


>  Which then opens the
> question whether the two printk()-s shouldn't be folded (at the
> price of requiring the format string to be a literal).

I would err away from that option if possible, just for flexibility sake.

>
> I wonder though whether we wouldn't be better off following
> Linux'es WARN() et al infrastructure, rather than extending the
> ASSERT() one.
>
> Jan

Keir implied that this might like to be extended to BUG()s and WARN()s,
which I am happy to do if that is the consensus.

~Andrew

>
>> +    BUG();                                                      \
>> +} while (0)
>> +#endif /* assert_printk_failed */
>> +
>>  #ifdef CONFIG_ASSERTS
>>  #define ASSERT(p) \
>>      do { if ( unlikely(!(p)) ) assert_failed(#p); } while (0)
>> +
>> +#define ASSERT_PRINTK(p, ...)                                   \
>> +    do { if ( unlikely(!(p)) )                                  \
>> +            assert_printk_failed(#p, __VA_ARGS__); } while (0)
>>  #else
>>  #define ASSERT(p) do { if ( 0 && (p) ); } while (0)
>> +#define ASSERT_PRINTK(p, ...) do { if ( 0 && (p) ); } while (0)
>>  #endif
>>  
>>  #define ABS(_x) ({                              \
>
>

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