| Message ID | 20230724110247.10520-25-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tools/xenstore: drop TDB | expand |
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@suse.com> > --- > 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,
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@suse.com> > --- > 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. 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. Cheers,
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@suse.com> >> --- >> 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
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@suse.com> >> --- >> 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
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()) errno = errno_from_parents(conn, ctx, name, errno, perm); - return node; + + return err; } static struct buffered_data *new_buffer(void *ctx) @@ -1285,14 +1284,12 @@ const char *canonicalize(struct connection *conn, const void *ctx, return name; } -static struct node *get_node_canonicalized(struct connection *conn, - const void *ctx, - const char *name, - const char **canonical_name, - unsigned int perm, - bool allow_special) +static struct node *get_node(struct connection *conn, const void *ctx, + const char *name, const char **canonical_name, + unsigned int perm, bool allow_special) { const char *tmp_name; + struct node *node; if (!canonical_name) canonical_name = &tmp_name; @@ -1300,7 +1297,10 @@ static struct node *get_node_canonicalized(struct connection *conn, if (!*canonical_name) return NULL; - return get_node(conn, ctx, *canonical_name, perm); + node = read_node(conn, ctx, *canonical_name); + + return get_node_chk_perm(conn, ctx, node, *canonical_name, perm) + ? NULL : node; } static int send_directory(const void *ctx, struct connection *conn, @@ -1308,8 +1308,7 @@ static int send_directory(const void *ctx, struct connection *conn, { struct node *node; - node = get_node_canonicalized(conn, ctx, onearg(in), NULL, - XS_PERM_READ, false); + node = get_node(conn, ctx, onearg(in), NULL, XS_PERM_READ, false); if (!node) return errno; @@ -1330,8 +1329,7 @@ static int send_directory_part(const void *ctx, struct connection *conn, return EINVAL; /* First arg is node name. */ - node = get_node_canonicalized(conn, ctx, in->buffer, NULL, - XS_PERM_READ, false); + node = get_node(conn, ctx, in->buffer, NULL, XS_PERM_READ, false); if (!node) return errno; @@ -1380,8 +1378,7 @@ static int do_read(const void *ctx, struct connection *conn, { struct node *node; - node = get_node_canonicalized(conn, ctx, onearg(in), NULL, - XS_PERM_READ, false); + node = get_node(conn, ctx, onearg(in), NULL, XS_PERM_READ, false); if (!node) return errno; @@ -1595,8 +1592,7 @@ static int do_write(const void *ctx, struct connection *conn, offset = strlen(vec[0]) + 1; datalen = in->used - offset; - node = get_node_canonicalized(conn, ctx, vec[0], &name, XS_PERM_WRITE, - false); + node = get_node(conn, ctx, vec[0], &name, XS_PERM_WRITE, false); if (!node) { /* No permissions, invalid input? */ if (errno != ENOENT) @@ -1624,8 +1620,7 @@ static int do_mkdir(const void *ctx, struct connection *conn, struct node *node; const char *name; - node = get_node_canonicalized(conn, ctx, onearg(in), &name, - XS_PERM_WRITE, false); + node = get_node(conn, ctx, onearg(in), &name, XS_PERM_WRITE, false); /* If it already exists, fine. */ if (!node) { @@ -1754,8 +1749,7 @@ static int do_rm(const void *ctx, struct connection *conn, const char *name; char *parentname; - node = get_node_canonicalized(conn, ctx, onearg(in), &name, - XS_PERM_WRITE, false); + node = get_node(conn, ctx, onearg(in), &name, XS_PERM_WRITE, false); if (!node) { /* Didn't exist already? Fine, if parent exists. */ if (errno == ENOENT) { @@ -1797,8 +1791,7 @@ static int do_get_perms(const void *ctx, struct connection *conn, unsigned int len; struct node_perms perms; - node = get_node_canonicalized(conn, ctx, onearg(in), NULL, XS_PERM_READ, - true); + node = get_node(conn, ctx, onearg(in), NULL, XS_PERM_READ, true); if (!node) return errno; @@ -1842,8 +1835,8 @@ static int do_set_perms(const void *ctx, struct connection *conn, return ENOENT; /* We must own node to do this (tools can do this too). */ - node = get_node_canonicalized(conn, ctx, in->buffer, &name, - XS_PERM_WRITE | XS_PERM_OWNER, true); + node = get_node(conn, ctx, in->buffer, &name, + XS_PERM_WRITE | XS_PERM_OWNER, true); if (!node) return errno;
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@suse.com> --- V3: - new patch --- tools/xenstore/xenstored_core.c | 57 +++++++++++++++------------------ 1 file changed, 25 insertions(+), 32 deletions(-)