Re: [PATCH 09/11] tools/xenstore: add hashtable_replace() function

[Juergen Gross <jgross@xxxxxxxx>]

On 20.06.23 14:43, Julien Grall wrote:
> Hi Juergen,
>
> On 30/05/2023 10:13, Juergen Gross wrote:
> > For an effective way to replace a hashtable entry add a new function
> > hashtable_replace().
> >
> > While at it let hashtable_add() fail if an entry with the specified
> > key does already exist.
> Please explain why you want to do it. I also think this change should be in its 
> own patch.
Okay to both.

> > This is in preparation to replace TDB with a more simple data storage.
> >
> > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> > ---
> >   tools/xenstore/hashtable.c | 52 ++++++++++++++++++++++++++++++--------
> >   tools/xenstore/hashtable.h | 16 ++++++++++++
> >   2 files changed, 58 insertions(+), 10 deletions(-)
> >
> > diff --git a/tools/xenstore/hashtable.c b/tools/xenstore/hashtable.c
> > index 9daddd9782..f358bec5ae 100644
> > --- a/tools/xenstore/hashtable.c
> > +++ b/tools/xenstore/hashtable.c
> > @@ -141,11 +141,32 @@ static int hashtable_expand(struct hashtable *h)
> >       return 0;
> >   }
> > +static struct entry *hashtable_search_entry(const struct hashtable *h,
> > +                        const void *k)
> > +{
> > +    struct entry *e;
> > +    unsigned int hashvalue, index;
> > +
> > +    hashvalue = hash(h, k);
> > +    index = indexFor(h->tablelength,hashvalue);
> Missing space after ','.
Thanks for noticing.

> > +    e = h->table[index];
> > +    while (NULL != e)
> > +    {
> While you are moving the code. I think you can reduce the code size with:
>
> for ( e = h->table[index]; e != NULL; e = e->next )
>     if (....)
Will change it.

> > +        /* Check hash value to short circuit heavier comparison */
> > +        if ((hashvalue == e->h) && (h->eqfn(k, e->k))) return e;
> > +        e = e->next;
> > +    }
> > +    return NULL;
> > +}
> > +
> >   int hashtable_add(struct hashtable *h, const void *k, void *v)
> >   {
> > -    /* This method allows duplicate keys - but they shouldn't be used */
> >       unsigned int index;
> >       struct entry *e;
> > +
> > +    if (hashtable_search_entry(h, k))
> > +        return EEXIST;
> > +
> >       if (++(h->entrycount) > h->loadlimit)
> >       {
> >           /* Ignore the return value. If expand fails, we should
> > @@ -176,17 +197,28 @@ int hashtable_add(struct hashtable *h, const void *k, 
> > void *v)
> >   void *hashtable_search(const struct hashtable *h, const void *k)
> >   {
> >       struct entry *e;
> > -    unsigned int hashvalue, index;
> > -    hashvalue = hash(h,k);
> > -    index = indexFor(h->tablelength,hashvalue);
> > -    e = h->table[index];
> > -    while (NULL != e)
> > +
> > +    e = hashtable_search_entry(h, k);
> > +    return e ? e->v : NULL;
> > +}
> > +
> > +int hashtable_replace(struct hashtable *h, const void *k, void *v)
> > +{
> > +    struct entry *e;
> > +
> > +    e = hashtable_search_entry(h, k);
> > +    if (!e)
> > +        return ENOENT;
> > +
> > +    if (h->flags & HASHTABLE_FREE_VALUE)
> >       {
> > -        /* Check hash value to short circuit heavier comparison */
> > -        if ((hashvalue == e->h) && (h->eqfn(k, e->k))) return e->v;
> > -        e = e->next;
> > +        talloc_free(e->v);
> > +        talloc_steal(e, v);
> >       }
> > -    return NULL;
> > +
> > +    e->v = v;
> > +
> Coding style: Above you don't add a newline before return but here you do. I 
> don't particularly care which one you want to use in xenstored. But can you at 
> least be consistent?
Okay.

> > +    return 0;

Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature