Messages in this thread Patch in this message | | | Date | Fri, 14 Jul 2017 17:57:44 +0200 | From | Petr Mladek <> | 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.
| |