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

[Julien Grall <julien@xxxxxxx>]

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.
> 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 ','.

> +    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 (....)

> +        /* 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?
> +    return 0;
>   }
>   
>   void
> diff --git a/tools/xenstore/hashtable.h b/tools/xenstore/hashtable.h
> index 792f6cda7b..214aea1b3d 100644
> --- a/tools/xenstore/hashtable.h
> +++ b/tools/xenstore/hashtable.h
> @@ -51,6 +51,22 @@ create_hashtable(const void *ctx, const char *name,
>   int
>   hashtable_add(struct hashtable *h, const void *k, void *v);
>   
> +/*****************************************************************************
> + * hashtable_replace
> +
> + * @name        hashtable_nsert
> + * @param   h   the hashtable to insert into
> + * @param   k   the key - hashtable claims ownership and will free on removal
> + * @param   v   the value - does not claim ownership
> + * @return      zero for successful insertion
> + *
> + * This function does check for an entry being present before replacing it
> + * with a new value.
> + */
> +
> +int
> +hashtable_replace(struct hashtable *h, const void *k, void *v);
> +
>   /*****************************************************************************
>    * hashtable_search
>      
--
Julien Grall