Re: [PATCH v5 05/14] tools/xenstore: use accounting buffering for node accounting

[Julien Grall <julien@xxxxxxx>]

Hi Juergen,

On 08/05/2023 12:47, Juergen Gross wrote:
> Add the node accounting to the accounting information buffering in
> order to avoid having to undo it in case of failure.
>
> This requires to call domain_nbentry_dec() before any changes to the
> data base, as it can return an error now.
>
> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> ---
> V5:
> - add error handling after domain_nbentry_dec() calls (Julien Grall)
> ---
>   tools/xenstore/xenstored_core.c   | 29 +++++++----------------------
>   tools/xenstore/xenstored_domain.h |  4 ++--
>   2 files changed, 9 insertions(+), 24 deletions(-)
>
> diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
> index 8392bdec9b..22da434e2a 100644
> --- a/tools/xenstore/xenstored_core.c
> +++ b/tools/xenstore/xenstored_core.c
> @@ -1454,7 +1454,6 @@ 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_nbentry_dec(conn, get_node_owner(node));
>   
>   	/*
>          * It is not possible to easily revert the changes in a transaction.
> @@ -1645,6 +1644,9 @@ static int delnode_sub(const void *ctx, struct connection 
> *conn,
>         if (ret > 0)
>                 return WALK_TREE_SUCCESS_STOP;
>   
> +	if (domain_nbentry_dec(conn, get_node_owner(node)))
> +               return WALK_TREE_ERROR_STOP;
I think there is a potential issue with the buffering here. In case of 
failure, the node could have been removed, but the quota would not be 
properly accounted.
Also, I think a comment would be warrant to explain why we are returning 
WALK_TREE_ERROR_STOP here when...
> +
>         /* In case of error stop the walk. */
>         if (!ret && do_tdb_delete(conn, &key, &node->acc))
>                 return WALK_TREE_SUCCESS_STOP;
... this is not the case when do_tdb_delete() fails for some reasons.

> @@ -1657,8 +1659,6 @@ 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_nbentry_dec(conn, get_node_owner(node));
> -
>         return WALK_TREE_RM_CHILDENTRY;
>   }
>   
> @@ -1797,29 +1797,14 @@ static int do_set_perms(const void *ctx, struct connection *conn,
>                 return EPERM;
>   
>   	old_perms = node->perms;
> -       domain_nbentry_dec(conn, get_node_owner(node));
> +       if (domain_nbentry_dec(conn, get_node_owner(node)))
> +               return ENOMEM;
>         node->perms = perms;
> -       if (domain_nbentry_inc(conn, get_node_owner(node))) {
> -               node->perms = old_perms;
> -               /*
> -                * This should never fail because we had a reference on the
> -                * domain before and Xenstored is single-threaded.
> -                */
> -               domain_nbentry_inc(conn, get_node_owner(node));
> +       if (domain_nbentry_inc(conn, get_node_owner(node)))
>                 return ENOMEM;
> -       }
> -
> -       if (write_node(conn, node, false)) {
> -               int saved_errno = errno;
>   
> -		domain_nbentry_dec(conn, get_node_owner(node));
> -               node->perms = old_perms;
> -               /* No failure possible as above. */
> -               domain_nbentry_inc(conn, get_node_owner(node));
> -
> -               errno = saved_errno;
> +       if (write_node(conn, node, false))
>                 return errno;
> -       }
>   
>   	fire_watches(conn, ctx, name, node, false, &old_perms);
>         send_ack(conn, XS_SET_PERMS);
> diff --git a/tools/xenstore/xenstored_domain.h 
> b/tools/xenstore/xenstored_domain.h
> index e40657216b..466549709f 100644
> --- a/tools/xenstore/xenstored_domain.h
> +++ b/tools/xenstore/xenstored_domain.h
> @@ -25,9 +25,9 @@
>    * a per transaction array.
>    */
>   enum accitem {
> +       ACC_NODES,
>         ACC_REQ_N,              /* Number of elements per request. */
> -       ACC_NODES = ACC_REQ_N,
> -       ACC_TR_N,               /* Number of elements per transaction. */
> +       ACC_TR_N = ACC_REQ_N,   /* Number of elements per transaction. */
>         ACC_CHD_N = ACC_TR_N,   /* max(ACC_REQ_N, ACC_TR_N), for changed dom. */
>         ACC_N = ACC_TR_N,       /* Number of elements per domain. */
>   };

Cheers,

--
Julien Grall