linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 1/2] modpost: rework and consolidate logging interface
@ 2020-02-26 14:26 Jessica Yu
  2020-02-26 14:26 ` [PATCH v2 2/2] modpost: return error if module is missing ns imports and MODULE_ALLOW_MISSING_NAMESPACE_IMPORTS=n Jessica Yu
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Jessica Yu @ 2020-02-26 14:26 UTC (permalink / raw)
  To: Masahiro Yamada; +Cc: Matthias Maennich, Joe Perches, linux-kernel, Jessica Yu

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 = "";
 	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);
+	fprintf(stderr, "modpost: ");
 
 	va_start(arglist, fmt);
 	vfprintf(stderr, fmt, arglist);
 	va_end(arglist);
-}
 
-PRINTF void merror(const char *fmt, ...)
-{
-	va_list arglist;
-
-	fprintf(stderr, "ERROR: ");
+	if (loglevel == LOG_FATAL)
+		exit(1);
 
-	va_start(arglist, fmt);
-	vfprintf(stderr, fmt, arglist);
-	va_end(arglist);
 }
 
 static inline bool strends(const char *str, const char *postfix)
@@ -113,7 +109,7 @@ static int is_vmlinux(const char *modname)
 void *do_nofail(void *ptr, const char *expr)
 {
 	if (!ptr)
-		fatal("modpost: Memory allocation failure: %s.\n", expr);
+		fatal("Memory allocation failure: %s.\n", expr);
 
 	return ptr;
 }
@@ -2021,7 +2017,7 @@ static void read_symbols(const char *modname)
 
 	license = get_modinfo(&info, "license");
 	if (!license && !is_vmlinux(modname))
-		warn("modpost: missing MODULE_LICENSE() in %s\n"
+		warn("missing MODULE_LICENSE() in %s\n"
 		     "see include/linux/module.h for "
 		     "more information\n", modname);
 	while (license) {
@@ -2152,15 +2148,15 @@ static void check_for_gpl_usage(enum export exp, const char *m, const char *s)
 
 	switch (exp) {
 	case export_gpl:
-		fatal("modpost: GPL-incompatible module %s%s "
+		fatal("GPL-incompatible module %s%s "
 		      "uses GPL-only symbol '%s'\n", m, e, s);
 		break;
 	case export_unused_gpl:
-		fatal("modpost: GPL-incompatible module %s%s "
+		fatal("GPL-incompatible module %s%s "
 		      "uses GPL-only symbol marked UNUSED '%s'\n", m, e, s);
 		break;
 	case export_gpl_future:
-		warn("modpost: GPL-incompatible module %s%s "
+		warn("GPL-incompatible module %s%s "
 		      "uses future GPL-only symbol '%s'\n", m, e, s);
 		break;
 	case export_plain:
@@ -2178,7 +2174,7 @@ static void check_for_unused(enum export exp, const char *m, const char *s)
 	switch (exp) {
 	case export_unused:
 	case export_unused_gpl:
-		warn("modpost: module %s%s "
+		warn("module %s%s "
 		      "uses symbol '%s' marked UNUSED\n", m, e, s);
 		break;
 	default:
@@ -2197,14 +2193,11 @@ static int check_exports(struct module *mod)
 		exp = find_symbol(s->name);
 		if (!exp || exp->module == mod) {
 			if (have_vmlinux && !s->weak) {
-				if (warn_unresolved) {
-					warn("\"%s\" [%s.ko] undefined!\n",
-					     s->name, mod->name);
-				} else {
-					merror("\"%s\" [%s.ko] undefined!\n",
-					       s->name, mod->name);
+				warn_unless(!warn_unresolved,
+					    "\"%s\" [%s.ko] undefined!\n",
+					    s->name, mod->name);
+				if (!warn_unresolved)
 					err = 1;
-				}
 			}
 			continue;
 		}
@@ -2653,7 +2646,7 @@ int main(int argc, char **argv)
 	if (dump_write)
 		write_dump(dump_write);
 	if (sec_mismatch_count && sec_mismatch_fatal)
-		fatal("modpost: Section mismatches detected.\n"
+		fatal("Section mismatches detected.\n"
 		      "Set CONFIG_SECTION_MISMATCH_WARN_ONLY=y to allow them.\n");
 	for (n = 0; n < SYMBOL_HASH_SIZE; n++) {
 		struct symbol *s;
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)
-- 
2.16.4


^ permalink raw reply related	[flat|nested] 7+ messages in thread
* [PATCH v2 2/2] modpost: return error if module is missing ns imports and MODULE_ALLOW_MISSING_NAMESPACE_IMPORTS=n
  2020-02-26 14:26 [PATCH v2 1/2] modpost: rework and consolidate logging interface Jessica Yu
@ 2020-02-26 14:26 ` Jessica Yu
  2020-03-03 15:21   ` Masahiro Yamada
  2020-03-03 14:42 ` [PATCH v2 1/2] modpost: rework and consolidate logging interface Masahiro Yamada
  2020-03-03 15:23 ` Masahiro Yamada
  2 siblings, 1 reply; 7+ messages in thread
From: Jessica Yu @ 2020-02-26 14:26 UTC (permalink / raw)
  To: Masahiro Yamada; +Cc: Matthias Maennich, Joe Perches, linux-kernel, Jessica Yu

Currently when CONFIG_MODULE_ALLOW_MISSING_NAMESPACE_IMPORTS=n, modpost only warns
when a module is missing namespace imports. Under this configuration, such a module
cannot be loaded into the kernel anyway, as the module loader would reject it.
We might as well return a build error when a module is missing namespace imports
under CONFIG_MODULE_ALLOW_MISSING_NAMESPACE_IMPORTS=n, so that the build
warning does not go ignored/unnoticed.

Signed-off-by: Jessica Yu <jeyu@kernel.org>
---
 scripts/Makefile.modpost | 15 ++++++++-------
 scripts/mod/modpost.c    | 14 +++++++++++---
 2 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
index b4d3f2d122ac..957eed6a17a5 100644
--- a/scripts/Makefile.modpost
+++ b/scripts/Makefile.modpost
@@ -46,13 +46,14 @@ include scripts/Kbuild.include
 kernelsymfile := $(objtree)/Module.symvers
 modulesymfile := $(firstword $(KBUILD_EXTMOD))/Module.symvers
 
-MODPOST = scripts/mod/modpost						\
-	$(if $(CONFIG_MODVERSIONS),-m)					\
-	$(if $(CONFIG_MODULE_SRCVERSION_ALL),-a)			\
-	$(if $(KBUILD_EXTMOD),-i,-o) $(kernelsymfile)			\
-	$(if $(KBUILD_EXTMOD),$(addprefix -e ,$(KBUILD_EXTRA_SYMBOLS)))	\
-	$(if $(KBUILD_EXTMOD),-o $(modulesymfile))			\
-	$(if $(CONFIG_SECTION_MISMATCH_WARN_ONLY),,-E)			\
+MODPOST = scripts/mod/modpost								\
+	$(if $(CONFIG_MODVERSIONS),-m)							\
+	$(if $(CONFIG_MODULE_SRCVERSION_ALL),-a)					\
+	$(if $(KBUILD_EXTMOD),-i,-o) $(kernelsymfile)					\
+	$(if $(KBUILD_EXTMOD),$(addprefix -e ,$(KBUILD_EXTRA_SYMBOLS)))			\
+	$(if $(KBUILD_EXTMOD),-o $(modulesymfile))					\
+	$(if $(CONFIG_SECTION_MISMATCH_WARN_ONLY),,-E)					\
+	$(if $(CONFIG_MODULE_ALLOW_MISSING_NAMESPACE_IMPORTS)$(KBUILD_NSDEPS),-N) 	\
 	$(if $(KBUILD_MODPOST_WARN),-w)
 
 ifdef MODPOST_VMLINUX
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 3201a2ac5cc4..d164be63c2e3 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -39,6 +39,8 @@ static int sec_mismatch_count = 0;
 static int sec_mismatch_fatal = 0;
 /* ignore missing files */
 static int ignore_missing_files;
+/* If set to 1, only warn (instead of error) about missing ns imports */
+static int allow_missing_ns_imports = 0;
 
 enum export {
 	export_plain,      export_unused,     export_gpl,
@@ -2209,8 +2211,11 @@ static int check_exports(struct module *mod)
 
 		if (exp->namespace &&
 		    !module_imports_namespace(mod, exp->namespace)) {
-			warn("module %s uses symbol %s from namespace %s, but does not import it.\n",
-			     basename, exp->name, exp->namespace);
+			warn_unless(!allow_missing_ns_imports,
+				    "module %s uses symbol %s from namespace %s, but does not import it.\n",
+				    basename, exp->name, exp->namespace);
+			if (!allow_missing_ns_imports)
+				err = 1;
 			add_namespace(&mod->missing_namespaces, exp->namespace);
 		}
 
@@ -2553,7 +2558,7 @@ int main(int argc, char **argv)
 	struct ext_sym_list *extsym_iter;
 	struct ext_sym_list *extsym_start = NULL;
 
-	while ((opt = getopt(argc, argv, "i:e:mnsT:o:awEd:")) != -1) {
+	while ((opt = getopt(argc, argv, "i:e:mnsT:o:awENd:")) != -1) {
 		switch (opt) {
 		case 'i':
 			kernel_read = optarg;
@@ -2591,6 +2596,9 @@ int main(int argc, char **argv)
 		case 'E':
 			sec_mismatch_fatal = 1;
 			break;
+		case 'N':
+			allow_missing_ns_imports = 1;
+			break;
 		case 'd':
 			missing_namespace_deps = optarg;
 			break;
-- 
2.16.4


^ permalink raw reply related	[flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] modpost: rework and consolidate logging interface
  2020-02-26 14:26 [PATCH v2 1/2] modpost: rework and consolidate logging interface Jessica Yu
  2020-02-26 14:26 ` [PATCH v2 2/2] modpost: return error if module is missing ns imports and MODULE_ALLOW_MISSING_NAMESPACE_IMPORTS=n Jessica Yu
