[PATCH] modpost: move the namespace field in Module.symvers last

[PATCH] modpost: move the namespace field in Module.symvers last

[Jessica Yu]

In order to preserve backwards compatability with kmod tools, we have to
move the namespace field in Module.symvers last, as the depmod -e -E
option looks at the first three fields in Module.symvers to check symbol
versions (and it's expected they stay in the original order of crc,
symbol, module).

Fixes: cb9b55d21fe0 ("modpost: add support for symbol namespaces")
Cc: stable@vger.kernel.org
Signed-off-by: Jessica Yu <jeyu@kernel.org>
---
 Documentation/kbuild/modules.rst |  4 ++--
 scripts/export_report.pl         |  2 +-
 scripts/mod/modpost.c            | 24 ++++++++++++------------
 3 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/Documentation/kbuild/modules.rst b/Documentation/kbuild/modules.rst
index 69fa48ee93d6..e0b45a257f21 100644
--- a/Documentation/kbuild/modules.rst
+++ b/Documentation/kbuild/modules.rst
@@ -470,9 +470,9 @@ build.
 
 	The syntax of the Module.symvers file is::
 
-	<CRC>       <Symbol>          <Namespace>  <Module>                         <Export Type>
+	<CRC>       <Symbol>         <Module>                         <Export Type>     <Namespace>
 
-	0xe1cc2a05  usb_stor_suspend  USB_STORAGE  drivers/usb/storage/usb-storage  EXPORT_SYMBOL_GPL
+	0xe1cc2a05  usb_stor_suspend drivers/usb/storage/usb-storage  EXPORT_SYMBOL_GPL USB_STORAGE
 
 	The fields are separated by tabs and values may be empty (e.g.
 	if no namespace is defined for an exported symbol).
diff --git a/scripts/export_report.pl b/scripts/export_report.pl
index 548330e8c4e7..feb3d5542a62 100755
--- a/scripts/export_report.pl
+++ b/scripts/export_report.pl
@@ -94,7 +94,7 @@ if (defined $opt{'o'}) {
 #
 while ( <$module_symvers> ) {
 	chomp;
-	my (undef, $symbol, $namespace, $module, $gpl) = split('\t');
+	my (undef, $symbol, $module, $gpl, $namespace) = split('\t');
 	$SYMBOL { $symbol } =  [ $module , "0" , $symbol, $gpl];
 }
 close($module_symvers);
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 7edfdb2f4497..6ab235354f36 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -2427,7 +2427,7 @@ static void write_if_changed(struct buffer *b, const char *fname)
 }
 
 /* parse Module.symvers file. line format:
- * 0x12345678<tab>symbol<tab>module[[<tab>export]<tab>something]
+ * 0x12345678<tab>symbol<tab>module<tab>export<tab>namespace
  **/
 static void read_dump(const char *fname, unsigned int kernel)
 {
@@ -2440,7 +2440,7 @@ static void read_dump(const char *fname, unsigned int kernel)
 		return;
 
 	while ((line = get_next_line(&pos, file, size))) {
-		char *symname, *namespace, *modname, *d, *export, *end;
+		char *symname, *namespace, *modname, *d, *export;
 		unsigned int crc;
 		struct module *mod;
 		struct symbol *s;
@@ -2448,16 +2448,16 @@ static void read_dump(const char *fname, unsigned int kernel)
 		if (!(symname = strchr(line, '\t')))
 			goto fail;
 		*symname++ = '\0';
-		if (!(namespace = strchr(symname, '\t')))
-			goto fail;
-		*namespace++ = '\0';
-		if (!(modname = strchr(namespace, '\t')))
+		if (!(modname = strchr(symname, '\t')))
 			goto fail;
 		*modname++ = '\0';
-		if ((export = strchr(modname, '\t')) != NULL)
-			*export++ = '\0';
-		if (export && ((end = strchr(export, '\t')) != NULL))
-			*end = '\0';
+		if (!(export = strchr(modname, '\t')))
+			goto fail;
+		*export++ = '\0';
+		if (!(namespace = strchr(export, '\t')))
+			goto fail;
+		*namespace++ = '\0';
+
 		crc = strtoul(line, &d, 16);
 		if (*symname == '\0' || *modname == '\0' || *d != '\0')
 			goto fail;
@@ -2508,9 +2508,9 @@ static void write_dump(const char *fname)
 				namespace = symbol->namespace;
 				buf_printf(&buf, "0x%08x\t%s\t%s\t%s\t%s\n",
 					   symbol->crc, symbol->name,
-					   namespace ? namespace : "",
 					   symbol->module->name,
-					   export_str(symbol->export));
+					   export_str(symbol->export),
+					   namespace ? namespace : "");
 			}
 			symbol = symbol->next;
 		}
-- 
2.16.4

Re: [PATCH] modpost: move the namespace field in Module.symvers last

[Jessica Yu]

+++ Jessica Yu [04/03/20 18:03 +0100]:
>In order to preserve backwards compatability with kmod tools, we have to
>move the namespace field in Module.symvers last, as the depmod -e -E
>option looks at the first three fields in Module.symvers to check symbol
>versions (and it's expected they stay in the original order of crc,
>symbol, module).
>
>Fixes: cb9b55d21fe0 ("modpost: add support for symbol namespaces")
>Cc: stable@vger.kernel.org
>Signed-off-by: Jessica Yu <jeyu@kernel.org>

