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

[Julien Grall <julien@xxxxxxx>]

On 18/01/2023 06:17, Juergen Gross wrote:
> 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.
That makes sense now. Thanks for the explanation.

> I can add a sentence to the commit message.
Yes please.

The rest of this patch looks good to me.

Cheers,

--
Julien Grall