@ 2020-03-03 14:42 ` Masahiro Yamada
  2020-03-03 14:57   ` Jessica Yu
  2020-03-03 15:23 ` Masahiro Yamada
  2 siblings, 1 reply; 7+ messages in thread
From: Masahiro Yamada @ 2020-03-03 14:42 UTC (permalink / raw)
  To: Jessica Yu; +Cc: Matthias Maennich, Joe Perches, Linux Kernel Mailing List

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);







--
Best Regards
Masahiro Yamada

^ permalink raw reply	[flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] modpost: rework and consolidate logging interface
  2020-03-03 14:42 ` [PATCH v2 1/2] modpost: rework and consolidate logging interface Masahiro Yamada
@ 2020-03-03 14:57   ` Jessica Yu
  2020-03-04 11:14     ` Matthias Maennich
  0 siblings, 1 reply; 7+ messages in thread
From: Jessica Yu @ 2020-03-03 14:57 UTC (permalink / raw)
  To: Masahiro Yamada; +Cc: Matthias Maennich, Joe Perches, Linux Kernel Mailing List

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

^ permalink raw reply	[flat|nested] 7+ messages in thread
* Re: [PATCH v2 2/2] modpost: return error if module is missing ns imports and MODULE_ALLOW_MISSING_NAMESPACE_IMPORTS=n
  2020-02-26 14:26 ` [PATCH v2 2/2] modpost: return error if module is missing ns imports and MODULE_ALLOW_MISSING_NAMESPACE_IMPORTS=n Jessica Yu
@ 2020-03-03 15:21   ` Masahiro Yamada
  0 siblings, 0 replies; 7+ messages in thread
