[PATCH 1/4] scripts/nsdeps: use $MODORDER to obtain correct modules.order path

[PATCH 1/4] scripts/nsdeps: use $MODORDER to obtain correct modules.order path

[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

[PATCH 2/4] scripts/nsdeps: don't prepend $srctree if *.mod already contains full paths

[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

[PATCH 3/4] nsdeps: remove stale .ns_deps files before generating new ones

[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

[PATCH 4/4] scripts/nsdeps: make sure to pass all module source files to spatch

[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

Re: [PATCH 2/4] scripts/nsdeps: don't prepend $srctree if *.mod already contains full paths

[Masahiro Yamada]

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

Re: [PATCH 3/4] nsdeps: remove stale .ns_deps files before generating new ones

[Masahiro Yamada]

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

Re: [PATCH 1/4] scripts/nsdeps: use $MODORDER to obtain correct modules.order path

[Masahiro Yamada]

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

Re: [PATCH 4/4] scripts/nsdeps: make sure to pass all module source files to spatch

[Masahiro Yamada]

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

Re: [PATCH 4/4] scripts/nsdeps: make sure to pass all module source files to spatch

[Jessica Yu]

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

Re: [PATCH 4/4] scripts/nsdeps: make sure to pass all module source files to spatch

[Masahiro Yamada]

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

Re: [PATCH 4/4] scripts/nsdeps: make sure to pass all module source files to spatch

[Jessica Yu]

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

Re: [PATCH 4/4] scripts/nsdeps: make sure to pass all module source files to spatch

[Masahiro Yamada]

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

Re: [PATCH 4/4] scripts/nsdeps: make sure to pass all module source files to spatch

[Jessica Yu]

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

Re: [PATCH 4/4] scripts/nsdeps: make sure to pass all module source files to spatch

[Masahiro Yamada]

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