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

[Jessica Yu]

+++ Masahiro Yamada [03/03/20 23:42 +0900]:
> On Wed, Feb 26, 2020 at 11:26 PM 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 as a conditional warn
> > (warn_unless()). The conditional warn is useful in determining whether
> > to use merror() or warn() based on a condition. This reduces code
> > duplication overall.
> >
> > Signed-off-by: Jessica Yu <jeyu@xxxxxxxxxx>
> > ---
> > v2:
> >   - modpost_log: initialize level to ""
> >   - remove parens () from case labels
> >
> >  scripts/mod/modpost.c | 69 +++++++++++++++++++++++----------------------------
> >  scripts/mod/modpost.h | 22 +++++++++++++---
> >  2 files changed, 50 insertions(+), 41 deletions(-)
> >
> > diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> > index 7edfdb2f4497..3201a2ac5cc4 100644
> > --- a/scripts/mod/modpost.c
> > +++ b/scripts/mod/modpost.c
> > @@ -51,41 +51,37 @@ 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, ...)
> >  {
> > +       char *level = "";
>
>
> You can add 'const'.
>
>
>          const char *level = "";
>
>
>
> >         va_list arglist;
> >
> > -       fprintf(stderr, "FATAL: ");
> > -
> > -       va_start(arglist, fmt);
> > -       vfprintf(stderr, fmt, arglist);
> > -       va_end(arglist);
> > -
> > -       exit(1);
> > -}
> > -
> > -PRINTF void warn(const char *fmt, ...)
> > -{
> > -       va_list arglist;
> > +       switch(loglevel) {
> > +       case LOG_WARN:
> > +               level = "WARNING: ";
> > +               break;
> > +       case LOG_ERROR:
> > +               level = "ERROR: ";
> > +               break;
> > +       case LOG_FATAL:
> > +               level = "FATAL: ";
> > +               break;
> > +       default: /* invalid loglevel, ignore */
> > +               break;
> > +       }
> >
> > -       fprintf(stderr, "WARNING: ");
> > +       fprintf(stderr, level);
>
>
>
> If I apply this patch, I see this warning:
>
> scripts/mod/modpost.c: In function âmodpost_logâ:
> scripts/mod/modpost.c:77:2: warning: format not a string literal and
> no format arguments [-Wformat-security]
>  fprintf(stderr, level);
>  ^~~~~~~
>
>
> Please write like this:
>
>
>     fprintf(stderr, "%s", level);
>
>
>
>
> Or, you can delete 'level', then write
> string literals directly in fprintf().
>
>
> switch(loglevel) {
> case LOG_WARN:
>        fprintf(stderr, "WARNING: ");
>        break;
> case LOG_ERROR:
>        fprintf(stderr, "ERROR: ");
>        break;
> case LOG_FATAL:
>        fprintf(stderr, "FATAL: ");
>        break;
> }
>
>
>
>
> > +       fprintf(stderr, "modpost: ");
> >
> >         va_start(arglist, fmt);
> >         vfprintf(stderr, fmt, arglist);
> >         va_end(arglist);
> > -}
>
> <snip>
>
> > diff --git a/scripts/mod/modpost.h b/scripts/mod/modpost.h
> > index 64a82d2d85f6..631d07714f7a 100644
> > --- a/scripts/mod/modpost.h
> > +++ b/scripts/mod/modpost.h
> > @@ -198,6 +198,22 @@ void *grab_file(const char *filename, unsigned long *size);
> >  char* get_next_line(unsigned long *pos, void *file, unsigned long size);
> >  void release_file(void *file, unsigned long size);
> >
> > -void fatal(const char *fmt, ...);
> > -void warn(const char *fmt, ...);
> > -void merror(const char *fmt, ...);
> > +enum loglevel {
> > +       LOG_WARN,
> > +       LOG_ERROR,
> > +       LOG_FATAL
> > +};
> > +
> > +void modpost_log(enum loglevel loglevel, const char *fmt, ...);
> > +
> > +#define warn(fmt, args...)     modpost_log(LOG_WARN, fmt, ##args)
> > +#define merror(fmt, args...)   modpost_log(LOG_ERROR, fmt, ##args)
> > +#define fatal(fmt, args...)    modpost_log(LOG_FATAL, fmt, ##args)
> > +/* Warn unless condition is true, then use merror() */
> > +#define warn_unless(condition, fmt, args...)   \
> > +do {                                           \
> > +       if (condition)                          \
> > +               merror(fmt, ##args);            \
> > +       else                                    \
> > +               warn(fmt, ##args);              \
> > +} while (0)
>
>
> Hmm, warn_unless() is not intuitive naming...
>
> You could use modpost_log() directly in C code,
> what do you think?
>
>
>            modpost_log(allow_missing_ns_imports ? LOG_WARN : LOG_ERROR,
>                        "module %s uses symbol %s from namespace %s,
> but does not import it.\n",
>                        basename, exp->name, exp->namespace);

Yeah, I wasn't sure if I should expose modpost_log() and call it
directly, so I wrapped it in warn_unless(). But I think it's not a big
deal, so I'll just change it to a direct call. Thank you for the review!

Jessica