| Message ID | 149399504137.29022.4985100698645275511.stgit@bahia.lan |
|---|---|
| State | New |
| Headers | show |
On 05/05/2017 09:37 AM, Greg Kurz wrote: > All paths in the virtfs directory now start with "./" (except the virtfs > root itself which is exactly "."). > > We hence don't need to skip leading '/' characters anymore, nor to handle > the empty path case. Also, since virtfs will only ever be supported on > linux+glibc hosts, we can use strchrnul() and come up with a much simplier > code to walk through the path elements. And we don't need to dup() the > passed directory fd. > > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > hw/9pfs/9p-local.c | 5 ----- > hw/9pfs/9p-util.c | 26 ++++++++++---------------- > 2 files changed, 10 insertions(+), 21 deletions(-) > > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c > index 92262f3c3e37..bb6e296df317 100644 > --- a/hw/9pfs/9p-local.c > +++ b/hw/9pfs/9p-local.c > @@ -54,11 +54,6 @@ int local_open_nofollow(FsContext *fs_ctx, const char *path, int flags, > { > LocalData *data = fs_ctx->private; > > - /* All paths are relative to the path data->mountfd points to */ > - while (*path == '/') { > - path++; > - } Is it worth adding any assert()s in place of the deleted code? Otherwise looks okay.
On Fri, 5 May 2017 12:01:55 -0500 Eric Blake <eblake@redhat.com> wrote: > On 05/05/2017 09:37 AM, Greg Kurz wrote: > > All paths in the virtfs directory now start with "./" (except the virtfs > > root itself which is exactly "."). > > > > We hence don't need to skip leading '/' characters anymore, nor to handle > > the empty path case. Also, since virtfs will only ever be supported on > > linux+glibc hosts, we can use strchrnul() and come up with a much simplier > > code to walk through the path elements. And we don't need to dup() the > > passed directory fd. > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > --- > > hw/9pfs/9p-local.c | 5 ----- > > hw/9pfs/9p-util.c | 26 ++++++++++---------------- > > 2 files changed, 10 insertions(+), 21 deletions(-) > > > > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c > > index 92262f3c3e37..bb6e296df317 100644 > > --- a/hw/9pfs/9p-local.c > > +++ b/hw/9pfs/9p-local.c > > @@ -54,11 +54,6 @@ int local_open_nofollow(FsContext *fs_ctx, const char *path, int flags, > > { > > LocalData *data = fs_ctx->private; > > > > - /* All paths are relative to the path data->mountfd points to */ > > - while (*path == '/') { > > - path++; > > - } > > Is it worth adding any assert()s in place of the deleted code? > The assert() added by this patch ensures that we never pass an empty string to relative_openat_nofollow(), which isn't related to this hunk of deleted code... so I'm not sure I understand the question :-\ > Otherwise looks okay. >
On Tue, 9 May 2017 11:23:05 +0200 Greg Kurz <groug@kaod.org> wrote: > On Fri, 5 May 2017 12:01:55 -0500 > Eric Blake <eblake@redhat.com> wrote: > > > On 05/05/2017 09:37 AM, Greg Kurz wrote: > > > All paths in the virtfs directory now start with "./" (except the virtfs > > > root itself which is exactly "."). > > > > > > We hence don't need to skip leading '/' characters anymore, nor to handle > > > the empty path case. Also, since virtfs will only ever be supported on > > > linux+glibc hosts, we can use strchrnul() and come up with a much simplier > > > code to walk through the path elements. And we don't need to dup() the > > > passed directory fd. > > > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > > --- > > > hw/9pfs/9p-local.c | 5 ----- > > > hw/9pfs/9p-util.c | 26 ++++++++++---------------- > > > 2 files changed, 10 insertions(+), 21 deletions(-) > > > > > > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c > > > index 92262f3c3e37..bb6e296df317 100644 > > > --- a/hw/9pfs/9p-local.c > > > +++ b/hw/9pfs/9p-local.c > > > @@ -54,11 +54,6 @@ int local_open_nofollow(FsContext *fs_ctx, const char *path, int flags, > > > { > > > LocalData *data = fs_ctx->private; > > > > > > - /* All paths are relative to the path data->mountfd points to */ > > > - while (*path == '/') { > > > - path++; > > > - } > > > > Is it worth adding any assert()s in place of the deleted code? > > > > The assert() added by this patch ensures that we never pass an empty > string to relative_openat_nofollow(), which isn't related to this > hunk of deleted code... so I'm not sure I understand the question :-\ > Ping ? > > Otherwise looks okay. > > >
On 05/09/2017 04:23 AM, Greg Kurz wrote: > On Fri, 5 May 2017 12:01:55 -0500 > Eric Blake <eblake@redhat.com> wrote: > >> On 05/05/2017 09:37 AM, Greg Kurz wrote: >>> All paths in the virtfs directory now start with "./" (except the virtfs >>> root itself which is exactly "."). >>> >>> We hence don't need to skip leading '/' characters anymore, nor to handle >>> the empty path case. Also, since virtfs will only ever be supported on >>> linux+glibc hosts, we can use strchrnul() and come up with a much simplier >>> code to walk through the path elements. And we don't need to dup() the >>> passed directory fd. >>> >>> Signed-off-by: Greg Kurz <groug@kaod.org> >>> --- >>> hw/9pfs/9p-local.c | 5 ----- >>> hw/9pfs/9p-util.c | 26 ++++++++++---------------- >>> 2 files changed, 10 insertions(+), 21 deletions(-) >>> >>> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c >>> index 92262f3c3e37..bb6e296df317 100644 >>> --- a/hw/9pfs/9p-local.c >>> +++ b/hw/9pfs/9p-local.c >>> @@ -54,11 +54,6 @@ int local_open_nofollow(FsContext *fs_ctx, const char *path, int flags, >>> { >>> LocalData *data = fs_ctx->private; >>> >>> - /* All paths are relative to the path data->mountfd points to */ >>> - while (*path == '/') { >>> - path++; >>> - } >> >> Is it worth adding any assert()s in place of the deleted code? >> > > The assert() added by this patch ensures that we never pass an empty > string to relative_openat_nofollow(), which isn't related to this > hunk of deleted code... so I'm not sure I understand the question :-\ I was thinking if it is worth replacing the deleted while() loop with an assert(*path != '/'); to prove that we have already taken care of ensuring a relative path. You're right that you added another assert(*path) elsewhere in the patch, and that one looked fine.
On Thu, 18 May 2017 09:23:07 -0500 Eric Blake <eblake@redhat.com> wrote: > On 05/09/2017 04:23 AM, Greg Kurz wrote: > > On Fri, 5 May 2017 12:01:55 -0500 > > Eric Blake <eblake@redhat.com> wrote: > > > >> On 05/05/2017 09:37 AM, Greg Kurz wrote: > >>> All paths in the virtfs directory now start with "./" (except the virtfs > >>> root itself which is exactly "."). > >>> > >>> We hence don't need to skip leading '/' characters anymore, nor to handle > >>> the empty path case. Also, since virtfs will only ever be supported on > >>> linux+glibc hosts, we can use strchrnul() and come up with a much simplier > >>> code to walk through the path elements. And we don't need to dup() the > >>> passed directory fd. > >>> > >>> Signed-off-by: Greg Kurz <groug@kaod.org> > >>> --- > >>> hw/9pfs/9p-local.c | 5 ----- > >>> hw/9pfs/9p-util.c | 26 ++++++++++---------------- > >>> 2 files changed, 10 insertions(+), 21 deletions(-) > >>> > >>> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c > >>> index 92262f3c3e37..bb6e296df317 100644 > >>> --- a/hw/9pfs/9p-local.c > >>> +++ b/hw/9pfs/9p-local.c > >>> @@ -54,11 +54,6 @@ int local_open_nofollow(FsContext *fs_ctx, const char *path, int flags, > >>> { > >>> LocalData *data = fs_ctx->private; > >>> > >>> - /* All paths are relative to the path data->mountfd points to */ > >>> - while (*path == '/') { > >>> - path++; > >>> - } > >> > >> Is it worth adding any assert()s in place of the deleted code? > >> > > > > The assert() added by this patch ensures that we never pass an empty > > string to relative_openat_nofollow(), which isn't related to this > > hunk of deleted code... so I'm not sure I understand the question :-\ > > I was thinking if it is worth replacing the deleted while() loop with an > > assert(*path != '/'); > > to prove that we have already taken care of ensuring a relative path. If you're talking about this one in relative_openat_nofollow(): assert(path[0] != '/'); well, it was there before and it was supposed to handle two things that should never happen: 1) passing an absolute path, which would cause openat() to ignore dirfd and access the absolute path in the host 2) having consecutive slashes in the path, which would cause "" to be passed to openat() and get ENOENT Maybe it would make more sense to handle 1) by moving the assert() to local_open_nofollow(), in place of the deleted loop. 2) is more a question of laziness... since all the paths that ever get there come from the backend code, I just assume that it is up to the backend to provide paths without consecutive slahes. But I guess, it shouldn't be hard to change the logic to deal with that gracefully. > You're right that you added another assert(*path) elsewhere in the > patch, and that one looked fine. >
diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c index 92262f3c3e37..bb6e296df317 100644 --- a/hw/9pfs/9p-local.c +++ b/hw/9pfs/9p-local.c @@ -54,11 +54,6 @@ int local_open_nofollow(FsContext *fs_ctx, const char *path, int flags, { LocalData *data = fs_ctx->private; - /* All paths are relative to the path data->mountfd points to */ - while (*path == '/') { - path++; - } - return relative_openat_nofollow(data->mountfd, path, flags, mode); } diff --git a/hw/9pfs/9p-util.c b/hw/9pfs/9p-util.c index fdb4d5737635..e46399a9119d 100644 --- a/hw/9pfs/9p-util.c +++ b/hw/9pfs/9p-util.c @@ -17,14 +17,11 @@ int relative_openat_nofollow(int dirfd, const char *path, int flags, mode_t mode) { - int fd; + int fd = dirfd; - fd = dup(dirfd); - if (fd == -1) { - return -1; - } + assert(*path); - while (*path) { + while (*path && fd != -1) { const char *c; int next_fd; char *head; @@ -33,25 +30,22 @@ int relative_openat_nofollow(int dirfd, const char *path, int flags, assert(path[0] != '/'); head = g_strdup(path); - c = strchr(path, '/'); - if (c) { + c = strchrnul(path, '/'); + if (*c) { + /* Intermediate path element */ head[c - path] = 0; + path = c + 1; next_fd = openat_dir(fd, head); } else { + /* Rightmost path element */ next_fd = openat_file(fd, head, flags, mode); + path = c; } g_free(head); - if (next_fd == -1) { + if (dirfd != fd) { close_preserve_errno(fd); - return -1; } - close(fd); fd = next_fd; - - if (!c) { - break; - } - path = c + 1; } return fd;
All paths in the virtfs directory now start with "./" (except the virtfs root itself which is exactly "."). We hence don't need to skip leading '/' characters anymore, nor to handle the empty path case. Also, since virtfs will only ever be supported on linux+glibc hosts, we can use strchrnul() and come up with a much simplier code to walk through the path elements. And we don't need to dup() the passed directory fd. Signed-off-by: Greg Kurz <groug@kaod.org> --- hw/9pfs/9p-local.c | 5 ----- hw/9pfs/9p-util.c | 26 ++++++++++---------------- 2 files changed, 10 insertions(+), 21 deletions(-)