| Message ID | 20230117091124.22170-13-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tools/xenstore: do some cleanup and fixes | expand |
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@suse.com> > --- > 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,
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@suse.com> >> --- >> 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
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@suse.com> >>> --- >>> 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,
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); 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); /*****************************************************************************
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. Signed-off-by: Juergen Gross <jgross@suse.com> --- V3: - new patch --- tools/xenstore/hashtable.c | 10 +++++----- tools/xenstore/hashtable.h | 4 ++-- 2 files changed, 7 insertions(+), 7 deletions(-)