Re: [PATCH v2 07/19] tools/xenstore: introduce dummy nodes for special watch paths

2023-01-11 Thread Julien Grall 

Hi Juergen,

On 11/01/2023 06:41, Juergen Gross wrote:

On 20.12.22 20:39, Julien Grall wrote:

+static void fire_special_watches(const char *name)
+{
+    void *ctx = talloc_new(NULL);
+    struct node *node;
+
+    if (!ctx)
+    return;
+
+    node = read_node(NULL, ctx, name);
+
+    if (node)
+    fire_watches(NULL, ctx, name, node, true, NULL);


NIT: I would consider to log an error (maybe only once) if 'node' is 
NULL. The purpose is to help debugging Xenstored.


I think we can log it always, as this is really a bad problem.


That works for me. If it is always logged, then I guess this would need 
to be a syslog message as well.


Cheers,

--
Julien Grall

Re: [PATCH v2 07/19] tools/xenstore: introduce dummy nodes for special watch paths

2023-01-10 Thread Juergen Gross 

On 20.12.22 20:39, Julien Grall wrote:

Hi Juergen,

On 13/12/2022 16:00, 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.

The node accounting needs to reflect that change by adding the special
nodes in the domain_entry_fix() call in setup_structure().

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

Move the check for a valid node name from get_node() to
get_node_canonicalized(), as it allows to use get_node() for the
special nodes, too.

In order to avoid read and write accesses to the special nodes use a
special variant for obtaining the current node data for the permission
handling.

This allows to simplify quite some code. In future sub-nodes of the
special nodes will be possible due to this change, allowing more fine
grained permission control of special events for specific domains.

Signed-off-by: Juergen Gross 
---
V2:
- add get_spec_node()
- expand commit message (Julien Grall)
---
  tools/xenstore/xenstored_control.c |   3 -
  tools/xenstore/xenstored_core.c    |  92 +---
  tools/xenstore/xenstored_domain.c  | 162 -
  tools/xenstore/xenstored_domain.h  |   6 --
  tools/xenstore/xenstored_watch.c   |  17 +--
  5 files changed, 80 insertions(+), 200 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..f96714e1b8 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 on top of get_acc_domid() needs to be updated.


Oh, yes.




+   ? domid : conn->id;
  }


[...]


+static void fire_special_watches(const char *name)
+{
+    void *ctx = talloc_new(NULL);
+    struct node *node;
+
+    if (!ctx)
+    return;
+
+    node = read_node(NULL, ctx, name);
+
+    if (node)
+    fire_watches(NULL, ctx, name, node, true, NULL);


NIT: I would consider to log an error (maybe only once) if 'node' is NULL. The 
purpose is to help debugging Xenstored.


I think we can log it always, as this is really a bad problem.


Juergen


OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key


OpenPGP_signature
Description: OpenPGP digital signature

Re: [PATCH v2 07/19] tools/xenstore: introduce dummy nodes for special watch paths

2022-12-20 Thread Julien Grall 

Hi Juergen,

On 13/12/2022 16:00, 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.

The node accounting needs to reflect that change by adding the special
nodes in the domain_entry_fix() call in setup_structure().

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

Move the check for a valid node name from get_node() to
get_node_canonicalized(), as it allows to use get_node() for the
special nodes, too.

In order to avoid read and write accesses to the special nodes use a
special variant for obtaining the current node data for the permission
handling.

This allows to simplify quite some code. In future sub-nodes of the
special nodes will be possible due to this change, allowing more fine
grained permission control of special events for specific domains.

Signed-off-by: Juergen Gross 
---
V2:
- add get_spec_node()
- expand commit message (Julien Grall)
---
  tools/xenstore/xenstored_control.c |   3 -
  tools/xenstore/xenstored_core.c|  92 +---
  tools/xenstore/xenstored_domain.c  | 162 -
  tools/xenstore/xenstored_domain.h  |   6 --
  tools/xenstore/xenstored_watch.c   |  17 +--
  5 files changed, 80 insertions(+), 200 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..f96714e1b8 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 on top of get_acc_domid() needs to be updated.


+  ? domid : conn->id;
  }
  


