| Message ID | 20230530091333.7678-10-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tools/xenstore: drop TDB | expand |
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@suse.com> > --- > 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 >
On 20.06.23 14:43, Julien Grall wrote: > 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. Okay to both. > >> >> This is in preparation to replace TDB with a more simple data storage. >> >> Signed-off-by: Juergen Gross <jgross@suse.com> >> --- >> 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 ','. Thanks for noticing. > >> + 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 (....) Will change it. > >> + /* 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? Okay. > >> + return 0; Juergen
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); + e = h->table[index]; + while (NULL != e) + { + /* 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; + + 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
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. This is in preparation to replace TDB with a more simple data storage. Signed-off-by: Juergen Gross <jgross@suse.com> --- tools/xenstore/hashtable.c | 52 ++++++++++++++++++++++++++++++-------- tools/xenstore/hashtable.h | 16 ++++++++++++ 2 files changed, 58 insertions(+), 10 deletions(-)