First, I apologize for not having caught this mistake earlier. I still
have questions about the Module.symvers format, please see below.

>---
> Documentation/kbuild/modules.rst |  4 ++--
> scripts/export_report.pl         |  2 +-
> scripts/mod/modpost.c            | 24 ++++++++++++------------
> 3 files changed, 15 insertions(+), 15 deletions(-)
>
>diff --git a/Documentation/kbuild/modules.rst b/Documentation/kbuild/modules.rst
>index 69fa48ee93d6..e0b45a257f21 100644
>--- a/Documentation/kbuild/modules.rst
>+++ b/Documentation/kbuild/modules.rst
>@@ -470,9 +470,9 @@ build.
>
> 	The syntax of the Module.symvers file is::
>
>-	<CRC>       <Symbol>          <Namespace>  <Module>                         <Export Type>
>+	<CRC>       <Symbol>         <Module>                         <Export Type>     <Namespace>
>
>-	0xe1cc2a05  usb_stor_suspend  USB_STORAGE  drivers/usb/storage/usb-storage  EXPORT_SYMBOL_GPL
>+	0xe1cc2a05  usb_stor_suspend drivers/usb/storage/usb-storage  EXPORT_SYMBOL_GPL USB_STORAGE
>
> 	The fields are separated by tabs and values may be empty (e.g.
> 	if no namespace is defined for an exported symbol).
>diff --git a/scripts/export_report.pl b/scripts/export_report.pl
>index 548330e8c4e7..feb3d5542a62 100755
>--- a/scripts/export_report.pl
>+++ b/scripts/export_report.pl
>@@ -94,7 +94,7 @@ if (defined $opt{'o'}) {
> #
> while ( <$module_symvers> ) {
> 	chomp;
>-	my (undef, $symbol, $namespace, $module, $gpl) = split('\t');
>+	my (undef, $symbol, $module, $gpl, $namespace) = split('\t');
> 	$SYMBOL { $symbol } =  [ $module , "0" , $symbol, $gpl];
> }
> close($module_symvers);
>diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
>index 7edfdb2f4497..6ab235354f36 100644
>--- a/scripts/mod/modpost.c
>+++ b/scripts/mod/modpost.c
>@@ -2427,7 +2427,7 @@ static void write_if_changed(struct buffer *b, const char *fname)
> }
>
> /* parse Module.symvers file. line format:
>- * 0x12345678<tab>symbol<tab>module[[<tab>export]<tab>something]
>+ * 0x12345678<tab>symbol<tab>module<tab>export<tab>namespace

So, this comment was a source of confusion for me and Matthias I
think. It suggests that the export field is optional, and that even
following the export field there may also be "something" else,
whatever that is.

I suspect that there were historical reasons behind that comment that
are no longer accurate. We have been unconditionally printing the
export type since 2.6.18 (commit bd5cbcedf44), which is over a decade
ago now. And let me explain the read_dump() changes...

>  **/
> static void read_dump(const char *fname, unsigned int kernel)
> {
>@@ -2440,7 +2440,7 @@ static void read_dump(const char *fname, unsigned int kernel)
> 		return;
>
> 	while ((line = get_next_line(&pos, file, size))) {
>-		char *symname, *namespace, *modname, *d, *export, *end;
>+		char *symname, *namespace, *modname, *d, *export;
> 		unsigned int crc;
> 		struct module *mod;
> 		struct symbol *s;
>@@ -2448,16 +2448,16 @@ static void read_dump(const char *fname, unsigned int kernel)
> 		if (!(symname = strchr(line, '\t')))
> 			goto fail;
> 		*symname++ = '\0';
>-		if (!(namespace = strchr(symname, '\t')))
>-			goto fail;
>-		*namespace++ = '\0';
>-		if (!(modname = strchr(namespace, '\t')))
>+		if (!(modname = strchr(symname, '\t')))
> 			goto fail;
> 		*modname++ = '\0';
>-		if ((export = strchr(modname, '\t')) != NULL)
>-			*export++ = '\0';
>-		if (export && ((end = strchr(export, '\t')) != NULL))
>-			*end = '\0';

I believe the original read_dump() code treated the export field here
as optional, to support pre <= 2.6.18 Module.symvers (which does not
have the export type field). But I don't believe we have to support
this case anymore, right? It's ages ago. So I cleaned up this area,
made each field non-optional (but empty string "" for namespace is
allowed), and updated the comment.

>+		if (!(export = strchr(modname, '\t')))
>+			goto fail;
>+		*export++ = '\0';
>+		if (!(namespace = strchr(export, '\t')))
>+			goto fail;
>+		*namespace++ = '\0';
>+
> 		crc = strtoul(line, &d, 16);
> 		if (*symname == '\0' || *modname == '\0' || *d != '\0')
> 			goto fail;
>@@ -2508,9 +2508,9 @@ static void write_dump(const char *fname)
> 				namespace = symbol->namespace;
> 				buf_printf(&buf, "0x%08x\t%s\t%s\t%s\t%s\n",
> 					   symbol->crc, symbol->name,
>-					   namespace ? namespace : "",
> 					   symbol->module->name,
>-					   export_str(symbol->export));
>+					   export_str(symbol->export),
>+					   namespace ? namespace : "");
> 			}
> 			symbol = symbol->next;
> 		}
>-- 
>2.16.4
>

