Today check_store() is only testing the correctness of the node tree.

Add verification of the accounting data (number of nodes)  and correct
the data if it is wrong.

Do the initial check_store() call only after Xenstore entries of a
live update have been read.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/xenstore/xenstored_core.c   | 62 ++++++++++++++++------
 tools/xenstore/xenstored_domain.c | 86 +++++++++++++++++++++++++++++++
 tools/xenstore/xenstored_domain.h |  4 ++
 3 files changed, 137 insertions(+), 15 deletions(-)

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 3099077a86..e201f14053 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -2389,8 +2389,6 @@ void setup_structure(bool live_update)
 		manual_node("@introduceDomain", NULL);
 		domain_nbentry_fix(dom0_domid, 5, true);
 	}
-
-	check_store();
 }
 
 static unsigned int hash_from_key_fn(void *k)
@@ -2433,20 +2431,28 @@ int remember_string(struct hashtable *hash, const char *str)
  * As we go, we record each node in the given reachable hashtable.  These
  * entries will be used later in clean_store.
  */
+
+struct check_store_data {
+	struct hashtable *reachable;
+	struct hashtable *domains;
+};
+
 static int check_store_step(const void *ctx, struct connection *conn,
 			    struct node *node, void *arg)
 {
-	struct hashtable *reachable = arg;
+	struct check_store_data *data = arg;
 
-	if (hashtable_search(reachable, (void *)node->name)) {
+	if (hashtable_search(data->reachable, (void *)node->name)) {
 		log("check_store: '%s' is duplicated!", node->name);
 		return recovery ? WALK_TREE_RM_CHILDENTRY
 				: WALK_TREE_SKIP_CHILDREN;
 	}
 
-	if (!remember_string(reachable, node->name))
+	if (!remember_string(data->reachable, node->name))
 		return WALK_TREE_ERROR_STOP;
 
+	domain_check_acc_add(node, data->domains);
+
 	return WALK_TREE_OK;
 }
 
@@ -2496,37 +2502,61 @@ static int clean_store_(TDB_CONTEXT *tdb, TDB_DATA key, TDB_DATA val,
  * Given the list of reachable nodes, iterate over the whole store, and
  * remove any that were not reached.
  */
-static void clean_store(struct hashtable *reachable)
+static void clean_store(struct check_store_data *data)
 {
-	tdb_traverse(tdb_ctx, &clean_store_, reachable);
+	tdb_traverse(tdb_ctx, &clean_store_, data->reachable);
+	domain_check_acc(data->domains);
 }
 
+int check_store_path(const char *name, struct check_store_data *data)
+{
+	struct node *node;
+
+	node = read_node(NULL, NULL, name);
+	if (!node) {
+		log("check_store: error %d reading special node '%s'", errno,
+		    name);
+		return errno;
+	}
+
+	return check_store_step(NULL, NULL, node, data);
+}
 
 void check_store(void)
 {
-	struct hashtable *reachable;
 	struct walk_funcs walkfuncs = {
 		.enter = check_store_step,
 		.enoent = check_store_enoent,
 	};
+	struct check_store_data data;
 
 	/* Don't free values (they are all void *1) */
-	reachable = create_hashtable(NULL, 16, hash_from_key_fn, keys_equal_fn,
-				     HASHTABLE_FREE_KEY);
-	if (!reachable) {
+	data.reachable = create_hashtable(NULL, 16, hash_from_key_fn,
+					  keys_equal_fn, HASHTABLE_FREE_KEY);
+	if (!data.reachable) {
 		log("check_store: ENOMEM");
 		return;
 	}
 
+	data.domains = domain_check_acc_init();
+	if (!data.domains) {
+		log("check_store: ENOMEM");
+		goto out_hash;
+	}
+
 	log("Checking store ...");
-	if (walk_node_tree(NULL, NULL, "/", &walkfuncs, reachable)) {
+	if (walk_node_tree(NULL, NULL, "/", &walkfuncs, &data)) {
 		if (errno == ENOMEM)
 			log("check_store: ENOMEM");
-	} else if (!check_transactions(reachable))
-		clean_store(reachable);
+	} else if (!check_store_path("@introduceDomain", &data) &&
+		   !check_store_path("@releaseDomain", &data) &&
+		   !check_transactions(data.reachable))
+		clean_store(&data);
 	log("Checking store complete.");
 
-	hashtable_destroy(reachable);
+	hashtable_destroy(data.domains);
+ out_hash:
+	hashtable_destroy(data.reachable);
 }
 
 
@@ -2925,6 +2955,8 @@ int main(int argc, char *argv[])
 		lu_read_state();
 #endif
 
+	check_store();
+
 	/* Get ready to listen to the tools. */
 	initialize_fds(&sock_pollfd_idx, &timeout);
 
diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
index cb1f09c297..a3c4fb7f93 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -1573,6 +1573,92 @@ void read_state_connection(const void *ctx, const void *state)
 	read_state_buffered_data(ctx, conn, sc);
 }
 
+struct domain_acc {
+	unsigned int domid;
+	int nodes;
+};
+
+static int domain_check_acc_init_sub(const void *k, void *v, void *arg)
+{
+	struct hashtable *domains = arg;
+	struct domain *d = v;
+	struct domain_acc *dom;
+
+	dom = talloc_zero(NULL, struct domain_acc);
+	if (!dom)
+		return -1;
+
+	dom->domid = d->domid;
+	/*
+	 * Set the initial value to the negative one of the current domain.
+	 * If everything is correct incrementing the value for each node will
+	 * result in dom->nodes being 0 at the end.
+	 */
+	dom->nodes = -d->nbentry;
+
+	if (!hashtable_insert(domains, &dom->domid, dom)) {
+		talloc_free(dom);
+		return -1;
+	}
+
+	return 0;
+}
+
+struct hashtable *domain_check_acc_init(void)
+{
+	struct hashtable *domains;
+
+	domains = create_hashtable(NULL, 8, domhash_fn, domeq_fn,
+				   HASHTABLE_FREE_VALUE);
+	if (!domains)
+		return NULL;
+
+	if (hashtable_iterate(domhash, domain_check_acc_init_sub, domains)) {
+		hashtable_destroy(domains);
+		return NULL;
+	}
+
+	return domains;
+}
+
+void domain_check_acc_add(const struct node *node, struct hashtable *domains)
+{
+	struct domain_acc *dom;
+	unsigned int domid;
+
+	domid = node->perms.p[0].id;
+	dom = hashtable_search(domains, &domid);
+	if (!dom)
+		log("Node %s owned by unknown domain %u", node->name, domid);
+	else
+		dom->nodes++;
+}
+
+static int domain_check_acc_sub(const void *k, void *v, void *arg)
+{
+	struct domain_acc *dom = v;
+	struct domain *d;
+
+	if (!dom->nodes)
+		return 0;
+
+	log("Correct accounting data for domain %u: nodes are %d off",
+	    dom->domid, dom->nodes);
+
+	d = find_domain_struct(dom->domid);
+	if (!d)
+		return 0;
+
+	d->nbentry += dom->nodes;
+
+	return 0;
+}
+
+void domain_check_acc(struct hashtable *domains)
+{
+	hashtable_iterate(domains, domain_check_acc_sub, NULL);
+}
+
 /*
  * Local variables:
  *  mode: C
diff --git a/tools/xenstore/xenstored_domain.h b/tools/xenstore/xenstored_domain.h
index 22996e2576..dc4660861e 100644
--- a/tools/xenstore/xenstored_domain.h
+++ b/tools/xenstore/xenstored_domain.h
@@ -129,4 +129,8 @@ const char *dump_state_connections(FILE *fp);
 
 void read_state_connection(const void *ctx, const void *state);
 
+struct hashtable *domain_check_acc_init(void);
+void domain_check_acc_add(const struct node *node, struct hashtable *domains);
+void domain_check_acc(struct hashtable *domains);
+
 #endif /* _XENSTORED_DOMAIN_H */
-- 
2.35.3

Hi Juergen,

On 17/01/2023 09:11, Juergen Gross wrote:
> Today check_store() is only testing the correctness of the node tree.
> 
> Add verification of the accounting data (number of nodes)  and correct

NIT: one too many space before 'and'.

> the data if it is wrong.
> 
> Do the initial check_store() call only after Xenstore entries of a
> live update have been read.

Can you clarify whether this is needed for the rest of the patch, or 
simply a nice thing to have in general?

> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>   tools/xenstore/xenstored_core.c   | 62 ++++++++++++++++------
>   tools/xenstore/xenstored_domain.c | 86 +++++++++++++++++++++++++++++++
>   tools/xenstore/xenstored_domain.h |  4 ++
>   3 files changed, 137 insertions(+), 15 deletions(-)
> 
> diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
> index 3099077a86..e201f14053 100644
> --- a/tools/xenstore/xenstored_core.c
> +++ b/tools/xenstore/xenstored_core.c
> @@ -2389,8 +2389,6 @@ void setup_structure(bool live_update)
>   		manual_node("@introduceDomain", NULL);
>   		domain_nbentry_fix(dom0_domid, 5, true);
>   	}
> -
> -	check_store();
>   }
>   
>   static unsigned int hash_from_key_fn(void *k)
> @@ -2433,20 +2431,28 @@ int remember_string(struct hashtable *hash, const char *str)
>    * As we go, we record each node in the given reachable hashtable.  These
>    * entries will be used later in clean_store.
>    */
> +
> +struct check_store_data {
> +	struct hashtable *reachable;
> +	struct hashtable *domains;
> +};
> +
>   static int check_store_step(const void *ctx, struct connection *conn,
>   			    struct node *node, void *arg)
>   {
> -	struct hashtable *reachable = arg;
> +	struct check_store_data *data = arg;
>   
> -	if (hashtable_search(reachable, (void *)node->name)) {
> +	if (hashtable_search(data->reachable, (void *)node->name)) {

IIUC the cast is only necessary because hashtable_search() expects a 
non-const value. I can't think for a reason for the key to be non-const. 
So I will look to send a follow-up patch.

> +
> +void domain_check_acc_add(const struct node *node, struct hashtable *domains)
> +{
> +	struct domain_acc *dom;
> +	unsigned int domid;
> +
> +	domid = node->perms.p[0].id;

This could be replaced with get_node_owner().

> +	dom = hashtable_search(domains, &domid);
> +	if (!dom)
> +		log("Node %s owned by unknown domain %u", node->name, domid);
> +	else
> +		dom->nodes++;
> +}
> +
> +static int domain_check_acc_sub(const void *k, void *v, void *arg)

I find the suffix 'sub' misleading because we have a function with a 
very similar name (instead suffixed 'sub'). Yet, AFAICT, it is not meant 
to substract.

So I would prefix with '_cb' instead.

Cheers,

-- 
Julien Grall

On 17.01.23 23:36, Julien Grall wrote:
> Hi Juergen,
> 
> On 17/01/2023 09:11, Juergen Gross wrote:
>> Today check_store() is only testing the correctness of the node tree.
>>
>> Add verification of the accounting data (number of nodes)  and correct
> 
> NIT: one too many space before 'and'.
> 
>> the data if it is wrong.
>>
>> Do the initial check_store() call only after Xenstore entries of a
>> live update have been read.
> 
> Can you clarify whether this is needed for the rest of the patch, or simply a 
> nice thing to have in general?

I'll add: "This is wanted to make sure the accounting data is correct after
a live update."

> 
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>   tools/xenstore/xenstored_core.c   | 62 ++++++++++++++++------
>>   tools/xenstore/xenstored_domain.c | 86 +++++++++++++++++++++++++++++++
>>   tools/xenstore/xenstored_domain.h |  4 ++
>>   3 files changed, 137 insertions(+), 15 deletions(-)
>>
>> diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
>> index 3099077a86..e201f14053 100644
>> --- a/tools/xenstore/xenstored_core.c
>> +++ b/tools/xenstore/xenstored_core.c
>> @@ -2389,8 +2389,6 @@ void setup_structure(bool live_update)
>>           manual_node("@introduceDomain", NULL);
>>           domain_nbentry_fix(dom0_domid, 5, true);
>>       }
>> -
>> -    check_store();
>>   }
>>   static unsigned int hash_from_key_fn(void *k)
>> @@ -2433,20 +2431,28 @@ int remember_string(struct hashtable *hash, const char 
>> *str)
>>    * As we go, we record each node in the given reachable hashtable.  These
>>    * entries will be used later in clean_store.
>>    */
>> +
>> +struct check_store_data {
>> +    struct hashtable *reachable;
>> +    struct hashtable *domains;
>> +};
>> +
>>   static int check_store_step(const void *ctx, struct connection *conn,
>>                   struct node *node, void *arg)
>>   {
>> -    struct hashtable *reachable = arg;
>> +    struct check_store_data *data = arg;
>> -    if (hashtable_search(reachable, (void *)node->name)) {
>> +    if (hashtable_search(data->reachable, (void *)node->name)) {
> 
> IIUC the cast is only necessary because hashtable_search() expects a non-const 
> value. I can't think for a reason for the key to be non-const. So I will look to 
> send a follow-up patch.

It is possible, but nasty, due to talloc_free() not taking a const pointer.

> 
>> +
>> +void domain_check_acc_add(const struct node *node, struct hashtable *domains)
>> +{
>> +    struct domain_acc *dom;
>> +    unsigned int domid;
>> +
>> +    domid = node->perms.p[0].id;
> 
> This could be replaced with get_node_owner().

Indeed.

> 
>> +    dom = hashtable_search(domains, &domid);
>> +    if (!dom)
>> +        log("Node %s owned by unknown domain %u", node->name, domid);
>> +    else
>> +        dom->nodes++;
>> +}
>> +
>> +static int domain_check_acc_sub(const void *k, void *v, void *arg)
> 
> I find the suffix 'sub' misleading because we have a function with a very 
> similar name (instead suffixed 'sub'). Yet, AFAICT, it is not meant to substract.
> 
> So I would prefix with '_cb' instead.

Fine with me.


Juergen

Hi Juergen,

On 18/01/2023 06:23, Juergen Gross wrote:
> On 17.01.23 23:36, Julien Grall wrote:
>> Hi Juergen,
>>
>> On 17/01/2023 09:11, Juergen Gross wrote:
>>> Today check_store() is only testing the correctness of the node tree.
>>>
>>> Add verification of the accounting data (number of nodes)  and correct
>>
>> NIT: one too many space before 'and'.
>>
>>> the data if it is wrong.
>>>
>>> Do the initial check_store() call only after Xenstore entries of a
>>> live update have been read.
>>
>> Can you clarify whether this is needed for the rest of the patch, or 
>> simply a nice thing to have in general?
> 
> I'll add: "This is wanted to make sure the accounting data is correct after
> a live update."

Fine with me.

> 
>>
>>>
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>> ---
>>>   tools/xenstore/xenstored_core.c   | 62 ++++++++++++++++------
>>>   tools/xenstore/xenstored_domain.c | 86 +++++++++++++++++++++++++++++++
>>>   tools/xenstore/xenstored_domain.h |  4 ++
>>>   3 files changed, 137 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/tools/xenstore/xenstored_core.c 
>>> b/tools/xenstore/xenstored_core.c
>>> index 3099077a86..e201f14053 100644
>>> --- a/tools/xenstore/xenstored_core.c
>>> +++ b/tools/xenstore/xenstored_core.c
>>> @@ -2389,8 +2389,6 @@ void setup_structure(bool live_update)
>>>           manual_node("@introduceDomain", NULL);
>>>           domain_nbentry_fix(dom0_domid, 5, true);
>>>       }
>>> -
>>> -    check_store();
>>>   }
>>>   static unsigned int hash_from_key_fn(void *k)
>>> @@ -2433,20 +2431,28 @@ int remember_string(struct hashtable *hash, 
>>> const char *str)
>>>    * As we go, we record each node in the given reachable hashtable.  
>>> These
>>>    * entries will be used later in clean_store.
>>>    */
>>> +
>>> +struct check_store_data {
>>> +    struct hashtable *reachable;
>>> +    struct hashtable *domains;
>>> +};
>>> +
>>>   static int check_store_step(const void *ctx, struct connection *conn,
>>>                   struct node *node, void *arg)
>>>   {
>>> -    struct hashtable *reachable = arg;
>>> +    struct check_store_data *data = arg;
>>> -    if (hashtable_search(reachable, (void *)node->name)) {
>>> +    if (hashtable_search(data->reachable, (void *)node->name)) {
>>
>> IIUC the cast is only necessary because hashtable_search() expects a 
>> non-const value. I can't think for a reason for the key to be 
>> non-const. So I will look to send a follow-up patch.
> 
> It is possible, but nasty, due to talloc_free() not taking a const pointer.

I am not sure I understand your reasoning. Looking at 
hashtable_search(), I don't see a call to talloc_free().

Anyway, this is not directly related to this patch. So I will have a 
look separately.

Cheers,

-- 
Julien Grall

On 18.01.23 10:35, Julien Grall wrote:
> Hi Juergen,
> 
> On 18/01/2023 06:23, Juergen Gross wrote:
>> On 17.01.23 23:36, Julien Grall wrote:
>>> Hi Juergen,
>>>
>>> On 17/01/2023 09:11, Juergen Gross wrote:
>>>> Today check_store() is only testing the correctness of the node tree.
>>>>
>>>> Add verification of the accounting data (number of nodes)  and correct
>>>
>>> NIT: one too many space before 'and'.
>>>
>>>> the data if it is wrong.
>>>>
>>>> Do the initial check_store() call only after Xenstore entries of a
>>>> live update have been read.
>>>
>>> Can you clarify whether this is needed for the rest of the patch, or simply a 
>>> nice thing to have in general?
>>
>> I'll add: "This is wanted to make sure the accounting data is correct after
>> a live update."
> 
> Fine with me.
> 
>>
>>>
>>>>
>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>> ---
>>>>   tools/xenstore/xenstored_core.c   | 62 ++++++++++++++++------
>>>>   tools/xenstore/xenstored_domain.c | 86 +++++++++++++++++++++++++++++++
>>>>   tools/xenstore/xenstored_domain.h |  4 ++
>>>>   3 files changed, 137 insertions(+), 15 deletions(-)
>>>>
>>>> diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
>>>> index 3099077a86..e201f14053 100644
>>>> --- a/tools/xenstore/xenstored_core.c
>>>> +++ b/tools/xenstore/xenstored_core.c
>>>> @@ -2389,8 +2389,6 @@ void setup_structure(bool live_update)
>>>>           manual_node("@introduceDomain", NULL);
>>>>           domain_nbentry_fix(dom0_domid, 5, true);
>>>>       }
>>>> -
>>>> -    check_store();
>>>>   }
>>>>   static unsigned int hash_from_key_fn(void *k)
>>>> @@ -2433,20 +2431,28 @@ int remember_string(struct hashtable *hash, const 
>>>> char *str)
>>>>    * As we go, we record each node in the given reachable hashtable. These
>>>>    * entries will be used later in clean_store.
>>>>    */
>>>> +
>>>> +struct check_store_data {
>>>> +    struct hashtable *reachable;
>>>> +    struct hashtable *domains;
>>>> +};
>>>> +
>>>>   static int check_store_step(const void *ctx, struct connection *conn,
>>>>                   struct node *node, void *arg)
>>>>   {
>>>> -    struct hashtable *reachable = arg;
>>>> +    struct check_store_data *data = arg;
>>>> -    if (hashtable_search(reachable, (void *)node->name)) {
>>>> +    if (hashtable_search(data->reachable, (void *)node->name)) {
>>>
>>> IIUC the cast is only necessary because hashtable_search() expects a 
>>> non-const value. I can't think for a reason for the key to be non-const. So I 
>>> will look to send a follow-up patch.
>>
>> It is possible, but nasty, due to talloc_free() not taking a const pointer.
> 
> I am not sure I understand your reasoning. Looking at hashtable_search(), I 
> don't see a call to talloc_free().

Oh, I thought you were referring to the key in general.


Juergen