Re: [PATCH v3 12/17] tools/xenstore: don't let hashtable_remove() return the removed value

[Julien Grall <julien@xxxxxxx>]

Hi Juergen,

On 17/01/2023 09:11, Juergen Gross wrote:
> Letting hashtable_remove() return the value of the removed element is
> not used anywhere in Xenstore, and it conflicts with a hashtable
> created specifying the HASHTABLE_FREE_VALUE flag.
>
> So just drop returning the value.
Any reason this can't be void? If there are, then I would consider to 
return a bool as the return can only be 2 values.
> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> ---
> V3:
> - new patch
> ---
>   tools/xenstore/hashtable.c | 10 +++++-----
>   tools/xenstore/hashtable.h |  4 ++--
>   2 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/tools/xenstore/hashtable.c b/tools/xenstore/hashtable.c
> index 299549c51e..6738719e47 100644
> --- a/tools/xenstore/hashtable.c
> +++ b/tools/xenstore/hashtable.c
> @@ -214,7 +214,7 @@ hashtable_search(struct hashtable *h, void *k)
>   }
>   
>   /*****************************************************************************/
> -void * /* returns value associated with key */
> +int
>   hashtable_remove(struct hashtable *h, void *k)
>   {
>       /* TODO: consider compacting the table when the load factor drops enough,
> @@ -222,7 +222,6 @@ hashtable_remove(struct hashtable *h, void *k)
>   
>       struct entry *e;
>       struct entry **pE;
> -    void *v;
>       unsigned int hashvalue, index;
>   
>       hashvalue = hash(h,k);
> @@ -236,16 +235,17 @@ hashtable_remove(struct hashtable *h, void *k)
>           {
>               *pE = e->next;
>               h->entrycount--;
> -            v = e->v;
>               if (h->flags & HASHTABLE_FREE_KEY)
>                   free(e->k);
> +            if (h->flags & HASHTABLE_FREE_VALUE)
> +                free(e->v);
I don't quite understand how this change is related to this patch.

>               free(e);
> -            return v;
> +            return 1;
>           }
>           pE = &(e->next);
>           e = e->next;
>       }
> -    return NULL;
> +    return 0;
>   }
>   
>   /*****************************************************************************/
> diff --git a/tools/xenstore/hashtable.h b/tools/xenstore/hashtable.h
> index 6d65431f96..d6feb1b038 100644
> --- a/tools/xenstore/hashtable.h
> +++ b/tools/xenstore/hashtable.h
> @@ -68,10 +68,10 @@ hashtable_search(struct hashtable *h, void *k);
>    * @name        hashtable_remove
>    * @param   h   the hashtable to remove the item from
>    * @param   k   the key to search for  - does not claim ownership
> - * @return      the value associated with the key, or NULL if none found
> + * @return      0 if element not found
>    */
>   
> -void * /* returns value */
> +int
>   hashtable_remove(struct hashtable *h, void *k);
>   
>   /*****************************************************************************
Cheers,

--
Julien Grall