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.
