lkml.org 
[lkml]   [2001]   [Sep]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] Magic SysRq loglevel fix.
    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.

    > > There was a time when I thought that was a good idea.
    > >
    > > Go ahead, laugh.

    Nah, I don't want to laugh at it. More like cry at it.
    It's set me back from other work by too many hours already.

    > > Anyway, that sort of unresolved state is bad, and is the source of all
    > > of this song and dance. I think the right answer is to force handlers to
    > > open their own calls to printk, and to keep whats going on with the
    > > console_loglevel and printk buffer nice and clean.
    > >
    > > The cost is that messages like this:
    > >
    > > SysRq : Loglevel switched to X
    > >
    > > will have to become more like this:
    > >
    > > SysRq : Loglevel
    > > Loglevel switched to X

    Yes, that's ugly and shouldn't be done.

    I made an OK state machine (previous and next states) of it.
    Your patch moves restoring console_loglevel to a place
    that makes sense.

    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.

    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.

    ~Randy
    -
    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/

    \
     
     \ /
      Last update: 2005-03-22 13:03    [W:0.028 / U:2.152 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site