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

[Juergen Gross <jgross@xxxxxxxx>]

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