Date:   Thu, 3 Feb 2022 07:48:11 +0000
From:   Christophe Leroy <christophe.leroy@...roup.eu>
To:     Luis Chamberlain <mcgrof@...nel.org>,
        Aaron Tomlin <atomlin@...hat.com>,
        Michal Suchanek <msuchanek@...e.de>
CC:     "cl@...ux.com" <cl@...ux.com>,
        "pmladek@...e.com" <pmladek@...e.com>,
        "mbenes@...e.cz" <mbenes@...e.cz>,
        "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
        "jeyu@...nel.org" <jeyu@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-modules@...r.kernel.org" <linux-modules@...r.kernel.org>,
        "live-patching@...r.kernel.org" <live-patching@...r.kernel.org>,
        "atomlin@...mlin.com" <atomlin@...mlin.com>,
        "ghalat@...hat.com" <ghalat@...hat.com>,
        "allen.lkml@...il.com" <allen.lkml@...il.com>,
        "void@...ifault.com" <void@...ifault.com>,
        "joe@...ches.com" <joe@...ches.com>
Subject: Re: [RFC PATCH v4 00/13] module: core code clean up


Le 03/02/2022 à 01:20, Luis Chamberlain a écrit :
> On Sun, Jan 30, 2022 at 09:32:01PM +0000, Aaron Tomlin wrote:
>> Hi Luis,
>>
>> As per your suggestion [1], this is an attempt to refactor and split
>> optional code out of core module support code into separate components.
>> This version is based on branch mcgrof/modules-next since a97ac8cb24a3/or
>> modules-5.17-rc1. Please let me know your thoughts.
>>
>> Changes since v1 [2]:
> 
> Thanks for all this work Aaron! Can you drop the RFC prefix,
> rebase onto linus' latest tree (as he already merged my
> modules-next, so his tree is more up to date), and submit again?
> 
> I'll then apply this to my modules-next, and then ask Christophe to
> rebase on top of that.
> 
> Michal, you'd be up next if you want to go through modules-next.
> 
> Aaron, please Cc Christophe and Michal on your next respin.
> 

I went quickly through v4. That's a great and useful job I think, it 
should ease future work on modules. So I will be happy to apply my 
changes on top of this series.

I may have more comments after reviewing in more details and rebasing my 
series on top of it, but at the time being I have at least one comment:

All function prototypes in header files are pointlessly prepended with 
'extern' keyword. This was done that way in the old days, but has been 
deprecated as this keyword does nothing on function prototypes but adds 
visual pollution when looking at the files.

See below the output of checkpatch on internal.h

Regardless, I'm looking forward to rebasing my series on top of yours.

Thanks
Christophe

WARNING: Use #include <linux/module.h> instead of <asm/module.h>
#10: FILE: kernel/module/internal.h:10:
+#include <asm/module.h>

CHECK: spaces preferred around that '-' (ctx:VxV)
#18: FILE: kernel/module/internal.h:18:
+#define INIT_OFFSET_MASK (1UL << (BITS_PER_LONG-1))
                                                 ^

CHECK: extern prototypes should be avoided in .h files
#84: FILE: kernel/module/internal.h:84:
+extern int mod_verify_sig(const void *mod, struct load_info *info);

CHECK: extern prototypes should be avoided in .h files
#85: FILE: kernel/module/internal.h:85:
+extern int try_to_force_load(struct module *mod, const char *reason);

CHECK: extern prototypes should be avoided in .h files
#86: FILE: kernel/module/internal.h:86:
+extern bool find_symbol(struct find_symbol_arg *fsa);

CHECK: extern prototypes should be avoided in .h files
#87: FILE: kernel/module/internal.h:87:
+extern struct module *find_module_all(const char *name, size_t len, 
bool even_unformed);

CHECK: extern prototypes should be avoided in .h files
#88: FILE: kernel/module/internal.h:88:
+extern unsigned long kernel_symbol_value(const struct kernel_symbol *sym);

CHECK: extern prototypes should be avoided in .h files
#89: FILE: kernel/module/internal.h:89:
+extern int cmp_name(const void *name, const void *sym);

CHECK: extern prototypes should be avoided in .h files
#90: FILE: kernel/module/internal.h:90:
+extern long get_offset(struct module *mod, unsigned int *size, Elf_Shdr 
*sechdr,

CHECK: extern prototypes should be avoided in .h files
#92: FILE: kernel/module/internal.h:92:
+extern char *module_flags(struct module *mod, char *buf);

CHECK: extern prototypes should be avoided in .h files
#95: FILE: kernel/module/internal.h:95:
+extern int copy_module_elf(struct module *mod, struct load_info *info);

CHECK: extern prototypes should be avoided in .h files
#96: FILE: kernel/module/internal.h:96:
+extern void free_module_elf(struct module *mod);

CHECK: Please use a blank line after function/struct/union/enum declarations
#102: FILE: kernel/module/internal.h:102:
+}
+static inline void free_module_elf(struct module *mod) { }

