lkml.org 
[lkml]   [2020]   [Mar]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2 1/2] modpost: rework and consolidate logging interface
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

\
 
 \ /
  Last update: 2020-03-04 12:17    [W:0.061 / U:9.896 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site