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

[Juergen Gross <jgross@xxxxxxxx>]

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)
V6:
- return WALK_TREE_ERROR_STOP after failed do_tdb_delete()
- add comment why calling corrupt() is fine (Julien Grall)
---
 tools/xenstore/xenstored_core.c   | 37 ++++++++++++-------------------
 tools/xenstore/xenstored_domain.h |  4 ++--
 2 files changed, 16 insertions(+), 25 deletions(-)

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 8392bdec9b..0a9c88ca67 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,9 +1644,12 @@ 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;
+
        /* In case of error stop the walk. */
        if (!ret && do_tdb_delete(conn, &key, &node->acc))
-               return WALK_TREE_SUCCESS_STOP;
+               return WALK_TREE_ERROR_STOP;
 
        /*
         * Fire the watches now, when we can still see the node permissions.
@@ -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;
 }
 
@@ -1679,6 +1679,12 @@ int rm_node(struct connection *conn, const void *ctx, 
const char *name)
        ret = walk_node_tree(ctx, conn, name, &walkfuncs, (void *)name);
        if (ret < 0) {
                if (ret == WALK_TREE_ERROR_STOP) {
+                       /*
+                        * This can't be triggered by an unprivileged guest,
+                        * so calling corrupt() is fine here.
+                        * In fact it is needed in order to fix a potential
+                        * accounting inconsistency.
+                        */
                        corrupt(conn, "error when deleting sub-nodes of %s\n",
                                name);
                        errno = EIO;
@@ -1797,29 +1803,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. */
 };
-- 
2.35.3