Re: [PATCH] modpost: move the namespace field in Module.symvers last

[Matthias Maennich]

Hi Jessica!
Thanks for working on this!

On Wed, Mar 04, 2020 at 06:03:45PM +0100, Jessica Yu wrote:
>In order to preserve backwards compatability with kmod tools, we have to
>move the namespace field in Module.symvers last, as the depmod -e -E
>option looks at the first three fields in Module.symvers to check symbol
>versions (and it's expected they stay in the original order of crc,
>symbol, module).
>
>Fixes: cb9b55d21fe0 ("modpost: add support for symbol namespaces")
>Cc: stable@vger.kernel.org

Please note, this patch did not actually go to stable@.

>Signed-off-by: Jessica Yu <jeyu@kernel.org>
>---
> Documentation/kbuild/modules.rst |  4 ++--
> scripts/export_report.pl         |  2 +-
> scripts/mod/modpost.c            | 24 ++++++++++++------------
> 3 files changed, 15 insertions(+), 15 deletions(-)
>
>diff --git a/Documentation/kbuild/modules.rst b/Documentation/kbuild/modules.rst
>index 69fa48ee93d6..e0b45a257f21 100644
>--- a/Documentation/kbuild/modules.rst
>+++ b/Documentation/kbuild/modules.rst
>@@ -470,9 +470,9 @@ build.
>
> 	The syntax of the Module.symvers file is::
>
>-	<CRC>       <Symbol>          <Namespace>  <Module>                         <Export Type>
>+	<CRC>       <Symbol>         <Module>                         <Export Type>     <Namespace>
>
>-	0xe1cc2a05  usb_stor_suspend  USB_STORAGE  drivers/usb/storage/usb-storage  EXPORT_SYMBOL_GPL
>+	0xe1cc2a05  usb_stor_suspend drivers/usb/storage/usb-storage  EXPORT_SYMBOL_GPL USB_STORAGE
>
> 	The fields are separated by tabs and values may be empty (e.g.
> 	if no namespace is defined for an exported symbol).
>diff --git a/scripts/export_report.pl b/scripts/export_report.pl
>index 548330e8c4e7..feb3d5542a62 100755
>--- a/scripts/export_report.pl
>+++ b/scripts/export_report.pl
>@@ -94,7 +94,7 @@ if (defined $opt{'o'}) {
> #
> while ( <$module_symvers> ) {
> 	chomp;
>-	my (undef, $symbol, $namespace, $module, $gpl) = split('\t');
>+	my (undef, $symbol, $module, $gpl, $namespace) = split('\t');
> 	$SYMBOL { $symbol } =  [ $module , "0" , $symbol, $gpl];
> }
> close($module_symvers);
>diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
>index 7edfdb2f4497..6ab235354f36 100644
>--- a/scripts/mod/modpost.c
>+++ b/scripts/mod/modpost.c
>@@ -2427,7 +2427,7 @@ static void write_if_changed(struct buffer *b, const char *fname)
> }
>
> /* parse Module.symvers file. line format:
>- * 0x12345678<tab>symbol<tab>module[[<tab>export]<tab>something]
>+ * 0x12345678<tab>symbol<tab>module<tab>export<tab>namespace
>  **/
> static void read_dump(const char *fname, unsigned int kernel)
> {
>@@ -2440,7 +2440,7 @@ static void read_dump(const char *fname, unsigned int kernel)
> 		return;
>
> 	while ((line = get_next_line(&pos, file, size))) {
>-		char *symname, *namespace, *modname, *d, *export, *end;
>+		char *symname, *namespace, *modname, *d, *export;
> 		unsigned int crc;
> 		struct module *mod;
> 		struct symbol *s;
>@@ -2448,16 +2448,16 @@ static void read_dump(const char *fname, unsigned int kernel)
> 		if (!(symname = strchr(line, '\t')))
> 			goto fail;
> 		*symname++ = '\0';
>-		if (!(namespace = strchr(symname, '\t')))
>-			goto fail;
>-		*namespace++ = '\0';
>-		if (!(modname = strchr(namespace, '\t')))
>+		if (!(modname = strchr(symname, '\t')))
> 			goto fail;
> 		*modname++ = '\0';
>-		if ((export = strchr(modname, '\t')) != NULL)
>-			*export++ = '\0';
>-		if (export && ((end = strchr(export, '\t')) != NULL))
>-			*end = '\0';
>+		if (!(export = strchr(modname, '\t')))
>+			goto fail;
>+		*export++ = '\0';
>+		if (!(namespace = strchr(export, '\t')))

As mentioned below, we should probably treat namespace as an optional
field. Then this needs adjusting to handle that case. Similar to how
optional cases were handled before.

>+			goto fail;
>+		*namespace++ = '\0';
>+
> 		crc = strtoul(line, &d, 16);
> 		if (*symname == '\0' || *modname == '\0' || *d != '\0')
> 			goto fail;
>@@ -2508,9 +2508,9 @@ static void write_dump(const char *fname)
> 				namespace = symbol->namespace;
> 				buf_printf(&buf, "0x%08x\t%s\t%s\t%s\t%s\n",

This creates trailing tabs for symbols without namespace. If we treat
'namespace' as an optional field, we should probably make the tab
conditional as well? What do you think?

Cheers,
Matthias

> 					   symbol->crc, symbol->name,
>-					   namespace ? namespace : "",
> 					   symbol->module->name,
>-					   export_str(symbol->export));
>+					   export_str(symbol->export),
>+					   namespace ? namespace : "");
> 			}
> 			symbol = symbol->next;
> 		}
>-- 
>2.16.4
>

