Re: [Crash-utility] [PATCH v4 4/4] Add check if an syment element is installed one more time

Hello Lianbo and Philipp, syment_is_installed is used to check if the specific syment has been installed before. It checks whether the syment address and the given address are the same. symname_hash_search is used to search the syment which has the given name. It checks whether the name field of the syment and the given name are the same.

Please note the hash table can have more than 1 syments which have the same name but differ in address. What We want to avoid is the case if a syment has been installed before, it should never be installed again. So here we care about syment address, instead of syment name.

Like Philipp said, if we use symname_hash_search to implement syment_is_installed, we will use extra code to check the address of symnet returned by symname_hash_search. The code will not be as clear as we directly use syment_is_installed. Thanks, Tao Liu On Mon, Sep 27, 2021 at 5:54 PM Philipp Rudo <prudo(a)redhat.com&gt; wrote: > > On Mon, 27 Sep 2021 12:00:26 +0800 > lijiang <lijiang(a)redhat.com&gt; wrote: > > > > Date: Sat, 18 Sep 2021 15:59:32 +0800 > > > From: Tao Liu <ltao(a)redhat.com&gt; > > > To: crash-utility(a)redhat.com > > > Subject: [Crash-utility] [PATCH v4 4/4] Add check if an syment

> > > is installed one more time > > > Message-ID: <20210918075932.132339-5-ltao(a)redhat.com&gt; > > > Content-Type: text/plain; charset="US-ASCII" > > > > > > symname_hash_install won't check if spn has been installed before.

> > > it does, the second install will corrupt the hash table as well as > > > spn->cnt counting. This patch adds the check to avoid such risks. > > > > > > Signed-off-by: Tao Liu <ltao(a)redhat.com&gt; > > > --- > > > symbols.c | 16 +++++++++++++++- > > > 1 file changed, 15 insertions(+), 1 deletion(-) > > > > > > diff --git a/symbols.c b/symbols.c > > > index f7157b1..6d12c55 100644 > > > --- a/symbols.c > > > +++ b/symbols.c > > > @@ -1147,6 +1147,20 @@ mod_symtable_hash_remove_range(struct syment

> > > symname_hash_remove(st->mod_symname_hash, sp); > > > } > > > > > > +static inline int > > > +syment_is_installed(struct syment *table[], struct syment *spn) > > > +{ > > > + struct syment *sp; > > > + int index; > > > + > > > + index = SYMNAME_HASH_INDEX(spn->name); > > > + for (sp = table[index]; sp; sp = sp->name_hash_next) { > > > + if (sp == spn) > > > + return TRUE; > > > + } > > > + return FALSE; > > > +} > > > + > > > /* > > > * Install a single static kernel symbol into the symname_hash. > > > */ > > > @@ -1156,7 +1170,7 @@ symname_hash_install(struct syment *table[],

> > > struct syment *sp; > > > int index; > > > > > > - if (!spn) > > > + if (!spn || syment_is_installed(table, spn)) > > > return; > > > > Here, why don't we call the existing symname_hash_search() and > > redefine a new syment_is_installed()? Could you please describe more > > details? > > I thought about that, too. In my opinion the problem with reusing > symname_hash_search is that it searches for symbols with the same name. > But the name doesn't necessarily have to be unique. So when > symname_hash_search returns a symbol you still need to check if it is > the same symbol and if not iterate through all symbols with the same > name to check if any of them is. In the end I think that code will be > more complex than having this additional syment_is_installed. > > Thanks > Philipp > > > > > Thanks. > > Lianbo > > > > > > > > index = SYMNAME_HASH_INDEX(spn->name); > > > -- > > > 2.29.2 > > > > -- > > Crash-utility mailing list > > Crash-utility(a)redhat.com > > https://listman.redhat.com/mailman/listinfo/crash-utility > > > > -- > Crash-utility mailing list > Crash-utility(a)redhat.com > https://listman.redhat.com/mailman/listinfo/crash-utility >