Re: [PATCH 09/11] tools/xenstore: add hashtable_replace() function

[Juergen Gross]

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 <jgr...@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

  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;
+

@@ -176,17 +197,28 @@ int hashtable_add(struct hashtable *h, const void *k, void *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

OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

OpenPGP_signature
Description: OpenPGP digital signature