Messages in this thread |  | | Date | Sat, 22 Sep 2001 01:16:39 -0400 | From | Crutcher Dunnavant <> | Subject | Re: [PATCH] Magic SysRq loglevel fix. |
| |
++ 21/09/01 18:25 -0700 - Randy.Dunlap: > Crutcher Dunnavant wrote: > > > > Attached is a fix for this part of the sysrq code. > > > > ++ 21/09/01 17:08 -0400 - Crutcher Dunnavant: > > > ++ 19/09/01 08:56 -0700 - Randy.Dunlap: > > > > It always sets console_loglevel and then restores > > > > console_loglevel from orig_log_level, so Alt+SysRq+# > > > > handling is severely broken. > > > > > > > > If someone (Crutcher ?) wants to patch it, that's fine. > > > > If I patched it, I would just add a > > > > next_loglevel = -1; > > > > at the beginning of __handle_sysrq_nolock() and then > > > > let the loglevel handler(s) set next_loglevel. > > > > If next_loglevel != -1 at the end of __handle_sysrq_nolock(), > > > > set console_loglevel to next_loglevel. > > > > > > I'm looking real close at this right now, and there are a couple of > > > problems, and a simple, but ugly solution. > > > > > > The entire reason that console_loglevel is touched _after_ the call to > > > the second level handler is actually for the loglevel handler's > > > printout. I was trying to minimize change in the display, but horked it. > > > > > > Here is the problem. > > > > > > SysRq events use action messages which get printed by the top level > > > handler before calling the second level handler, the call line is: > > > > > > orig_log_level = console_loglevel; > > > console_loglevel = 7; > > > printk(KERN_INFO "SysRq : "); > > > > > > op_p = __sysrq_get_key_op(key); > > > ... > > > printk ("%s", op_p->action_msg); > > > op_p->handler(key, pt_regs, kbd, tty); > > > ... > > > console_loglevel = orig_log_level; > > > > > > > > > The killer here is the fact that the action message format string does > > > not carry a newline, allowing people to register strings which leave the > > > printk state open. The loglevel handler then fills in the loglevel, and > > > closes the printk state. > > /me switches to a decent keyboard to test with. > > No, the killer is that console_loglevel is restored from > orig_log_level after having been modified in the loglevel > handler.
But the _reason_ is that the action message does not carry a new line.
> Bottom line: I don't care which code goes into the sysrq.c, > but I hope that you (and others) learn to do some basic testing > before unleashing it. I don't expect all Linux kernel code > to be thoroughly tested before it is added to a kernel, > especially for areas like VM and file systems. > But some basic level of testing should have been done on it, > and I can't tell that it was done.
A whole lot of basic testing was done. The locking and the tables and the registration all work correctly. I screwed up the loglevel stuff, and it took 6 months for anyone to notice. Some people think that the registration functions should be valid to call even when CONFIG_MAGIC_SYSRQ is not defined, but this is a style thing.
Waving your hands and saying "Why wasn't this tested!!!" over and over is annoying, and doesn't accomplish anything.
> There is still room for some more/small improvements here. Nothing > earth-shattering. For example, go_sync() and do_emergency_sync() > don't need to save console_loglevel or set it to 7 (they have both > already been done in __handle_sysrq_nolock()). > My patch eliminated this cruft.
No. go_sync() and do_emergency_sync() should NOT make assumptions about the nature of console_loglevel, they should behave as they have been. The commingling of loglevel settings was wrong in the first place.
-- Crutcher <crutcher@datastacks.com> GCS d--- s+:>+:- a-- C++++$ UL++++$ L+++$>++++ !E PS+++ PE Y+ PGP+>++++ R-(+++) !tv(+++) b+(++++) G+ e>++++ h+>++ r* y+>*$ - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
|  |