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;
Date:   Tue, 3 Mar 2020 15:57:36 +0100
From:   Jessica Yu <jeyu@kernel.org>
To:     Masahiro Yamada <masahiroy@kernel.org>
Cc:     Matthias Maennich <maennich@google.com>,
        Joe Perches <joe@perches.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 1/2] modpost: rework and consolidate logging interface
Message-ID: <20200303145736.GA16460@linux-8ccs>
References: <20200226142608.19499-1-jeyu@kernel.org>
 <CAK7LNAQZAgobbTTTpLKNActYCYP7UdVgdE-Oz+pvvRxsxd_uaw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAK7LNAQZAgobbTTTpLKNActYCYP7UdVgdE-Oz+pvvRxsxd_uaw@mail.gmail.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

+++ Masahiro Yamada [03/03/20 23:42 +0900]:
>On Wed, Feb 26, 2020 at 11:26 PM 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 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@kernel.org>
>> ---
>> 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