* [PATCH 1/4] scripts/nsdeps: use $MODORDER to obtain correct modules.order path @ 2019-10-28 15:14 Jessica Yu 2019-10-28 15:14 ` [PATCH 2/4] scripts/nsdeps: don't prepend $srctree if *.mod already contains full paths Jessica Yu ` (3 more replies) 0 siblings, 4 replies; 14+ messages in thread From: Jessica Yu @ 2019-10-28 15:14 UTC (permalink / raw) To: linux-kernel; +Cc: Matthias Maennich, Masahiro Yamada, Jessica Yu The nsdeps script calls generate_deps() for every module in the modules.order file. It prepends $objtree to obtain the full path of the top-level modules.order file. This produces incorrect results when calling nsdeps for an external module, as only the ns dependencies for in-tree modules listed in $objtree/modules.order are generated rather than the ns dependencies for the external module. To fix this, just use the MODORDER variable provided by kbuild - it uses the correct path for the relevant modules.order file (either in-tree or the one produced by the external module build). Signed-off-by: Jessica Yu <jeyu@kernel.org> --- So, not being too familiar with kbuild, I am not sure if MODORDER was the appropriate kbuild variable to use, but I could not find anything else that gives us the modules.order path. Masahiro, please let me know if this is appropriate usage. scripts/nsdeps | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/nsdeps b/scripts/nsdeps index dda6fbac016e..54d2ab8f9e5c 100644 --- a/scripts/nsdeps +++ b/scripts/nsdeps @@ -52,7 +52,7 @@ generate_deps() { done } -for f in `cat $objtree/modules.order`; do +for f in `cat $MODORDER`; do generate_deps $f done -- 2.16.4 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/4] scripts/nsdeps: don't prepend $srctree if *.mod already contains full paths 2019-10-28 15:14 [PATCH 1/4] scripts/nsdeps: use $MODORDER to obtain correct modules.order path Jessica Yu @ 2019-10-28 15:14 ` Jessica Yu 2019-10-29 8:59 ` Masahiro Yamada 2019-10-28 15:14 ` [PATCH 3/4] nsdeps: remove stale .ns_deps files before generating new ones Jessica Yu ` (2 subsequent siblings) 3 siblings, 1 reply; 14+ messages in thread From: Jessica Yu @ 2019-10-28 15:14 UTC (permalink / raw) To: linux-kernel; +Cc: Matthias Maennich, Masahiro Yamada, Jessica Yu When building in-tree modules, the *.mod file contains relative paths. When building external modules, the resulting *.mod file contains absolute paths. Allow for the nsdeps script to account for both types of paths and only prepend $srctree in the case of relative paths. Otherwise, the script will append $srctree to the path regardless and it will error out with file not found errors if the path was already absolute to begin with. Signed-off-by: Jessica Yu <jeyu@kernel.org> --- The sed regex is getting more ugly. It's not my strong point :/ If anyone has a better regex to prepend $srctree for every relative path encountered while ignoring absolute paths, I'm all ears. scripts/nsdeps | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/nsdeps b/scripts/nsdeps index 54d2ab8f9e5c..9ddcd5cb96b1 100644 --- a/scripts/nsdeps +++ b/scripts/nsdeps @@ -33,7 +33,7 @@ generate_deps() { if [ ! -f "$ns_deps_file" ]; then return; fi local mod_source_files=`cat $mod_file | sed -n 1p \ | sed -e 's/\.o/\.c/g' \ - | sed "s|[^ ]* *|${srctree}/&|g"` + | sed -E "s%(^|\s)([^/][^ ]*)%\1$srctree/\2%g"` for ns in `cat $ns_deps_file`; do echo "Adding namespace $ns to module $mod_name (if needed)." generate_deps_for_ns $ns $mod_source_files -- 2.16.4 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 2/4] scripts/nsdeps: don't prepend $srctree if *.mod already contains full paths 2019-10-28 15:14 ` [PATCH 2/4] scripts/nsdeps: don't prepend $srctree if *.mod already contains full paths Jessica Yu @ 2019-10-29 8:59 ` Masahiro Yamada 0 siblings, 0 replies; 14+ messages in thread From: Masahiro Yamada @ 2019-10-29 8:59 UTC (permalink / raw) To: Jessica Yu; +Cc: Linux Kernel Mailing List, Matthias Maennich On Tue, Oct 29, 2019 at 12:14 AM Jessica Yu <jeyu@kernel.org> wrote: > > When building in-tree modules, the *.mod file contains relative paths. > When building external modules, the resulting *.mod file contains absolute > paths. Not necessarily true. Kbuild does not impose any restriction about absolute/relative path. M= can be a relative path. > Allow for the nsdeps script to account for both types of paths and > only prepend $srctree in the case of relative paths. Otherwise, the script > will append $srctree to the path regardless and it will error out with file > not found errors if the path was already absolute to begin with. > > Signed-off-by: Jessica Yu <jeyu@kernel.org> > --- > > The sed regex is getting more ugly. It's not my strong point :/ If anyone > has a better regex to prepend $srctree for every relative path encountered > while ignoring absolute paths, I'm all ears. It is not the problem of sed regex ugliness. You can prefix $srctree/ unlesss building external modules. if [ "$KBUILD_EXTMOD" ]; then src_prefix= else src_prefix=$srctree/ fi Then, sed "s|[^ ]* *|${src_prefix}&|g"` Caution: not tested at all > scripts/nsdeps | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/scripts/nsdeps b/scripts/nsdeps > index 54d2ab8f9e5c..9ddcd5cb96b1 100644 > --- a/scripts/nsdeps > +++ b/scripts/nsdeps > @@ -33,7 +33,7 @@ generate_deps() { > if [ ! -f "$ns_deps_file" ]; then return; fi > local mod_source_files=`cat $mod_file | sed -n 1p \ > | sed -e 's/\.o/\.c/g' \ > - | sed "s|[^ ]* *|${srctree}/&|g"` > + | sed -E "s%(^|\s)([^/][^ ]*)%\1$srctree/\2%g"` > for ns in `cat $ns_deps_file`; do > echo "Adding namespace $ns to module $mod_name (if needed)." > generate_deps_for_ns $ns $mod_source_files > -- > 2.16.4 > -- Best Regards Masahiro Yamada ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 3/4] nsdeps: remove stale .ns_deps files before generating new ones 2019-10-28 15:14 [PATCH 1/4] scripts/nsdeps: use $MODORDER to obtain correct modules.order path Jessica Yu 2019-10-28 15:14 ` [PATCH 2/4] scripts/nsdeps: don't prepend $srctree if *.mod already contains full paths Jessica Yu @ 2019-10-28 15:14 ` Jessica Yu 2019-10-29 10:45 ` Masahiro Yamada 2019-10-28 15:14 ` [PATCH 4/4] scripts/nsdeps: make sure to pass all module source files to spatch Jessica Yu 2019-10-29 12:48 ` [PATCH 1/4] scripts/nsdeps: use $MODORDER to obtain correct modules.order path Masahiro Yamada 3 siblings, 1 reply; 14+ messages in thread From: Jessica Yu @ 2019-10-28 15:14 UTC (permalink / raw) To: linux-kernel; +Cc: Matthias Maennich, Masahiro Yamada, Jessica Yu When adding or removing namespaces, calling `make` does not necessarily remove existing stale .ns_deps files. That is, one could remove a namespace, call make, and while modpost writes the correct, new .ns_deps files, stale ones are not removed from the source tree, thus producing incorrect results when running `make nsdeps`, i.e., inserting MODULE_IMPORT_NS() statements for namespaces that have been removed. Clean up old .ns_deps files before generating new ones with modpost. Signed-off-by: Jessica Yu <jeyu@kernel.org> --- Makefile | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Makefile b/Makefile index ffd7a912fc46..22f9894b346b 100644 --- a/Makefile +++ b/Makefile @@ -1685,6 +1685,8 @@ tags TAGS cscope gtags: FORCE PHONY += nsdeps nsdeps: modules + @find $(if $(KBUILD_EXTMOD), $(KBUILD_EXTMOD), .) $(RCS_FIND_IGNORE) \ + -name '*.ns_deps' -type f -print | xargs rm -f $(Q)$(MAKE) -f $(srctree)/scripts/Makefile.modpost nsdeps $(Q)$(CONFIG_SHELL) $(srctree)/scripts/$@ -- 2.16.4 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 3/4] nsdeps: remove stale .ns_deps files before generating new ones 2019-10-28 15:14 ` [PATCH 3/4] nsdeps: remove stale .ns_deps files before generating new ones Jessica Yu @ 2019-10-29 10:45 ` Masahiro Yamada 0 siblings, 0 replies; 14+ messages in thread From: Masahiro Yamada @ 2019-10-29 10:45 UTC (permalink / raw) To: Jessica Yu; +Cc: Linux Kernel Mailing List, Matthias Maennich On Tue, Oct 29, 2019 at 12:14 AM Jessica Yu <jeyu@kernel.org> wrote: > > When adding or removing namespaces, calling `make` does not necessarily > remove existing stale .ns_deps files. That is, one could remove a > namespace, call make, and while modpost writes the correct, new .ns_deps > files, stale ones are not removed from the source tree, thus producing > incorrect results when running `make nsdeps`, i.e., inserting > MODULE_IMPORT_NS() statements for namespaces that have been removed. > Clean up old .ns_deps files before generating new ones with modpost. Hmm, correct. But, removing *.ns_deps every time is not an elegant solution. I want to try a different approach. > > Signed-off-by: Jessica Yu <jeyu@kernel.org> > --- > Makefile | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/Makefile b/Makefile > index ffd7a912fc46..22f9894b346b 100644 > --- a/Makefile > +++ b/Makefile > @@ -1685,6 +1685,8 @@ tags TAGS cscope gtags: FORCE > PHONY += nsdeps > > nsdeps: modules > + @find $(if $(KBUILD_EXTMOD), $(KBUILD_EXTMOD), .) $(RCS_FIND_IGNORE) \ > + -name '*.ns_deps' -type f -print | xargs rm -f > $(Q)$(MAKE) -f $(srctree)/scripts/Makefile.modpost nsdeps > $(Q)$(CONFIG_SHELL) $(srctree)/scripts/$@ > > -- > 2.16.4 > -- Best Regards Masahiro Yamada ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 4/4] scripts/nsdeps: make sure to pass all module source files to spatch 2019-10-28 15:14 [PATCH 1/4] scripts/nsdeps: use $MODORDER to obtain correct modules.order path Jessica Yu 2019-10-28 15:14 ` [PATCH 2/4] scripts/nsdeps: don't prepend $srctree if *.mod already contains full paths Jessica Yu 2019-10-28 15:14 ` [PATCH 3/4] nsdeps: remove stale .ns_deps files before generating new ones Jessica Yu @ 2019-10-28 15:14 ` Jessica Yu 2019-10-29 12:57 ` Masahiro Yamada 2019-10-29 12:48 ` [PATCH 1/4] scripts/nsdeps: use $MODORDER to obtain correct modules.order path Masahiro Yamada 3 siblings, 1 reply; 14+ messages in thread From: Jessica Yu @ 2019-10-28 15:14 UTC (permalink / raw) To: linux-kernel; +Cc: Matthias Maennich, Masahiro Yamada, Jessica Yu The nsdeps script passes a list of the module source files to generate_deps_for_ns() as a space delimited string named $mod_source_files, which then passes it to spatch. But since $mod_source_files is not encased in quotes, each source file in that string is treated as a separate shell function argument (as $2, $3, $4, etc.). However, the spatch invocation only refers to $2, so only the first file out of $mod_source_files is processed by spatch. This causes problems (namely, the MODULE_IMPORT_NS() statement doesn't get inserted) when a module is composed of many source files and the "main" module file containing the MODULE_LICENSE() statement is not the first file listed in $mod_source_files. Fix this by encasing $mod_source_files in quotes so that the entirety of the string is treated as a single argument and can be referred to as $2. Signed-off-by: Jessica Yu <jeyu@kernel.org> --- scripts/nsdeps | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/nsdeps b/scripts/nsdeps index 9ddcd5cb96b1..5055b059a81b 100644 --- a/scripts/nsdeps +++ b/scripts/nsdeps @@ -36,7 +36,7 @@ generate_deps() { | sed -E "s%(^|\s)([^/][^ ]*)%\1$srctree/\2%g"` for ns in `cat $ns_deps_file`; do echo "Adding namespace $ns to module $mod_name (if needed)." - generate_deps_for_ns $ns $mod_source_files + generate_deps_for_ns $ns "$mod_source_files" # sort the imports for source_file in $mod_source_files; do sed '/MODULE_IMPORT_NS/Q' $source_file > ${source_file}.tmp -- 2.16.4 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 4/4] scripts/nsdeps: make sure to pass all module source files to spatch 2019-10-28 15:14 ` [PATCH 4/4] scripts/nsdeps: make sure to pass all module source files to spatch Jessica Yu @ 2019-10-29 12:57 ` Masahiro Yamada 2019-10-30 16:17 ` Jessica Yu 0 siblings, 1 reply; 14+ messages in thread From: Masahiro Yamada @ 2019-10-29 12:57 UTC (permalink / raw) To: Jessica Yu; +Cc: Linux Kernel Mailing List, Matthias Maennich On Tue, Oct 29, 2019 at 12:14 AM Jessica Yu <jeyu@kernel.org> wrote: > > The nsdeps script passes a list of the module source files to > generate_deps_for_ns() as a space delimited string named $mod_source_files, > which then passes it to spatch. But since $mod_source_files is not encased > in quotes, each source file in that string is treated as a separate shell > function argument (as $2, $3, $4, etc.). However, the spatch invocation > only refers to $2, so only the first file out of $mod_source_files is > processed by spatch. > > This causes problems (namely, the MODULE_IMPORT_NS() statement doesn't > get inserted) when a module is composed of many source files and the > "main" module file containing the MODULE_LICENSE() statement is not the > first file listed in $mod_source_files. Fix this by encasing > $mod_source_files in quotes so that the entirety of the string is > treated as a single argument and can be referred to as $2. > > Signed-off-by: Jessica Yu <jeyu@kernel.org> > --- > scripts/nsdeps | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/scripts/nsdeps b/scripts/nsdeps > index 9ddcd5cb96b1..5055b059a81b 100644 > --- a/scripts/nsdeps > +++ b/scripts/nsdeps > @@ -36,7 +36,7 @@ generate_deps() { > | sed -E "s%(^|\s)([^/][^ ]*)%\1$srctree/\2%g"` > for ns in `cat $ns_deps_file`; do > echo "Adding namespace $ns to module $mod_name (if needed)." > - generate_deps_for_ns $ns $mod_source_files > + generate_deps_for_ns $ns "$mod_source_files" > # sort the imports > for source_file in $mod_source_files; do > sed '/MODULE_IMPORT_NS/Q' $source_file > ${source_file}.tmp I think this change is correct, but did you succeed in nsdeps for composite modules with this patch only? I think the following is needed too: diff --git a/scripts/nsdeps b/scripts/nsdeps index dda6fbac016e..5a23ea616446 100644 --- a/scripts/nsdeps +++ b/scripts/nsdeps @@ -31,9 +31,9 @@ generate_deps() { local mod_file=`echo $@ | sed -e 's/\.ko/\.mod/'` local ns_deps_file=`echo $@ | sed -e 's/\.ko/\.ns_deps/'` if [ ! -f "$ns_deps_file" ]; then return; fi - local mod_source_files=`cat $mod_file | sed -n 1p \ + local mod_source_files="`cat $mod_file | sed -n 1p \ | sed -e 's/\.o/\.c/g' \ - | sed "s|[^ ]* *|${srctree}/&|g"` + | sed "s|[^ ]* *|${srctree}/&|g"`" for ns in `cat $ns_deps_file`; do echo "Adding namespace $ns to module $mod_name (if needed)." generate_deps_for_ns $ns $mod_source_files Without this, a module that consists of two files will be expanded to: local mod_source_files=source1.c source2.c -- Best Regards Masahiro Yamada ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 4/4] scripts/nsdeps: make sure to pass all module source files to spatch 2019-10-29 12:57 ` Masahiro Yamada @ 2019-10-30 16:17 ` Jessica Yu 2019-10-31 12:27 ` Masahiro Yamada 0 siblings, 1 reply; 14+ messages in thread From: Jessica Yu @ 2019-10-30 16:17 UTC (permalink / raw) To: Masahiro Yamada; +Cc: Linux Kernel Mailing List, Matthias Maennich +++ Masahiro Yamada [29/10/19 21:57 +0900]: >On Tue, Oct 29, 2019 at 12:14 AM Jessica Yu <jeyu@kernel.org> wrote: >> >> The nsdeps script passes a list of the module source files to >> generate_deps_for_ns() as a space delimited string named $mod_source_files, >> which then passes it to spatch. But since $mod_source_files is not encased >> in quotes, each source file in that string is treated as a separate shell >> function argument (as $2, $3, $4, etc.). However, the spatch invocation >> only refers to $2, so only the first file out of $mod_source_files is >> processed by spatch. >> >> This causes problems (namely, the MODULE_IMPORT_NS() statement doesn't >> get inserted) when a module is composed of many source files and the >> "main" module file containing the MODULE_LICENSE() statement is not the >> first file listed in $mod_source_files. Fix this by encasing >> $mod_source_files in quotes so that the entirety of the string is >> treated as a single argument and can be referred to as $2. >> >> Signed-off-by: Jessica Yu <jeyu@kernel.org> >> --- >> scripts/nsdeps | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/scripts/nsdeps b/scripts/nsdeps >> index 9ddcd5cb96b1..5055b059a81b 100644 >> --- a/scripts/nsdeps >> +++ b/scripts/nsdeps >> @@ -36,7 +36,7 @@ generate_deps() { >> | sed -E "s%(^|\s)([^/][^ ]*)%\1$srctree/\2%g"` >> for ns in `cat $ns_deps_file`; do >> echo "Adding namespace $ns to module $mod_name (if needed)." >> - generate_deps_for_ns $ns $mod_source_files >> + generate_deps_for_ns $ns "$mod_source_files" >> # sort the imports >> for source_file in $mod_source_files; do >> sed '/MODULE_IMPORT_NS/Q' $source_file > ${source_file}.tmp > >I think this change is correct, but >did you succeed in nsdeps for composite modules >with this patch only? > >I think the following is needed too: > > >diff --git a/scripts/nsdeps b/scripts/nsdeps >index dda6fbac016e..5a23ea616446 100644 >--- a/scripts/nsdeps >+++ b/scripts/nsdeps >@@ -31,9 +31,9 @@ generate_deps() { > local mod_file=`echo $@ | sed -e 's/\.ko/\.mod/'` > local ns_deps_file=`echo $@ | sed -e 's/\.ko/\.ns_deps/'` > if [ ! -f "$ns_deps_file" ]; then return; fi >- local mod_source_files=`cat $mod_file | sed -n 1p \ >+ local mod_source_files="`cat $mod_file | sed -n 1p > \ > | sed -e 's/\.o/\.c/g' \ >- | sed "s|[^ ]* *|${srctree}/&|g"` >+ | sed "s|[^ ]* *|${srctree}/&|g"`" > for ns in `cat $ns_deps_file`; do > echo "Adding namespace $ns to module $mod_name (if needed)." > generate_deps_for_ns $ns $mod_source_files > > >Without this, a module that consists of two files >will be expanded to: > >local mod_source_files=source1.c source2.c Yes, I was able to have nsdeps work for composite modules with just my patch. Without this patch applied, the script produces the following expansion of the generate_deps_for_ns call, (I just added a test namespace MODULE): Adding namespace MODULE to module fs/nfs/nfs.ko. + generate_deps_for_ns MODULE /tmp/ppyu/linux/fs/nfs/client.c /tmp/ppyu/linux/fs/nfs/dir.c /tmp/ppyu/linux/fs/nfs/file.c /tmp/ppyu/linux/fs/nfs/getroot.c /tmp/ppyu/linux/fs/nfs/inode.c /tmp/ppyu/linux/fs/nfs/super.c /tmp/ppyu/linux/fs/nfs/io.c /tmp/ppyu/linux/fs/nfs/direct.c /tmp/ppyu/linux/fs/nfs/pagelist.c /tmp/ppyu/linux/fs/nfs/read.c /tmp/ppyu/linux/fs/nfs/symlink.c /tmp/ppyu/linux/fs/nfs/unlink.c /tmp/ppyu/linux/fs/nfs/write.c /tmp/ppyu/linux/fs/nfs/namespace.c /tmp/ppyu/linux/fs/nfs/mount_clnt.c /tmp/ppyu/linux/fs/nfs/nfstrace.c /tmp/ppyu/linux/fs/nfs/export.c /tmp/ppyu/linux/fs/nfs/sysfs.c /tmp/ppyu/linux/fs/nfs/sysctl.c /tmp/ppyu/linux/fs/nfs/fscache.c /tmp/ppyu/linux/fs/nfs/fscache-index.c + /usr/bin/spatch --very-quiet --in-place --sp-file /tmp/ppyu/linux/scripts/coccinelle/misc/add_namespace.cocci -D ns=MODULE /tmp/ppyu/linux/fs/nfs/client.c So only the first file got included in the spatch invocation. But the spatch call gets fixed with all the files when quotes are added in the call to generate_deps_for_ns. But we need to include your change anyway, to make the script more robust. It would probably prevent more shell script related bugs in the future (Like [1]). I can respin this patch only while the other ones are superceded by your patchset. [1] https://unix.stackexchange.com/a/131767 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 4/4] scripts/nsdeps: make sure to pass all module source files to spatch 2019-10-30 16:17 ` Jessica Yu @ 2019-10-31 12:27 ` Masahiro Yamada 2019-10-31 13:41 ` Jessica Yu 0 siblings, 1 reply; 14+ messages in thread From: Masahiro Yamada @ 2019-10-31 12:27 UTC (permalink / raw) To: Jessica Yu; +Cc: Linux Kernel Mailing List, Matthias Maennich On Thu, Oct 31, 2019 at 1:17 AM Jessica Yu <jeyu@kernel.org> wrote: > > +++ Masahiro Yamada [29/10/19 21:57 +0900]: > >On Tue, Oct 29, 2019 at 12:14 AM Jessica Yu <jeyu@kernel.org> wrote: > >> > >> The nsdeps script passes a list of the module source files to > >> generate_deps_for_ns() as a space delimited string named $mod_source_files, > >> which then passes it to spatch. But since $mod_source_files is not encased > >> in quotes, each source file in that string is treated as a separate shell > >> function argument (as $2, $3, $4, etc.). However, the spatch invocation > >> only refers to $2, so only the first file out of $mod_source_files is > >> processed by spatch. > >> > >> This causes problems (namely, the MODULE_IMPORT_NS() statement doesn't > >> get inserted) when a module is composed of many source files and the > >> "main" module file containing the MODULE_LICENSE() statement is not the > >> first file listed in $mod_source_files. Fix this by encasing > >> $mod_source_files in quotes so that the entirety of the string is > >> treated as a single argument and can be referred to as $2. > >> > >> Signed-off-by: Jessica Yu <jeyu@kernel.org> > >> --- > >> scripts/nsdeps | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/scripts/nsdeps b/scripts/nsdeps > >> index 9ddcd5cb96b1..5055b059a81b 100644 > >> --- a/scripts/nsdeps > >> +++ b/scripts/nsdeps > >> @@ -36,7 +36,7 @@ generate_deps() { > >> | sed -E "s%(^|\s)([^/][^ ]*)%\1$srctree/\2%g"` > >> for ns in `cat $ns_deps_file`; do > >> echo "Adding namespace $ns to module $mod_name (if needed)." > >> - generate_deps_for_ns $ns $mod_source_files > >> + generate_deps_for_ns $ns "$mod_source_files" > >> # sort the imports > >> for source_file in $mod_source_files; do > >> sed '/MODULE_IMPORT_NS/Q' $source_file > ${source_file}.tmp > > > >I think this change is correct, but > >did you succeed in nsdeps for composite modules > >with this patch only? > > > >I think the following is needed too: > > > > > >diff --git a/scripts/nsdeps b/scripts/nsdeps > >index dda6fbac016e..5a23ea616446 100644 > >--- a/scripts/nsdeps > >+++ b/scripts/nsdeps > >@@ -31,9 +31,9 @@ generate_deps() { > > local mod_file=`echo $@ | sed -e 's/\.ko/\.mod/'` > > local ns_deps_file=`echo $@ | sed -e 's/\.ko/\.ns_deps/'` > > if [ ! -f "$ns_deps_file" ]; then return; fi > >- local mod_source_files=`cat $mod_file | sed -n 1p \ > >+ local mod_source_files="`cat $mod_file | sed -n 1p > > \ > > | sed -e 's/\.o/\.c/g' \ > >- | sed "s|[^ ]* *|${srctree}/&|g"` > >+ | sed "s|[^ ]* *|${srctree}/&|g"`" > > for ns in `cat $ns_deps_file`; do > > echo "Adding namespace $ns to module $mod_name (if needed)." > > generate_deps_for_ns $ns $mod_source_files > > > > > >Without this, a module that consists of two files > >will be expanded to: > > > >local mod_source_files=source1.c source2.c > > Yes, I was able to have nsdeps work for composite modules with just my > patch. Without this patch applied, the script produces the following > expansion of the generate_deps_for_ns call, (I just added a test > namespace MODULE): > > Adding namespace MODULE to module fs/nfs/nfs.ko. > + generate_deps_for_ns MODULE /tmp/ppyu/linux/fs/nfs/client.c /tmp/ppyu/linux/fs/nfs/dir.c /tmp/ppyu/linux/fs/nfs/file.c /tmp/ppyu/linux/fs/nfs/getroot.c /tmp/ppyu/linux/fs/nfs/inode.c /tmp/ppyu/linux/fs/nfs/super.c /tmp/ppyu/linux/fs/nfs/io.c /tmp/ppyu/linux/fs/nfs/direct.c /tmp/ppyu/linux/fs/nfs/pagelist.c /tmp/ppyu/linux/fs/nfs/read.c /tmp/ppyu/linux/fs/nfs/symlink.c /tmp/ppyu/linux/fs/nfs/unlink.c /tmp/ppyu/linux/fs/nfs/write.c /tmp/ppyu/linux/fs/nfs/namespace.c /tmp/ppyu/linux/fs/nfs/mount_clnt.c /tmp/ppyu/linux/fs/nfs/nfstrace.c /tmp/ppyu/linux/fs/nfs/export.c /tmp/ppyu/linux/fs/nfs/sysfs.c /tmp/ppyu/linux/fs/nfs/sysctl.c /tmp/ppyu/linux/fs/nfs/fscache.c /tmp/ppyu/linux/fs/nfs/fscache-index.c > + /usr/bin/spatch --very-quiet --in-place --sp-file /tmp/ppyu/linux/scripts/coccinelle/misc/add_namespace.cocci -D ns=MODULE /tmp/ppyu/linux/fs/nfs/client.c > > So only the first file got included in the spatch invocation. But the > spatch call gets fixed with all the files when quotes are added in the > call to generate_deps_for_ns. > > But we need to include your change anyway, to make the script more > robust. Hmm. With this patch only, I see "bad variable name" error. masahiro@grover:~/ref/linux$ make -j8 nsdeps DESCEND objtool CALL scripts/atomic/check-atomics.sh CALL scripts/checksyscalls.sh CHK include/generated/compile.h Building modules, stage 2. MODPOST 20 modules WARNING: module nfs uses symbol foo from namespace USB_STORAGE, but does not import it. Building modules, stage 2. MODPOST 20 modules ./scripts/nsdeps: 34: local: ./fs/nfs/dir.c: bad variable name make: *** [Makefile;1689: nsdeps] Error 2 > It would probably prevent more shell script related bugs in > the future (Like [1]). I can respin this patch only while the other > ones are superceded by your patchset. > > [1] https://unix.stackexchange.com/a/131767 Anyway. Is this patch aiming for v5.4 (i.e. fixes) or v5.5-rc1 ? If you touch the mod_source_files line, we will have a conflict because https://patchwork.kernel.org/patch/11217839/ is touching the same line. How should we organize the patch order? -- Best Regards Masahiro Yamada ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 4/4] scripts/nsdeps: make sure to pass all module source files to spatch 2019-10-31 12:27 ` Masahiro Yamada @ 2019-10-31 13:41 ` Jessica Yu 2019-11-01 5:56 ` Masahiro Yamada 0 siblings, 1 reply; 14+ messages in thread From: Jessica Yu @ 2019-10-31 13:41 UTC (permalink / raw) To: Masahiro Yamada; +Cc: Linux Kernel Mailing List, Matthias Maennich +++ Masahiro Yamada [31/10/19 21:27 +0900]: >On Thu, Oct 31, 2019 at 1:17 AM Jessica Yu <jeyu@kernel.org> wrote: >> >> +++ Masahiro Yamada [29/10/19 21:57 +0900]: >> >On Tue, Oct 29, 2019 at 12:14 AM Jessica Yu <jeyu@kernel.org> wrote: >> >> >> >> The nsdeps script passes a list of the module source files to >> >> generate_deps_for_ns() as a space delimited string named $mod_source_files, >> >> which then passes it to spatch. But since $mod_source_files is not encased >> >> in quotes, each source file in that string is treated as a separate shell >> >> function argument (as $2, $3, $4, etc.). However, the spatch invocation >> >> only refers to $2, so only the first file out of $mod_source_files is >> >> processed by spatch. >> >> >> >> This causes problems (namely, the MODULE_IMPORT_NS() statement doesn't >> >> get inserted) when a module is composed of many source files and the >> >> "main" module file containing the MODULE_LICENSE() statement is not the >> >> first file listed in $mod_source_files. Fix this by encasing >> >> $mod_source_files in quotes so that the entirety of the string is >> >> treated as a single argument and can be referred to as $2. >> >> >> >> Signed-off-by: Jessica Yu <jeyu@kernel.org> >> >> --- >> >> scripts/nsdeps | 2 +- >> >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> >> >> diff --git a/scripts/nsdeps b/scripts/nsdeps >> >> index 9ddcd5cb96b1..5055b059a81b 100644 >> >> --- a/scripts/nsdeps >> >> +++ b/scripts/nsdeps >> >> @@ -36,7 +36,7 @@ generate_deps() { >> >> | sed -E "s%(^|\s)([^/][^ ]*)%\1$srctree/\2%g"` >> >> for ns in `cat $ns_deps_file`; do >> >> echo "Adding namespace $ns to module $mod_name (if needed)." >> >> - generate_deps_for_ns $ns $mod_source_files >> >> + generate_deps_for_ns $ns "$mod_source_files" >> >> # sort the imports >> >> for source_file in $mod_source_files; do >> >> sed '/MODULE_IMPORT_NS/Q' $source_file > ${source_file}.tmp >> > >> >I think this change is correct, but >> >did you succeed in nsdeps for composite modules >> >with this patch only? >> > >> >I think the following is needed too: >> > >> > >> >diff --git a/scripts/nsdeps b/scripts/nsdeps >> >index dda6fbac016e..5a23ea616446 100644 >> >--- a/scripts/nsdeps >> >+++ b/scripts/nsdeps >> >@@ -31,9 +31,9 @@ generate_deps() { >> > local mod_file=`echo $@ | sed -e 's/\.ko/\.mod/'` >> > local ns_deps_file=`echo $@ | sed -e 's/\.ko/\.ns_deps/'` >> > if [ ! -f "$ns_deps_file" ]; then return; fi >> >- local mod_source_files=`cat $mod_file | sed -n 1p \ >> >+ local mod_source_files="`cat $mod_file | sed -n 1p >> > \ >> > | sed -e 's/\.o/\.c/g' \ >> >- | sed "s|[^ ]* *|${srctree}/&|g"` >> >+ | sed "s|[^ ]* *|${srctree}/&|g"`" >> > for ns in `cat $ns_deps_file`; do >> > echo "Adding namespace $ns to module $mod_name (if needed)." >> > generate_deps_for_ns $ns $mod_source_files >> > >> > >> >Without this, a module that consists of two files >> >will be expanded to: >> > >> >local mod_source_files=source1.c source2.c >> >> Yes, I was able to have nsdeps work for composite modules with just my >> patch. Without this patch applied, the script produces the following >> expansion of the generate_deps_for_ns call, (I just added a test >> namespace MODULE): >> >> Adding namespace MODULE to module fs/nfs/nfs.ko. >> + generate_deps_for_ns MODULE /tmp/ppyu/linux/fs/nfs/client.c /tmp/ppyu/linux/fs/nfs/dir.c /tmp/ppyu/linux/fs/nfs/file.c /tmp/ppyu/linux/fs/nfs/getroot.c /tmp/ppyu/linux/fs/nfs/inode.c /tmp/ppyu/linux/fs/nfs/super.c /tmp/ppyu/linux/fs/nfs/io.c /tmp/ppyu/linux/fs/nfs/direct.c /tmp/ppyu/linux/fs/nfs/pagelist.c /tmp/ppyu/linux/fs/nfs/read.c /tmp/ppyu/linux/fs/nfs/symlink.c /tmp/ppyu/linux/fs/nfs/unlink.c /tmp/ppyu/linux/fs/nfs/write.c /tmp/ppyu/linux/fs/nfs/namespace.c /tmp/ppyu/linux/fs/nfs/mount_clnt.c /tmp/ppyu/linux/fs/nfs/nfstrace.c /tmp/ppyu/linux/fs/nfs/export.c /tmp/ppyu/linux/fs/nfs/sysfs.c /tmp/ppyu/linux/fs/nfs/sysctl.c /tmp/ppyu/linux/fs/nfs/fscache.c /tmp/ppyu/linux/fs/nfs/fscache-index.c >> + /usr/bin/spatch --very-quiet --in-place --sp-file /tmp/ppyu/linux/scripts/coccinelle/misc/add_namespace.cocci -D ns=MODULE /tmp/ppyu/linux/fs/nfs/client.c >> >> So only the first file got included in the spatch invocation. But the >> spatch call gets fixed with all the files when quotes are added in the >> call to generate_deps_for_ns. >> >> But we need to include your change anyway, to make the script more >> robust. > >Hmm. >With this patch only, I see "bad variable name" error. > > > >masahiro@grover:~/ref/linux$ make -j8 nsdeps > DESCEND objtool > CALL scripts/atomic/check-atomics.sh > CALL scripts/checksyscalls.sh > CHK include/generated/compile.h > Building modules, stage 2. > MODPOST 20 modules >WARNING: module nfs uses symbol foo from namespace USB_STORAGE, but >does not import it. > Building modules, stage 2. > MODPOST 20 modules >./scripts/nsdeps: 34: local: ./fs/nfs/dir.c: bad variable name >make: *** [Makefile;1689: nsdeps] Error 2 Hm, I was having trouble reproducing this until I changed the shell to dash, /bin/sh is a symlink to bash on my system, that might explain slightly different behavior. In any case, we should add quotes in both places. >> It would probably prevent more shell script related bugs in >> the future (Like [1]). I can respin this patch only while the other >> ones are superceded by your patchset. >> >> [1] https://unix.stackexchange.com/a/131767 > >Anyway. > >Is this patch aiming for v5.4 (i.e. fixes) or v5.5-rc1 ? I am hoping for fixes, we should try to get all the small bugs out of nsdeps by 5.4 if we can.. >If you touch the mod_source_files line, >we will have a conflict because >https://patchwork.kernel.org/patch/11217839/ >is touching the same line. > >How should we organize the patch order? Would you like to fold these changes into your nsdeps improvements patchset? Since it's a pretty trivial change. Thanks! ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 4/4] scripts/nsdeps: make sure to pass all module source files to spatch 2019-10-31 13:41 ` Jessica Yu @ 2019-11-01 5:56 ` Masahiro Yamada 2019-11-05 12:52 ` Jessica Yu 0 siblings, 1 reply; 14+ messages in thread From: Masahiro Yamada @ 2019-11-01 5:56 UTC (permalink / raw) To: Jessica Yu; +Cc: Linux Kernel Mailing List, Matthias Maennich On Thu, Oct 31, 2019 at 10:41 PM Jessica Yu <jeyu@kernel.org> wrote: > > +++ Masahiro Yamada [31/10/19 21:27 +0900]: > >On Thu, Oct 31, 2019 at 1:17 AM Jessica Yu <jeyu@kernel.org> wrote: > >> > >> +++ Masahiro Yamada [29/10/19 21:57 +0900]: > >> >On Tue, Oct 29, 2019 at 12:14 AM Jessica Yu <jeyu@kernel.org> wrote: > >> >> > >> >> The nsdeps script passes a list of the module source files to > >> >> generate_deps_for_ns() as a space delimited string named $mod_source_files, > >> >> which then passes it to spatch. But since $mod_source_files is not encased > >> >> in quotes, each source file in that string is treated as a separate shell > >> >> function argument (as $2, $3, $4, etc.). However, the spatch invocation > >> >> only refers to $2, so only the first file out of $mod_source_files is > >> >> processed by spatch. > >> >> > >> >> This causes problems (namely, the MODULE_IMPORT_NS() statement doesn't > >> >> get inserted) when a module is composed of many source files and the > >> >> "main" module file containing the MODULE_LICENSE() statement is not the > >> >> first file listed in $mod_source_files. Fix this by encasing > >> >> $mod_source_files in quotes so that the entirety of the string is > >> >> treated as a single argument and can be referred to as $2. > >> >> > >> >> Signed-off-by: Jessica Yu <jeyu@kernel.org> > >> >> --- > >> >> scripts/nsdeps | 2 +- > >> >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> >> > >> >> diff --git a/scripts/nsdeps b/scripts/nsdeps > >> >> index 9ddcd5cb96b1..5055b059a81b 100644 > >> >> --- a/scripts/nsdeps > >> >> +++ b/scripts/nsdeps > >> >> @@ -36,7 +36,7 @@ generate_deps() { > >> >> | sed -E "s%(^|\s)([^/][^ ]*)%\1$srctree/\2%g"` > >> >> for ns in `cat $ns_deps_file`; do > >> >> echo "Adding namespace $ns to module $mod_name (if needed)." > >> >> - generate_deps_for_ns $ns $mod_source_files > >> >> + generate_deps_for_ns $ns "$mod_source_files" > >> >> # sort the imports > >> >> for source_file in $mod_source_files; do > >> >> sed '/MODULE_IMPORT_NS/Q' $source_file > ${source_file}.tmp > >> > > >> >I think this change is correct, but > >> >did you succeed in nsdeps for composite modules > >> >with this patch only? > >> > > >> >I think the following is needed too: > >> > > >> > > >> >diff --git a/scripts/nsdeps b/scripts/nsdeps > >> >index dda6fbac016e..5a23ea616446 100644 > >> >--- a/scripts/nsdeps > >> >+++ b/scripts/nsdeps > >> >@@ -31,9 +31,9 @@ generate_deps() { > >> > local mod_file=`echo $@ | sed -e 's/\.ko/\.mod/'` > >> > local ns_deps_file=`echo $@ | sed -e 's/\.ko/\.ns_deps/'` > >> > if [ ! -f "$ns_deps_file" ]; then return; fi > >> >- local mod_source_files=`cat $mod_file | sed -n 1p \ > >> >+ local mod_source_files="`cat $mod_file | sed -n 1p > >> > \ > >> > | sed -e 's/\.o/\.c/g' \ > >> >- | sed "s|[^ ]* *|${srctree}/&|g"` > >> >+ | sed "s|[^ ]* *|${srctree}/&|g"`" > >> > for ns in `cat $ns_deps_file`; do > >> > echo "Adding namespace $ns to module $mod_name (if needed)." > >> > generate_deps_for_ns $ns $mod_source_files > >> > > >> > > >> >Without this, a module that consists of two files > >> >will be expanded to: > >> > > >> >local mod_source_files=source1.c source2.c > >> > >> Yes, I was able to have nsdeps work for composite modules with just my > >> patch. Without this patch applied, the script produces the following > >> expansion of the generate_deps_for_ns call, (I just added a test > >> namespace MODULE): > >> > >> Adding namespace MODULE to module fs/nfs/nfs.ko. > >> + generate_deps_for_ns MODULE /tmp/ppyu/linux/fs/nfs/client.c /tmp/ppyu/linux/fs/nfs/dir.c /tmp/ppyu/linux/fs/nfs/file.c /tmp/ppyu/linux/fs/nfs/getroot.c /tmp/ppyu/linux/fs/nfs/inode.c /tmp/ppyu/linux/fs/nfs/super.c /tmp/ppyu/linux/fs/nfs/io.c /tmp/ppyu/linux/fs/nfs/direct.c /tmp/ppyu/linux/fs/nfs/pagelist.c /tmp/ppyu/linux/fs/nfs/read.c /tmp/ppyu/linux/fs/nfs/symlink.c /tmp/ppyu/linux/fs/nfs/unlink.c /tmp/ppyu/linux/fs/nfs/write.c /tmp/ppyu/linux/fs/nfs/namespace.c /tmp/ppyu/linux/fs/nfs/mount_clnt.c /tmp/ppyu/linux/fs/nfs/nfstrace.c /tmp/ppyu/linux/fs/nfs/export.c /tmp/ppyu/linux/fs/nfs/sysfs.c /tmp/ppyu/linux/fs/nfs/sysctl.c /tmp/ppyu/linux/fs/nfs/fscache.c /tmp/ppyu/linux/fs/nfs/fscache-index.c > >> + /usr/bin/spatch --very-quiet --in-place --sp-file /tmp/ppyu/linux/scripts/coccinelle/misc/add_namespace.cocci -D ns=MODULE /tmp/ppyu/linux/fs/nfs/client.c > >> > >> So only the first file got included in the spatch invocation. But the > >> spatch call gets fixed with all the files when quotes are added in the > >> call to generate_deps_for_ns. > >> > >> But we need to include your change anyway, to make the script more > >> robust. > > > >Hmm. > >With this patch only, I see "bad variable name" error. > > > > > > > >masahiro@grover:~/ref/linux$ make -j8 nsdeps > > DESCEND objtool > > CALL scripts/atomic/check-atomics.sh > > CALL scripts/checksyscalls.sh > > CHK include/generated/compile.h > > Building modules, stage 2. > > MODPOST 20 modules > >WARNING: module nfs uses symbol foo from namespace USB_STORAGE, but > >does not import it. > > Building modules, stage 2. > > MODPOST 20 modules > >./scripts/nsdeps: 34: local: ./fs/nfs/dir.c: bad variable name > >make: *** [Makefile;1689: nsdeps] Error 2 > > Hm, I was having trouble reproducing this until I changed the shell to > dash, /bin/sh is a symlink to bash on my system, that might explain > slightly different behavior. In any case, we should add quotes in both > places. > > >> It would probably prevent more shell script related bugs in > >> the future (Like [1]). I can respin this patch only while the other > >> ones are superceded by your patchset. > >> > >> [1] https://unix.stackexchange.com/a/131767 > > > >Anyway. > > > >Is this patch aiming for v5.4 (i.e. fixes) or v5.5-rc1 ? > > I am hoping for fixes, we should try to get all the small bugs out of > nsdeps by 5.4 if we can.. > > >If you touch the mod_source_files line, > >we will have a conflict because > >https://patchwork.kernel.org/patch/11217839/ > >is touching the same line. > > > >How should we organize the patch order? > > Would you like to fold these changes into your nsdeps improvements > patchset? Since it's a pretty trivial change. > > Thanks! > > My change set is quite big, so I am planning to send it for v5.5-rc1. If you want to fix nsdeps for composite modules earlier, please apply this patch to your tree and send another pull request. -- Best Regards Masahiro Yamada ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 4/4] scripts/nsdeps: make sure to pass all module source files to spatch 2019-11-01 5:56 ` Masahiro Yamada @ 2019-11-05 12:52 ` Jessica Yu 2019-11-05 12:57 ` Masahiro Yamada 0 siblings, 1 reply; 14+ messages in thread From: Jessica Yu @ 2019-11-05 12:52 UTC (permalink / raw) To: Masahiro Yamada; +Cc: Linux Kernel Mailing List, Matthias Maennich +++ Masahiro Yamada [01/11/19 14:56 +0900]: >On Thu, Oct 31, 2019 at 10:41 PM Jessica Yu <jeyu@kernel.org> wrote: >> >> +++ Masahiro Yamada [31/10/19 21:27 +0900]: >> >On Thu, Oct 31, 2019 at 1:17 AM Jessica Yu <jeyu@kernel.org> wrote: >> >> >> >> +++ Masahiro Yamada [29/10/19 21:57 +0900]: >> >> >On Tue, Oct 29, 2019 at 12:14 AM Jessica Yu <jeyu@kernel.org> wrote: >> >> >> >> >> >> The nsdeps script passes a list of the module source files to >> >> >> generate_deps_for_ns() as a space delimited string named $mod_source_files, >> >> >> which then passes it to spatch. But since $mod_source_files is not encased >> >> >> in quotes, each source file in that string is treated as a separate shell >> >> >> function argument (as $2, $3, $4, etc.). However, the spatch invocation >> >> >> only refers to $2, so only the first file out of $mod_source_files is >> >> >> processed by spatch. >> >> >> >> >> >> This causes problems (namely, the MODULE_IMPORT_NS() statement doesn't >> >> >> get inserted) when a module is composed of many source files and the >> >> >> "main" module file containing the MODULE_LICENSE() statement is not the >> >> >> first file listed in $mod_source_files. Fix this by encasing >> >> >> $mod_source_files in quotes so that the entirety of the string is >> >> >> treated as a single argument and can be referred to as $2. >> >> >> >> >> >> Signed-off-by: Jessica Yu <jeyu@kernel.org> >> >> >> --- >> >> >> scripts/nsdeps | 2 +- >> >> >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> >> >> >> >> diff --git a/scripts/nsdeps b/scripts/nsdeps >> >> >> index 9ddcd5cb96b1..5055b059a81b 100644 >> >> >> --- a/scripts/nsdeps >> >> >> +++ b/scripts/nsdeps >> >> >> @@ -36,7 +36,7 @@ generate_deps() { >> >> >> | sed -E "s%(^|\s)([^/][^ ]*)%\1$srctree/\2%g"` >> >> >> for ns in `cat $ns_deps_file`; do >> >> >> echo "Adding namespace $ns to module $mod_name (if needed)." >> >> >> - generate_deps_for_ns $ns $mod_source_files >> >> >> + generate_deps_for_ns $ns "$mod_source_files" >> >> >> # sort the imports >> >> >> for source_file in $mod_source_files; do >> >> >> sed '/MODULE_IMPORT_NS/Q' $source_file > ${source_file}.tmp >> >> > >> >> >I think this change is correct, but >> >> >did you succeed in nsdeps for composite modules >> >> >with this patch only? >> >> > >> >> >I think the following is needed too: >> >> > >> >> > >> >> >diff --git a/scripts/nsdeps b/scripts/nsdeps >> >> >index dda6fbac016e..5a23ea616446 100644 >> >> >--- a/scripts/nsdeps >> >> >+++ b/scripts/nsdeps >> >> >@@ -31,9 +31,9 @@ generate_deps() { >> >> > local mod_file=`echo $@ | sed -e 's/\.ko/\.mod/'` >> >> > local ns_deps_file=`echo $@ | sed -e 's/\.ko/\.ns_deps/'` >> >> > if [ ! -f "$ns_deps_file" ]; then return; fi >> >> >- local mod_source_files=`cat $mod_file | sed -n 1p \ >> >> >+ local mod_source_files="`cat $mod_file | sed -n 1p >> >> > \ >> >> > | sed -e 's/\.o/\.c/g' \ >> >> >- | sed "s|[^ ]* *|${srctree}/&|g"` >> >> >+ | sed "s|[^ ]* *|${srctree}/&|g"`" >> >> > for ns in `cat $ns_deps_file`; do >> >> > echo "Adding namespace $ns to module $mod_name (if needed)." >> >> > generate_deps_for_ns $ns $mod_source_files >> >> > >> >> > >> >> >Without this, a module that consists of two files >> >> >will be expanded to: >> >> > >> >> >local mod_source_files=source1.c source2.c >> >> >> >> Yes, I was able to have nsdeps work for composite modules with just my >> >> patch. Without this patch applied, the script produces the following >> >> expansion of the generate_deps_for_ns call, (I just added a test >> >> namespace MODULE): >> >> >> >> Adding namespace MODULE to module fs/nfs/nfs.ko. >> >> + generate_deps_for_ns MODULE /tmp/ppyu/linux/fs/nfs/client.c /tmp/ppyu/linux/fs/nfs/dir.c /tmp/ppyu/linux/fs/nfs/file.c /tmp/ppyu/linux/fs/nfs/getroot.c /tmp/ppyu/linux/fs/nfs/inode.c /tmp/ppyu/linux/fs/nfs/super.c /tmp/ppyu/linux/fs/nfs/io.c /tmp/ppyu/linux/fs/nfs/direct.c /tmp/ppyu/linux/fs/nfs/pagelist.c /tmp/ppyu/linux/fs/nfs/read.c /tmp/ppyu/linux/fs/nfs/symlink.c /tmp/ppyu/linux/fs/nfs/unlink.c /tmp/ppyu/linux/fs/nfs/write.c /tmp/ppyu/linux/fs/nfs/namespace.c /tmp/ppyu/linux/fs/nfs/mount_clnt.c /tmp/ppyu/linux/fs/nfs/nfstrace.c /tmp/ppyu/linux/fs/nfs/export.c /tmp/ppyu/linux/fs/nfs/sysfs.c /tmp/ppyu/linux/fs/nfs/sysctl.c /tmp/ppyu/linux/fs/nfs/fscache.c /tmp/ppyu/linux/fs/nfs/fscache-index.c >> >> + /usr/bin/spatch --very-quiet --in-place --sp-file /tmp/ppyu/linux/scripts/coccinelle/misc/add_namespace.cocci -D ns=MODULE /tmp/ppyu/linux/fs/nfs/client.c >> >> >> >> So only the first file got included in the spatch invocation. But the >> >> spatch call gets fixed with all the files when quotes are added in the >> >> call to generate_deps_for_ns. >> >> >> >> But we need to include your change anyway, to make the script more >> >> robust. >> > >> >Hmm. >> >With this patch only, I see "bad variable name" error. >> > >> > >> > >> >masahiro@grover:~/ref/linux$ make -j8 nsdeps >> > DESCEND objtool >> > CALL scripts/atomic/check-atomics.sh >> > CALL scripts/checksyscalls.sh >> > CHK include/generated/compile.h >> > Building modules, stage 2. >> > MODPOST 20 modules >> >WARNING: module nfs uses symbol foo from namespace USB_STORAGE, but >> >does not import it. >> > Building modules, stage 2. >> > MODPOST 20 modules >> >./scripts/nsdeps: 34: local: ./fs/nfs/dir.c: bad variable name >> >make: *** [Makefile;1689: nsdeps] Error 2 >> >> Hm, I was having trouble reproducing this until I changed the shell to >> dash, /bin/sh is a symlink to bash on my system, that might explain >> slightly different behavior. In any case, we should add quotes in both >> places. >> >> >> It would probably prevent more shell script related bugs in >> >> the future (Like [1]). I can respin this patch only while the other >> >> ones are superceded by your patchset. >> >> >> >> [1] https://unix.stackexchange.com/a/131767 >> > >> >Anyway. >> > >> >Is this patch aiming for v5.4 (i.e. fixes) or v5.5-rc1 ? >> >> I am hoping for fixes, we should try to get all the small bugs out of >> nsdeps by 5.4 if we can.. >> >> >If you touch the mod_source_files line, >> >we will have a conflict because >> >https://patchwork.kernel.org/patch/11217839/ >> >is touching the same line. >> > >> >How should we organize the patch order? >> >> Would you like to fold these changes into your nsdeps improvements >> patchset? Since it's a pretty trivial change. >> >> Thanks! >> >> > >My change set is quite big, so I >am planning to send it for v5.5-rc1. OK, sounds good. >If you want to fix nsdeps for composite modules earlier, >please apply this patch to your tree >and send another pull request. I plan to send this in for -rc7, since it's a minor fix. For the conflict, if these patches are not pushed anywhere yet (at least I did not see them on the kbuild for-next branch yet), perhaps you could rebase on top of -rc7 and put your patchset on top? Thanks, Jessica ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 4/4] scripts/nsdeps: make sure to pass all module source files to spatch 2019-11-05 12:52 ` Jessica Yu @ 2019-11-05 12:57 ` Masahiro Yamada 0 siblings, 0 replies; 14+ messages in thread From: Masahiro Yamada @ 2019-11-05 12:57 UTC (permalink / raw) To: Jessica Yu; +Cc: Linux Kernel Mailing List, Matthias Maennich On Tue, Nov 5, 2019 at 9:52 PM Jessica Yu <jeyu@kernel.org> wrote: > > +++ Masahiro Yamada [01/11/19 14:56 +0900]: > >On Thu, Oct 31, 2019 at 10:41 PM Jessica Yu <jeyu@kernel.org> wrote: > >> > >> +++ Masahiro Yamada [31/10/19 21:27 +0900]: > >> >On Thu, Oct 31, 2019 at 1:17 AM Jessica Yu <jeyu@kernel.org> wrote: > >> >> > >> >> +++ Masahiro Yamada [29/10/19 21:57 +0900]: > >> >> >On Tue, Oct 29, 2019 at 12:14 AM Jessica Yu <jeyu@kernel.org> wrote: > >> >> >> > >> >> >> The nsdeps script passes a list of the module source files to > >> >> >> generate_deps_for_ns() as a space delimited string named $mod_source_files, > >> >> >> which then passes it to spatch. But since $mod_source_files is not encased > >> >> >> in quotes, each source file in that string is treated as a separate shell > >> >> >> function argument (as $2, $3, $4, etc.). However, the spatch invocation > >> >> >> only refers to $2, so only the first file out of $mod_source_files is > >> >> >> processed by spatch. > >> >> >> > >> >> >> This causes problems (namely, the MODULE_IMPORT_NS() statement doesn't > >> >> >> get inserted) when a module is composed of many source files and the > >> >> >> "main" module file containing the MODULE_LICENSE() statement is not the > >> >> >> first file listed in $mod_source_files. Fix this by encasing > >> >> >> $mod_source_files in quotes so that the entirety of the string is > >> >> >> treated as a single argument and can be referred to as $2. > >> >> >> > >> >> >> Signed-off-by: Jessica Yu <jeyu@kernel.org> > >> >> >> --- > >> >> >> scripts/nsdeps | 2 +- > >> >> >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> >> >> > >> >> >> diff --git a/scripts/nsdeps b/scripts/nsdeps > >> >> >> index 9ddcd5cb96b1..5055b059a81b 100644 > >> >> >> --- a/scripts/nsdeps > >> >> >> +++ b/scripts/nsdeps > >> >> >> @@ -36,7 +36,7 @@ generate_deps() { > >> >> >> | sed -E "s%(^|\s)([^/][^ ]*)%\1$srctree/\2%g"` > >> >> >> for ns in `cat $ns_deps_file`; do > >> >> >> echo "Adding namespace $ns to module $mod_name (if needed)." > >> >> >> - generate_deps_for_ns $ns $mod_source_files > >> >> >> + generate_deps_for_ns $ns "$mod_source_files" > >> >> >> # sort the imports > >> >> >> for source_file in $mod_source_files; do > >> >> >> sed '/MODULE_IMPORT_NS/Q' $source_file > ${source_file}.tmp > >> >> > > >> >> >I think this change is correct, but > >> >> >did you succeed in nsdeps for composite modules > >> >> >with this patch only? > >> >> > > >> >> >I think the following is needed too: > >> >> > > >> >> > > >> >> >diff --git a/scripts/nsdeps b/scripts/nsdeps > >> >> >index dda6fbac016e..5a23ea616446 100644 > >> >> >--- a/scripts/nsdeps > >> >> >+++ b/scripts/nsdeps > >> >> >@@ -31,9 +31,9 @@ generate_deps() { > >> >> > local mod_file=`echo $@ | sed -e 's/\.ko/\.mod/'` > >> >> > local ns_deps_file=`echo $@ | sed -e 's/\.ko/\.ns_deps/'` > >> >> > if [ ! -f "$ns_deps_file" ]; then return; fi > >> >> >- local mod_source_files=`cat $mod_file | sed -n 1p \ > >> >> >+ local mod_source_files="`cat $mod_file | sed -n 1p > >> >> > \ > >> >> > | sed -e 's/\.o/\.c/g' \ > >> >> >- | sed "s|[^ ]* *|${srctree}/&|g"` > >> >> >+ | sed "s|[^ ]* *|${srctree}/&|g"`" > >> >> > for ns in `cat $ns_deps_file`; do > >> >> > echo "Adding namespace $ns to module $mod_name (if needed)." > >> >> > generate_deps_for_ns $ns $mod_source_files > >> >> > > >> >> > > >> >> >Without this, a module that consists of two files > >> >> >will be expanded to: > >> >> > > >> >> >local mod_source_files=source1.c source2.c > >> >> > >> >> Yes, I was able to have nsdeps work for composite modules with just my > >> >> patch. Without this patch applied, the script produces the following > >> >> expansion of the generate_deps_for_ns call, (I just added a test > >> >> namespace MODULE): > >> >> > >> >> Adding namespace MODULE to module fs/nfs/nfs.ko. > >> >> + generate_deps_for_ns MODULE /tmp/ppyu/linux/fs/nfs/client.c /tmp/ppyu/linux/fs/nfs/dir.c /tmp/ppyu/linux/fs/nfs/file.c /tmp/ppyu/linux/fs/nfs/getroot.c /tmp/ppyu/linux/fs/nfs/inode.c /tmp/ppyu/linux/fs/nfs/super.c /tmp/ppyu/linux/fs/nfs/io.c /tmp/ppyu/linux/fs/nfs/direct.c /tmp/ppyu/linux/fs/nfs/pagelist.c /tmp/ppyu/linux/fs/nfs/read.c /tmp/ppyu/linux/fs/nfs/symlink.c /tmp/ppyu/linux/fs/nfs/unlink.c /tmp/ppyu/linux/fs/nfs/write.c /tmp/ppyu/linux/fs/nfs/namespace.c /tmp/ppyu/linux/fs/nfs/mount_clnt.c /tmp/ppyu/linux/fs/nfs/nfstrace.c /tmp/ppyu/linux/fs/nfs/export.c /tmp/ppyu/linux/fs/nfs/sysfs.c /tmp/ppyu/linux/fs/nfs/sysctl.c /tmp/ppyu/linux/fs/nfs/fscache.c /tmp/ppyu/linux/fs/nfs/fscache-index.c > >> >> + /usr/bin/spatch --very-quiet --in-place --sp-file /tmp/ppyu/linux/scripts/coccinelle/misc/add_namespace.cocci -D ns=MODULE /tmp/ppyu/linux/fs/nfs/client.c > >> >> > >> >> So only the first file got included in the spatch invocation. But the > >> >> spatch call gets fixed with all the files when quotes are added in the > >> >> call to generate_deps_for_ns. > >> >> > >> >> But we need to include your change anyway, to make the script more > >> >> robust. > >> > > >> >Hmm. > >> >With this patch only, I see "bad variable name" error. > >> > > >> > > >> > > >> >masahiro@grover:~/ref/linux$ make -j8 nsdeps > >> > DESCEND objtool > >> > CALL scripts/atomic/check-atomics.sh > >> > CALL scripts/checksyscalls.sh > >> > CHK include/generated/compile.h > >> > Building modules, stage 2. > >> > MODPOST 20 modules > >> >WARNING: module nfs uses symbol foo from namespace USB_STORAGE, but > >> >does not import it. > >> > Building modules, stage 2. > >> > MODPOST 20 modules > >> >./scripts/nsdeps: 34: local: ./fs/nfs/dir.c: bad variable name > >> >make: *** [Makefile;1689: nsdeps] Error 2 > >> > >> Hm, I was having trouble reproducing this until I changed the shell to > >> dash, /bin/sh is a symlink to bash on my system, that might explain > >> slightly different behavior. In any case, we should add quotes in both > >> places. > >> > >> >> It would probably prevent more shell script related bugs in > >> >> the future (Like [1]). I can respin this patch only while the other > >> >> ones are superceded by your patchset. > >> >> > >> >> [1] https://unix.stackexchange.com/a/131767 > >> > > >> >Anyway. > >> > > >> >Is this patch aiming for v5.4 (i.e. fixes) or v5.5-rc1 ? > >> > >> I am hoping for fixes, we should try to get all the small bugs out of > >> nsdeps by 5.4 if we can.. > >> > >> >If you touch the mod_source_files line, > >> >we will have a conflict because > >> >https://patchwork.kernel.org/patch/11217839/ > >> >is touching the same line. > >> > > >> >How should we organize the patch order? > >> > >> Would you like to fold these changes into your nsdeps improvements > >> patchset? Since it's a pretty trivial change. > >> > >> Thanks! > >> > >> > > > >My change set is quite big, so I > >am planning to send it for v5.5-rc1. > > OK, sounds good. > > >If you want to fix nsdeps for composite modules earlier, > >please apply this patch to your tree > >and send another pull request. > > I plan to send this in for -rc7, since it's a minor fix. OK. > For the conflict, if these patches are not pushed anywhere yet (at > least I did not see them on the kbuild for-next branch yet), perhaps > you could rebase on top of -rc7 and put your patchset on top? No problem. I will do so. Thanks. -- Best Regards Masahiro Yamada ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/4] scripts/nsdeps: use $MODORDER to obtain correct modules.order path 2019-10-28 15:14 [PATCH 1/4] scripts/nsdeps: use $MODORDER to obtain correct modules.order path Jessica Yu ` (2 preceding siblings ...) 2019-10-28 15:14 ` [PATCH 4/4] scripts/nsdeps: make sure to pass all module source files to spatch Jessica Yu @ 2019-10-29 12:48 ` Masahiro Yamada 3 siblings, 0 replies; 14+ messages in thread From: Masahiro Yamada @ 2019-10-29 12:48 UTC (permalink / raw) To: Jessica Yu; +Cc: Linux Kernel Mailing List, Matthias Maennich On Tue, Oct 29, 2019 at 12:14 AM Jessica Yu <jeyu@kernel.org> wrote: > > The nsdeps script calls generate_deps() for every module in the > modules.order file. It prepends $objtree to obtain the full path of the > top-level modules.order file. This produces incorrect results when > calling nsdeps for an external module, as only the ns dependencies for > in-tree modules listed in $objtree/modules.order are generated rather > than the ns dependencies for the external module. To fix this, just use > the MODORDER variable provided by kbuild - it uses the correct path for > the relevant modules.order file (either in-tree or the one produced by > the external module build). > > Signed-off-by: Jessica Yu <jeyu@kernel.org> > --- > > So, not being too familiar with kbuild, I am not sure if MODORDER was the > appropriate kbuild variable to use, but I could not find anything else that > gives us the modules.order path. Masahiro, please let me know if this is > appropriate usage. Right, MODORDER allows you to get access to the right modules.order depending on what you are building although this patch alone is not useful since nsdeps does not work with external modules. > scripts/nsdeps | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/scripts/nsdeps b/scripts/nsdeps > index dda6fbac016e..54d2ab8f9e5c 100644 > --- a/scripts/nsdeps > +++ b/scripts/nsdeps > @@ -52,7 +52,7 @@ generate_deps() { > done > } > > -for f in `cat $objtree/modules.order`; do > +for f in `cat $MODORDER`; do > generate_deps $f > done > > -- > 2.16.4 > -- Best Regards Masahiro Yamada ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2019-11-05 12:57 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-10-28 15:14 [PATCH 1/4] scripts/nsdeps: use $MODORDER to obtain correct modules.order path Jessica Yu 2019-10-28 15:14 ` [PATCH 2/4] scripts/nsdeps: don't prepend $srctree if *.mod already contains full paths Jessica Yu 2019-10-29 8:59 ` Masahiro Yamada 2019-10-28 15:14 ` [PATCH 3/4] nsdeps: remove stale .ns_deps files before generating new ones Jessica Yu 2019-10-29 10:45 ` Masahiro Yamada 2019-10-28 15:14 ` [PATCH 4/4] scripts/nsdeps: make sure to pass all module source files to spatch Jessica Yu 2019-10-29 12:57 ` Masahiro Yamada 2019-10-30 16:17 ` Jessica Yu 2019-10-31 12:27 ` Masahiro Yamada 2019-10-31 13:41 ` Jessica Yu 2019-11-01 5:56 ` Masahiro Yamada 2019-11-05 12:52 ` Jessica Yu 2019-11-05 12:57 ` Masahiro Yamada 2019-10-29 12:48 ` [PATCH 1/4] scripts/nsdeps: use $MODORDER to obtain correct modules.order path 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).