Re: [PATCH v2 10/19] tools/xenstore: change per-domain node accounting interface

[Julien Grall <julien@xxxxxxx>]

Hi,

On 13/12/2022 16:00, Juergen Gross wrote:
> Rework the interface and the internals of the per-domain node
> accounting:
>
> - rename the functions to domain_nbentry_*() in order to better match
>    the related counter name
>
> - switch from node pointer to domid as interface, as all nodes have the
>    owner filled in
The downside is now you have may place open-coding 
"...->perms->p[0].id". IHMO this is making the code more complicated. So 
can you introduce a few wrappers that would take a node and then convert 
to the owner?
> - use a common internal function for adding a value to the counter
>
> For the transaction case add a helper function to get the list head
> of the per-transaction changed domains, enabling to eliminate the
> transaction_entry_*() functions.
>
> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> ---
>   tools/xenstore/xenstored_core.c        |  22 ++---
>   tools/xenstore/xenstored_domain.c      | 122 +++++++++++--------------
>   tools/xenstore/xenstored_domain.h      |  10 +-
>   tools/xenstore/xenstored_transaction.c |  15 +--
>   tools/xenstore/xenstored_transaction.h |   7 +-
>   5 files changed, 72 insertions(+), 104 deletions(-)
>
> diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
> index f96714e1b8..61569cecbb 100644
> --- a/tools/xenstore/xenstored_core.c
> +++ b/tools/xenstore/xenstored_core.c
> @@ -1459,7 +1459,7 @@ static void destroy_node_rm(struct connection *conn, 
> struct node *node)
>   static int destroy_node(struct connection *conn, struct node *node)
>   {
>         destroy_node_rm(conn, node);
> -       domain_entry_dec(conn, node);
> +       domain_nbentry_dec(conn, node->perms.p[0].id);
>   
>   	/*
>          * It is not possible to easily revert the changes in a transaction.
> @@ -1498,7 +1498,7 @@ static struct node *create_node(struct connection *conn, 
> const void *ctx,
>         for (i = node; i; i = i->parent) {
>                 /* i->parent is set for each new node, so check quota. */
>                 if (i->parent &&
> -                   domain_entry(conn) >= quota_nb_entry_per_domain) {
> +                   domain_nbentry(conn) >= quota_nb_entry_per_domain) {
>                         ret = ENOSPC;
>                         goto err;
>                 }
> @@ -1509,7 +1509,7 @@ static struct node *create_node(struct connection *conn, 
> const void *ctx,
>   
>   		/* Account for new node */
>                 if (i->parent) {
> -                       if (domain_entry_inc(conn, i)) {
> +                       if (domain_nbentry_inc(conn, i->perms.p[0].id)) {
>                                 destroy_node_rm(conn, i);
>                                 return NULL;
>                         }
> @@ -1662,7 +1662,7 @@ static int delnode_sub(const void *ctx, struct connection 
> *conn,
>         watch_exact = strcmp(root, node->name);
>         fire_watches(conn, ctx, node->name, node, watch_exact, NULL);
>   
> -	domain_entry_dec(conn, node);
> +       domain_nbentry_dec(conn, node->perms.p[0].id);
>   
>   	return WALK_TREE_RM_CHILDENTRY;
>   }
> @@ -1802,25 +1802,25 @@ static int do_set_perms(const void *ctx, struct 
> connection *conn,
>                 return EPERM;
>   
>   	old_perms = node->perms;
> -       domain_entry_dec(conn, node);
> +       domain_nbentry_dec(conn, node->perms.p[0].id);
>         node->perms = perms;
> -       if (domain_entry_inc(conn, node)) {
> +       if (domain_nbentry_inc(conn, node->perms.p[0].id)) {
>                 node->perms = old_perms;
>                 /*
>                  * This should never fail because we had a reference on the
>                  * domain before and Xenstored is single-threaded.
>                  */
> -               domain_entry_inc(conn, node);
> +               domain_nbentry_inc(conn, node->perms.p[0].id);
>                 return ENOMEM;
>         }
>   
>   	if (write_node(conn, node, false)) {
>                 int saved_errno = errno;
>   
> -		domain_entry_dec(conn, node);
> +               domain_nbentry_dec(conn, node->perms.p[0].id);
>                 node->perms = old_perms;
>                 /* No failure possible as above. */
> -               domain_entry_inc(conn, node);
> +               domain_nbentry_inc(conn, node->perms.p[0].id);
>   
>   		errno = saved_errno;
>                 return errno;
> @@ -2392,7 +2392,7 @@ void setup_structure(bool live_update)
>                 manual_node("/tool/xenstored", NULL);
>                 manual_node("@releaseDomain", NULL);
>                 manual_node("@introduceDomain", NULL);
> -               domain_entry_fix(dom0_domid, 5, true);
> +               domain_nbentry_fix(dom0_domid, 5, true);
>         }
>   
>   	check_store();
> @@ -3400,7 +3400,7 @@ void read_state_node(const void *ctx, const void *state)
>         if (write_node_raw(NULL, &key, node, true))
>                 barf("write node error restoring node");
>   
> -	if (domain_entry_inc(&conn, node))
> +       if (domain_nbentry_inc(&conn, node->perms.p[0].id))
>                 barf("node accounting error restoring node");
>   
>   	talloc_free(node);
> diff --git a/tools/xenstore/xenstored_domain.c 
> b/tools/xenstore/xenstored_domain.c
> index 3216119e83..40b24056c5 100644
> --- a/tools/xenstore/xenstored_domain.c
> +++ b/tools/xenstore/xenstored_domain.c
> @@ -249,7 +249,7 @@ static int domain_tree_remove_sub(const void *ctx, struct 
> connection *conn,
>                 domain->nbentry--;
>                 node->perms.p[0].id = priv_domid;
>                 node->acc.memory = 0;
> -               domain_entry_inc(NULL, node);
> +               domain_nbentry_inc(NULL, priv_domid);
>                 if (write_node_raw(NULL, &key, node, true)) {
>                         /* That's unfortunate. We only can try to continue. */
>                         syslog(LOG_ERR,
> @@ -559,7 +559,7 @@ int acc_fix_domains(struct list_head *head, bool update)
>         int cnt;
>   
>   	list_for_each_entry(cd, head, list) {
> -               cnt = domain_entry_fix(cd->domid, cd->nbentry, update);
> +               cnt = domain_nbentry_fix(cd->domid, cd->nbentry, update);
>                 if (!update) {
>                         if (cnt >= quota_nb_entry_per_domain)
>                                 return ENOSPC;
> @@ -604,18 +604,19 @@ static struct changed_domain 
> *acc_get_changed_domain(const void *ctx,
>         return cd;
>   }
>   
> -int acc_add_dom_nbentry(const void *ctx, struct list_head *head, int val,
> -                       unsigned int domid)
> +static int acc_add_dom_nbentry(const void *ctx, struct list_head *head, int 
> val,
> +                              unsigned int domid)
>   {
>         struct changed_domain *cd;
>   
>   	cd = acc_get_changed_domain(ctx, head, domid);
>         if (!cd)
> -               return errno;
> +               return 0;
>   
> +	errno = 0;
>         cd->nbentry += val;
>   
> -	return 0;
> +       return cd->nbentry;
You just introduced this helper in the previous patch (i.e. #9). So can 
you get the interface correct from the start? This will make easier to 
review the series.
I don't mind too much if you add the static here. Although, it would 
have been nice if we avoid changing code just introduced.
>   }
>   
>   static void domain_conn_reset(struct domain *domain)
> @@ -988,30 +989,6 @@ void domain_deinit(void)
>                 xenevtchn_unbind(xce_handle, virq_port);
>   }
>   
> -int domain_entry_inc(struct connection *conn, struct node *node)
> -{
> -       struct domain *d;
> -       unsigned int domid;
> -
> -       if (!node->perms.p)
> -               return 0;
> -
> -       domid = node->perms.p[0].id;
> -
> -       if (conn && conn->transaction) {
> -               transaction_entry_inc(conn->transaction, domid);
> -       } else {
> -               d = (conn && domid == conn->id && conn->domain) ? conn->domain
> -                   : find_or_alloc_existing_domain(domid);
> -               if (d)
> -                       d->nbentry++;
> -               else
> -                       return ENOMEM;
> -       }
> -
> -       return 0;
> -}
> -
>   /*
>    * Check whether a domain was created before or after a specific generation
>    * count (used for testing whether a node permission is older than a domain).
> @@ -1079,62 +1056,67 @@ int domain_adjust_node_perms(struct node *node)
>         return 0;
>   }
>   
> -void domain_entry_dec(struct connection *conn, struct node *node)
> +static int domain_nbentry_add(struct connection *conn, unsigned int domid,
> +                             int add, bool dom_exists)
The name of the variable suggests that that if it is false then it 
doesn't exists. However, looking at how you use it, it is more a "Can 
struct domain be allocated?". So I would rename it to 
"dom_alloc_allowed" or similar.
>   {
>         struct domain *d;
> -       unsigned int domid;
> -
> -       if (!node->perms.p)
> -               return;
> +       struct list_head *head;
> +       int ret;
>   
> -	domid = node->perms.p ? node->perms.p[0].id : conn->id;
> +       if (conn && domid == conn->id && conn->domain)
> +               d = conn->domain;
> +       else if (dom_exists) {
> +               d = find_domain_struct(domid);
> +               if (!d) {
> +                       errno = ENOENT;
> +                       corrupt(conn, "Missing domain %u\n", domid);
> +                       return -1;
> +               }
> +       } else {
> +               d = find_or_alloc_existing_domain(domid);
> +               if (!d) {
> +                       errno = ENOMEM;
> +                       return -1;
> +               }
> +       }
>   
>   	if (conn && conn->transaction) {
> -               transaction_entry_dec(conn->transaction, domid);
> -       } else {
> -               d = (conn && domid == conn->id && conn->domain) ? conn->domain
> -                   : find_domain_struct(domid);
> -               if (d) {
> -                       d->nbentry--;
> -               } else {
> -                       errno = ENOENT;
> -                       corrupt(conn,
> -                               "Node \"%s\" owned by non-existing domain %u\n",
> -                               node->name, domid);
> +               head = transaction_get_changed_domains(conn->transaction);
> +               ret = acc_add_dom_nbentry(conn->transaction, head, add, domid);
> +               if (errno) {
> +                       fail_transaction(conn->transaction);
> +                       return -1;
>                 }
> +               return d->nbentry + ret;
It is not entirely clear why you are return "d->nbentry + ret" here. If 
it is ...
>         }
> +
> +       d->nbentry += add;
> +
> +       return d->nbentry;
>   }
>   
> -int domain_entry_fix(unsigned int domid, int num, bool update)
> +int domain_nbentry_inc(struct connection *conn, unsigned int domid)
>   {
> -       struct domain *d;
> -       int cnt;
> +       return (domain_nbentry_add(conn, domid, 1, false) < 0) ? errno : 0;
> +}
>   
> -	if (update) {
> -               d = find_domain_struct(domid);
> -               assert(d);
> -       } else {
> -               /*
> -                * We are called first with update == false in order to catch
> -                * any error. So do a possible allocation and check for error
> -                * only in this case, as in the case of update == true nothing
> -                * can go wrong anymore as the allocation already happened.
> -                */
> -               d = find_or_alloc_existing_domain(domid);
> -               if (!d)
> -                       return -1;
> -       }
> +int domain_nbentry_dec(struct connection *conn, unsigned int domid)
> +{
> +       return (domain_nbentry_add(conn, domid, -1, true) < 0) ? errno : 0;
... to make sure domain_nbentry_add() is not returning a negative value. 
Then it would not work.
A good example imagine you have a transaction removing nodes from tree 
but not adding any. Then the "ret" would be negative.
Meanwhile the nodes are also removed outside of the transaction. So the 
sum of "d->nbentry + ret" would be negative resulting to a failure here.
Such change of behavior should pointed in the commit message. But then I 
am not convinced this should be part of this commit which is mainly 
reworking an interface (e.g. no functional change is expected).
Cheers,

--
Julien Grall