Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20200306160206.5609-1-jeyu@kernel.org> <CAK7LNARZ4VgaCa_TiDBG-99amBGTTXTQMs9LsK3nO4k+y-5KDQ@mail.gmail.com>
 <20200309095914.GA18870@linux-8ccs.fritz.box>
In-Reply-To: <20200309095914.GA18870@linux-8ccs.fritz.box>
From:   Masahiro Yamada <masahiroy@kernel.org>
Date:   Mon, 9 Mar 2020 19:20:35 +0900
Message-ID: <CAK7LNARf9CzZ8dcK5O5vxUoncpDdpzSZctd0YuKeWyfDG_cdwA@mail.gmail.com>
Subject: Re: [PATCH v3 1/2] modpost: rework and consolidate logging interface
To:     Jessica Yu <jeyu@kernel.org>
Cc:     Matthias Maennich <maennich@google.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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 clean=
up is
> >> to reduce code duplication when deciding whether or not to warn or err=
or
> >> 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 =E2=80=98modpost_log=E2=80=99:
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


--=20
Best Regards
Masahiro Yamada