Messages in this thread | | | From | Masahiro Yamada <> | Date | Mon, 9 Mar 2020 19:20:35 +0900 | Subject | Re: [PATCH v3 1/2] modpost: rework and consolidate logging interface |
| |
On Mon, Mar 9, 2020 at 6:59 PM Jessica Yu <jeyu@kernel.org> wrote: > > +++ Masahiro Yamada [09/03/20 09:40 +0900]: > >Hi Jessica, > > > > > > > >On Sat, Mar 7, 2020 at 1:02 AM Jessica Yu <jeyu@kernel.org> wrote: > >> > >> Rework modpost's logging interface by consolidating merror(), warn(), and > >> fatal() to use a single function, modpost_log(). Introduce different > >> logging levels (WARN, ERROR, FATAL) as well. The purpose of this cleanup is > >> to reduce code duplication when deciding whether or not to warn or error > >> out based on a condition. > >> > >> Signed-off-by: Jessica Yu <jeyu@kernel.org> > >> --- > >> v3: > >> - remove level variable from modpost_log and just call fprintf in each > >> case > >> - remove warn_unless and just call modpost_log() directly > >> - fix checkpatch error: > >> ERROR: space required before the open parenthesis '(' > >> #102: FILE: scripts/mod/modpost.c:61: > >> + switch(loglevel) { > >> > >> scripts/mod/modpost.c | 68 ++++++++++++++++++++++----------------------------- > >> scripts/mod/modpost.h | 14 ++++++++--- > >> 2 files changed, 40 insertions(+), 42 deletions(-) > >> > >> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c > >> index 7edfdb2f4497..a2329235a6db 100644 > >> --- a/scripts/mod/modpost.c > >> +++ b/scripts/mod/modpost.c > >> @@ -51,41 +51,34 @@ enum export { > >> > >> #define MODULE_NAME_LEN (64 - sizeof(Elf_Addr)) > >> > >> -#define PRINTF __attribute__ ((format (printf, 1, 2))) > >> +#define PRINTF __attribute__ ((format (printf, 2, 3))) > >> > >> -PRINTF void fatal(const char *fmt, ...) > >> +PRINTF void modpost_log(enum loglevel loglevel, const char *fmt, ...) > >> { > > > > > >This series looks good to me. > > > >I can queue it up to kbuild tree > >if there is no objection. > > > > > >I just noticed one nit. > > > >Now that modpost_log() is the only user of PRINTF, > >we can delete PRITNF, and directly add the attribute > >to modpost_log(), like this: > > > > > >void __attribute__((format(printf, 2, 3))) > >modpost_log(enum loglevel loglevel, const char *fmt, ...) > >{ > > ... > >} > > > > > >If you agree, I can modify it when I apply it. > > Yes, I agree with this change. Thank you! > > One more thing, it's not immediately obvious to me why the first patch > would cause those kbuild warnings :-/ I'll see if I have any luck > reproducing them locally.. >
Hmm, this warning option is not so new.
At least, commit 7c0d35a339db6 (4 years ago) fixed a similar one.
I use GCC 7.4
If I apply your previous version, I see build log like follows:
$ gcc --version gcc (Ubuntu 7.4.0-1ubuntu1~18.04.1) 7.4.0 Copyright (C) 2017 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
masahiro@pug:~/ref/linux$ make defconfig all HOSTCC scripts/kconfig/conf.o HOSTCC scripts/kconfig/confdata.o HOSTCC scripts/kconfig/expr.o LEX scripts/kconfig/lexer.lex.c YACC scripts/kconfig/parser.tab.[ch] HOSTCC scripts/kconfig/lexer.lex.o HOSTCC scripts/kconfig/parser.tab.o HOSTCC scripts/kconfig/preprocess.o HOSTCC scripts/kconfig/symbol.o HOSTCC scripts/kconfig/util.o HOSTLD scripts/kconfig/conf *** Default configuration is based on 'x86_64_defconfig' # # configuration written to .config # SYSTBL arch/x86/include/generated/asm/syscalls_32.h SYSHDR arch/x86/include/generated/asm/unistd_32_ia32.h SYSHDR arch/x86/include/generated/asm/unistd_64_x32.h SYSTBL arch/x86/include/generated/asm/syscalls_64.h SYSHDR arch/x86/include/generated/uapi/asm/unistd_32.h SYSHDR arch/x86/include/generated/uapi/asm/unistd_64.h SYSHDR arch/x86/include/generated/uapi/asm/unistd_x32.h HOSTCC arch/x86/tools/relocs_32.o HOSTCC arch/x86/tools/relocs_64.o HOSTCC arch/x86/tools/relocs_common.o HOSTLD arch/x86/tools/relocs HOSTCC scripts/selinux/genheaders/genheaders HOSTCC scripts/selinux/mdp/mdp HOSTCC scripts/kallsyms HOSTCC scripts/sorttable HOSTCC scripts/asn1_compiler HOSTCC scripts/extract-cert WRAP arch/x86/include/generated/uapi/asm/bpf_perf_event.h WRAP arch/x86/include/generated/uapi/asm/errno.h WRAP arch/x86/include/generated/uapi/asm/fcntl.h WRAP arch/x86/include/generated/uapi/asm/ioctl.h WRAP arch/x86/include/generated/uapi/asm/ioctls.h WRAP arch/x86/include/generated/uapi/asm/ipcbuf.h WRAP arch/x86/include/generated/uapi/asm/param.h WRAP arch/x86/include/generated/uapi/asm/poll.h WRAP arch/x86/include/generated/uapi/asm/resource.h WRAP arch/x86/include/generated/uapi/asm/socket.h WRAP arch/x86/include/generated/uapi/asm/sockios.h WRAP arch/x86/include/generated/uapi/asm/termbits.h WRAP arch/x86/include/generated/uapi/asm/termios.h WRAP arch/x86/include/generated/uapi/asm/types.h WRAP arch/x86/include/generated/asm/early_ioremap.h WRAP arch/x86/include/generated/asm/export.h WRAP arch/x86/include/generated/asm/mcs_spinlock.h WRAP arch/x86/include/generated/asm/mm-arch-hooks.h WRAP arch/x86/include/generated/asm/mmiowb.h WRAP arch/x86/include/generated/asm/dma-contiguous.h UPD include/config/kernel.release UPD include/generated/uapi/linux/version.h UPD include/generated/utsrelease.h CC scripts/mod/empty.o HOSTCC scripts/mod/mk_elfconfig MKELF scripts/mod/elfconfig.h HOSTCC scripts/mod/modpost.o scripts/mod/modpost.c: In function ‘modpost_log’: scripts/mod/modpost.c:75:2: warning: format not a string literal and no format arguments [-Wformat-security] fprintf(stderr, level); ^~~~~~~ CC scripts/mod/devicetable-offsets.s UPD scripts/mod/devicetable-offsets.h HOSTCC scripts/mod/file2alias.o HOSTCC scripts/mod/sumversion.o HOSTLD scripts/mod/modpost CC kernel/bounds.s UPD include/generated/bounds.h UPD include/generated/timeconst.h CC arch/x86/kernel/asm-offsets.s
-- Best Regards Masahiro Yamada
| |