Re: [PATCH v3 16/17] tools/xenstore: let check_store() check the accounting data

[Julien Grall <julien@xxxxxxx>]

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@xxxxxxxx>
> ---
>   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