Re: [Qemu-devel] [PATCH] 9pfs: fix readdir() for 9p2000.u

[Greg Kurz]

On Tue, 19 Sep 2017 16:28:58 +0200
Jan Dakinevich <address@hidden> wrote:

> -- >8 --  
> Greg,
> 
> Usually, I use 9p as rootfs and run qemu in one of these way. First,
> using initrd with built-in 9p modules:
> 
> $ sudo /path/to/qemu-system-x86_64 \
>     -machine accel=kvm -m 1G -nographic -vga none -serial mon:stdio \
>     -kernel $PWD/rootfs/boot/vmlinuz-3.16.0-4-amd64 \
>     -initrd $PWD/rootfs/boot/initrd.img-3.16.0-4-amd64 \
>     -append "root=rootfs rw rootfstype=9p 
> rootflags=trans=virtio,version=9p2000.u console=ttyS0" \
>     -fsdev local,id=fsdev0,path=$PWD/rootfs,security_model=passthrough \
>     -device virtio-9p-pci,fsdev=fsdev0,mount_tag=rootfs
> 
> Second, with compiled into the kernel 9p support and without initrd (but
> this way quite tricky):
> 
> $ sudo /path/to/qemu-system-x86_64 \
>     -machine accel=kvm -m 1G -nographic -vga none -serial mon:stdio \
>     -kernel /path/to/bzImage \
>     -append "root=/dev/root rw rootfstype=9p 
> rootflags=trans=virtio,version=9p2000.u console=ttyS0" \
>     -fsdev local,id=fsdev0,path=$PWD/rootfs,security_model=passthrough \
>     -device virtio-9p-pci,fsdev=fsdev0,mount_tag=/dev/root
> 

Thanks. I'll give a try.

> This patch mostly uses your commit message, because it is much more
> informative than I could ever suggest.
> 

No problem.

> --
> Best regards
> Jan Dakinevich
> -- >8 -- 

BTW, 'git am' takes all the mail body down to the '---' to build the commit
message, and assumes the patch starts at the first 'diff -'. Anything in
between is silently dropped.

This means that the commit for this patch had to be manually edited to
remove the '-- 8< --' ... '-- >8 --' part.

When you have something to add in the mail that shouldn't go into the
commit message, just put it between the '---' and the patch and it
will be ignored by 'git am' as explained above.

> If the client is using 9p2000.u, the following occurs:
> 
> $ cd ${virtfs_shared_dir}
> $ mkdir -p a/b/c
> $ ls a/b
> ls: cannot access 'a/b/a': No such file or directory
> ls: cannot access 'a/b/b': No such file or directory
> a  b  c
> 
> instead of the expected:
> 
> $ ls a/b
> c
> 
> This is a regression introduced by commit f57f5878578a;
> local_name_to_path() now resolves ".." and "." in paths,
> and v9fs_do_readdir_with_stat()->stat_to_v9stat() then
> copies the basename of the resulting path to the response.
> With the example above, this means that "." and ".." are
> turned into "b" and "a" respectively...
> 
> Actually, the name we need to pass is the d_name field of
> the dirent. Meanwhile, name argument of stat_to_v9stat is
> preserved, since it used to symbolic links resolving.
> 
> To satisfy stat_to_v9stat(), v9fs_stat() takes a care of the
> logic from old stat_to_v9stat() for determining basename of
> the given path.
> 
> Signed-off-by: Greg Kurz <address@hidden>

Even if we agreed on the list that this was the way to go, I'm not
the author, so it shouldn't contain my Signed-off-by yet... It will
have to when I repost it for the pull request though

Anyway, thanks for finding the issue and taking time to fix it.

I hope to merge this and the other patches soon.

Cheers,

--
Greg

> Signed-off-by: Jan Dakinevich <address@hidden>
> ---
>  hw/9pfs/9p.c | 18 +++++++-----------
>  1 file changed, 7 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 4d4ed85..6cb9dfc 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -804,11 +804,11 @@ static uint32_t stat_to_v9mode(const struct stat *stbuf)
>  }
>  
>  static int coroutine_fn stat_to_v9stat(V9fsPDU *pdu, V9fsPath *name,
> +                                       const char *basename,
>                                         const struct stat *stbuf,
>                                         V9fsStat *v9stat)
>  {
>      int err;
> -    const char *str;
>  
>      memset(v9stat, 0, sizeof(*v9stat));
>  
> @@ -842,14 +842,7 @@ static int coroutine_fn stat_to_v9stat(V9fsPDU *pdu, 
> V9fsPath *name,
>                  "HARDLINKCOUNT", (unsigned long)stbuf->st_nlink);
>      }
>  
> -    str = strrchr(name->data, '/');
> -    if (str) {
> -        str += 1;
> -    } else {
> -        str = name->data;
> -    }
> -
> -    v9fs_string_sprintf(&v9stat->name, "%s", str);
> +    v9fs_string_sprintf(&v9stat->name, "%s", basename);
>  
>      v9stat->size = 61 +
>          v9fs_string_size(&v9stat->name) +
> @@ -1058,6 +1051,7 @@ static void coroutine_fn v9fs_stat(void *opaque)
>      struct stat stbuf;
>      V9fsFidState *fidp;
>      V9fsPDU *pdu = opaque;
> +    char *basename;
>  
>      err = pdu_unmarshal(pdu, offset, "d", &fid);
>      if (err < 0) {
> @@ -1074,7 +1068,9 @@ static void coroutine_fn v9fs_stat(void *opaque)
>      if (err < 0) {
>          goto out;
>      }
> -    err = stat_to_v9stat(pdu, &fidp->path, &stbuf, &v9stat);
> +    basename = g_path_get_basename(fidp->path.data);
> +    err = stat_to_v9stat(pdu, &fidp->path, basename, &stbuf, &v9stat);
> +    g_free(basename);
>      if (err < 0) {
>          goto out;
>      }
> @@ -1750,7 +1746,7 @@ static int coroutine_fn 
> v9fs_do_readdir_with_stat(V9fsPDU *pdu,
>          if (err < 0) {
>              break;
>          }
> -        err = stat_to_v9stat(pdu, &path, &stbuf, &v9stat);
> +        err = stat_to_v9stat(pdu, &path, dent->d_name, &stbuf, &v9stat);
>          if (err < 0) {
>              break;
>          }



-- 
Gregory Kurz                                     address@hidden
                                                 address@hidden
Software Engineer @ IBM/LTC                      http://www.ibm.com
Tel 33-5-6218-1607

"Anarchy is about taking complete responsibility for yourself."
        Alan Moore.