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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.LFD.2.01.0910140943040.6146@localhost.localdomain>
Date:	Wed, 14 Oct 2009 09:49:34 -0700 (PDT)
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Simon Kagstrom <simon.kagstrom@...insight.net>
cc:	linux-mtd <linux-mtd@...ts.infradead.org>,
	Ingo Molnar <mingo@...e.hu>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Artem Bityutskiy <dedekind1@...il.com>,
	David Woodhouse <dwmw2@...radead.org>,
	LKML <linux-kernel@...r.kernel.org>,
	"Koskinen Aaro (Nokia-D/Helsinki)" <aaro.koskinen@...ia.com>,
	Alan Cox <alan@...rguk.ukuu.org.uk>
Subject: Re: [PATCH v6 4/5]: core: Add kernel message dumper to call on oopses
 and panics

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/