linux-kernel - Re: [PATCH] printk: Move printk

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20170714155744.GD32632@pathway.suse.cz>
Date:   Fri, 14 Jul 2017 17:57:44 +0200
From:   Petr Mladek <pmladek@...e.com>
To:     Joe Perches <joe@...ches.com>
Cc:     Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] printk: Move printk_delay to separate file

On Fri 2017-07-07 11:08:59, Joe Perches wrote:
> printk.c is a huge file with too many local functions for a
> human to read and easily parse.
> 
> Start to separate out bits into smaller files.
> 
> Miscellanea:
> 
> o Rename suppress_message_printing to printk_suppress_message

Some renaming might make sense. But please, do it in a separate patch.
It will make the changes much easier to review.

Some existing names are ugly. But there should be a good reason
to rename them, for example, that the existing one causes confusion.
We should be careful here. Otherwise, it will be a nightmare to
search the history or backport important fixes.

Also note that some variables are exported a strange way for
external utilities line crash, makedumpfile, see
log_buf_vmcoreinfo_setup(). Renaming such variables would
require fixing all the external applications.

> o Add function definitions to printk.h

We should not declare functions in a global header without a reason.
It would allow to use them anywhere and it is not always intended
or even safe.

If we want to make some function available in global, we should
do so in a separate patch with a reasonable explanation.

If we want to share them between kernel/printk/*.c sources,
we should declare them in a local header, e.g.
kernel/printk/internal.h

> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index fc47863f629c..b8e63a5f1558 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -1709,8 +1640,7 @@ asmlinkage int vprintk_emit(int facility, int level,
>  		in_sched = true;
>  	}
>  
> -	boot_delay_msec(level);
> -	printk_delay();
> +	printk_delay(level);

This makes me nervous. It is not even documented. Please, do not do
any hidden changes in general! The patch that splits the sources
should only shuffle the code. It might do only some minimal
necessary changes like removing "static" for functions that
newly need to be accessed from other source file.

Best Regards,
Petr

PS: Please, split only the console stuff as a start. It will be
the most helpful thing. Also we are still maintainers beginners.
I would like to start slowly, preferably get some feedback from
more experienced developers, and get one such change into the
mainline before we do many more changes.