| Message ID | 20230722081646.4136-2-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tools/xenstore: fix get_spec_node() | expand |
Hi Juergen, On 22/07/2023 09:16, Juergen Gross wrote: > -static struct watch *add_watch(struct connection *conn, char *path, char *token, > - bool relative, bool no_quota_check) > +static struct watch *add_watch(struct connection *conn, const char *path, > + const char *token, bool relative, > + bool no_quota_check) > { > struct watch *watch; > > @@ -218,12 +219,14 @@ int do_watch(const void *ctx, struct connection *conn, struct buffered_data *in) > { > struct watch *watch; > char *vec[2]; > + const char *path; > bool relative; > > if (get_strings(in, vec, ARRAY_SIZE(vec)) != ARRAY_SIZE(vec)) > return EINVAL; > > - errno = check_watch_path(conn, ctx, &(vec[0]), &relative); > + path = vec[0]; > + errno = check_watch_path(conn, ctx, &path, &relative); From my understanding, check_watch_path() could update vec[0] which is then used below. But with your change, vec[0] would not be updated anymore. I can see two possible approach: 1) get_strings() is taking a const as well 2) replace all use of vec[0] with path I have a slight preference for the former. But I can also understand this is more work. So choose whichever you prefer. Cheers,
On 22/07/2023 16:20, Julien Grall wrote: > Hi Juergen, > > On 22/07/2023 09:16, Juergen Gross wrote: >> -static struct watch *add_watch(struct connection *conn, char *path, >> char *token, >> - bool relative, bool no_quota_check) >> +static struct watch *add_watch(struct connection *conn, const char >> *path, >> + const char *token, bool relative, >> + bool no_quota_check) >> { >> struct watch *watch; >> @@ -218,12 +219,14 @@ int do_watch(const void *ctx, struct connection >> *conn, struct buffered_data *in) >> { >> struct watch *watch; >> char *vec[2]; >> + const char *path; >> bool relative; >> if (get_strings(in, vec, ARRAY_SIZE(vec)) != ARRAY_SIZE(vec)) >> return EINVAL; >> - errno = check_watch_path(conn, ctx, &(vec[0]), &relative); >> + path = vec[0]; >> + errno = check_watch_path(conn, ctx, &path, &relative); > > From my understanding, check_watch_path() could update vec[0] which is > then used below. But with your change, vec[0] would not be updated anymore. > > I can see two possible approach: > 1) get_strings() is taking a const as well I have a patch doing this. I will send it on Monday. Cheers,
On 22.07.23 18:44, Julien Grall wrote: > > > On 22/07/2023 16:20, Julien Grall wrote: >> Hi Juergen, >> >> On 22/07/2023 09:16, Juergen Gross wrote: >>> -static struct watch *add_watch(struct connection *conn, char *path, char >>> *token, >>> - bool relative, bool no_quota_check) >>> +static struct watch *add_watch(struct connection *conn, const char *path, >>> + const char *token, bool relative, >>> + bool no_quota_check) >>> { >>> struct watch *watch; >>> @@ -218,12 +219,14 @@ int do_watch(const void *ctx, struct connection *conn, >>> struct buffered_data *in) >>> { >>> struct watch *watch; >>> char *vec[2]; >>> + const char *path; >>> bool relative; >>> if (get_strings(in, vec, ARRAY_SIZE(vec)) != ARRAY_SIZE(vec)) >>> return EINVAL; >>> - errno = check_watch_path(conn, ctx, &(vec[0]), &relative); >>> + path = vec[0]; >>> + errno = check_watch_path(conn, ctx, &path, &relative); >> >> From my understanding, check_watch_path() could update vec[0] which is then >> used below. But with your change, vec[0] would not be updated anymore. >> >> I can see two possible approach: >> 1) get_strings() is taking a const as well > > I have a patch doing this. I will send it on Monday. Hmm, looking at this I think this will collide with my patch. Would you mind me doing the conversion of get_strings() in my patch, too? Juergen
Hi, On 24/07/2023 08:10, Juergen Gross wrote: > On 22.07.23 18:44, Julien Grall wrote: >> >> >> On 22/07/2023 16:20, Julien Grall wrote: >>> Hi Juergen, >>> >>> On 22/07/2023 09:16, Juergen Gross wrote: >>>> -static struct watch *add_watch(struct connection *conn, char *path, >>>> char *token, >>>> - bool relative, bool no_quota_check) >>>> +static struct watch *add_watch(struct connection *conn, const char >>>> *path, >>>> + const char *token, bool relative, >>>> + bool no_quota_check) >>>> { >>>> struct watch *watch; >>>> @@ -218,12 +219,14 @@ int do_watch(const void *ctx, struct >>>> connection *conn, struct buffered_data *in) >>>> { >>>> struct watch *watch; >>>> char *vec[2]; >>>> + const char *path; >>>> bool relative; >>>> if (get_strings(in, vec, ARRAY_SIZE(vec)) != ARRAY_SIZE(vec)) >>>> return EINVAL; >>>> - errno = check_watch_path(conn, ctx, &(vec[0]), &relative); >>>> + path = vec[0]; >>>> + errno = check_watch_path(conn, ctx, &path, &relative); >>> >>> From my understanding, check_watch_path() could update vec[0] which >>> is then used below. But with your change, vec[0] would not be updated >>> anymore. >>> >>> I can see two possible approach: >>> 1) get_strings() is taking a const as well >> >> I have a patch doing this. I will send it on Monday. > > Hmm, looking at this I think this will collide with my patch. Yes, it will for check_watch_patch(). > > Would you mind me doing the conversion of get_strings() in my patch, too? Sure. Cheers,
diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c index a1d3047e48..3d3c39bd70 100644 --- a/tools/xenstore/xenstored_core.c +++ b/tools/xenstore/xenstored_core.c @@ -1216,25 +1216,26 @@ static char *perms_to_strings(const void *ctx, const struct node_perms *perms, return strings; } -char *canonicalize(struct connection *conn, const void *ctx, const char *node) +const char *canonicalize(struct connection *conn, const void *ctx, + const char *node) { const char *prefix; if (!node || (node[0] == '/') || (node[0] == '@')) - return (char *)node; + return node; prefix = get_implicit_path(conn); if (prefix) return talloc_asprintf(ctx, "%s/%s", prefix, node); - return (char *)node; + return node; } static struct node *get_node_canonicalized(struct connection *conn, const void *ctx, const char *name, - char **canonical_name, + const char **canonical_name, unsigned int perm) { - char *tmp_name; + const char *tmp_name; if (!canonical_name) canonical_name = &tmp_name; @@ -1249,7 +1250,7 @@ static struct node *get_node_canonicalized(struct connection *conn, } static struct node *get_spec_node(struct connection *conn, const void *ctx, - const char *name, char **canonical_name, + const char *name, const char **canonical_name, unsigned int perm) { if (name[0] == '@') @@ -1539,7 +1540,7 @@ static int do_write(const void *ctx, struct connection *conn, unsigned int offset, datalen; struct node *node; char *vec[1] = { NULL }; /* gcc4 + -W + -Werror fucks code. */ - char *name; + const char *name; /* Extra "strings" can be created by binary data. */ if (get_strings(in, vec, ARRAY_SIZE(vec)) < ARRAY_SIZE(vec)) @@ -1574,7 +1575,7 @@ static int do_mkdir(const void *ctx, struct connection *conn, struct buffered_data *in) { struct node *node; - char *name; + const char *name; node = get_node_canonicalized(conn, ctx, onearg(in), &name, XS_PERM_WRITE); @@ -1703,7 +1704,7 @@ static int do_rm(const void *ctx, struct connection *conn, { struct node *node; int ret; - char *name; + const char *name; char *parentname; node = get_node_canonicalized(conn, ctx, onearg(in), &name, @@ -1765,7 +1766,8 @@ static int do_set_perms(const void *ctx, struct connection *conn, struct buffered_data *in) { struct node_perms perms, old_perms; - char *name, *permstr; + const char *name; + char *permstr; struct node *node; perms.num = xenstore_count_strings(in->buffer, in->used); diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h index 84a611cbb5..4184a4b7ef 100644 --- a/tools/xenstore/xenstored_core.h +++ b/tools/xenstore/xenstored_core.h @@ -224,7 +224,8 @@ void send_event(struct buffered_data *req, struct connection *conn, void send_ack(struct connection *conn, enum xsd_sockmsg_type type); /* Canonicalize this path if possible. */ -char *canonicalize(struct connection *conn, const void *ctx, const char *node); +const char *canonicalize(struct connection *conn, const void *ctx, + const char *node); /* Get access permissions. */ unsigned int perm_for_conn(struct connection *conn, diff --git a/tools/xenstore/xenstored_watch.c b/tools/xenstore/xenstored_watch.c index 4195c59e17..f92fbeb73a 100644 --- a/tools/xenstore/xenstored_watch.c +++ b/tools/xenstore/xenstored_watch.c @@ -161,7 +161,7 @@ static int destroy_watch(void *_watch) } static int check_watch_path(struct connection *conn, const void *ctx, - char **path, bool *relative) + const char **path, bool *relative) { /* Check if valid event. */ if (strstarts(*path, "@")) { @@ -184,8 +184,9 @@ static int check_watch_path(struct connection *conn, const void *ctx, return errno; } -static struct watch *add_watch(struct connection *conn, char *path, char *token, - bool relative, bool no_quota_check) +static struct watch *add_watch(struct connection *conn, const char *path, + const char *token, bool relative, + bool no_quota_check) { struct watch *watch; @@ -218,12 +219,14 @@ int do_watch(const void *ctx, struct connection *conn, struct buffered_data *in) { struct watch *watch; char *vec[2]; + const char *path; bool relative; if (get_strings(in, vec, ARRAY_SIZE(vec)) != ARRAY_SIZE(vec)) return EINVAL; - errno = check_watch_path(conn, ctx, &(vec[0]), &relative); + path = vec[0]; + errno = check_watch_path(conn, ctx, &path, &relative); if (errno) return errno; @@ -258,7 +261,8 @@ int do_unwatch(const void *ctx, struct connection *conn, struct buffered_data *in) { struct watch *watch; - char *node, *vec[2]; + const char *node; + char *vec[2]; if (get_strings(in, vec, ARRAY_SIZE(vec)) != ARRAY_SIZE(vec)) return EINVAL; @@ -336,7 +340,7 @@ void read_state_watch(const void *ctx, const void *state) { const struct xs_state_watch *sw = state; struct connection *conn; - char *path, *token; + const char *path, *token; bool relative; conn = get_connection_by_id(sw->conn_id);
The return type of canonicalize() can be modified to const char *. This avoids the need to cast the const away from the input parameter. There need to be quite some other functions modified to take const parameters in order to avoid further casts. Signed-off-by: Juergen Gross <jgross@suse.com> --- tools/xenstore/xenstored_core.c | 22 ++++++++++++---------- tools/xenstore/xenstored_core.h | 3 ++- tools/xenstore/xenstored_watch.c | 16 ++++++++++------ 3 files changed, 24 insertions(+), 17 deletions(-)