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

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

[Greg Kurz]

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.

Signed-off-by: Greg Kurz <groug@kaod.org>
---

Hi Jan,

I found this will testing your patches. I'd appreciate if you could
review this. Then I'll push all these patches to 9p-next and send
a pull request.

Thanks,

--
Greg

 hw/9pfs/9p.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 0a37c8bd1361..0474e9e787c0 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -1748,6 +1748,9 @@ static int coroutine_fn v9fs_do_readdir_with_stat(V9fsPDU *pdu,
         if (err < 0) {
             break;
         }
+        v9fs_path_free(&path);
+
+        v9fs_path_sprintf(&path, "%s", dent->d_name);
         err = stat_to_v9stat(pdu, &path, &stbuf, &v9stat);
         if (err < 0) {
             break;

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

[Jan Dakinevich]

Hi Greg,

I am also thinking about this... :-(

As I see, stat_to_v9stat() is also used for symbolic links resolving and
requires full path, not only d_name.

With your patch I have the following:

$ ln -s /usr usr
$ ls -l
ls: reading directory .: Invalid argument
total 0

On 09/18/2017 06:46 PM, Greg Kurz wrote:
> 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.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
> 
> Hi Jan,
> 
> I found this will testing your patches. I'd appreciate if you could
> review this. Then I'll push all these patches to 9p-next and send
> a pull request.
> 
> Thanks,
> 
> --
> Greg
> 
>  hw/9pfs/9p.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 0a37c8bd1361..0474e9e787c0 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -1748,6 +1748,9 @@ static int coroutine_fn v9fs_do_readdir_with_stat(V9fsPDU *pdu,
>          if (err < 0) {
>              break;
>          }
> +        v9fs_path_free(&path);
> +
> +        v9fs_path_sprintf(&path, "%s", dent->d_name);
>          err = stat_to_v9stat(pdu, &path, &stbuf, &v9stat);
>          if (err < 0) {
>              break;
> 

-- 
Best regards
Jan Dakinevich

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

[Greg Kurz]

On Mon, 18 Sep 2017 23:43:17 +0300
Jan Dakinevich <jan.dakinevich@gmail.com> wrote:

> ---
> Greg,
> 
> What do you think about this way?

I couldn't reproduce the issue with the symbolic link... can you
provide your QEMU command line and the mount options of the 9p
filesystem ?

Anyway, I had the very same patch in mind because only v9fs_stat() needs the
basename actually and it shouldn't be open-coded in stat_to_v9stat().

Please resend with proper changelog and Signed-off-by and I'll gladly
take it.

Cheers,

--
Greg

> ---
>  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 d152f5c..4697c00 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -826,11 +826,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));
>  
> @@ -864,14 +864,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) +
> @@ -1080,6 +1073,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) {
> @@ -1096,7 +1090,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;
>      }
> @@ -1772,7 +1768,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                                     kurzgreg@fr.ibm.com
                                                 gkurz@linux.vnet.ibm.com
Software Engineer @ IBM/LTC                      http://www.ibm.com
Tel 33-5-6218-1607

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

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

[Jan Dakinevich]

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

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

--
Best regards
Jan Dakinevich
-- >8 --
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 <groug@kaod.org>
Signed-off-by: Jan Dakinevich <jan.dakinevich@gmail.com>
---
 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;
         }
-- 
2.10.1

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

[Greg Kurz]

On Tue, 19 Sep 2017 16:28:58 +0200
Jan Dakinevich <jan.dakinevich@gmail.com> 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 <groug@kaod.org>

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 <jan.dakinevich@gmail.com>
> ---
>  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                                     kurzgreg@fr.ibm.com
                                                 gkurz@linux.vnet.ibm.com
Software Engineer @ IBM/LTC                      http://www.ibm.com
Tel 33-5-6218-1607

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