Re: [PATCH v6 4/5]: core: Add kernel message dumper to call on oopses and panics

[Linus Torvalds <torvalds@linux-foundation.org>]

On Wed, 14 Oct 2009, Simon Kagstrom wrote:
>
> ChangeLog:
> Review comments from Linus Torvalds and Anders Grafström:
>        * Rename structures and file names
>        * Remove setup callback and unify panic/oops callbacks and
>          instead add a reason parameter
>        * Use a regular spinlock and try it when dumping (fail
>          if held)
>        * Check if the dumper is already registered
>        * Various style fixes/cleanup

Ok, looks fine to me now. 

I do end up having one minor nit: let's change the calling convention of 
the dump function to either be:

	void (*dump)(void *priv, enum kmsg_dump_reason reason,
		const char *s1, unsigned long l1,
		const char *s2, unsigned long l2);

or let's just remove the 'priv' data from the dump entirely. 

Right now, you pass in the whole 'kmsg_dumper' data structure, and then 
you seem to expect that users look up their private context by looking 
into that data structure with 'dumper->priv'. That's just ugly. 

So if you want to have a callback value, just pass that in for the 
callback.

And if you want to pass in the whole 'kmsg_dumper' data structure, then 
use that pointer _itself_ as the context (ie you would embed the 
'kmsg_dumper' in some data structure, and then you do

	struct my_data *my_data = container_of(dumper, struct my_data, dumper);

or something like that.

But your current implementation mixes _both_ of the above approaches. 
Which one are people going to use? Both work, and no, "there are multiple 
ways to do the same thing" is not an advantage, it just leads to 
confusion. And confusion isn't good, whatever the perl people say.

		Linus