[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 09/20] tools/xenstore: introduce dummy nodes for special watch paths



On 06.11.22 22:38, Julien Grall wrote:
Hi Juergen,

On 01/11/2022 15:28, Juergen Gross wrote:
Instead of special casing the permission handling and watch event
firing for the special watch paths "@introduceDomain" and
"@releaseDomain", use static dummy nodes added to the data base when
starting Xenstore.

A few questions I think needs to be answered in the commit message:

Okay. Will add.

   - Does this means we could write data in "@..."  node?
   - How about creating sub nodes?

Normal Xenstore operations won't succeed due to path name checks.

But I just realized that at some time a chunk of the patch must have been
lost, as I originally had special checks for set/get permissions in place.
Those are no longer there and need to be re-added.

  - What does it mean for the accounting? Before, it was accounted to no-one but now it seems to be accounted to the owner (which may not be dom0).

Yes. And this is how it should be.

This allows to simplify quite some code.

The diff is quite nice to have, but I am not entirely convinced this is making the code easier to understand.

Is this patch helping you for another goal?

It will allow to add more fine grained permissions when adding the support
for reporting the domid in special events (e.g. allowing a stubdom to watch
the @release event of its target domain).



Note that this requires to rework the calls of fire_watches() for the
special events in order to avoid leaking memory.

Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
---
  tools/xenstore/xenstored_control.c |   3 -
  tools/xenstore/xenstored_core.c    |  67 +++++++-----
  tools/xenstore/xenstored_domain.c  | 162 ++++-------------------------
  tools/xenstore/xenstored_domain.h  |   6 --
  tools/xenstore/xenstored_watch.c   |  17 +--
  5 files changed, 63 insertions(+), 192 deletions(-)

diff --git a/tools/xenstore/xenstored_control.c b/tools/xenstore/xenstored_control.c
index d1aaa00bf4..41e6992591 100644
--- a/tools/xenstore/xenstored_control.c
+++ b/tools/xenstore/xenstored_control.c
@@ -676,9 +676,6 @@ static const char *lu_dump_state(const void *ctx, struct connection *conn)
      if (ret)
          goto out;
      ret = dump_state_connections(fp);
-    if (ret)
-        goto out;
-    ret = dump_state_special_nodes(fp);
      if (ret)
          goto out;
      ret = dump_state_nodes(fp, ctx);
diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 1650821922..cadb339486 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -616,7 +616,8 @@ static void get_acc_data(TDB_DATA *key, struct node_account_data *acc)
  static unsigned int get_acc_domid(struct connection *conn, TDB_DATA *key,
                    unsigned int domid)
  {
-    return (!conn || key->dptr[0] == '/') ? domid : conn->id;
+    return (!conn || key->dptr[0] == '/' || key->dptr[0] == '@')

The comment above says it is sufficient to check for '/'. But now, you are also checking for '@'.

Will fix the comment.


+           ? domid : conn->id;
  }
  int do_tdb_write(struct connection *conn, TDB_DATA *key, TDB_DATA *data,
@@ -1780,14 +1781,6 @@ static int do_set_perms(const void *ctx, struct connection *conn,
      if (perms.p[0].perms & XS_PERM_IGNORE)
          return ENOENT;
-    /* First arg is node name. */
-    if (strstarts(in->buffer, "@")) {
-        if (set_perms_special(conn, in->buffer, &perms))
-            return errno;
-        send_ack(conn, XS_SET_PERMS);
-        return 0;
-    }

So there are a slight change in behavior here. Before, the permission would be directly set even if we are in a transaction. Now, this will only be set if the transaction has been committed.

I am split on whether this would be considered as an ABI breakage. The new behavior seems better, but anyone rely on the (bogus?) previous behavior would have a surprise. At minimum, the change should at least be pointed out in the commit message.

I don't think this will be a real problem, as the old behavior was more like
a bug. I'll spell it out in the commit message.


[...]

  static int domain_tree_remove_sub(const void *ctx, struct connection *conn,
                    struct node *node, void *arg)
  {
@@ -297,8 +273,24 @@ static void domain_tree_remove(struct domain *domain)
                     "error when looking for orphaned nodes\n");
      }
-    remove_domid_from_perm(&dom_release_perms, domain);
-    remove_domid_from_perm(&dom_introduce_perms, domain);
+    walk_node_tree(domain, NULL, "@releaseDomain", &walkfuncs, domain);
+    walk_node_tree(domain, NULL, "@introduceDomain", &walkfuncs, domain);
+}
+
+static void fire_special_watches(const char *name)
+{
+    void *ctx = talloc_new(NULL);
+    struct node *node;

I can foresee how one may want to abuse for this function. So I would add an assert(name[0] == '@') to match clear this should only for watches starting with '@'.

Okay.


+
+    if (!ctx)
+        return;
+
+    node = read_node(NULL, ctx, name);
+
+    if (node)
+        fire_watches(NULL, ctx, name, node, true, NULL);

Shouldn't we throw a message if we can't retrieve @...?


Yes, I can add that.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.