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

[Juergen Gross <jgross@xxxxxxxx>]

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 <jgross@xxxxxxxx>
> > ---
> > 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

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature