Re: [PATCH v4 05/13] tools/xenstore: use accounting buffering for node accounting

[Julien Grall <julien@xxxxxxx>]

Hi Juergen,

On 05/04/2023 08:03, Juergen Gross wrote:
> Add the node accounting to the accounting information buffering in
> order to avoid having to undo it in case of failure.
>
> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> ---
>   tools/xenstore/xenstored_core.c   | 21 ++-------------------
>   tools/xenstore/xenstored_domain.h |  4 ++--
>   2 files changed, 4 insertions(+), 21 deletions(-)
>
> diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
> index 84335f5f3d..92a40ccf3f 100644
> --- a/tools/xenstore/xenstored_core.c
> +++ b/tools/xenstore/xenstored_core.c
> @@ -1452,7 +1452,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.
> @@ -1797,27 +1796,11 @@ static int do_set_perms(const void *ctx, struct 
> connection *conn,
>         old_perms = node->perms;
>         domain_nbentry_dec(conn, get_node_owner(node));
IIRC, we originally said that domain_nbentry_dec() could never fail in a 
non-transaction case. But with your current rework, the function can now 
fail because of an allocation failure.
Therefore, shouldn't we now check the error? (Possibly in a patch 
beforehand).
>         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 6355ad4f37..e669f57b80 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_N = ACC_TR_N,       /* Number of elements per domain. */
>   };
>   
Cheers,

--
Julien Grall