From: Masahiro Yamada @ 2020-03-03 15:21 UTC (permalink / raw)
  To: Jessica Yu; +Cc: Matthias Maennich, Joe Perches, Linux Kernel Mailing List

On Wed, Feb 26, 2020 at 11:26 PM Jessica Yu <jeyu@kernel.org> wrote:
>
> Currently when CONFIG_MODULE_ALLOW_MISSING_NAMESPACE_IMPORTS=n, modpost only warns
> when a module is missing namespace imports. Under this configuration, such a module
> cannot be loaded into the kernel anyway, as the module loader would reject it.
> We might as well return a build error when a module is missing namespace imports
> under CONFIG_MODULE_ALLOW_MISSING_NAMESPACE_IMPORTS=n, so that the build
> warning does not go ignored/unnoticed.
>
> Signed-off-by: Jessica Yu <jeyu@kernel.org>
> ---


When you send v3, could you please fix the
following checkpatch warning and error?



WARNING: Possible unwrapped commit description (prefer a maximum 75
chars per line)
#59:
Currently when CONFIG_MODULE_ALLOW_MISSING_NAMESPACE_IMPORTS=n,
modpost only warns

ERROR: do not initialise statics to 0
#107: FILE: scripts/mod/modpost.c:43:
+static int allow_missing_ns_imports = 0;







