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

[Greg Kurz]

On Thu, 18 May 2017 09:23:07 -0500
Eric Blake <address@hidden> wrote:

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

pgp2Z0he8TTaE.pgp
Description: OpenPGP digital signature