Message-ID: <20200309103935.GB18870@linux-8ccs.fritz.box>
Date:   Mon, 9 Mar 2020 11:39:36 +0100
From:   Jessica Yu <jeyu@...nel.org>
To:     Masahiro Yamada <masahiroy@...nel.org>
Cc:     Matthias Maennich <maennich@...gle.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 1/2] modpost: rework and consolidate logging interface

+++ Masahiro Yamada [09/03/20 19:20 +0900]:
>On Mon, Mar 9, 2020 at 6:59 PM Jessica Yu <jeyu@...nel.org> wrote:
>>
>> +++ Masahiro Yamada [09/03/20 09:40 +0900]:
>> >Hi Jessica,
>> >
>> >
>> >
>> >On Sat, Mar 7, 2020 at 1:02 AM Jessica Yu <jeyu@...nel.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@...nel.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.

Ah, sorry, I mean the kbuild 0-day bot errors. I am just realizing
the 0-day bot emails are not CC'd to lkml. Here is the error I got
from the bot:

---

I love your patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.6-rc4 next-20200306]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Jessica-Yu/modpost-rework-and-consolidate-logging-interface/20200307-052346
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 30fe0d07fd7b27d41d9b31a224052cc4e910947a
config: sh-randconfig-a001-20200306 (attached as .config)
compiler: sh4-linux-gcc (GCC) 7.5.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.5.0 make.cross ARCH=sh 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@...el.com>

All errors (new ones prefixed by >>):

>> ERROR: modpost: "adc_single" [arch/sh/boards/mach-hp6xx/hp6xx_apm.ko] undefined!

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

>
>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

Powered by blists - more mailing lists