[...]


+static void fire_special_watches(const char *name)
+{
+   void *ctx = talloc_new(NULL);
+   struct node *node;
+
+   if (!ctx)
+   return;
+
+   node = read_node(NULL, ctx, name);
+
+   if (node)
+   fire_watches(NULL, ctx, name, node, true, NULL);


NIT: I would consider to log an error (maybe only once) if 'node' is 
NULL. The purpose is to help debugging Xenstored.


The rest of the code LGTM.

Cheers,

--
Julien Grall

[PATCH v2 07/19] tools/xenstore: introduce dummy nodes for special watch paths

2022-12-13 Thread Juergen Gross 
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.

The node accounting needs to reflect that change by adding the special
nodes in the domain_entry_fix() call in setup_structure().

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

Move the check for a valid node name from get_node() to
get_node_canonicalized(), as it allows to use get_node() for the
special nodes, too.

In order to avoid read and write accesses to the special nodes use a
special variant for obtaining the current node data for the permission
handling.

This allows to simplify quite some code. In future sub-nodes of the
special nodes will be possible due to this change, allowing more fine
grained permission control of special events for specific domains.

Signed-off-by: Juergen Gross 
---
V2:
- add get_spec_node()
- expand commit message (Julien Grall)
---
 tools/xenstore/xenstored_control.c |   3 -
 tools/xenstore/xenstored_core.c|  92 +---
 tools/xenstore/xenstored_domain.c  | 162 -
 tools/xenstore/xenstored_domain.h  |   6 --
 tools/xenstore/xenstored_watch.c   |  17 +--
 5 files changed, 80 insertions(+), 200 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..f96714e1b8 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] == '@')
+  ? domid : conn->id;
 }
 
 int do_tdb_write(struct connection *conn, TDB_DATA *key, TDB_DATA *data,
@@ -958,10 +959,6 @@ static struct node *get_node(struct connection *conn,
 {
struct node *node;
 
-   if (!name || !is_valid_nodename(name)) {
-   errno = EINVAL;
-   return NULL;
-   }
node = read_node(conn, ctx, name);
/* If we don't have permission, we don't have node. */
if (node) {
@@ -1250,9 +1247,23 @@ static struct node *get_node_canonicalized(struct 
connection *conn,
*canonical_name = canonicalize(conn, ctx, name);
if (!*canonical_name)
return NULL;
+   if (!is_valid_nodename(*canonical_name)) {
+   errno = EINVAL;
+   return NULL;
+   }
return get_node(conn, ctx, *canonical_name, perm);
 }
 
+static struct node *get_spec_node(struct connection *conn, const void *ctx,
+ const char *name, char **canonical_name,
+ unsigned int perm)
+{
+   if (name[0] == '@')
+   return get_node(conn, ctx, name, perm);
+
+   return get_node_canonicalized(conn, ctx, name, canonical_name, perm);
+}
+
 static int send_directory(const void *ctx, struct connection *conn,
  struct buffered_data *in)
 {
@@ -1737,8 +1748,7 @@ static int do_get_perms(const void *ctx, struct 
connection *conn,
char *strings;
unsigned int len;
 
-   node = get_node_canonicalized(conn, ctx, onearg(in), NULL,
- XS_PERM_READ);
+   node = get_spec_node(conn, ctx, onearg(in), NULL, XS_PERM_READ);
if (!node)
return errno;
 
@@ -1780,17 +1790,9 @@ 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, ))
-   return errno;
-   send_ack(conn, XS_SET_PERMS);
-   return 0;
-   }
-
/* We must own node to do this (tools can do this too). */
-   node = get_node_canonicalized(conn, ctx, in->buffer, ,
- XS_PERM_WRITE | XS_PERM_OWNER);
+   node = get_spec_node(conn, ctx, in->buffer, ,
+XS_PERM_WRITE | XS_PERM_OWNER);
if