Messages in this thread | | | Date | Thu, 5 Mar 2020 16:22:50 -0800 | From | Lucas De Marchi <> | Subject | Re: [PATCH] modpost: move the namespace field in Module.symvers last |
| |
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 >>
| |