Re: [PATCH v3 1/2] modpost: rework and consolidate logging interface

[Jessica Yu]

+++ Masahiro Yamada [09/03/20 09:40 +0900]:
> Hi Jessica,
>
>
>
> On Sat, Mar 7, 2020 at 1:02 AM Jessica Yu <jeyu@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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..

Thanks,

Jessica