Re: [PATCH v8 04/13] module: Move livepatch support to a separate file

From: Petr Mladek <pmladek@suse.com>
To: Christophe Leroy <christophe.leroy@csgroup.eu>
Cc: Aaron Tomlin <atomlin@redhat.com>,
	"mcgrof@kernel.org" <mcgrof@kernel.org>,
	"cl@linux.com" <cl@linux.com>, "mbenes@suse.cz" <mbenes@suse.cz>,
	"akpm@linux-foundation.org" <akpm@linux-foundation.org>,
	"jeyu@kernel.org" <jeyu@kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-modules@vger.kernel.org" <linux-modules@vger.kernel.org>,
	"void@manifault.com" <void@manifault.com>,
	"atomlin@atomlin.com" <atomlin@atomlin.com>,
	"allen.lkml@gmail.com" <allen.lkml@gmail.com>,
	"joe@perches.com" <joe@perches.com>,
	"msuchanek@suse.de" <msuchanek@suse.de>,
	"oleksandr@natalenko.name" <oleksandr@natalenko.name>
Subject: Re: [PATCH v8 04/13] module: Move livepatch support to a separate file
Date: Mon, 28 Feb 2022 11:56:40 +0100	[thread overview]
Message-ID: <YhyqaGO+vbGOifpR@alley> (raw)
In-Reply-To: <fb1bb248-bd3f-0990-cdfd-d186b7579411@csgroup.eu>

On Fri 2022-02-25 16:49:31, Christophe Leroy wrote:
> 
> 
> Le 25/02/2022 à 10:34, Petr Mladek a écrit :
> > On Tue 2022-02-22 14:12:54, Aaron Tomlin wrote:
> >> No functional change.
> >>
> >> This patch migrates livepatch support (i.e. used during module
> >> add/or load and remove/or deletion) from core module code into
> >> kernel/module/livepatch.c. At the moment it contains code to
> >> persist Elf information about a given livepatch module, only.
> >>
> > --- del.p	2022-02-24 16:55:26.570054922 +0100
> > +++ add.p	2022-02-24 16:56:04.766781394 +0100
> > @@ -3,14 +3,14 @@
> >    * section header table, section string table, and symtab section
> >    * index from info to mod->klp_info.
> >    */
> > -static int copy_module_elf(struct module *mod, struct load_info *info)
> > +int copy_module_elf(struct module *mod, struct load_info *info)
> 
> That's not a hidden change. That's part of the move, that's required.

Sure. I was not talking about this line. I kept it to show the context.

> >   {
> >   	unsigned int size, symndx;
> >   	int ret;
> >   
> >   	size = sizeof(*mod->klp_info);
> >   	mod->klp_info = kmalloc(size, GFP_KERNEL);
> > -	if (mod->klp_info == NULL)
> > +	if (!mod->klp_info)
> >   		return -ENOMEM;
> >   
> >   	/* Elf header */
> > @@ -20,7 +20,7 @@ static int copy_module_elf(struct module
> >   	/* Elf section header table */
> >   	size = sizeof(*info->sechdrs) * info->hdr->e_shnum;
> >   	mod->klp_info->sechdrs = kmemdup(info->sechdrs, size, GFP_KERNEL);
> > -	if (mod->klp_info->sechdrs == NULL) {
> > +	if (!mod->klp_info->sechdrs) {
> >   		ret = -ENOMEM;
> >   		goto free_info;
> >   	}
> > @@ -28,7 +28,7 @@ static int copy_module_elf(struct module
> >   	/* Elf section name string table */
> >   	size = info->sechdrs[info->hdr->e_shstrndx].sh_size;
> >   	mod->klp_info->secstrings = kmemdup(info->secstrings, size, GFP_KERNEL);
> > -	if (mod->klp_info->secstrings == NULL) {
> > +	if (!mod->klp_info->secstrings) {
> >   		ret = -ENOMEM;
> >   		goto free_sechdrs;
> >   	}
> > @@ -43,8 +43,7 @@ static int copy_module_elf(struct module
> >   	 * to core_kallsyms.symtab since the copy of the symtab in module
> >   	 * init memory is freed at the end of do_init_module().
> >   	 */
> > -	mod->klp_info->sechdrs[symndx].sh_addr = \
> > -		(unsigned long) mod->core_kallsyms.symtab;
> > +	mod->klp_info->sechdrs[symndx].sh_addr = (unsigned long)mod->core_kallsyms.symtab;
> >   
> >   	return 0;
> > 
> > 
> > Please do not do these small coding style changes. It complicates the
> > review and increases the risk of regressions. Different people
> > have different preferences. Just imagine that every half a year
> > someone update style of a code by his personal preferences. The
> > real changes will then get lost in a lot of noise.
> 
> I disagree here. We are not talking about people's preference here but 
> compliance with documented Linux kernel Codying Style and handling of 
> official checkpatch.pl script reports.

Really?

1. I restored

	+	if (mod->klp_info->secstrings == NULL) {

   and checkpatch.pl is happy.

2. I do not see anythinkg about if (xxx == NULL) checks in
   Documentation/process/coding-style.rst

3. $> git grep "if (.* == NULL" | wc -l
   15041

4. The result of
	-	mod->klp_info->sechdrs[symndx].sh_addr = \
	-		(unsigned long) mod->core_kallsyms.symtab;
	+	mod->klp_info->sechdrs[symndx].sh_addr = (unsigned long)mod->core_kallsyms.symtab;

   is 90 characeters long and Documentation/process/coding-style.rst says:

	2) Breaking long lines and strings
	----------------------------------

	Coding style is all about readability and maintainability using commonly
	available tools.

	The preferred limit on the length of a single line is 80 columns.

	Statements longer than 80 columns should be broken into sensible chunks,
	unless exceeding 80 columns significantly increases readability and does
	not hide information.

   checkpatch.pl accepts lines up to 100 columns but 80 are still
   preferred.

> You are right that randomly updating the style every half a year would 
> be a nightmare and would kill blamability of changes.
> 
> However when moving big peaces of code like this, blamability is broken 
> anyway and this is a very good opportunity to increase compliance of 
> kernel code to its own codying style. But doing it in several steps 
> increases code churn and has no real added value.

From Documentation/process/submitting-patches.rst:

	One significant exception is when moving code from one file to
	another -- in this case you should not modify the moved code at all in
	the same patch which moves it.  This clearly delineates the act of
	moving the code and your changes.  This greatly aids review of the
	actual differences and allows tools to better track the history of
	the code itself.

> > 
> > Coding style changes might be acceptable only when the code is
> > reworked or when it significantly improves readability.
> 
> When code is moved around it is also a good opportunity.

No!

I would not have complained if it did not complicate my review.
But it did!

Best Regards,
Petr