Re: [patch 2/3] Add a WARN() macro; this is WARN_ON() + printk arguments

["Vegard Nossum" <vegard.nossum@gmail.com>]

Hi!

On Wed, May 7, 2008 at 8:21 AM, Arjan van de Ven <arjan@infradead.org> wrote:
> Subject: Add a WARN() macro; this is WARN_ON() + printk arguments
>  From: Arjan van de Ven <arjan@linux.intel.com>
>
>  Add a WARN() macro that acts like WARN_ON(), with the added feature that
>  it takes a printk like argument that is printed as part of the warning
>  message.

[...]

>  +#ifndef WARN
>  +#define WARN(condition, format...) ({                                          \
>  +       int __ret_warn_on = !!(condition);                              \
>  +       if (unlikely(__ret_warn_on))                                    \
>  +               __WARN_printf(format);                                  \
>  +       unlikely(__ret_warn_on);                                        \
>  +})
>  +#endif

Is there a good reason why this is not a static inline function? If
I've understood correctly, we want to turn as many macros as possible
into functions, and I don't see an immediate reason why this one can't
be one.

>  +void warn_slowpath(const char *file, int line, const char *fmt, ...)
>  +{
>  +       va_list args;
>  +
>  +
>  +       char function[KSYM_SYMBOL_LEN];
>  +       unsigned long caller = (unsigned long) __builtin_return_address(0);

If WARN() is made a static inline, you can call
__builtin_return_address(0) there and pass it into here instead. This
seems like a kind of low-level internal function anyway, because of
the file/line info.

OTOH, why can't you use __FUNCTION__ or __func__ to determine the
caller (in WARN) rather than doing it here, at run-time? If it's to
save space (or something like that), I think it should be documented?

>  +       sprint_symbol(function, caller);
>  +
>  +       printk(KERN_WARNING "------------[ cut here ]------------\n");
>  +       printk(KERN_WARNING "WARNING: at %s:%d %s()\n", file,
>  +               line, function);
>  +       va_start(args, fmt);
>  +       vprintk(fmt, args);
>  +       va_end(args);
>  +
>  +       print_modules();
>  +       dump_stack();
>  +       print_oops_end_marker();
>  +       add_taint(TAINT_WARN);
>  +}
>  +EXPORT_SYMBOL(warn_slowpath);
>   #endif


Vegard

-- 
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
	-- E. W. Dijkstra, EWD1036