Re: [RFC] New kernel-message logging API (take 2)

From: Geert Uytterhoeven
Date: Fri Sep 28 2007 - 03:31:35 EST


On Thu, 27 Sep 2007, Vegard Nossum wrote:
> It should be possible to optimize out multi-line (block) entries
> based on log-level filtering even though the log-level is only given
> in the first call (the initializer). It may take the shape of an
> if-block that spans several macros. This is not very elegant or robust
> if the macros are used incorrectly, however. Aborting a message can
> also be hard this way (since the abort would usually appear inside an
> if-statement that tests for some abnormal condition, thus appear in a
> different block, and thoroughly mess up the bracket order).
>
> Example: {
> #define kprint_block_init(block, loglevel) \
> if(loglevel > CONFIG_KPRINT_LOGLEVEL_MAX) { \
> kprint_real_block_init(block, loglevel);
>
> #define kprint_block(block, fmt, ...) \
> kprint_real_block(block, fmt, ## __VA_ARGS__);
>
> #define kprint_block_flush(block) \
> kprint_real_block_flush(block); \
> }
>
> /* Thus, this C code: */
> kprint_block_init(&block, KPRINT_INFO);
> kprint_block(&block, "Hello world");
> kprint_block_flush(&block);
>
> /* Would pre-process into this: */
> if(6 < 4) {
> kprint_real_block_init(&block, 6);
> kprint_real_block(&block, "Hello world");
> kprint_block_flush(&block);
> }
> }

If-blocks spanning macros are really dangerous!

E.g. an Ethernet driver may want to do:

kprint_block(&block, "MAC ");
for (i = 0; i < 6; i++) {
card->mac[i] = obtain_mac_byte_from_hw(i);
kprint_block(&block, "%02x", card->mac[i]);
}

This looks (and should be) innocent, but the actual MAC addres retrieval
would never be executed if loglevel <= CONFIG_KPRINT_LOGLEVEL_MAX.

Can't you store the loglevel in the kprint_block and check it in all
successive kprint_*() macros? If gcc knows it's constant, it can optimize
the non-wanted code away. As other fields in struct kprint_block cannot be
constant (they store internal state), you have to split it like:

struct kprint_block {
int loglevel;
struct real_kprint_block real; /* internal state */
}

and pass &block.real() instead of &block to all successive internal functions.
I haven't tried this, so let's hope gcc is actually smart enough...

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/