[PATCH v2 04/13] tools/xenstore: let hashtable_insert() return 0 on success

Juergen Gross posted 13 patches 2 years, 10 months ago
There is a newer version of this series
[PATCH v2 04/13] tools/xenstore: let hashtable_insert() return 0 on success
Posted by Juergen Gross 2 years, 10 months ago
Today hashtable_insert() returns 0 in case of an error. Change that to
let it return an errno value in the error case and 0 in case of success.

Even if not used today, do the same switch for the return value of
hashtable_expand().

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/xenstore/hashtable.c             | 15 ++++++++++-----
 tools/xenstore/hashtable.h             |  2 +-
 tools/xenstore/xenstored_core.c        |  4 ++--
 tools/xenstore/xenstored_domain.c      |  4 ++--
 tools/xenstore/xenstored_transaction.c |  4 ++--
 5 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/tools/xenstore/hashtable.c b/tools/xenstore/hashtable.c
index ab1e687d0b..084d562b22 100644
--- a/tools/xenstore/hashtable.c
+++ b/tools/xenstore/hashtable.c
@@ -105,14 +105,15 @@ static int hashtable_expand(struct hashtable *h)
     struct entry **pE;
     unsigned int newsize, i, index;
     /* Check we're not hitting max capacity */
-    if (h->primeindex == (PRIME_TABLE_LEN - 1)) return 0;
+    if (h->primeindex == (PRIME_TABLE_LEN - 1))
+        return ENOSPC;
     newsize = primes[++(h->primeindex)];
 
     newtable = talloc_realloc(h, h->table, struct entry *, newsize);
     if (!newtable)
     {
         h->primeindex--;
-        return 0;
+        return ENOMEM;
     }
 
     h->table = newtable;
@@ -136,7 +137,7 @@ static int hashtable_expand(struct hashtable *h)
 
     h->tablelength = newsize;
     h->loadlimit   = loadlimit(h->primeindex);
-    return -1;
+    return 0;
 }
 
 int hashtable_insert(struct hashtable *h, void *k, void *v)
@@ -153,7 +154,11 @@ int hashtable_insert(struct hashtable *h, void *k, void *v)
         hashtable_expand(h);
     }
     e = talloc_zero(h, struct entry);
-    if (NULL == e) { --(h->entrycount); return 0; } /*oom*/
+    if (NULL == e)
+    {
+        --h->entrycount;
+       return ENOMEM;
+    }
     e->h = hash(h,k);
     index = indexFor(h->tablelength,e->h);
     e->k = k;
@@ -164,7 +169,7 @@ int hashtable_insert(struct hashtable *h, void *k, void *v)
         talloc_steal(e, v);
     e->next = h->table[index];
     h->table[index] = e;
-    return -1;
+    return 0;
 }
 
 void *hashtable_search(const struct hashtable *h, const void *k)
diff --git a/tools/xenstore/hashtable.h b/tools/xenstore/hashtable.h
index 0e1a6f61c2..ee4e25cbbf 100644
--- a/tools/xenstore/hashtable.h
+++ b/tools/xenstore/hashtable.h
@@ -36,7 +36,7 @@ create_hashtable(const void *ctx, const char *name,
  * @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      non-zero for successful insertion
+ * @return      zero for successful insertion
  *
  * This function will cause the table to expand if the insertion would take
  * the ratio of entries to table size over the maximum load factor.
diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 6ce7be3161..c5433e5b3f 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -2394,7 +2394,7 @@ int remember_string(struct hashtable *hash, const char *str)
 	char *k = talloc_strdup(NULL, str);
 
 	if (!k)
-		return 0;
+		return ENOMEM;
 	return hashtable_insert(hash, k, (void *)1);
 }
 
@@ -2428,7 +2428,7 @@ static int check_store_step(const void *ctx, struct connection *conn,
 				: WALK_TREE_SKIP_CHILDREN;
 	}
 
-	if (!remember_string(data->reachable, node->name))
+	if (remember_string(data->reachable, node->name))
 		return WALK_TREE_ERROR_STOP;
 
 	domain_check_acc_add(node, data->domains);
diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
index 6d40aefc63..be9175a461 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -540,7 +540,7 @@ static struct domain *alloc_domain(const void *context, unsigned int domid)
 	domain->generation = generation;
 	domain->introduced = false;
 
-	if (!hashtable_insert(domhash, &domain->domid, domain)) {
+	if (hashtable_insert(domhash, &domain->domid, domain)) {
 		talloc_free(domain);
 		errno = ENOMEM;
 		return NULL;
@@ -1792,7 +1792,7 @@ static int domain_check_acc_init_sub(const void *k, void *v, void *arg)
 	 */
 	dom->nodes = -d->acc[ACC_NODES].val;
 
-	if (!hashtable_insert(domains, &dom->domid, dom)) {
+	if (hashtable_insert(domains, &dom->domid, dom)) {
 		talloc_free(dom);
 		return -1;
 	}
diff --git a/tools/xenstore/xenstored_transaction.c b/tools/xenstore/xenstored_transaction.c
index e8968d7a1d..c615c0de13 100644
--- a/tools/xenstore/xenstored_transaction.c
+++ b/tools/xenstore/xenstored_transaction.c
@@ -600,13 +600,13 @@ int check_transactions(struct hashtable *hash)
 		list_for_each_entry(trans, &conn->transaction_list, list) {
 			tname = talloc_asprintf(trans, "%"PRIu64,
 						trans->generation);
-			if (!tname || !remember_string(hash, tname))
+			if (!tname || remember_string(hash, tname))
 				goto nomem;
 
 			list_for_each_entry(i, &trans->accessed, list) {
 				if (!i->ta_node)
 					continue;
-				if (!remember_string(hash, i->trans_name))
+				if (remember_string(hash, i->trans_name))
 					goto nomem;
 			}
 
-- 
2.35.3
Re: [PATCH v2 04/13] tools/xenstore: let hashtable_insert() return 0 on success
Posted by Julien Grall 2 years, 9 months ago
Hi,

On 30/03/2023 09:50, Juergen Gross wrote:
> Today hashtable_insert() returns 0 in case of an error. Change that to
> let it return an errno value in the error case and 0 in case of success.

I usually find such change risky because it makes the backport more 
complex if we introduce a new call to hashtable_insert() and it is also 
quite difficult to review (the compiler would not help to confirm all 
the callers have changed).

So can you provide a compelling reason for doing the change? 
(consistency would not be one IMO)

Cheers,

-- 
Julien Grall
Re: [PATCH v2 04/13] tools/xenstore: let hashtable_insert() return 0 on success
Posted by Juergen Gross 2 years, 9 months ago
On 03.05.23 15:03, Julien Grall wrote:
> Hi,
> 
> On 30/03/2023 09:50, Juergen Gross wrote:
>> Today hashtable_insert() returns 0 in case of an error. Change that to
>> let it return an errno value in the error case and 0 in case of success.
> 
> I usually find such change risky because it makes the backport more complex if 
> we introduce a new call to hashtable_insert() and it is also quite difficult to 
> review (the compiler would not help to confirm all the callers have changed).
> 
> So can you provide a compelling reason for doing the change? (consistency would 
> not be one IMO)

The motivation was consistency. :-)

The alternative would be to really set errno in the error case, which
would add additional lines.

I'm not really feeling strong here, BTW.


Juergen