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

[Juergen Gross <jgross@xxxxxxxx>]

On 17.01.23 23:03, Julien Grall wrote:
> 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.
I think you are right. Switching to void should be fine.

> > 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.
With not returning the value pointer any longer there would be no way
for the caller to free it, so it must be freed by hashtable_remove()
if the related flag was set.

I can add a sentence to the commit message.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature