Re: [PATCH v2 07/19] tools/xenstore: introduce dummy nodes for special watch paths
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
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
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
