++ 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/
This archive was generated by hypermail 2b29 : Sun Sep 23 2001 - 21:00:46 EST