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:   Wed, 4 Mar 2020 11:14:22 +0000
From:   Matthias Maennich <maennich@google.com>
To:     Jessica Yu <jeyu@kernel.org>
Cc:     Masahiro Yamada <masahiroy@kernel.org>,
        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: <20200304111422.GA66900@google.com>
References: <20200226142608.19499-1-jeyu@kernel.org>
 <CAK7LNAQZAgobbTTTpLKNActYCYP7UdVgdE-Oz+pvvRxsxd_uaw@mail.gmail.com>
 <20200303145736.GA16460@linux-8ccs>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20200303145736.GA16460@linux-8ccs>
User-Agent: Mutt/1.12.2 (2019-09-21)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Mar 03, 2020 at 03:57:36PM +0100, Jessica Yu wrote:
>+++ 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)

The only thing that bothered me a bit was the inconsistent naming with
'merror'. I know `error` is reserved, but refactoring this whole code
(thanks for that!) seems like a code opportunity to clean this up. (Or
is it just me?)

Cheers,
Matthias

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