| 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
| ||
|
Date: Wed, 1 Jul 2009 00:25:20 +0200
From: Ingo Molnar <mingo@...e.hu>
To: Robin Getz <rgetz@...ckfin.uclinux.org>,
Linus Torvalds <torvalds@...ux-foundation.org>,
Andrew Morton <akpm@...ux-foundation.org>
Cc: linux-kernel@...r.kernel.org
Subject: Re: RFC - printk handling more than one CON_BOOT
* Robin Getz <rgetz@...ckfin.uclinux.org> wrote:
> Today, register_console() assumes the following usage (with
> respect to boot consoles/early_printk).
>
> The first console to register with register_console() with a flags
> set to CON_BOOT is the one and only bootconsole.
>
> If another register_console() is called with an additional
> CON_BOOT, today it is silently rejected.
>
> As soon as a console without the CON_BOOT set calls
> register_console(), the one and only bootconsole is automatically
> unregistered.
>
> Once there is a "standard" console - register_console() will
> silently reject any consoles with it's CON_BOOT, set.
>
>
> This changeset allows multiple boot consoles, and changes the
> functionality to, be mostly the same as the above.
>
> Any number of bootconsoles can be registered. A "real" console
> will unregister all the bootconsoles. Once a "real" console is
> registered, no more bootconsoles can be added.
>
> Thoughts?
>
> The use case is to have a console buffer which goes to serial, and
> a console buffer which goes to a fixed memory buffer at the same
> time. (serial is what most people use, but if serial is hosed for
> some reason, having things in a buffer (which gets printed out by
> the bootloader) is the only way to tell what is going on).
Looks genuinely useful ...
> If you don't object I will send a properly formatted patch...
Find some minor comments below:
> -------------
>
>
> Index: kernel/printk.c
> ===================================================================
> --- kernel/printk.c (revision 6860)
> +++ kernel/printk.c (working copy)
> @@ -1126,9 +1126,24 @@
> unsigned long flags;
> struct console *bootconsole = NULL;
>
> + /* before we register a new CON_BOOT console, make sure we don't
> + * already have a valid console
> + */
please use the customary comment style:
/*
* Comment .....
* ...... goes here:
*/
specified in Documentation/CodingStyle.
> if (console_drivers) {
> - if (console->flags & CON_BOOT)
> - return;
> + if (console->flags & CON_BOOT) {
> + for (bootconsole = console_drivers;
> + bootconsole != NULL;
> + bootconsole = bootconsole->next) {
> + if (!(bootconsole->flags & CON_BOOT))
> + break;
> + }
> + if ((console->flags & CON_BOOT) &&
> + !(bootconsole->flags & CON_BOOT))
> + return;
> + if (!(bootconsole->flags & CON_BOOT))
> + bootconsole = NULL;
> + }
This is really ugly to read mainly because the meat of the function,
the loop over all console drivers, is two indentation levels to the
right which creates line break artifacts. I'd suggest to put this
portion into a helper inline.
Also, the name 'bootconsole' is way too long for an iterator.
Something like 'con' would be more than enough. (and the new console
that is being registered could be new_con or so, to bring it in line
with this naming.)
Also, the new semantics here seem overly complex. Why not have a
state variable that tells us whether we are in the early boot phase
or not and warn about early consoles that get registered too late
and real consoles that get registered too early?
That way the CON_BOOT flag is really reduntant in terms of
semantics: all consoles registered before console_init() are early
ones, and all consoles registered after that are final consoles. (or
something like that)
Is there any reason why we'd want to do something more complex than
such simple rules?
> +
> if (console_drivers->flags & CON_BOOT)
> bootconsole = console_drivers;
> }
> @@ -1195,15 +1210,32 @@
> if (!(console->flags & CON_ENABLED))
> return;
>
> - if (bootconsole && (console->flags & CON_CONSDEV)) {
> - printk(KERN_INFO "console handover: boot [%s%d] -> real [%s%d]\n",
> - bootconsole->name, bootconsole->index,
> - console->name, console->index);
> - unregister_console(bootconsole);
> + if (bootconsole && (console->flags & CON_CONSDEV) &&
> + !(console->flags & CON_BOOT)) {
> + /* we need to iterate through twice, to make sure we print
> + * everything out, before we unregister the console(s)
> + */
(please fix the comment.)
> + printk(KERN_INFO "console handover: ");
> + for (bootconsole = console_drivers; bootconsole != NULL;
> + bootconsole = bootconsole->next) {
> + if (bootconsole->flags & CON_BOOT)
> + printk("boot [%s%d] ", bootconsole->name,
> + bootconsole->index);
> + }
> + printk(" -> real [%s%d]\n", console->name, console->index);
> + for (bootconsole = console_drivers; bootconsole != NULL;
> + bootconsole = bootconsole->next) {
> + if (bootconsole->flags & CON_BOOT)
> + unregister_console(bootconsole);
> + }
this too should probably move into a helper inline.
> console->flags &= ~CON_PRINTBUFFER;
> } else {
> - printk(KERN_INFO "console [%s%d] enabled\n",
> - console->name, console->index);
> + if (console->flags & CON_BOOT)
> + printk(KERN_INFO "bootconsole [%s%d] enabled\n",
> + console->name, console->index);
> + else
> + printk(KERN_INFO "console [%s%d] enabled\n",
> + console->name, console->index);
multi-line branch statemens should preferably come with curly
braces. And this could probably be written simpler as well, as:
printk(KERN_INFO "%sconsole [%s%d] enabled\n",
(con->flags & CON_BOOT) ? "boot" : "",
con->name, con->index);
Ingo
--
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/
Powered by blists - more mailing lists