Re: [PATCH v4 14/19] tools/xenstore: merge get_spec_node() into get_node_canonicalized()

2023-08-18 Thread Julien Grall

Hi Juergen,

On 14/08/2023 08:47, Juergen Gross wrote:

Add a "allow_special" parameter to get_node_canonicalized() allowing
to merge get_spec_node() into get_node_canonicalized().

Add the same parameter to is_valid_nodename(), as this will simplify
check_watch_path().

This is done in preparation to introducing a get_node() variant
returning a pointer to const struct node.

Note that this will change how special node names are going to be
validated, as now the normal restrictions for node names will be
applied:

- they can't end with "/"
- they can't contain "//"
- they can't contain characters other than the ones allowed for normal
   nodes
- the length of the node name is restricted by the max path length
   quota

For defined special node names this isn't any real restriction, though.

Signed-off-by: Juergen Gross 


Reviewed-by: Julien Grall 

Cheers,

--
Julien Grall



[PATCH v4 14/19] tools/xenstore: merge get_spec_node() into get_node_canonicalized()

2023-08-14 Thread Juergen Gross
Add a "allow_special" parameter to get_node_canonicalized() allowing
to merge get_spec_node() into get_node_canonicalized().

Add the same parameter to is_valid_nodename(), as this will simplify
check_watch_path().

This is done in preparation to introducing a get_node() variant
returning a pointer to const struct node.

Note that this will change how special node names are going to be
validated, as now the normal restrictions for node names will be
applied:

- they can't end with "/"
- they can't contain "//"
- they can't contain characters other than the ones allowed for normal
  nodes
- the length of the node name is restricted by the max path length
  quota

For defined special node names this isn't any real restriction, though.

Signed-off-by: Juergen Gross 
---
V3:
- new patch
V4:
- expand commit message (Julien Grall)
---
 tools/xenstore/xenstored_core.c  | 45 +---
 tools/xenstore/xenstored_core.h  |  3 ++-
 tools/xenstore/xenstored_watch.c | 19 +-
 3 files changed, 26 insertions(+), 41 deletions(-)

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index f8e5e7b697..0ebe4bb7d2 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -1219,13 +1219,14 @@ static bool valid_chars(const char *node)
   "0123456789-/_@") == strlen(node));
 }
 
-bool is_valid_nodename(const struct connection *conn, const char *node)
+bool is_valid_nodename(const struct connection *conn, const char *node,
+  bool allow_special)
 {
int local_off = 0;
unsigned int domid;
 
-   /* Must start in /. */
-   if (!strstarts(node, "/"))
+   /* Must start in / or - if special nodes are allowed - in @. */
+   if (!strstarts(node, "/") && (!allow_special || !strstarts(node, "@")))
return false;
 
/* Cannot end in / (unless it's just "/"). */
@@ -1293,7 +1294,8 @@ static struct node *get_node_canonicalized(struct 
connection *conn,
   const void *ctx,
   const char *name,
   const char **canonical_name,
-  unsigned int perm)
+  unsigned int perm,
+  bool allow_special)
 {
const char *tmp_name;
 
@@ -1302,33 +1304,20 @@ static struct node *get_node_canonicalized(struct 
connection *conn,
*canonical_name = canonicalize(conn, ctx, name);
if (!*canonical_name)
return NULL;
-   if (!is_valid_nodename(conn, *canonical_name)) {
+   if (!is_valid_nodename(conn, *canonical_name, allow_special)) {
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, const char **canonical_name,
- unsigned int perm)
-{
-   if (name[0] == '@') {
-   if (canonical_name)
-   *canonical_name = name;
-   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)
 {
struct node *node;
 
node = get_node_canonicalized(conn, ctx, onearg(in), NULL,
- XS_PERM_READ);
+ XS_PERM_READ, false);
if (!node)
return errno;
 
@@ -1350,7 +1339,7 @@ static int send_directory_part(const void *ctx, struct 
connection *conn,
 
/* First arg is node name. */
node = get_node_canonicalized(conn, ctx, in->buffer, NULL,
- XS_PERM_READ);
+ XS_PERM_READ, false);
if (!node)
return errno;
 
@@ -1400,7 +1389,7 @@ static int do_read(const void *ctx, struct connection 
*conn,
struct node *node;
 
node = get_node_canonicalized(conn, ctx, onearg(in), NULL,
- XS_PERM_READ);
+ XS_PERM_READ, false);
if (!node)
return errno;
 
@@ -1614,7 +1603,8 @@ static int do_write(const void *ctx, struct connection 
*conn,
offset = strlen(vec[0]) + 1;
datalen = in->used - offset;
 
-   node = get_node_canonicalized(conn, ctx, vec[0], , XS_PERM_WRITE);
+   node = get_node_canonicalized(conn, ctx, vec[0], , XS_PERM_WRITE,
+ false);
if (!node) {
/* No permissions, invalid input? */
if (errno != ENOENT)
@@ -1643,7 +1633,7 @@ static int