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

[Julien Grall <julien@xxxxxxx>]

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.
Cheers,

--
Julien Grall