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

[Juergen Gross <jgross@xxxxxxxx>]

On 12.08.23 14:03, Julien Grall wrote:
> Hi Juergen,
>
> One more remark.
>
> 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)
> As you use bool, I find a bit confusing that 'true' would be returned on error 
> while 'false' indicate it is ok. I feel the inverse is more intuitive.
Depends how you are looking at it. On the caller side it is IMO more
intuitive the other way round (it is a common pattern for function returning
an int to return 0 in the no-error case).

OTOH returning "true" for the no-error case seems to be the standard in
Xenstore for return type bool.

> I can understand this is a matter of taste. So the only thing I would ask is 
> documenting this oddity as I can foresee someone that misinterpreting the return.
I'll switch err to success.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature