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

[Julien Grall <julien@xxxxxxx>]

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:
  - Does this means we could write data in "@..."  node?
  - How about creating sub nodes?
  - 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).
> 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?

> 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 '@'.
> +              ? 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.
[...]

>   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 '@'.
> +
> +       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 @...?

Cheers,

--
Julien Grall