Re: [PATCH v3 24/25] tools/xenstore: rework get_node()

[Juergen Gross <jgross@xxxxxxxx>]

On 12.08.23 13:56, Julien Grall wrote:
> Hi Juergen,
>
> On 24/07/2023 12:02, Juergen Gross wrote:
> > Today get_node_canonicalized() is the only caller of get_node().
> >
> > In order to prepare introducing a get_node() variant returning a
> > pointer to const struct node, do the following restructuring:
> >
> > - move the call of read_node() from get_node() into
> >    get_node_canonicalized()
> >
> > - rename get_node() to get_node_chk_perm()
> >
> > - rename get_node_canonicalized() to get_node()
> >
> > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> > ---
> > V3:
> > - new patch
> > ---
> >   tools/xenstore/xenstored_core.c | 57 +++++++++++++++------------------
> >   1 file changed, 25 insertions(+), 32 deletions(-)
> >
> > diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
> > index ec20bc042d..fa07bc0c31 100644
> > --- a/tools/xenstore/xenstored_core.c
> > +++ b/tools/xenstore/xenstored_core.c
> > @@ -996,27 +996,26 @@ static int errno_from_parents(struct connection *conn, 
> > const void *ctx,
> >    * If it fails, returns NULL and sets errno.
> >    * Temporary memory allocations are done with ctx.
> >    */
> > -static struct node *get_node(struct connection *conn,
> > -                 const void *ctx,
> > -                 const char *name,
> > -                 unsigned int perm)
> > +static bool get_node_chk_perm(struct connection *conn, const void *ctx,
> > +                  const struct node *node, const char *name,
> > +                  unsigned int perm)
> >   {
> > -    struct node *node;
> >       struct node_perms perms;
> > +    bool err = false;
> > -    node = read_node(conn, ctx, name);
> >       /* If we don't have permission, we don't have node. */
> >       if (node) {
> >           node_to_node_perms(node, &perms);
> >           if ((perm_for_conn(conn, &perms) & perm) != perm) {
> >               errno = EACCES;
> > -            node = NULL;
> > +            err = true;
> >           }
> >       }
> >       /* Clean up errno if they weren't supposed to know. */
> > -    if (!node && !read_node_can_propagate_errno())
> > +    if (err && !read_node_can_propagate_errno())
> Looking at the caller for get_node_chk_perm(), node could be NULL. In this case, 
> err would be false. So there is a change of behavior here.
> It is not entirely clear why it is fine. But it might be better to have err 
> equals to true when node is NULL.
Yes, you are right. I'll switch to:

+       bool err = !node;


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature