| Message ID | 20191010151443.7399-2-maennich@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | export/modpost: avoid renaming __ksymtab entries for symbol namespaces | expand |
On Thu, Oct 10, 2019 at 04:14:40PM +0100, Matthias Maennich wrote: > Let the function 'sym_update_namespace' take care of updating the > namespace for a symbol. While this currently only replaces one single > location where namespaces are updated, in a following patch, this > function will get more call sites. > > The function signature is intentionally close to sym_update_crc and > taking the name by char* seems like unnecessary work as the symbol has > to be looked up again. In a later patch of this series, this concern > will be addressed. > > This function ensures that symbol::namespace is either NULL or has a > valid non-empty value. Previously, the empty string was considered 'no > namespace' as well and this lead to confusion. > > Signed-off-by: Matthias Maennich <maennich@google.com> > --- > scripts/mod/modpost.c | 21 ++++++++++++++++++--- > 1 file changed, 18 insertions(+), 3 deletions(-) > > diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c > index 4d2cdb4d71e3..9f5dcdff4d2f 100644 > --- a/scripts/mod/modpost.c > +++ b/scripts/mod/modpost.c > @@ -362,6 +362,22 @@ static char *sym_extract_namespace(const char **symname) > return namespace; > } > > +static void sym_update_namespace(const char *symname, const char *namespace) > +{ > + struct symbol *s = find_symbol(symname); > + /* That symbol should have been created earlier and thus this is > + * actually an assertion. */ > + if (!s) { > + merror("Could not update namespace(%s) for symbol %s\n", > + namespace, symname); > + return; > + } > + > + free(s->namespace); > + s->namespace = > + namespace && namespace[0] ? NOFAIL(strdup(namespace)) : NULL; > +} You made me look up C operator precedence again, but it's fine so: Acked-by: Will Deacon <will@kernel.org> Will
On Thu, Oct 10, 2019 at 04:14:40PM +0100, Matthias Maennich wrote: > Let the function 'sym_update_namespace' take care of updating the > namespace for a symbol. While this currently only replaces one single > location where namespaces are updated, in a following patch, this > function will get more call sites. > > The function signature is intentionally close to sym_update_crc and > taking the name by char* seems like unnecessary work as the symbol has > to be looked up again. In a later patch of this series, this concern > will be addressed. > > This function ensures that symbol::namespace is either NULL or has a > valid non-empty value. Previously, the empty string was considered 'no > namespace' as well and this lead to confusion. > > Signed-off-by: Matthias Maennich <maennich@google.com> > --- > scripts/mod/modpost.c | 21 ++++++++++++++++++--- > 1 file changed, 18 insertions(+), 3 deletions(-) > > diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c > index 4d2cdb4d71e3..9f5dcdff4d2f 100644 > --- a/scripts/mod/modpost.c > +++ b/scripts/mod/modpost.c > @@ -362,6 +362,22 @@ static char *sym_extract_namespace(const char **symname) > return namespace; > } > > +static void sym_update_namespace(const char *symname, const char *namespace) > +{ > + struct symbol *s = find_symbol(symname); > + /* That symbol should have been created earlier and thus this is > + * actually an assertion. */ Do we care about checkpatch issues in tools? If so, you need a blank line before the comment :) Anyway, not a big deal Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
On Sat, Oct 12, 2019 at 12:33 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Thu, Oct 10, 2019 at 04:14:40PM +0100, Matthias Maennich wrote: > > Let the function 'sym_update_namespace' take care of updating the > > namespace for a symbol. While this currently only replaces one single > > location where namespaces are updated, in a following patch, this > > function will get more call sites. > > > > The function signature is intentionally close to sym_update_crc and > > taking the name by char* seems like unnecessary work as the symbol has > > to be looked up again. In a later patch of this series, this concern > > will be addressed. > > > > This function ensures that symbol::namespace is either NULL or has a > > valid non-empty value. Previously, the empty string was considered 'no > > namespace' as well and this lead to confusion. > > > > Signed-off-by: Matthias Maennich <maennich@google.com> > > --- > > scripts/mod/modpost.c | 21 ++++++++++++++++++--- > > 1 file changed, 18 insertions(+), 3 deletions(-) > > > > diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c > > index 4d2cdb4d71e3..9f5dcdff4d2f 100644 > > --- a/scripts/mod/modpost.c > > +++ b/scripts/mod/modpost.c > > @@ -362,6 +362,22 @@ static char *sym_extract_namespace(const char **symname) > > return namespace; > > } > > > > +static void sym_update_namespace(const char *symname, const char *namespace) > > +{ > > + struct symbol *s = find_symbol(symname); > > + /* That symbol should have been created earlier and thus this is > > + * actually an assertion. */ > > Do we care about checkpatch issues in tools? Personally, I do. > > If so, you need a blank line before the comment :) One more minor issue, the block comment style is not correct. Please do like this: /* * Blah Blah ... * Blah Blha ... */ With those addressed, Reviewed-by: Masahiro Yamada <yamada.masahiro@socionext.com> > > Anyway, not a big deal > > Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c index 4d2cdb4d71e3..9f5dcdff4d2f 100644 --- a/scripts/mod/modpost.c +++ b/scripts/mod/modpost.c @@ -362,6 +362,22 @@ static char *sym_extract_namespace(const char **symname) return namespace; } +static void sym_update_namespace(const char *symname, const char *namespace) +{ + struct symbol *s = find_symbol(symname); + /* That symbol should have been created earlier and thus this is + * actually an assertion. */ + if (!s) { + merror("Could not update namespace(%s) for symbol %s\n", + namespace, symname); + return; + } + + free(s->namespace); + s->namespace = + namespace && namespace[0] ? NOFAIL(strdup(namespace)) : NULL; +} + /** * Add an exported symbol - it may have already been added without a * CRC, in this case just update the CRC @@ -383,8 +399,7 @@ static struct symbol *sym_add_exported(const char *name, const char *namespace, s->module = mod; } } - free(s->namespace); - s->namespace = namespace ? strdup(namespace) : NULL; + sym_update_namespace(name, namespace); s->preloaded = 0; s->vmlinux = is_vmlinux(mod->name); s->kernel = 0; @@ -2196,7 +2211,7 @@ static int check_exports(struct module *mod) else basename = mod->name; - if (exp->namespace && exp->namespace[0]) { + if (exp->namespace) { add_namespace(&mod->required_namespaces, exp->namespace);
Let the function 'sym_update_namespace' take care of updating the namespace for a symbol. While this currently only replaces one single location where namespaces are updated, in a following patch, this function will get more call sites. The function signature is intentionally close to sym_update_crc and taking the name by char* seems like unnecessary work as the symbol has to be looked up again. In a later patch of this series, this concern will be addressed. This function ensures that symbol::namespace is either NULL or has a valid non-empty value. Previously, the empty string was considered 'no namespace' as well and this lead to confusion. Signed-off-by: Matthias Maennich <maennich@google.com> --- scripts/mod/modpost.c | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-)