CHECK: Please use a blank line after function/struct/union/enum declarations
#114: FILE: kernel/module/internal.h:114:
+}
+static inline void module_decompress_cleanup(struct load_info *info)

CHECK: extern prototypes should be avoided in .h files
#128: FILE: kernel/module/internal.h:128:
+extern void mod_tree_insert(struct module *mod);

CHECK: extern prototypes should be avoided in .h files
#129: FILE: kernel/module/internal.h:129:
+extern void mod_tree_remove_init(struct module *mod);

CHECK: extern prototypes should be avoided in .h files
#130: FILE: kernel/module/internal.h:130:
+extern void mod_tree_remove(struct module *mod);

CHECK: extern prototypes should be avoided in .h files
#131: FILE: kernel/module/internal.h:131:
+extern struct module *mod_find(unsigned long addr);

ERROR: do not initialise statics to 0
#133: FILE: kernel/module/internal.h:133:
+static unsigned long module_addr_min = -1UL, module_addr_max = 0;

CHECK: extern prototypes should be avoided in .h files
#153: FILE: kernel/module/internal.h:153:
+extern int module_sig_check(struct load_info *info, int flags);

CHECK: extern prototypes should be avoided in .h files
#162: FILE: kernel/module/internal.h:162:
+extern void kmemleak_load_module(const struct module *mod, const struct 
load_info *info);

CHECK: extern prototypes should be avoided in .h files
#170: FILE: kernel/module/internal.h:170:
+extern void init_build_id(struct module *mod, const struct load_info 
*info);

CHECK: extern prototypes should be avoided in .h files
#175: FILE: kernel/module/internal.h:175:
+extern void layout_symtab(struct module *mod, struct load_info *info);

CHECK: extern prototypes should be avoided in .h files
#176: FILE: kernel/module/internal.h:176:
+extern void add_kallsyms(struct module *mod, const struct load_info *info);

CHECK: extern prototypes should be avoided in .h files
#177: FILE: kernel/module/internal.h:177:
+extern bool sect_empty(const Elf_Shdr *sect);

CHECK: extern prototypes should be avoided in .h files
#178: FILE: kernel/module/internal.h:178:
+extern const char *find_kallsyms_symbol(struct module *mod, unsigned 
long addr,

CHECK: extern prototypes should be avoided in .h files
#191: FILE: kernel/module/internal.h:191:
+extern int mod_sysfs_setup(struct module *mod, const struct load_info 
*info,

CHECK: extern prototypes should be avoided in .h files
#193: FILE: kernel/module/internal.h:193:
+extern void mod_sysfs_fini(struct module *mod);

CHECK: extern prototypes should be avoided in .h files
#194: FILE: kernel/module/internal.h:194:
+extern void module_remove_modinfo_attrs(struct module *mod, int end);

CHECK: extern prototypes should be avoided in .h files
#195: FILE: kernel/module/internal.h:195:
+extern void del_usage_links(struct module *mod);

CHECK: extern prototypes should be avoided in .h files
#196: FILE: kernel/module/internal.h:196:
+extern void init_param_lock(struct module *mod);

CHECK: Please use a blank line after function/struct/union/enum declarations
#205: FILE: kernel/module/internal.h:205:
+}
+static inline void mod_sysfs_fini(struct module *mod) { }

CHECK: extern prototypes should be avoided in .h files
#212: FILE: kernel/module/internal.h:212:
+extern int check_version(const struct load_info *info,

CHECK: extern prototypes should be avoided in .h files
#214: FILE: kernel/module/internal.h:214:
+extern int check_modstruct_version(const struct load_info *info, struct 
module *mod);

CHECK: extern prototypes should be avoided in .h files
#215: FILE: kernel/module/internal.h:215:
+extern int same_magic(const char *amagic, const char *bmagic, bool 
has_crcs);

CHECK: Alignment should match open parenthesis
#232: FILE: kernel/module/internal.h:232:
+static inline int same_magic(const char *amagic, const char *bmagic,
+			    bool has_crcs)

total: 1 errors, 1 warnings, 34 checks, 236 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
       mechanically convert to the typical style using --fix or 
--fix-inplace.

kernel/module/internal.h has style problems, please review.

NOTE: If any of the errors are false positives, please report
       them to the maintainer, see CHECKPATCH in MAINTAINERS.

Powered by blists - more mailing lists