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

Linus Torvalds
2009-10-14T16:49:34Z

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