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

[Geert Uytterhoeven <geert@linux-m68k.org>]

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@linux-m68k.org

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