* [PATCH] modpost: return error if module is missing ns imports and MODULE_ALLOW_MISSING_NAMESPACE_IMPORTS=n
@ 2020-02-14 14:37 Jessica Yu
2020-02-17 14:56 ` Matthias Maennich
0 siblings, 1 reply; 5+ messages in thread
From: Jessica Yu @ 2020-02-14 14:37 UTC (permalink / raw)
To: Masahiro Yamada, Matthias Maennich; +Cc: 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 | 1 +
scripts/mod/modpost.c | 19 +++++++++++++++----
2 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
index b4d3f2d122ac..a53660f910a9 100644
--- a/scripts/Makefile.modpost
+++ b/scripts/Makefile.modpost
@@ -53,6 +53,7 @@ MODPOST = scripts/mod/modpost \
$(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),,-N) \
$(if $(KBUILD_MODPOST_WARN),-w)
ifdef MODPOST_VMLINUX
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 7edfdb2f4497..53e966f7d557 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;
+/* Return an error when there are missing namespace imports */
+static int missing_ns_import_error = 0;
enum export {
export_plain, export_unused, export_gpl,
@@ -2216,9 +2218,15 @@ 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);
- add_namespace(&mod->missing_namespaces, exp->namespace);
+ if (missing_ns_import_error) {
+ merror("module %s uses symbol %s from namespace %s, but does not import it.\n",
+ basename, exp->name, exp->namespace);
+ err = 1;
+ } else {
+ warn("module %s uses symbol %s from namespace %s, but does not import it.\n",
+ basename, exp->name, exp->namespace);
+ }
+ add_namespace(&mod->missing_namespaces, exp->namespace);
}
if (!mod->gpl_compatible)
@@ -2560,7 +2568,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;
@@ -2598,6 +2606,9 @@ int main(int argc, char **argv)
case 'E':
sec_mismatch_fatal = 1;
break;
+ case 'N':
+ missing_ns_import_error = 1;
+ break;
case 'd':
missing_namespace_deps = optarg;
break;
--
2.16.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] modpost: return error if module is missing ns imports and MODULE_ALLOW_MISSING_NAMESPACE_IMPORTS=n
2020-02-14 14:37 [PATCH] modpost: return error if module is missing ns imports and MODULE_ALLOW_MISSING_NAMESPACE_IMPORTS=n Jessica Yu
@ 2020-02-17 14:56 ` Matthias Maennich
2020-02-18 16:05 ` Jessica Yu
0 siblings, 1 reply; 5+ messages in thread
From: Matthias Maennich @ 2020-02-17 14:56 UTC (permalink / raw)
To: Jessica Yu; +Cc: Masahiro Yamada, linux-kernel, Martijn Coenen
Hi Jessica!
On Fri, Feb 14, 2020 at 03:37:09PM +0100, Jessica Yu 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.
I generally agree with the idea of the patch. Thanks for working on
this! I also can't remember any reason why I did not write it like this
initially. Probably just because I introduced this configuration option
quite late in the development process of the initial patches.
>
>Signed-off-by: Jessica Yu <jeyu@kernel.org>
>---
> scripts/Makefile.modpost | 1 +
> scripts/mod/modpost.c | 19 +++++++++++++++----
> 2 files changed, 16 insertions(+), 4 deletions(-)
>
>diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
>index b4d3f2d122ac..a53660f910a9 100644
>--- a/scripts/Makefile.modpost
>+++ b/scripts/Makefile.modpost
>@@ -53,6 +53,7 @@ MODPOST = scripts/mod/modpost \
> $(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),,-N) \
> $(if $(KBUILD_MODPOST_WARN),-w)
>
> ifdef MODPOST_VMLINUX
>diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
>index 7edfdb2f4497..53e966f7d557 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;
>+/* Return an error when there are missing namespace imports */
>+static int missing_ns_import_error = 0;
A more suitable name is maybe missing_ns_import_is_error or follow the
naming of the config option: allow_missing_ns_imports (with default = 1).
>
> enum export {
> export_plain, export_unused, export_gpl,
>@@ -2216,9 +2218,15 @@ 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);
>- add_namespace(&mod->missing_namespaces, exp->namespace);
>+ if (missing_ns_import_error) {
>+ merror("module %s uses symbol %s from namespace %s, but does not import it.\n",
>+ basename, exp->name, exp->namespace);
I would like to avoid the code duplication here. The string literal is
identical for both cases.
>+ err = 1;
Also, if we fail here, we might as well help the user to fix it by
suggesting to run `make nsdeps` (once per failed modpost run). Speaking
of which, `make nsdeps` is currently broken by this patch as it relies
on a successful (yet warning-full) build of the modules. So, in case of
`make nsdeps`, we probably have to omit the -N flag again when invoking
modpost.
Cheers,
Matthias
>+ } else {
>+ warn("module %s uses symbol %s from namespace %s, but does not import it.\n",
>+ basename, exp->name, exp->namespace);
>+ }
>+ add_namespace(&mod->missing_namespaces, exp->namespace);
> }
>
> if (!mod->gpl_compatible)
>@@ -2560,7 +2568,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;
>@@ -2598,6 +2606,9 @@ int main(int argc, char **argv)
> case 'E':
> sec_mismatch_fatal = 1;
> break;
>+ case 'N':
>+ missing_ns_import_error = 1;
>+ break;
> case 'd':
> missing_namespace_deps = optarg;
> break;
>--
>2.16.4
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] modpost: return error if module is missing ns imports and MODULE_ALLOW_MISSING_NAMESPACE_IMPORTS=n
2020-02-17 14:56 ` Matthias Maennich
@ 2020-02-18 16:05 ` Jessica Yu
2020-02-18 19:27 ` Masahiro Yamada
0 siblings, 1 reply; 5+ messages in thread
From: Jessica Yu @ 2020-02-18 16:05 UTC (permalink / raw)
To: Matthias Maennich; +Cc: Masahiro Yamada, linux-kernel, Martijn Coenen
+++ Matthias Maennich [17/02/20 14:56 +0000]:
>Hi Jessica!
>
>On Fri, Feb 14, 2020 at 03:37:09PM +0100, Jessica Yu 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.
>
>I generally agree with the idea of the patch. Thanks for working on
>this! I also can't remember any reason why I did not write it like this
>initially. Probably just because I introduced this configuration option
>quite late in the development process of the initial patches.
>
>>
>>Signed-off-by: Jessica Yu <jeyu@kernel.org>
>>---
>>scripts/Makefile.modpost | 1 +
>>scripts/mod/modpost.c | 19 +++++++++++++++----
>>2 files changed, 16 insertions(+), 4 deletions(-)
>>
>>diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
>>index b4d3f2d122ac..a53660f910a9 100644
>>--- a/scripts/Makefile.modpost
>>+++ b/scripts/Makefile.modpost
>>@@ -53,6 +53,7 @@ MODPOST = scripts/mod/modpost \
>> $(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),,-N) \
>> $(if $(KBUILD_MODPOST_WARN),-w)
>>
>>ifdef MODPOST_VMLINUX
>>diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
>>index 7edfdb2f4497..53e966f7d557 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;
>>+/* Return an error when there are missing namespace imports */
>>+static int missing_ns_import_error = 0;
>
>A more suitable name is maybe missing_ns_import_is_error or follow the
>naming of the config option: allow_missing_ns_imports (with default = 1).
>
>>
>>enum export {
>> export_plain, export_unused, export_gpl,
>>@@ -2216,9 +2218,15 @@ 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);
>>- add_namespace(&mod->missing_namespaces, exp->namespace);
>>+ if (missing_ns_import_error) {
>>+ merror("module %s uses symbol %s from namespace %s, but does not import it.\n",
>>+ basename, exp->name, exp->namespace);
>
>I would like to avoid the code duplication here. The string literal is
>identical for both cases.
Hm, but one is a call to merror() and the other to warn(). The
previous if (warn_unresolved) block does the same thing. I am not sure
how to simplify it to one call without introducing macro magic or
overcomplicating things. Or were you thinking of something else?
>>+ err = 1;
>
>Also, if we fail here, we might as well help the user to fix it by
>suggesting to run `make nsdeps` (once per failed modpost run). Speaking
>of which, `make nsdeps` is currently broken by this patch as it relies
>on a successful (yet warning-full) build of the modules. So, in case of
>`make nsdeps`, we probably have to omit the -N flag again when invoking
>modpost.
Good catch! Since KBUILD_NSDEPS is set when running `make nsdeps`,
maybe we can do something like:
diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
index a53660f910a9..145703ef8d3a 100644
--- a/scripts/Makefile.modpost
+++ b/scripts/Makefile.modpost
@@ -53,7 +53,7 @@ MODPOST = scripts/mod/modpost \
$(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),,-N) \
+ $(if $(CONFIG_MODULE_ALLOW_MISSING_NAMESPACE_IMPORTS),,$(if $(KBUILD_NSDEPS),,-N)) \
$(if $(KBUILD_MODPOST_WARN),-w)
ifdef MODPOST_VMLINUX
Thanks for the review!
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] modpost: return error if module is missing ns imports and MODULE_ALLOW_MISSING_NAMESPACE_IMPORTS=n
2020-02-18 16:05 ` Jessica Yu
@ 2020-02-18 19:27 ` Masahiro Yamada
2020-02-19 8:46 ` Jessica Yu
0 siblings, 1 reply; 5+ messages in thread
From: Masahiro Yamada @ 2020-02-18 19:27 UTC (permalink / raw)
To: Jessica Yu; +Cc: Matthias Maennich, Linux Kernel Mailing List, Martijn Coenen
On Wed, Feb 19, 2020 at 1:06 AM Jessica Yu <jeyu@kernel.org> wrote:
>
> +++ Matthias Maennich [17/02/20 14:56 +0000]:
> >Hi Jessica!
> >
> >On Fri, Feb 14, 2020 at 03:37:09PM +0100, Jessica Yu 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.
> >
> >I generally agree with the idea of the patch. Thanks for working on
> >this! I also can't remember any reason why I did not write it like this
> >initially. Probably just because I introduced this configuration option
> >quite late in the development process of the initial patches.
> >
> >>
> >>Signed-off-by: Jessica Yu <jeyu@kernel.org>
> >>---
> >>scripts/Makefile.modpost | 1 +
> >>scripts/mod/modpost.c | 19 +++++++++++++++----
> >>2 files changed, 16 insertions(+), 4 deletions(-)
> >>
> >>diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
> >>index b4d3f2d122ac..a53660f910a9 100644
> >>--- a/scripts/Makefile.modpost
> >>+++ b/scripts/Makefile.modpost
> >>@@ -53,6 +53,7 @@ MODPOST = scripts/mod/modpost \
> >> $(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),,-N) \
> >> $(if $(KBUILD_MODPOST_WARN),-w)
> >>
> >>ifdef MODPOST_VMLINUX
> >>diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> >>index 7edfdb2f4497..53e966f7d557 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;
> >>+/* Return an error when there are missing namespace imports */
> >>+static int missing_ns_import_error = 0;
> >
> >A more suitable name is maybe missing_ns_import_is_error or follow the
> >naming of the config option: allow_missing_ns_imports (with default = 1).
> >
> >>
> >>enum export {
> >> export_plain, export_unused, export_gpl,
> >>@@ -2216,9 +2218,15 @@ 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);
> >>- add_namespace(&mod->missing_namespaces, exp->namespace);
> >>+ if (missing_ns_import_error) {
> >>+ merror("module %s uses symbol %s from namespace %s, but does not import it.\n",
> >>+ basename, exp->name, exp->namespace);
> >
> >I would like to avoid the code duplication here. The string literal is
> >identical for both cases.
>
> Hm, but one is a call to merror() and the other to warn(). The
> previous if (warn_unresolved) block does the same thing. I am not sure
> how to simplify it to one call without introducing macro magic or
> overcomplicating things. Or were you thinking of something else?
I would not say this is a horrible duplication,
but if you avoid repeating the same string,
maybe you could do like this:
PRINTF log(enum loglevel loglevel, const char *fmt, ...)
BTW, you accidentally changed the indentation of
add_namespace(&mod->missing_namespaces, exp->namespace);
> >>+ err = 1;
> >
> >Also, if we fail here, we might as well help the user to fix it by
> >suggesting to run `make nsdeps` (once per failed modpost run). Speaking
> >of which, `make nsdeps` is currently broken by this patch as it relies
> >on a successful (yet warning-full) build of the modules. So, in case of
> >`make nsdeps`, we probably have to omit the -N flag again when invoking
> >modpost.
>
> Good catch! Since KBUILD_NSDEPS is set when running `make nsdeps`,
> maybe we can do something like:
>
> diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
> index a53660f910a9..145703ef8d3a 100644
> --- a/scripts/Makefile.modpost
> +++ b/scripts/Makefile.modpost
> @@ -53,7 +53,7 @@ MODPOST = scripts/mod/modpost \
> $(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),,-N) \
> + $(if $(CONFIG_MODULE_ALLOW_MISSING_NAMESPACE_IMPORTS),,$(if $(KBUILD_NSDEPS),,-N)) \
If you follow Matthias' suggestion
"follow the naming of the config option: allow_missing_ns_imports
(with default = 1)."
the option is inverted, and you can write it more simply:
$(if $(CONFIG_MODULE_ALLOW_MISSING_NAMESPACE_IMPORTS)$(KBUILD_NSDEPS),-n)
\
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] modpost: return error if module is missing ns imports and MODULE_ALLOW_MISSING_NAMESPACE_IMPORTS=n
2020-02-18 19:27 ` Masahiro Yamada
@ 2020-02-19 8:46 ` Jessica Yu
0 siblings, 0 replies; 5+ messages in thread
From: Jessica Yu @ 2020-02-19 8:46 UTC (permalink / raw)
To: Masahiro Yamada
Cc: Matthias Maennich, Linux Kernel Mailing List, Martijn Coenen
+++ Masahiro Yamada [19/02/20 04:27 +0900]:
>On Wed, Feb 19, 2020 at 1:06 AM Jessica Yu <jeyu@kernel.org> wrote:
>>
>> +++ Matthias Maennich [17/02/20 14:56 +0000]:
>> >Hi Jessica!
>> >
>> >On Fri, Feb 14, 2020 at 03:37:09PM +0100, Jessica Yu 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.
>> >
>> >I generally agree with the idea of the patch. Thanks for working on
>> >this! I also can't remember any reason why I did not write it like this
>> >initially. Probably just because I introduced this configuration option
>> >quite late in the development process of the initial patches.
>> >
>> >>
>> >>Signed-off-by: Jessica Yu <jeyu@kernel.org>
>> >>---
>> >>scripts/Makefile.modpost | 1 +
>> >>scripts/mod/modpost.c | 19 +++++++++++++++----
>> >>2 files changed, 16 insertions(+), 4 deletions(-)
>> >>
>> >>diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
>> >>index b4d3f2d122ac..a53660f910a9 100644
>> >>--- a/scripts/Makefile.modpost
>> >>+++ b/scripts/Makefile.modpost
>> >>@@ -53,6 +53,7 @@ MODPOST = scripts/mod/modpost \
>> >> $(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),,-N) \
>> >> $(if $(KBUILD_MODPOST_WARN),-w)
>> >>
>> >>ifdef MODPOST_VMLINUX
>> >>diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
>> >>index 7edfdb2f4497..53e966f7d557 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;
>> >>+/* Return an error when there are missing namespace imports */
>> >>+static int missing_ns_import_error = 0;
>> >
>> >A more suitable name is maybe missing_ns_import_is_error or follow the
>> >naming of the config option: allow_missing_ns_imports (with default = 1).
>> >
>> >>
>> >>enum export {
>> >> export_plain, export_unused, export_gpl,
>> >>@@ -2216,9 +2218,15 @@ 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);
>> >>- add_namespace(&mod->missing_namespaces, exp->namespace);
>> >>+ if (missing_ns_import_error) {
>> >>+ merror("module %s uses symbol %s from namespace %s, but does not import it.\n",
>> >>+ basename, exp->name, exp->namespace);
>> >
>> >I would like to avoid the code duplication here. The string literal is
>> >identical for both cases.
>>
>> Hm, but one is a call to merror() and the other to warn(). The
>> previous if (warn_unresolved) block does the same thing. I am not sure
>> how to simplify it to one call without introducing macro magic or
>> overcomplicating things. Or were you thinking of something else?
>
>
>I would not say this is a horrible duplication,
>but if you avoid repeating the same string,
>maybe you could do like this:
>
>PRINTF log(enum loglevel loglevel, const char *fmt, ...)
>
>
>
>BTW, you accidentally changed the indentation of
>add_namespace(&mod->missing_namespaces, exp->namespace);
>
>
>
>> >>+ err = 1;
>> >
>> >Also, if we fail here, we might as well help the user to fix it by
>> >suggesting to run `make nsdeps` (once per failed modpost run). Speaking
>> >of which, `make nsdeps` is currently broken by this patch as it relies
>> >on a successful (yet warning-full) build of the modules. So, in case of
>> >`make nsdeps`, we probably have to omit the -N flag again when invoking
>> >modpost.
>>
>> Good catch! Since KBUILD_NSDEPS is set when running `make nsdeps`,
>> maybe we can do something like:
>>
>> diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
>> index a53660f910a9..145703ef8d3a 100644
>> --- a/scripts/Makefile.modpost
>> +++ b/scripts/Makefile.modpost
>> @@ -53,7 +53,7 @@ MODPOST = scripts/mod/modpost \
>> $(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),,-N) \
>> + $(if $(CONFIG_MODULE_ALLOW_MISSING_NAMESPACE_IMPORTS),,$(if $(KBUILD_NSDEPS),,-N)) \
>
>
>If you follow Matthias' suggestion
>"follow the naming of the config option: allow_missing_ns_imports
>(with default = 1)."
>the option is inverted, and you can write it more simply:
>
>
> $(if $(CONFIG_MODULE_ALLOW_MISSING_NAMESPACE_IMPORTS)$(KBUILD_NSDEPS),-n)
> \
Yeah, that looks much better. Thanks!
Jessica
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-02-19 8:46 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-14 14:37 [PATCH] modpost: return error if module is missing ns imports and MODULE_ALLOW_MISSING_NAMESPACE_IMPORTS=n Jessica Yu
2020-02-17 14:56 ` Matthias Maennich
2020-02-18 16:05 ` Jessica Yu
2020-02-18 19:27 ` Masahiro Yamada
2020-02-19 8:46 ` Jessica Yu
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).