>  scripts/Makefile.modpost | 15 ++++++++-------
>  scripts/mod/modpost.c    | 14 +++++++++++---
>  2 files changed, 19 insertions(+), 10 deletions(-)
>
> diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
> index b4d3f2d122ac..957eed6a17a5 100644
> --- a/scripts/Makefile.modpost
> +++ b/scripts/Makefile.modpost
> @@ -46,13 +46,14 @@ include scripts/Kbuild.include
>  kernelsymfile := $(objtree)/Module.symvers
>  modulesymfile := $(firstword $(KBUILD_EXTMOD))/Module.symvers
>
> -MODPOST = scripts/mod/modpost                                          \
> -       $(if $(CONFIG_MODVERSIONS),-m)                                  \
> -       $(if $(CONFIG_MODULE_SRCVERSION_ALL),-a)                        \
> -       $(if $(KBUILD_EXTMOD),-i,-o) $(kernelsymfile)                   \
> -       $(if $(KBUILD_EXTMOD),$(addprefix -e ,$(KBUILD_EXTRA_SYMBOLS))) \
> -       $(if $(KBUILD_EXTMOD),-o $(modulesymfile))                      \
> -       $(if $(CONFIG_SECTION_MISMATCH_WARN_ONLY),,-E)                  \
> +MODPOST = scripts/mod/modpost                                                          \
> +       $(if $(CONFIG_MODVERSIONS),-m)                                                  \
> +       $(if $(CONFIG_MODULE_SRCVERSION_ALL),-a)                                        \
> +       $(if $(KBUILD_EXTMOD),-i,-o) $(kernelsymfile)                                   \
> +       $(if $(KBUILD_EXTMOD),$(addprefix -e ,$(KBUILD_EXTRA_SYMBOLS)))                 \
> +       $(if $(KBUILD_EXTMOD),-o $(modulesymfile))                                      \
> +       $(if $(CONFIG_SECTION_MISMATCH_WARN_ONLY),,-E)                                  \
> +       $(if $(CONFIG_MODULE_ALLOW_MISSING_NAMESPACE_IMPORTS)$(KBUILD_NSDEPS),-N)       \
>         $(if $(KBUILD_MODPOST_WARN),-w)
>
>  ifdef MODPOST_VMLINUX
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 3201a2ac5cc4..d164be63c2e3 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -39,6 +39,8 @@ static int sec_mismatch_count = 0;
>  static int sec_mismatch_fatal = 0;
>  /* ignore missing files */
>  static int ignore_missing_files;
> +/* If set to 1, only warn (instead of error) about missing ns imports */
> +static int allow_missing_ns_imports = 0;
>
>  enum export {
>         export_plain,      export_unused,     export_gpl,
> @@ -2209,8 +2211,11 @@ static int check_exports(struct module *mod)
>
>                 if (exp->namespace &&
>                     !module_imports_namespace(mod, exp->namespace)) {
> -                       warn("module %s uses symbol %s from namespace %s, but does not import it.\n",
> -                            basename, exp->name, exp->namespace);
> +                       warn_unless(!allow_missing_ns_imports,
> +                                   "module %s uses symbol %s from namespace %s, but does not import it.\n",
> +                                   basename, exp->name, exp->namespace);
> +                       if (!allow_missing_ns_imports)
> +                               err = 1;
>                         add_namespace(&mod->missing_namespaces, exp->namespace);
>                 }
>
> @@ -2553,7 +2558,7 @@ int main(int argc, char **argv)
>         struct ext_sym_list *extsym_iter;
>         struct ext_sym_list *extsym_start = NULL;
>
> -       while ((opt = getopt(argc, argv, "i:e:mnsT:o:awEd:")) != -1) {
> +       while ((opt = getopt(argc, argv, "i:e:mnsT:o:awENd:")) != -1) {
>                 switch (opt) {
>                 case 'i':
>                         kernel_read = optarg;
> @@ -2591,6 +2596,9 @@ int main(int argc, char **argv)
>                 case 'E':
>                         sec_mismatch_fatal = 1;
>                         break;
> +               case 'N':
> +                       allow_missing_ns_imports = 1;
> +                       break;
>                 case 'd':
>                         missing_namespace_deps = optarg;
>                         break;
> --
> 2.16.4
>


-- 
Best Regards
Masahiro Yamada

^ permalink raw reply	[flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] modpost: rework and consolidate logging interface
  2020-02-26 14:26 [PATCH v2 1/2] modpost: rework and consolidate logging interface Jessica Yu
  2020-02-26 14:26 ` [PATCH v2 2/2] modpost: return error if module is missing ns imports and MODULE_ALLOW_MISSING_NAMESPACE_IMPORTS=n Jessica Yu
  2020-03-03 14:42 ` [PATCH v2 1/2] modpost: rework and consolidate logging interface Masahiro Yamada
@ 2020-03-03 15:23 ` Masahiro Yamada
  2 siblings, 0 replies; 7+ messages in thread
From: Masahiro Yamada @ 2020-03-03 15:23 UTC (permalink / raw)
  To: Jessica Yu; +Cc: Matthias Maennich, Joe Perches, Linux Kernel Mailing List

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 = "";
>         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) {


One nit:

Please insert a space after 'switch'.

I see this checkpatch error:

ERROR: space required before the open parenthesis '('
#102: FILE: scripts/mod/modpost.c:61:
+ switch(loglevel) {




-- 
Best Regards
Masahiro Yamada

^ permalink raw reply	[flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] modpost: rework and consolidate logging interface
  2020-03-03 14:57   ` Jessica Yu
@ 2020-03-04 11:14     ` Matthias Maennich
  0 siblings, 0 replies; 7+ messages in thread
From: Matthias Maennich @ 2020-03-04 11:14 UTC (permalink / raw)
  To: Jessica Yu; +Cc: Masahiro Yamada, Joe Perches, Linux Kernel Mailing List

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

^ permalink raw reply	[flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-03-04 11:14 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-26 14:26 [PATCH v2 1/2] modpost: rework and consolidate logging interface Jessica Yu
2020-02-26 14:26 ` [PATCH v2 2/2] modpost: return error if module is missing ns imports and MODULE_ALLOW_MISSING_NAMESPACE_IMPORTS=n Jessica Yu
2020-03-03 15:21   ` Masahiro Yamada
2020-03-03 14:42 ` [PATCH v2 1/2] modpost: rework and consolidate logging interface Masahiro Yamada
2020-03-03 14:57   ` Jessica Yu
2020-03-04 11:14     ` Matthias Maennich
2020-03-03 15:23 ` Masahiro Yamada

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).