Re: [Qemu-devel] [PATCH 3/5] 9pfs: local: simplify file opening

[Greg Kurz]

On Thu, 18 May 2017 09:23:07 -0500
Eric Blake  wrote:

> On 05/09/2017 04:23 AM, Greg Kurz wrote:
> > On Fri, 5 May 2017 12:01:55 -0500
> > Eric Blake  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 
> >>> ---
> >>>  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.
> 


pgpAC4KDfpHVl.pgp
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 3/5] 9pfs: local: simplify file opening

[Eric Blake]

On 05/09/2017 04:23 AM, Greg Kurz wrote:
> On Fri, 5 May 2017 12:01:55 -0500
> Eric Blake  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 
>>> ---
>>>  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.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 3/5] 9pfs: local: simplify file opening

[Greg Kurz]

On Tue, 9 May 2017 11:23:05 +0200
Greg Kurz  wrote:

> On Fri, 5 May 2017 12:01:55 -0500
> Eric Blake  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 
> > > ---
> > >  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.
> >   
> 



pgpTZlfGVTktU.pgp
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 3/5] 9pfs: local: simplify file opening

[Greg Kurz]

On Fri, 5 May 2017 12:01:55 -0500
Eric Blake  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 
> > ---
> >  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.
> 



pgpaewemNFMPU.pgp
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 3/5] 9pfs: local: simplify file opening

[Eric Blake]

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PATCH 3/5] 9pfs: local: simplify file opening

[Greg Kurz]

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 
---
 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++;
-}
-
 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;