Re: [PATCH] modpost: move the namespace field in Module.symvers last

[Jessica Yu]

+++ Matthias Maennich [05/03/20 08:59 +0000]:
>Hi Jessica!
>Thanks for working on this!
>
>On Wed, Mar 04, 2020 at 06:03:45PM +0100, Jessica Yu wrote:
>>In order to preserve backwards compatability with kmod tools, we have to
>>move the namespace field in Module.symvers last, as the depmod -e -E
>>option looks at the first three fields in Module.symvers to check symbol
>>versions (and it's expected they stay in the original order of crc,
>>symbol, module).
>>
>>Fixes: cb9b55d21fe0 ("modpost: add support for symbol namespaces")
>>Cc: stable@vger.kernel.org
>
>Please note, this patch did not actually go to stable@.

Hi Matthias! Thanks, I missed that.

>>Signed-off-by: Jessica Yu <jeyu@kernel.org>
>>---
>>Documentation/kbuild/modules.rst |  4 ++--
>>scripts/export_report.pl         |  2 +-
>>scripts/mod/modpost.c            | 24 ++++++++++++------------
>>3 files changed, 15 insertions(+), 15 deletions(-)
>>
>>diff --git a/Documentation/kbuild/modules.rst b/Documentation/kbuild/modules.rst
>>index 69fa48ee93d6..e0b45a257f21 100644
>>--- a/Documentation/kbuild/modules.rst
>>+++ b/Documentation/kbuild/modules.rst
>>@@ -470,9 +470,9 @@ build.
>>
>>	The syntax of the Module.symvers file is::
>>
>>-	<CRC>       <Symbol>          <Namespace>  <Module>                         <Export Type>
>>+	<CRC>       <Symbol>         <Module>                         <Export Type>     <Namespace>
>>
>>-	0xe1cc2a05  usb_stor_suspend  USB_STORAGE  drivers/usb/storage/usb-storage  EXPORT_SYMBOL_GPL
>>+	0xe1cc2a05  usb_stor_suspend drivers/usb/storage/usb-storage  EXPORT_SYMBOL_GPL USB_STORAGE
>>
>>	The fields are separated by tabs and values may be empty (e.g.
>>	if no namespace is defined for an exported symbol).
>>diff --git a/scripts/export_report.pl b/scripts/export_report.pl
>>index 548330e8c4e7..feb3d5542a62 100755
>>--- a/scripts/export_report.pl
>>+++ b/scripts/export_report.pl
>>@@ -94,7 +94,7 @@ if (defined $opt{'o'}) {
>>#
>>while ( <$module_symvers> ) {
>>	chomp;
>>-	my (undef, $symbol, $namespace, $module, $gpl) = split('\t');
>>+	my (undef, $symbol, $module, $gpl, $namespace) = split('\t');
>>	$SYMBOL { $symbol } =  [ $module , "0" , $symbol, $gpl];
>>}
>>close($module_symvers);
>>diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
>>index 7edfdb2f4497..6ab235354f36 100644
>>--- a/scripts/mod/modpost.c
>>+++ b/scripts/mod/modpost.c
>>@@ -2427,7 +2427,7 @@ static void write_if_changed(struct buffer *b, const char *fname)
>>}
>>
>>/* parse Module.symvers file. line format:
>>- * 0x12345678<tab>symbol<tab>module[[<tab>export]<tab>something]
>>+ * 0x12345678<tab>symbol<tab>module<tab>export<tab>namespace
>> **/
>>static void read_dump(const char *fname, unsigned int kernel)
>>{
>>@@ -2440,7 +2440,7 @@ static void read_dump(const char *fname, unsigned int kernel)
>>		return;
>>
>>	while ((line = get_next_line(&pos, file, size))) {
>>-		char *symname, *namespace, *modname, *d, *export, *end;
>>+		char *symname, *namespace, *modname, *d, *export;
>>		unsigned int crc;
>>		struct module *mod;
>>		struct symbol *s;
>>@@ -2448,16 +2448,16 @@ static void read_dump(const char *fname, unsigned int kernel)
>>		if (!(symname = strchr(line, '\t')))
>>			goto fail;
>>		*symname++ = '\0';
>>-		if (!(namespace = strchr(symname, '\t')))
>>-			goto fail;
>>-		*namespace++ = '\0';
>>-		if (!(modname = strchr(namespace, '\t')))
>>+		if (!(modname = strchr(symname, '\t')))
>>			goto fail;
>>		*modname++ = '\0';
>>-		if ((export = strchr(modname, '\t')) != NULL)
>>-			*export++ = '\0';
>>-		if (export && ((end = strchr(export, '\t')) != NULL))
>>-			*end = '\0';
>>+		if (!(export = strchr(modname, '\t')))
>>+			goto fail;
>>+		*export++ = '\0';
>>+		if (!(namespace = strchr(export, '\t')))
>
>As mentioned below, we should probably treat namespace as an optional
>field. Then this needs adjusting to handle that case. Similar to how
>optional cases were handled before.

Hm, I think introducing optional fields would add unnecessary
complexity, and make future parsing harder. For example, say in the
distant future we add another field. If fields are optional, we are no
longer able to tell if the 4th field is a namespace or the new_field.
Whereas, if we made the fields mandatory (even if empty), we would see
crc<tab>symbol<tab>module<tab>export_type<tab><tab>new_field, and it's
clear that the namespace is empty. I hope that makes sense...

IMO, I think it's easiest to just establish the fact that
Module.symvers has 5 fields, and fields can be empty. If a field an
empty, then the next delimiter or end of line will just follow
immediately.

Just to reiterate, it is true namespaces are optional, and in the case
of no namespace, I would prefer it to be an empty string/field rather
than omitting it entirely.

>>+			goto fail;
>>+		*namespace++ = '\0';
>>+
>>		crc = strtoul(line, &d, 16);
>>		if (*symname == '\0' || *modname == '\0' || *d != '\0')
>>			goto fail;
>>@@ -2508,9 +2508,9 @@ static void write_dump(const char *fname)
>>				namespace = symbol->namespace;
>>				buf_printf(&buf, "0x%08x\t%s\t%s\t%s\t%s\n",
>
>This creates trailing tabs for symbols without namespace. If we treat
>'namespace' as an optional field, we should probably make the tab
>conditional as well? What do you think?

Yeah you're right, the trailing tab after an empty namespace looks
weird...but for reasons I cited above, I would like to keep it simple,
unless there are huge objections.

Thank you for the review!

Jessica

Re: [PATCH] modpost: move the namespace field in Module.symvers last

[Matthias Maennich]

On Thu, Mar 05, 2020 at 02:47:32PM +0100, Jessica Yu wrote:
>+++ Matthias Maennich [05/03/20 08:59 +0000]:
>>Hi Jessica!
>>Thanks for working on this!
>>
>>On Wed, Mar 04, 2020 at 06:03:45PM +0100, Jessica Yu wrote:
>>>In order to preserve backwards compatability with kmod tools, we have to
>>>move the namespace field in Module.symvers last, as the depmod -e -E
>>>option looks at the first three fields in Module.symvers to check symbol
>>>versions (and it's expected they stay in the original order of crc,
>>>symbol, module).
>>>
>>>Fixes: cb9b55d21fe0 ("modpost: add support for symbol namespaces")
>>>Cc: stable@vger.kernel.org
>>
>>Please note, this patch did not actually go to stable@.
>
>Hi Matthias! Thanks, I missed that.
>
>>>Signed-off-by: Jessica Yu <jeyu@kernel.org>
>>>---
>>>Documentation/kbuild/modules.rst |  4 ++--
>>>scripts/export_report.pl         |  2 +-
>>>scripts/mod/modpost.c            | 24 ++++++++++++------------
>>>3 files changed, 15 insertions(+), 15 deletions(-)
>>>
>>>diff --git a/Documentation/kbuild/modules.rst b/Documentation/kbuild/modules.rst
>>>index 69fa48ee93d6..e0b45a257f21 100644
>>>--- a/Documentation/kbuild/modules.rst
>>>+++ b/Documentation/kbuild/modules.rst
>>>@@ -470,9 +470,9 @@ build.
>>>
>>>	The syntax of the Module.symvers file is::
>>>
>>>-	<CRC>       <Symbol>          <Namespace>  <Module>                         <Export Type>
>>>+	<CRC>       <Symbol>         <Module>                         <Export Type>     <Namespace>
>>>
>>>-	0xe1cc2a05  usb_stor_suspend  USB_STORAGE  drivers/usb/storage/usb-storage  EXPORT_SYMBOL_GPL
>>>+	0xe1cc2a05  usb_stor_suspend drivers/usb/storage/usb-storage  EXPORT_SYMBOL_GPL USB_STORAGE
>>>
>>>	The fields are separated by tabs and values may be empty (e.g.
>>>	if no namespace is defined for an exported symbol).
>>>diff --git a/scripts/export_report.pl b/scripts/export_report.pl
>>>index 548330e8c4e7..feb3d5542a62 100755
>>>--- a/scripts/export_report.pl
>>>+++ b/scripts/export_report.pl
>>>@@ -94,7 +94,7 @@ if (defined $opt{'o'}) {
>>>#
>>>while ( <$module_symvers> ) {
>>>	chomp;
>>>-	my (undef, $symbol, $namespace, $module, $gpl) = split('\t');
>>>+	my (undef, $symbol, $module, $gpl, $namespace) = split('\t');
>>>	$SYMBOL { $symbol } =  [ $module , "0" , $symbol, $gpl];
>>>}
>>>close($module_symvers);
>>>diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
>>>index 7edfdb2f4497..6ab235354f36 100644
>>>--- a/scripts/mod/modpost.c
>>>+++ b/scripts/mod/modpost.c
>>>@@ -2427,7 +2427,7 @@ static void write_if_changed(struct buffer *b, const char *fname)
>>>}
>>>
>>>/* parse Module.symvers file. line format:
>>>- * 0x12345678<tab>symbol<tab>module[[<tab>export]<tab>something]
>>>+ * 0x12345678<tab>symbol<tab>module<tab>export<tab>namespace
>>>**/
>>>static void read_dump(const char *fname, unsigned int kernel)
>>>{
>>>@@ -2440,7 +2440,7 @@ static void read_dump(const char *fname, unsigned int kernel)
>>>		return;
>>>
>>>	while ((line = get_next_line(&pos, file, size))) {
>>>-		char *symname, *namespace, *modname, *d, *export, *end;
>>>+		char *symname, *namespace, *modname, *d, *export;
>>>		unsigned int crc;
>>>		struct module *mod;
>>>		struct symbol *s;
>>>@@ -2448,16 +2448,16 @@ static void read_dump(const char *fname, unsigned int kernel)
>>>		if (!(symname = strchr(line, '\t')))
>>>			goto fail;
>>>		*symname++ = '\0';
>>>-		if (!(namespace = strchr(symname, '\t')))
>>>-			goto fail;
>>>-		*namespace++ = '\0';
>>>-		if (!(modname = strchr(namespace, '\t')))
>>>+		if (!(modname = strchr(symname, '\t')))
>>>			goto fail;
>>>		*modname++ = '\0';
>>>-		if ((export = strchr(modname, '\t')) != NULL)
>>>-			*export++ = '\0';
>>>-		if (export && ((end = strchr(export, '\t')) != NULL))
>>>-			*end = '\0';
>>>+		if (!(export = strchr(modname, '\t')))
>>>+			goto fail;
>>>+		*export++ = '\0';
>>>+		if (!(namespace = strchr(export, '\t')))
>>
>>As mentioned below, we should probably treat namespace as an optional
>>field. Then this needs adjusting to handle that case. Similar to how
>>optional cases were handled before.
>
>Hm, I think introducing optional fields would add unnecessary
>complexity, and make future parsing harder. For example, say in the
>distant future we add another field. If fields are optional, we are no
>longer able to tell if the 4th field is a namespace or the new_field.
>Whereas, if we made the fields mandatory (even if empty), we would see
>crc<tab>symbol<tab>module<tab>export_type<tab><tab>new_field, and it's
>clear that the namespace is empty. I hope that makes sense...
>
>IMO, I think it's easiest to just establish the fact that
>Module.symvers has 5 fields, and fields can be empty. If a field an
>empty, then the next delimiter or end of line will just follow
>immediately.
>
>Just to reiterate, it is true namespaces are optional, and in the case
>of no namespace, I would prefer it to be an empty string/field rather
>than omitting it entirely.
>
>>>+			goto fail;
>>>+		*namespace++ = '\0';
>>>+
>>>		crc = strtoul(line, &d, 16);
>>>		if (*symname == '\0' || *modname == '\0' || *d != '\0')
>>>			goto fail;
>>>@@ -2508,9 +2508,9 @@ static void write_dump(const char *fname)
>>>				namespace = symbol->namespace;
>>>				buf_printf(&buf, "0x%08x\t%s\t%s\t%s\t%s\n",
>>
>>This creates trailing tabs for symbols without namespace. If we treat
>>'namespace' as an optional field, we should probably make the tab
>>conditional as well? What do you think?
>
>Yeah you're right, the trailing tab after an empty namespace looks
>weird...but for reasons I cited above, I would like to keep it simple,
>unless there are huge objections.

I think I don't want to object. Having trailing whitespaces in the file
just looks a bit weird. And I am not sure if this might confuse anyone.
E.g. think of this file ending up in some source control or packaging
where trailing spaces are removed, then this format is broken and can't
be reliably read. But maybe I am overthinking it.

Besides  this, the patch looks fine and I successfully tested it.
Maybe let others chime in for their opinion.

Cheers,
Matthias

>
>Thank you for the review!
>
>Jessica
>

Re: [PATCH] modpost: move the namespace field in Module.symvers last

[Lucas De Marchi]

Hi,

On Wed, Mar 04, 2020 at 06:22:23PM +0100, Jessica Yu wrote:
>+++ Jessica Yu [04/03/20 18:03 +0100]:
>>In order to preserve backwards compatability with kmod tools, we have to
>>move the namespace field in Module.symvers last, as the depmod -e -E
>>option looks at the first three fields in Module.symvers to check symbol
>>versions (and it's expected they stay in the original order of crc,
>>symbol, module).
>>
>>Fixes: cb9b55d21fe0 ("modpost: add support for symbol namespaces")
>>Cc: stable@vger.kernel.org
>>Signed-off-by: Jessica Yu <jeyu@kernel.org>
>
>First, I apologize for not having caught this mistake earlier. I still
>have questions about the Module.symvers format, please see below.
>
>>---
>>Documentation/kbuild/modules.rst |  4 ++--
>>scripts/export_report.pl         |  2 +-
>>scripts/mod/modpost.c            | 24 ++++++++++++------------
>>3 files changed, 15 insertions(+), 15 deletions(-)
>>
>>diff --git a/Documentation/kbuild/modules.rst b/Documentation/kbuild/modules.rst
>>index 69fa48ee93d6..e0b45a257f21 100644
>>--- a/Documentation/kbuild/modules.rst
>>+++ b/Documentation/kbuild/modules.rst
>>@@ -470,9 +470,9 @@ build.
>>
>>	The syntax of the Module.symvers file is::
>>
>>-	<CRC>       <Symbol>          <Namespace>  <Module>                         <Export Type>
>>+	<CRC>       <Symbol>         <Module>                         <Export Type>     <Namespace>
>>
>>-	0xe1cc2a05  usb_stor_suspend  USB_STORAGE  drivers/usb/storage/usb-storage  EXPORT_SYMBOL_GPL
>>+	0xe1cc2a05  usb_stor_suspend drivers/usb/storage/usb-storage  EXPORT_SYMBOL_GPL USB_STORAGE
>>
>>	The fields are separated by tabs and values may be empty (e.g.
>>	if no namespace is defined for an exported symbol).
>>diff --git a/scripts/export_report.pl b/scripts/export_report.pl
>>index 548330e8c4e7..feb3d5542a62 100755
>>--- a/scripts/export_report.pl
>>+++ b/scripts/export_report.pl
>>@@ -94,7 +94,7 @@ if (defined $opt{'o'}) {
>>#
>>while ( <$module_symvers> ) {
>>	chomp;
>>-	my (undef, $symbol, $namespace, $module, $gpl) = split('\t');
>>+	my (undef, $symbol, $module, $gpl, $namespace) = split('\t');
>>	$SYMBOL { $symbol } =  [ $module , "0" , $symbol, $gpl];
>>}
>>close($module_symvers);
>>diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
>>index 7edfdb2f4497..6ab235354f36 100644
>>--- a/scripts/mod/modpost.c
>>+++ b/scripts/mod/modpost.c
>>@@ -2427,7 +2427,7 @@ static void write_if_changed(struct buffer *b, const char *fname)
>>}
>>
>>/* parse Module.symvers file. line format:
>>- * 0x12345678<tab>symbol<tab>module[[<tab>export]<tab>something]
>>+ * 0x12345678<tab>symbol<tab>module<tab>export<tab>namespace
>
>So, this comment was a source of confusion for me and Matthias I
>think. It suggests that the export field is optional, and that even
>following the export field there may also be "something" else,
>whatever that is.
>
>I suspect that there were historical reasons behind that comment that
>are no longer accurate. We have been unconditionally printing the
>export type since 2.6.18 (commit bd5cbcedf44), which is over a decade
>ago now. And let me explain the read_dump() changes...

I think this is a good information to be amended in the commit message,
so 10 years from now we can get find this info in git log.

>
>> **/
>>static void read_dump(const char *fname, unsigned int kernel)
>>{
>>@@ -2440,7 +2440,7 @@ static void read_dump(const char *fname, unsigned int kernel)
>>		return;
>>
>>	while ((line = get_next_line(&pos, file, size))) {
>>-		char *symname, *namespace, *modname, *d, *export, *end;
>>+		char *symname, *namespace, *modname, *d, *export;
>>		unsigned int crc;
>>		struct module *mod;
>>		struct symbol *s;
>>@@ -2448,16 +2448,16 @@ static void read_dump(const char *fname, unsigned int kernel)
>>		if (!(symname = strchr(line, '\t')))
>>			goto fail;
>>		*symname++ = '\0';
>>-		if (!(namespace = strchr(symname, '\t')))
>>-			goto fail;
>>-		*namespace++ = '\0';
>>-		if (!(modname = strchr(namespace, '\t')))
>>+		if (!(modname = strchr(symname, '\t')))
>>			goto fail;
>>		*modname++ = '\0';
>>-		if ((export = strchr(modname, '\t')) != NULL)
>>-			*export++ = '\0';
>>-		if (export && ((end = strchr(export, '\t')) != NULL))
>>-			*end = '\0';
>
>I believe the original read_dump() code treated the export field here
>as optional, to support pre <= 2.6.18 Module.symvers (which does not
>have the export type field). But I don't believe we have to support
>this case anymore, right? It's ages ago. So I cleaned up this area,
>made each field non-optional (but empty string "" for namespace is
>allowed), and updated the comment.

Same here. And agreed.

thanks

>
>>+		if (!(export = strchr(modname, '\t')))
>>+			goto fail;
>>+		*export++ = '\0';
>>+		if (!(namespace = strchr(export, '\t')))
>>+			goto fail;
>>+		*namespace++ = '\0';
>>+
>>		crc = strtoul(line, &d, 16);
>>		if (*symname == '\0' || *modname == '\0' || *d != '\0')
>>			goto fail;
>>@@ -2508,9 +2508,9 @@ static void write_dump(const char *fname)
>>				namespace = symbol->namespace;
>>				buf_printf(&buf, "0x%08x\t%s\t%s\t%s\t%s\n",
>>					   symbol->crc, symbol->name,
>>-					   namespace ? namespace : "",
>>					   symbol->module->name,
>>-					   export_str(symbol->export));
>>+					   export_str(symbol->export),
>>+					   namespace ? namespace : "");
>>			}
>>			symbol = symbol->next;
>>		}
>>-- 
>>2.16.4
>>

Re: [PATCH] modpost: move the namespace field in Module.symvers last

[Lucas De Marchi]

On Wed, Mar 04, 2020 at 06:03:45PM +0100, Jessica Yu wrote:
>In order to preserve backwards compatability with kmod tools, we have to
>move the namespace field in Module.symvers last, as the depmod -e -E
>option looks at the first three fields in Module.symvers to check symbol
>versions (and it's expected they stay in the original order of crc,
>symbol, module).
>
>Fixes: cb9b55d21fe0 ("modpost: add support for symbol namespaces")
>Cc: stable@vger.kernel.org
>Signed-off-by: Jessica Yu <jeyu@kernel.org>

With the additional info provided in the commit message as requested in
the other email,

Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>

thanks
Lucas De Marchi

>---
> Documentation/kbuild/modules.rst |  4 ++--
> scripts/export_report.pl         |  2 +-
> scripts/mod/modpost.c            | 24 ++++++++++++------------
> 3 files changed, 15 insertions(+), 15 deletions(-)
>
>diff --git a/Documentation/kbuild/modules.rst b/Documentation/kbuild/modules.rst
>index 69fa48ee93d6..e0b45a257f21 100644
>--- a/Documentation/kbuild/modules.rst
>+++ b/Documentation/kbuild/modules.rst
>@@ -470,9 +470,9 @@ build.
>
> 	The syntax of the Module.symvers file is::
>
>-	<CRC>       <Symbol>          <Namespace>  <Module>                         <Export Type>
>+	<CRC>       <Symbol>         <Module>                         <Export Type>     <Namespace>
>
>-	0xe1cc2a05  usb_stor_suspend  USB_STORAGE  drivers/usb/storage/usb-storage  EXPORT_SYMBOL_GPL
>+	0xe1cc2a05  usb_stor_suspend drivers/usb/storage/usb-storage  EXPORT_SYMBOL_GPL USB_STORAGE
>
> 	The fields are separated by tabs and values may be empty (e.g.
> 	if no namespace is defined for an exported symbol).
>diff --git a/scripts/export_report.pl b/scripts/export_report.pl
>index 548330e8c4e7..feb3d5542a62 100755
>--- a/scripts/export_report.pl
>+++ b/scripts/export_report.pl
>@@ -94,7 +94,7 @@ if (defined $opt{'o'}) {
> #
> while ( <$module_symvers> ) {
> 	chomp;
>-	my (undef, $symbol, $namespace, $module, $gpl) = split('\t');
>+	my (undef, $symbol, $module, $gpl, $namespace) = split('\t');
> 	$SYMBOL { $symbol } =  [ $module , "0" , $symbol, $gpl];
> }
> close($module_symvers);
>diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
>index 7edfdb2f4497..6ab235354f36 100644
>--- a/scripts/mod/modpost.c
>+++ b/scripts/mod/modpost.c
>@@ -2427,7 +2427,7 @@ static void write_if_changed(struct buffer *b, const char *fname)
> }
>
> /* parse Module.symvers file. line format:
>- * 0x12345678<tab>symbol<tab>module[[<tab>export]<tab>something]
>+ * 0x12345678<tab>symbol<tab>module<tab>export<tab>namespace
>  **/
> static void read_dump(const char *fname, unsigned int kernel)
> {
>@@ -2440,7 +2440,7 @@ static void read_dump(const char *fname, unsigned int kernel)
> 		return;
>
> 	while ((line = get_next_line(&pos, file, size))) {
>-		char *symname, *namespace, *modname, *d, *export, *end;
>+		char *symname, *namespace, *modname, *d, *export;
> 		unsigned int crc;
> 		struct module *mod;
> 		struct symbol *s;
>@@ -2448,16 +2448,16 @@ static void read_dump(const char *fname, unsigned int kernel)
> 		if (!(symname = strchr(line, '\t')))
> 			goto fail;
> 		*symname++ = '\0';
>-		if (!(namespace = strchr(symname, '\t')))
>-			goto fail;
>-		*namespace++ = '\0';
>-		if (!(modname = strchr(namespace, '\t')))
>+		if (!(modname = strchr(symname, '\t')))
> 			goto fail;
> 		*modname++ = '\0';
>-		if ((export = strchr(modname, '\t')) != NULL)
>-			*export++ = '\0';
>-		if (export && ((end = strchr(export, '\t')) != NULL))
>-			*end = '\0';
>+		if (!(export = strchr(modname, '\t')))
>+			goto fail;
>+		*export++ = '\0';
>+		if (!(namespace = strchr(export, '\t')))
>+			goto fail;
>+		*namespace++ = '\0';
>+
> 		crc = strtoul(line, &d, 16);
> 		if (*symname == '\0' || *modname == '\0' || *d != '\0')
> 			goto fail;
>@@ -2508,9 +2508,9 @@ static void write_dump(const char *fname)
> 				namespace = symbol->namespace;
> 				buf_printf(&buf, "0x%08x\t%s\t%s\t%s\t%s\n",
> 					   symbol->crc, symbol->name,
>-					   namespace ? namespace : "",
> 					   symbol->module->name,
>-					   export_str(symbol->export));
>+					   export_str(symbol->export),
>+					   namespace ? namespace : "");
> 			}
> 			symbol = symbol->next;
> 		}
>-- 
>2.16.4
>