[PATCH for-9.0 0/3] qtest/virtio-9p-test.c: fix slow tests

[PATCH for-9.0 0/3] qtest/virtio-9p-test.c: fix slow tests

[Daniel Henrique Barboza]

Hi,

Thomas reported in [1] a problem that happened with the RISC-V machine
where some tests from virtio-9p-test.c were failing with '-m slow', i.e.
enabling slow tests.

In the end it wasn't a RISC-V specific problem. It just so happens that
the recently added riscv machine nodes runs the tests from
virtio-9p-test two times for each qos-test run: one with the
virtio-9p-device device and another with the virtio-9p-pci. The temp dir
for these tests is being created at the start of qos-test and removed
only at the end of qos-test, and the tests are leaving dirs and files
behind. virtio-9-device tests run first, creates stuff in the temp dir,
then when virtio-9p-pci tests runs again it'll fail because the previous
run left created dirs and files in the same temp dir. Here's a run that
exemplifies the problem:

$ MALLOC_PERTURB_=21 V=2 QTEST_QEMU_BINARY=./qemu-system-riscv64 ./tests/qtest/qos-test -m slow
(...)
# starting QEMU: exec ./qemu-system-riscv64 -qtest unix:/tmp/qtest-621710.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-621710.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -M virt,aclint=on,aia=aplic-imsic -fsdev local,id=fsdev0,path='/home/danielhb/work/qemu/build/qtest-9p-local-7E16K2',security_model=mapped-xattr -device virtio-9p-device,fsdev=fsdev0,mount_tag=qtest -accel qtest
( goes ok ...)
# starting QEMU: exec ./qemu-system-riscv64 -qtest unix:/tmp/qtest-621710.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-621710.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -M virt,aclint=on,aia=aplic-imsic -fsdev local,id=fsdev0,path='/home/danielhb/work/qemu/build/qtest-9p-local-7E16K2',security_model=mapped-xattr -device virtio-9p-pci,fsdev=fsdev0,addr=04.0,mount_tag=qtest -accel qtest
ok 168 /riscv64/virt/generic-pcihost/pci-bus-generic/pci-bus/virtio-9p-pci/virtio-9p/virtio-9p-tests/local/config
Received response 7 (RLERROR) instead of 73 (RMKDIR)
Rlerror has errno 17 (File exists)
**
ERROR:../tests/qtest/libqos/virtio-9p-client.c:275:v9fs_req_recv: assertion failed (hdr.id == id): (7 == 73)

As we can see we're running both 'virtio-9p-device' tests and 'virtio-9p-pci'
tests using the same '/home/danielhb/work/qemu/build/qtest-9p-local-7E16K2'
temp dir. 

The quick fix I came up with was to make each test clean themselves up
after each run. The tests were also consolidated, i.e. fewer tests with the
same coverage, because the 'unlikat' tests were doing the same thing the
'create' tests were doing but removing stuff after. Might as well keep just
the 'unlikat' tests.

I also went ahead and reverted 558f5c42efd ("tests/9pfs: Mark "local"
tests as "slow"") after realizing that the problem I was fixing is also
the same problem that this patch was trying to working around with the
skip [2]. I validated this change in this Gitlab pipeline:

https://gitlab.com/danielhb/qemu/-/pipelines/1227953967

[1] https://mail.gnu.org/archive/html/qemu-devel/2024-03/msg05807.html
[2] https://lists.nongnu.org/archive/html/qemu-devel/2020-11/msg05510.html

Daniel Henrique Barboza (3):
  qtest/virtio-9p-test.c: consolidate create dir, file and symlink tests
  qtest/virtio-9p-test.c: consolidate hardlink tests
  qtest/virtio-9p-test.c: remove g_test_slow() gate

 tests/qtest/virtio-9p-test.c | 155 +++++++++++------------------------
 1 file changed, 48 insertions(+), 107 deletions(-)

-- 
2.44.0

[PATCH for-9.0 1/3] qtest/virtio-9p-test.c: consolidate create dir, file and symlink tests

[Daniel Henrique Barboza]

The local 9p driver in virtio-9p-test.c its temporary dir right at the
start of qos-test (via virtio_9p_create_local_test_dir()) and only
deletes it after qos-test is finished (via
virtio_9p_remove_local_test_dir()).

This means that any qos-test machine that ends up running virtio-9p-test local
tests more than once will end up re-using the same temp dir. This is
what's happening in [1] after we introduced the riscv machine nodes: if
we enable slow tests with the '-m slow' flag using qemu-system-riscv64,
this is what happens:

- a temp dir is created, e.g. qtest-9p-local-WZLDL2;

- virtio-9p-device tests will run virtio-9p-test successfully;

- virtio-9p-pci tests will run virtio-9p-test, and fail right at the
  first slow test at fs_create_dir() because the "01" file was already
  created by fs_create_dir() test when running with the virtio-9p-device.

We can fix it by making every test clean up their changes in the
filesystem after they're done. But we don't need every test either:
what fs_create_file() does is already exercised in fs_unlinkat_dir(),
i.e. a dir is created, verified to be created, and then removed. Fixing
fs_create_file() would turn it into fs_unlikat_dir(), so we don't need
both. The same theme follows every test in virtio-9p-test.c, where the
'unlikat' variant does the same thing the 'create' does but with some
cleaning in the end.

Consolide some tests as follows:

- fs_create_dir() is removed. fs_unlinkat_dir() is renamed to
  fs_create_unlinkat_dir();

- fs_create_file() is removed. fs_unlinkat_file() is renamed to
  fs_create_unlinkat_file(). The "04" dir it uses is now being removed;

- fs_symlink_file() is removed. fs_unlinkat_symlink() is renamed to
  fs_create_unlinkat_symlink(). Both "real_file" and the "06" dir it
  creates is now being removed.

We're still missing the 'hardlink' tests. We'll do it in the next patch
since it's less trivial to consolidate than these.

[1] https://mail.gnu.org/archive/html/qemu-devel/2024-03/msg05807.html

Reported-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 tests/qtest/virtio-9p-test.c | 97 +++++++++++-------------------------
 1 file changed, 29 insertions(+), 68 deletions(-)

diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index 65e69491e5..cdbe3e78ea 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -506,26 +506,8 @@ static void fs_readdir_split_512(void *obj, void *data,
 
 /* tests using the 9pfs 'local' fs driver */
 
-static void fs_create_dir(void *obj, void *data, QGuestAllocator *t_alloc)
-{
-    QVirtio9P *v9p = obj;
-    v9fs_set_allocator(t_alloc);
-    struct stat st;
-    g_autofree char *root_path = virtio_9p_test_path("");
-    g_autofree char *new_dir = virtio_9p_test_path("01");
-
-    g_assert(root_path != NULL);
-
-    tattach({ .client = v9p });
-    tmkdir({ .client = v9p, .atPath = "/", .name = "01" });
-
-    /* check if created directory really exists now ... */
-    g_assert(stat(new_dir, &st) == 0);
-    /* ... and is actually a directory */
-    g_assert((st.st_mode & S_IFMT) == S_IFDIR);
-}
-
-static void fs_unlinkat_dir(void *obj, void *data, QGuestAllocator *t_alloc)
+static void fs_create_unlinkat_dir(void *obj, void *data,
+                                   QGuestAllocator *t_alloc)
 {
     QVirtio9P *v9p = obj;
     v9fs_set_allocator(t_alloc);
@@ -551,28 +533,13 @@ static void fs_unlinkat_dir(void *obj, void *data, QGuestAllocator *t_alloc)
     g_assert(stat(new_dir, &st) != 0);
 }
 
-static void fs_create_file(void *obj, void *data, QGuestAllocator *t_alloc)
-{
-    QVirtio9P *v9p = obj;
-    v9fs_set_allocator(t_alloc);
-    struct stat st;
-    g_autofree char *new_file = virtio_9p_test_path("03/1st_file");
-
-    tattach({ .client = v9p });
-    tmkdir({ .client = v9p, .atPath = "/", .name = "03" });
-    tlcreate({ .client = v9p, .atPath = "03", .name = "1st_file" });
-
-    /* check if created file exists now ... */
-    g_assert(stat(new_file, &st) == 0);
-    /* ... and is a regular file */
-    g_assert((st.st_mode & S_IFMT) == S_IFREG);
-}
-
-static void fs_unlinkat_file(void *obj, void *data, QGuestAllocator *t_alloc)
+static void fs_create_unlinkat_file(void *obj, void *data,
+                                    QGuestAllocator *t_alloc)
 {
     QVirtio9P *v9p = obj;
     v9fs_set_allocator(t_alloc);
     struct stat st;
+    g_autofree char *new_dir = virtio_9p_test_path("04");
     g_autofree char *new_file = virtio_9p_test_path("04/doa_file");
 
     tattach({ .client = v9p });
@@ -587,37 +554,22 @@ static void fs_unlinkat_file(void *obj, void *data, QGuestAllocator *t_alloc)
     tunlinkat({ .client = v9p, .atPath = "04", .name = "doa_file" });
     /* file should be gone now */
     g_assert(stat(new_file, &st) != 0);
-}
-
-static void fs_symlink_file(void *obj, void *data, QGuestAllocator *t_alloc)
-{
-    QVirtio9P *v9p = obj;
-    v9fs_set_allocator(t_alloc);
-    struct stat st;
-    g_autofree char *real_file = virtio_9p_test_path("05/real_file");
-    g_autofree char *symlink_file = virtio_9p_test_path("05/symlink_file");
 
-    tattach({ .client = v9p });
-    tmkdir({ .client = v9p, .atPath = "/", .name = "05" });
-    tlcreate({ .client = v9p, .atPath = "05", .name = "real_file" });
-    g_assert(stat(real_file, &st) == 0);
-    g_assert((st.st_mode & S_IFMT) == S_IFREG);
-
-    tsymlink({
-        .client = v9p, .atPath = "05", .name = "symlink_file",
-        .symtgt = "real_file"
+    /* also cleanup dir*/
+    tunlinkat({
+        .client = v9p, .atPath = "/", .name = "04",
+        .flags = P9_DOTL_AT_REMOVEDIR
     });
-
-    /* check if created link exists now */
-    g_assert(stat(symlink_file, &st) == 0);
+    g_assert(stat(new_dir, &st) != 0);
 }
 
-static void fs_unlinkat_symlink(void *obj, void *data,
-                                QGuestAllocator *t_alloc)
+static void fs_create_unlinkat_symlink(void *obj, void *data,
+                                       QGuestAllocator *t_alloc)
 {
     QVirtio9P *v9p = obj;
     v9fs_set_allocator(t_alloc);
     struct stat st;
+    g_autofree char *new_dir = virtio_9p_test_path("06");
     g_autofree char *real_file = virtio_9p_test_path("06/real_file");
     g_autofree char *symlink_file = virtio_9p_test_path("06/symlink_file");
 
@@ -636,6 +588,16 @@ static void fs_unlinkat_symlink(void *obj, void *data,
     tunlinkat({ .client = v9p, .atPath = "06", .name = "symlink_file" });
     /* symlink should be gone now */
     g_assert(stat(symlink_file, &st) != 0);
+
+    /* remove real file and dir */
+    tunlinkat({ .client = v9p, .atPath = "06", .name = "real_file" });
+    g_assert(stat(real_file, &st) != 0);
+
+    tunlinkat({
+        .client = v9p, .atPath = "/", .name = "06",
+        .flags = P9_DOTL_AT_REMOVEDIR
+    });
+    g_assert(stat(new_dir, &st) != 0);
 }
 
 static void fs_hardlink_file(void *obj, void *data, QGuestAllocator *t_alloc)
@@ -746,13 +708,12 @@ static void register_virtio_9p_test(void)
 
     opts.before = assign_9p_local_driver;
     qos_add_test("local/config", "virtio-9p", pci_config,  &opts);
-    qos_add_test("local/create_dir", "virtio-9p", fs_create_dir, &opts);
-    qos_add_test("local/unlinkat_dir", "virtio-9p", fs_unlinkat_dir, &opts);
-    qos_add_test("local/create_file", "virtio-9p", fs_create_file, &opts);
-    qos_add_test("local/unlinkat_file", "virtio-9p", fs_unlinkat_file, &opts);
-    qos_add_test("local/symlink_file", "virtio-9p", fs_symlink_file, &opts);
-    qos_add_test("local/unlinkat_symlink", "virtio-9p", fs_unlinkat_symlink,
-                 &opts);
+    qos_add_test("local/create_unlinkat_dir", "virtio-9p",
+                 fs_create_unlinkat_dir, &opts);
+    qos_add_test("local/create_unlinkat_file", "virtio-9p",
+                 fs_create_unlinkat_file, &opts);
+    qos_add_test("local/create_unlinkat_symlink", "virtio-9p",
+                 fs_create_unlinkat_symlink, &opts);
     qos_add_test("local/hardlink_file", "virtio-9p", fs_hardlink_file, &opts);
     qos_add_test("local/unlinkat_hardlink", "virtio-9p", fs_unlinkat_hardlink,
                  &opts);
-- 
2.44.0

Re: [PATCH for-9.0 1/3] qtest/virtio-9p-test.c: consolidate create dir, file and symlink tests

[Greg Kurz]

On Tue, 26 Mar 2024 10:26:04 -0300
Daniel Henrique Barboza <dbarboza@ventanamicro.com> wrote:

> The local 9p driver in virtio-9p-test.c its temporary dir right at the
> start of qos-test (via virtio_9p_create_local_test_dir()) and only
> deletes it after qos-test is finished (via
> virtio_9p_remove_local_test_dir()).
> 
> This means that any qos-test machine that ends up running virtio-9p-test local
> tests more than once will end up re-using the same temp dir. This is
> what's happening in [1] after we introduced the riscv machine nodes: if
> we enable slow tests with the '-m slow' flag using qemu-system-riscv64,
> this is what happens:
> 
> - a temp dir is created, e.g. qtest-9p-local-WZLDL2;
> 
> - virtio-9p-device tests will run virtio-9p-test successfully;
> 
> - virtio-9p-pci tests will run virtio-9p-test, and fail right at the
>   first slow test at fs_create_dir() because the "01" file was already
>   created by fs_create_dir() test when running with the virtio-9p-device.
> 
> We can fix it by making every test clean up their changes in the
> filesystem after they're done. But we don't need every test either:
> what fs_create_file() does is already exercised in fs_unlinkat_dir(),
> i.e. a dir is created, verified to be created, and then removed. Fixing
> fs_create_file() would turn it into fs_unlikat_dir(), so we don't need
> both. The same theme follows every test in virtio-9p-test.c, where the
> 'unlikat' variant does the same thing the 'create' does but with some
> cleaning in the end.
> 
> Consolide some tests as follows:
> 
> - fs_create_dir() is removed. fs_unlinkat_dir() is renamed to
>   fs_create_unlinkat_dir();
> 
> - fs_create_file() is removed. fs_unlinkat_file() is renamed to
>   fs_create_unlinkat_file(). The "04" dir it uses is now being removed;
> 
> - fs_symlink_file() is removed. fs_unlinkat_symlink() is renamed to
>   fs_create_unlinkat_symlink(). Both "real_file" and the "06" dir it
>   creates is now being removed.
> 

The  change looks good functionally but it breaks the legitimate assumption
that files "06/*" come from test #6 and so on... I think you should consider
renumbering to avoid confusion when debugging logs.

Since this will bring more hunks, please split this in enough reviewable
patches.

Cheers,

--
Greg

> We're still missing the 'hardlink' tests. We'll do it in the next patch
> since it's less trivial to consolidate than these.
> 
> [1] https://mail.gnu.org/archive/html/qemu-devel/2024-03/msg05807.html
> 
> Reported-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  tests/qtest/virtio-9p-test.c | 97 +++++++++++-------------------------
>  1 file changed, 29 insertions(+), 68 deletions(-)
> 
> diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
> index 65e69491e5..cdbe3e78ea 100644
> --- a/tests/qtest/virtio-9p-test.c
> +++ b/tests/qtest/virtio-9p-test.c
> @@ -506,26 +506,8 @@ static void fs_readdir_split_512(void *obj, void *data,
>  
>  /* tests using the 9pfs 'local' fs driver */
>  
> -static void fs_create_dir(void *obj, void *data, QGuestAllocator *t_alloc)
> -{
> -    QVirtio9P *v9p = obj;
> -    v9fs_set_allocator(t_alloc);
> -    struct stat st;
> -    g_autofree char *root_path = virtio_9p_test_path("");
> -    g_autofree char *new_dir = virtio_9p_test_path("01");
> -
> -    g_assert(root_path != NULL);
> -
> -    tattach({ .client = v9p });
> -    tmkdir({ .client = v9p, .atPath = "/", .name = "01" });
> -
> -    /* check if created directory really exists now ... */
> -    g_assert(stat(new_dir, &st) == 0);
> -    /* ... and is actually a directory */
> -    g_assert((st.st_mode & S_IFMT) == S_IFDIR);
> -}
> -
> -static void fs_unlinkat_dir(void *obj, void *data, QGuestAllocator *t_alloc)
> +static void fs_create_unlinkat_dir(void *obj, void *data,
> +                                   QGuestAllocator *t_alloc)
>  {
>      QVirtio9P *v9p = obj;
>      v9fs_set_allocator(t_alloc);
> @@ -551,28 +533,13 @@ static void fs_unlinkat_dir(void *obj, void *data, QGuestAllocator *t_alloc)
>      g_assert(stat(new_dir, &st) != 0);
>  }
>  
> -static void fs_create_file(void *obj, void *data, QGuestAllocator *t_alloc)
> -{
> -    QVirtio9P *v9p = obj;
> -    v9fs_set_allocator(t_alloc);
> -    struct stat st;
> -    g_autofree char *new_file = virtio_9p_test_path("03/1st_file");
> -
> -    tattach({ .client = v9p });
> -    tmkdir({ .client = v9p, .atPath = "/", .name = "03" });
> -    tlcreate({ .client = v9p, .atPath = "03", .name = "1st_file" });
> -
> -    /* check if created file exists now ... */
> -    g_assert(stat(new_file, &st) == 0);
> -    /* ... and is a regular file */
> -    g_assert((st.st_mode & S_IFMT) == S_IFREG);
> -}
> -
> -static void fs_unlinkat_file(void *obj, void *data, QGuestAllocator *t_alloc)
> +static void fs_create_unlinkat_file(void *obj, void *data,
> +                                    QGuestAllocator *t_alloc)
>  {
>      QVirtio9P *v9p = obj;
>      v9fs_set_allocator(t_alloc);
>      struct stat st;
> +    g_autofree char *new_dir = virtio_9p_test_path("04");
>      g_autofree char *new_file = virtio_9p_test_path("04/doa_file");
>  
>      tattach({ .client = v9p });
> @@ -587,37 +554,22 @@ static void fs_unlinkat_file(void *obj, void *data, QGuestAllocator *t_alloc)
>      tunlinkat({ .client = v9p, .atPath = "04", .name = "doa_file" });
>      /* file should be gone now */
>      g_assert(stat(new_file, &st) != 0);
> -}
> -
> -static void fs_symlink_file(void *obj, void *data, QGuestAllocator *t_alloc)
> -{
> -    QVirtio9P *v9p = obj;
> -    v9fs_set_allocator(t_alloc);
> -    struct stat st;
> -    g_autofree char *real_file = virtio_9p_test_path("05/real_file");
> -    g_autofree char *symlink_file = virtio_9p_test_path("05/symlink_file");
>  
> -    tattach({ .client = v9p });
> -    tmkdir({ .client = v9p, .atPath = "/", .name = "05" });
> -    tlcreate({ .client = v9p, .atPath = "05", .name = "real_file" });
> -    g_assert(stat(real_file, &st) == 0);
> -    g_assert((st.st_mode & S_IFMT) == S_IFREG);
> -
> -    tsymlink({
> -        .client = v9p, .atPath = "05", .name = "symlink_file",
> -        .symtgt = "real_file"
> +    /* also cleanup dir*/
> +    tunlinkat({
> +        .client = v9p, .atPath = "/", .name = "04",
> +        .flags = P9_DOTL_AT_REMOVEDIR
>      });
> -
> -    /* check if created link exists now */
> -    g_assert(stat(symlink_file, &st) == 0);
> +    g_assert(stat(new_dir, &st) != 0);
>  }
>  
> -static void fs_unlinkat_symlink(void *obj, void *data,
> -                                QGuestAllocator *t_alloc)
> +static void fs_create_unlinkat_symlink(void *obj, void *data,
> +                                       QGuestAllocator *t_alloc)
>  {
>      QVirtio9P *v9p = obj;
>      v9fs_set_allocator(t_alloc);
>      struct stat st;
> +    g_autofree char *new_dir = virtio_9p_test_path("06");
>      g_autofree char *real_file = virtio_9p_test_path("06/real_file");
>      g_autofree char *symlink_file = virtio_9p_test_path("06/symlink_file");
>  
> @@ -636,6 +588,16 @@ static void fs_unlinkat_symlink(void *obj, void *data,
>      tunlinkat({ .client = v9p, .atPath = "06", .name = "symlink_file" });
>      /* symlink should be gone now */
>      g_assert(stat(symlink_file, &st) != 0);
> +
> +    /* remove real file and dir */
> +    tunlinkat({ .client = v9p, .atPath = "06", .name = "real_file" });
> +    g_assert(stat(real_file, &st) != 0);
> +
> +    tunlinkat({
> +        .client = v9p, .atPath = "/", .name = "06",
> +        .flags = P9_DOTL_AT_REMOVEDIR
> +    });
> +    g_assert(stat(new_dir, &st) != 0);
>  }
>  
>  static void fs_hardlink_file(void *obj, void *data, QGuestAllocator *t_alloc)
> @@ -746,13 +708,12 @@ static void register_virtio_9p_test(void)
>  
>      opts.before = assign_9p_local_driver;
>      qos_add_test("local/config", "virtio-9p", pci_config,  &opts);
> -    qos_add_test("local/create_dir", "virtio-9p", fs_create_dir, &opts);
> -    qos_add_test("local/unlinkat_dir", "virtio-9p", fs_unlinkat_dir, &opts);
> -    qos_add_test("local/create_file", "virtio-9p", fs_create_file, &opts);
> -    qos_add_test("local/unlinkat_file", "virtio-9p", fs_unlinkat_file, &opts);
> -    qos_add_test("local/symlink_file", "virtio-9p", fs_symlink_file, &opts);
> -    qos_add_test("local/unlinkat_symlink", "virtio-9p", fs_unlinkat_symlink,
> -                 &opts);
> +    qos_add_test("local/create_unlinkat_dir", "virtio-9p",
> +                 fs_create_unlinkat_dir, &opts);
> +    qos_add_test("local/create_unlinkat_file", "virtio-9p",
> +                 fs_create_unlinkat_file, &opts);
> +    qos_add_test("local/create_unlinkat_symlink", "virtio-9p",
> +                 fs_create_unlinkat_symlink, &opts);
>      qos_add_test("local/hardlink_file", "virtio-9p", fs_hardlink_file, &opts);
>      qos_add_test("local/unlinkat_hardlink", "virtio-9p", fs_unlinkat_hardlink,
>                   &opts);



-- 
Greg

Re: [PATCH for-9.0 1/3] qtest/virtio-9p-test.c: consolidate create dir, file and symlink tests

[Daniel Henrique Barboza]

On 3/26/24 14:05, Greg Kurz wrote:
> On Tue, 26 Mar 2024 10:26:04 -0300
> Daniel Henrique Barboza <dbarboza@ventanamicro.com> wrote:
> 
>> The local 9p driver in virtio-9p-test.c its temporary dir right at the
>> start of qos-test (via virtio_9p_create_local_test_dir()) and only
>> deletes it after qos-test is finished (via
>> virtio_9p_remove_local_test_dir()).
>>
>> This means that any qos-test machine that ends up running virtio-9p-test local
>> tests more than once will end up re-using the same temp dir. This is
>> what's happening in [1] after we introduced the riscv machine nodes: if
>> we enable slow tests with the '-m slow' flag using qemu-system-riscv64,
>> this is what happens:
>>
>> - a temp dir is created, e.g. qtest-9p-local-WZLDL2;
>>
>> - virtio-9p-device tests will run virtio-9p-test successfully;
>>
>> - virtio-9p-pci tests will run virtio-9p-test, and fail right at the
>>    first slow test at fs_create_dir() because the "01" file was already
>>    created by fs_create_dir() test when running with the virtio-9p-device.
>>
>> We can fix it by making every test clean up their changes in the
>> filesystem after they're done. But we don't need every test either:
>> what fs_create_file() does is already exercised in fs_unlinkat_dir(),
>> i.e. a dir is created, verified to be created, and then removed. Fixing
>> fs_create_file() would turn it into fs_unlikat_dir(), so we don't need
>> both. The same theme follows every test in virtio-9p-test.c, where the
>> 'unlikat' variant does the same thing the 'create' does but with some
>> cleaning in the end.
>>
>> Consolide some tests as follows:
>>
>> - fs_create_dir() is removed. fs_unlinkat_dir() is renamed to
>>    fs_create_unlinkat_dir();
>>
>> - fs_create_file() is removed. fs_unlinkat_file() is renamed to
>>    fs_create_unlinkat_file(). The "04" dir it uses is now being removed;
>>
>> - fs_symlink_file() is removed. fs_unlinkat_symlink() is renamed to
>>    fs_create_unlinkat_symlink(). Both "real_file" and the "06" dir it
>>    creates is now being removed.
>>
> 
> The  change looks good functionally but it breaks the legitimate assumption
> that files "06/*" come from test #6 and so on... I think you should consider
> renumbering to avoid confusion when debugging logs.
> 
> Since this will bring more hunks, please split this in enough reviewable
> patches.

Fair enough. Let me cook a v2. Thanks,


Daniel

> 
> Cheers,
> 
> --
> Greg
> 
>> We're still missing the 'hardlink' tests. We'll do it in the next patch
>> since it's less trivial to consolidate than these.
>>
>> [1] https://mail.gnu.org/archive/html/qemu-devel/2024-03/msg05807.html
>>
>> Reported-by: Thomas Huth <thuth@redhat.com>
>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>> ---
>>   tests/qtest/virtio-9p-test.c | 97 +++++++++++-------------------------
>>   1 file changed, 29 insertions(+), 68 deletions(-)
>>
>> diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
>> index 65e69491e5..cdbe3e78ea 100644
>> --- a/tests/qtest/virtio-9p-test.c
>> +++ b/tests/qtest/virtio-9p-test.c
>> @@ -506,26 +506,8 @@ static void fs_readdir_split_512(void *obj, void *data,
>>   
>>   /* tests using the 9pfs 'local' fs driver */
>>   
>> -static void fs_create_dir(void *obj, void *data, QGuestAllocator *t_alloc)
>> -{
>> -    QVirtio9P *v9p = obj;
>> -    v9fs_set_allocator(t_alloc);
>> -    struct stat st;
>> -    g_autofree char *root_path = virtio_9p_test_path("");
>> -    g_autofree char *new_dir = virtio_9p_test_path("01");
>> -
>> -    g_assert(root_path != NULL);
>> -
>> -    tattach({ .client = v9p });
>> -    tmkdir({ .client = v9p, .atPath = "/", .name = "01" });
>> -
>> -    /* check if created directory really exists now ... */
>> -    g_assert(stat(new_dir, &st) == 0);
>> -    /* ... and is actually a directory */
>> -    g_assert((st.st_mode & S_IFMT) == S_IFDIR);
>> -}
>> -
>> -static void fs_unlinkat_dir(void *obj, void *data, QGuestAllocator *t_alloc)
>> +static void fs_create_unlinkat_dir(void *obj, void *data,
>> +                                   QGuestAllocator *t_alloc)
>>   {
>>       QVirtio9P *v9p = obj;
>>       v9fs_set_allocator(t_alloc);
>> @@ -551,28 +533,13 @@ static void fs_unlinkat_dir(void *obj, void *data, QGuestAllocator *t_alloc)
>>       g_assert(stat(new_dir, &st) != 0);
>>   }
>>   
>> -static void fs_create_file(void *obj, void *data, QGuestAllocator *t_alloc)
>> -{
>> -    QVirtio9P *v9p = obj;
>> -    v9fs_set_allocator(t_alloc);
>> -    struct stat st;
>> -    g_autofree char *new_file = virtio_9p_test_path("03/1st_file");
>> -
>> -    tattach({ .client = v9p });
>> -    tmkdir({ .client = v9p, .atPath = "/", .name = "03" });
>> -    tlcreate({ .client = v9p, .atPath = "03", .name = "1st_file" });
>> -
>> -    /* check if created file exists now ... */
>> -    g_assert(stat(new_file, &st) == 0);
>> -    /* ... and is a regular file */
>> -    g_assert((st.st_mode & S_IFMT) == S_IFREG);
>> -}
>> -
>> -static void fs_unlinkat_file(void *obj, void *data, QGuestAllocator *t_alloc)
>> +static void fs_create_unlinkat_file(void *obj, void *data,
>> +                                    QGuestAllocator *t_alloc)
>>   {
>>       QVirtio9P *v9p = obj;
>>       v9fs_set_allocator(t_alloc);
>>       struct stat st;
>> +    g_autofree char *new_dir = virtio_9p_test_path("04");
>>       g_autofree char *new_file = virtio_9p_test_path("04/doa_file");
>>   
>>       tattach({ .client = v9p });
>> @@ -587,37 +554,22 @@ static void fs_unlinkat_file(void *obj, void *data, QGuestAllocator *t_alloc)
>>       tunlinkat({ .client = v9p, .atPath = "04", .name = "doa_file" });
>>       /* file should be gone now */
>>       g_assert(stat(new_file, &st) != 0);
>> -}
>> -
>> -static void fs_symlink_file(void *obj, void *data, QGuestAllocator *t_alloc)
>> -{
>> -    QVirtio9P *v9p = obj;
>> -    v9fs_set_allocator(t_alloc);
>> -    struct stat st;
>> -    g_autofree char *real_file = virtio_9p_test_path("05/real_file");
>> -    g_autofree char *symlink_file = virtio_9p_test_path("05/symlink_file");
>>   
>> -    tattach({ .client = v9p });
>> -    tmkdir({ .client = v9p, .atPath = "/", .name = "05" });
>> -    tlcreate({ .client = v9p, .atPath = "05", .name = "real_file" });
>> -    g_assert(stat(real_file, &st) == 0);
>> -    g_assert((st.st_mode & S_IFMT) == S_IFREG);
>> -
>> -    tsymlink({
>> -        .client = v9p, .atPath = "05", .name = "symlink_file",
>> -        .symtgt = "real_file"
>> +    /* also cleanup dir*/
>> +    tunlinkat({
>> +        .client = v9p, .atPath = "/", .name = "04",
>> +        .flags = P9_DOTL_AT_REMOVEDIR
>>       });
>> -
>> -    /* check if created link exists now */
>> -    g_assert(stat(symlink_file, &st) == 0);
>> +    g_assert(stat(new_dir, &st) != 0);
>>   }
>>   
>> -static void fs_unlinkat_symlink(void *obj, void *data,
>> -                                QGuestAllocator *t_alloc)
>> +static void fs_create_unlinkat_symlink(void *obj, void *data,
>> +                                       QGuestAllocator *t_alloc)
>>   {
>>       QVirtio9P *v9p = obj;
>>       v9fs_set_allocator(t_alloc);
>>       struct stat st;
>> +    g_autofree char *new_dir = virtio_9p_test_path("06");
>>       g_autofree char *real_file = virtio_9p_test_path("06/real_file");
>>       g_autofree char *symlink_file = virtio_9p_test_path("06/symlink_file");
>>   
>> @@ -636,6 +588,16 @@ static void fs_unlinkat_symlink(void *obj, void *data,
>>       tunlinkat({ .client = v9p, .atPath = "06", .name = "symlink_file" });
>>       /* symlink should be gone now */
>>       g_assert(stat(symlink_file, &st) != 0);
>> +
>> +    /* remove real file and dir */
>> +    tunlinkat({ .client = v9p, .atPath = "06", .name = "real_file" });
>> +    g_assert(stat(real_file, &st) != 0);
>> +
>> +    tunlinkat({
>> +        .client = v9p, .atPath = "/", .name = "06",
>> +        .flags = P9_DOTL_AT_REMOVEDIR
>> +    });
>> +    g_assert(stat(new_dir, &st) != 0);
>>   }
>>   
>>   static void fs_hardlink_file(void *obj, void *data, QGuestAllocator *t_alloc)
>> @@ -746,13 +708,12 @@ static void register_virtio_9p_test(void)
>>   
>>       opts.before = assign_9p_local_driver;
>>       qos_add_test("local/config", "virtio-9p", pci_config,  &opts);
>> -    qos_add_test("local/create_dir", "virtio-9p", fs_create_dir, &opts);
>> -    qos_add_test("local/unlinkat_dir", "virtio-9p", fs_unlinkat_dir, &opts);
>> -    qos_add_test("local/create_file", "virtio-9p", fs_create_file, &opts);
>> -    qos_add_test("local/unlinkat_file", "virtio-9p", fs_unlinkat_file, &opts);
>> -    qos_add_test("local/symlink_file", "virtio-9p", fs_symlink_file, &opts);
>> -    qos_add_test("local/unlinkat_symlink", "virtio-9p", fs_unlinkat_symlink,
>> -                 &opts);
>> +    qos_add_test("local/create_unlinkat_dir", "virtio-9p",
>> +                 fs_create_unlinkat_dir, &opts);
>> +    qos_add_test("local/create_unlinkat_file", "virtio-9p",
>> +                 fs_create_unlinkat_file, &opts);
>> +    qos_add_test("local/create_unlinkat_symlink", "virtio-9p",
>> +                 fs_create_unlinkat_symlink, &opts);
>>       qos_add_test("local/hardlink_file", "virtio-9p", fs_hardlink_file, &opts);
>>       qos_add_test("local/unlinkat_hardlink", "virtio-9p", fs_unlinkat_hardlink,
>>                    &opts);
> 
> 
> 

Re: [PATCH for-9.0 1/3] qtest/virtio-9p-test.c: consolidate create dir, file and symlink tests

[Christian Schoenebeck]

On Tuesday, March 26, 2024 6:47:17 PM CET Daniel Henrique Barboza wrote:
> On 3/26/24 14:05, Greg Kurz wrote:
> > On Tue, 26 Mar 2024 10:26:04 -0300
> > Daniel Henrique Barboza <dbarboza@ventanamicro.com> wrote:
> > 
> >> The local 9p driver in virtio-9p-test.c its temporary dir right at the
> >> start of qos-test (via virtio_9p_create_local_test_dir()) and only
> >> deletes it after qos-test is finished (via
> >> virtio_9p_remove_local_test_dir()).
> >>
> >> This means that any qos-test machine that ends up running virtio-9p-test local
> >> tests more than once will end up re-using the same temp dir. This is
> >> what's happening in [1] after we introduced the riscv machine nodes: if
> >> we enable slow tests with the '-m slow' flag using qemu-system-riscv64,
> >> this is what happens:
> >>
> >> - a temp dir is created, e.g. qtest-9p-local-WZLDL2;
> >>
> >> - virtio-9p-device tests will run virtio-9p-test successfully;
> >>
> >> - virtio-9p-pci tests will run virtio-9p-test, and fail right at the
> >>    first slow test at fs_create_dir() because the "01" file was already
> >>    created by fs_create_dir() test when running with the virtio-9p-device.
> >>
> >> We can fix it by making every test clean up their changes in the
> >> filesystem after they're done. But we don't need every test either:
> >> what fs_create_file() does is already exercised in fs_unlinkat_dir(),
> >> i.e. a dir is created, verified to be created, and then removed. Fixing
> >> fs_create_file() would turn it into fs_unlikat_dir(), so we don't need
> >> both. The same theme follows every test in virtio-9p-test.c, where the
> >> 'unlikat' variant does the same thing the 'create' does but with some
> >> cleaning in the end.
> >>
> >> Consolide some tests as follows:
> >>
> >> - fs_create_dir() is removed. fs_unlinkat_dir() is renamed to
> >>    fs_create_unlinkat_dir();
> >>
> >> - fs_create_file() is removed. fs_unlinkat_file() is renamed to
> >>    fs_create_unlinkat_file(). The "04" dir it uses is now being removed;
> >>
> >> - fs_symlink_file() is removed. fs_unlinkat_symlink() is renamed to
> >>    fs_create_unlinkat_symlink(). Both "real_file" and the "06" dir it
> >>    creates is now being removed.
> >>
> > 
> > The  change looks good functionally but it breaks the legitimate assumption
> > that files "06/*" come from test #6 and so on... I think you should consider
> > renumbering to avoid confusion when debugging logs.
> > 
> > Since this will bring more hunks, please split this in enough reviewable
> > patches.
> 
> Fair enough. Let me cook a v2. Thanks,

Wouldn't it be much simpler to just change the name of the temporary
directory, such that it contains the device name as well? Then these tests
runs would run on independent directories and won't interfere with each other
and that wouldn't need much changes I guess.

/Christian

Re: [PATCH for-9.0 1/3] qtest/virtio-9p-test.c: consolidate create dir, file and symlink tests

[Daniel Henrique Barboza]

On 3/27/24 05:47, Christian Schoenebeck wrote:
> On Tuesday, March 26, 2024 6:47:17 PM CET Daniel Henrique Barboza wrote:
>> On 3/26/24 14:05, Greg Kurz wrote:
>>> On Tue, 26 Mar 2024 10:26:04 -0300
>>> Daniel Henrique Barboza <dbarboza@ventanamicro.com> wrote:
>>>
>>>> The local 9p driver in virtio-9p-test.c its temporary dir right at the
>>>> start of qos-test (via virtio_9p_create_local_test_dir()) and only
>>>> deletes it after qos-test is finished (via
>>>> virtio_9p_remove_local_test_dir()).
>>>>
>>>> This means that any qos-test machine that ends up running virtio-9p-test local
>>>> tests more than once will end up re-using the same temp dir. This is
>>>> what's happening in [1] after we introduced the riscv machine nodes: if
>>>> we enable slow tests with the '-m slow' flag using qemu-system-riscv64,
>>>> this is what happens:
>>>>
>>>> - a temp dir is created, e.g. qtest-9p-local-WZLDL2;
>>>>
>>>> - virtio-9p-device tests will run virtio-9p-test successfully;
>>>>
>>>> - virtio-9p-pci tests will run virtio-9p-test, and fail right at the
>>>>     first slow test at fs_create_dir() because the "01" file was already
>>>>     created by fs_create_dir() test when running with the virtio-9p-device.
>>>>
>>>> We can fix it by making every test clean up their changes in the
>>>> filesystem after they're done. But we don't need every test either:
>>>> what fs_create_file() does is already exercised in fs_unlinkat_dir(),
>>>> i.e. a dir is created, verified to be created, and then removed. Fixing
>>>> fs_create_file() would turn it into fs_unlikat_dir(), so we don't need
>>>> both. The same theme follows every test in virtio-9p-test.c, where the
>>>> 'unlikat' variant does the same thing the 'create' does but with some
>>>> cleaning in the end.
>>>>
>>>> Consolide some tests as follows:
>>>>
>>>> - fs_create_dir() is removed. fs_unlinkat_dir() is renamed to
>>>>     fs_create_unlinkat_dir();
>>>>
>>>> - fs_create_file() is removed. fs_unlinkat_file() is renamed to
>>>>     fs_create_unlinkat_file(). The "04" dir it uses is now being removed;
>>>>
>>>> - fs_symlink_file() is removed. fs_unlinkat_symlink() is renamed to
>>>>     fs_create_unlinkat_symlink(). Both "real_file" and the "06" dir it
>>>>     creates is now being removed.
>>>>
>>>
>>> The  change looks good functionally but it breaks the legitimate assumption
>>> that files "06/*" come from test #6 and so on... I think you should consider
>>> renumbering to avoid confusion when debugging logs.
>>>
>>> Since this will bring more hunks, please split this in enough reviewable
>>> patches.
>>
>> Fair enough. Let me cook a v2. Thanks,
> 
> Wouldn't it be much simpler to just change the name of the temporary
> directory, such that it contains the device name as well? Then these tests
> runs would run on independent directories and won't interfere with each other
> and that wouldn't need much changes I guess.

That's true. If we were just trying to fix the issue then I would go with this
approach since it's simpler. But given that we're also cutting half the tests while
retaining the coverage I think this approach is worth the extra code.


Thanks,


Daniel



> 
> /Christian
> 
> 

Re: [PATCH for-9.0 1/3] qtest/virtio-9p-test.c: consolidate create dir, file and symlink tests

[Christian Schoenebeck]

On Wednesday, March 27, 2024 10:33:27 AM CET Daniel Henrique Barboza wrote:
> On 3/27/24 05:47, Christian Schoenebeck wrote:
> > On Tuesday, March 26, 2024 6:47:17 PM CET Daniel Henrique Barboza wrote:
> >> On 3/26/24 14:05, Greg Kurz wrote:
> >>> On Tue, 26 Mar 2024 10:26:04 -0300
> >>> Daniel Henrique Barboza <dbarboza@ventanamicro.com> wrote:
> >>>
> >>>> The local 9p driver in virtio-9p-test.c its temporary dir right at the
> >>>> start of qos-test (via virtio_9p_create_local_test_dir()) and only
> >>>> deletes it after qos-test is finished (via
> >>>> virtio_9p_remove_local_test_dir()).
> >>>>
> >>>> This means that any qos-test machine that ends up running virtio-9p-test local
> >>>> tests more than once will end up re-using the same temp dir. This is
> >>>> what's happening in [1] after we introduced the riscv machine nodes: if
> >>>> we enable slow tests with the '-m slow' flag using qemu-system-riscv64,
> >>>> this is what happens:
> >>>>
> >>>> - a temp dir is created, e.g. qtest-9p-local-WZLDL2;
> >>>>
> >>>> - virtio-9p-device tests will run virtio-9p-test successfully;
> >>>>
> >>>> - virtio-9p-pci tests will run virtio-9p-test, and fail right at the
> >>>>     first slow test at fs_create_dir() because the "01" file was already
> >>>>     created by fs_create_dir() test when running with the virtio-9p-device.
> >>>>
> >>>> We can fix it by making every test clean up their changes in the
> >>>> filesystem after they're done. But we don't need every test either:
> >>>> what fs_create_file() does is already exercised in fs_unlinkat_dir(),
> >>>> i.e. a dir is created, verified to be created, and then removed. Fixing
> >>>> fs_create_file() would turn it into fs_unlikat_dir(), so we don't need
> >>>> both. The same theme follows every test in virtio-9p-test.c, where the
> >>>> 'unlikat' variant does the same thing the 'create' does but with some
> >>>> cleaning in the end.
> >>>>
> >>>> Consolide some tests as follows:
> >>>>
> >>>> - fs_create_dir() is removed. fs_unlinkat_dir() is renamed to
> >>>>     fs_create_unlinkat_dir();
> >>>>
> >>>> - fs_create_file() is removed. fs_unlinkat_file() is renamed to
> >>>>     fs_create_unlinkat_file(). The "04" dir it uses is now being removed;
> >>>>
> >>>> - fs_symlink_file() is removed. fs_unlinkat_symlink() is renamed to
> >>>>     fs_create_unlinkat_symlink(). Both "real_file" and the "06" dir it
> >>>>     creates is now being removed.
> >>>>
> >>>
> >>> The  change looks good functionally but it breaks the legitimate assumption
> >>> that files "06/*" come from test #6 and so on... I think you should consider
> >>> renumbering to avoid confusion when debugging logs.
> >>>
> >>> Since this will bring more hunks, please split this in enough reviewable
> >>> patches.
> >>
> >> Fair enough. Let me cook a v2. Thanks,
> > 
> > Wouldn't it be much simpler to just change the name of the temporary
> > directory, such that it contains the device name as well? Then these tests
> > runs would run on independent directories and won't interfere with each other
> > and that wouldn't need much changes I guess.
> 
> That's true. If we were just trying to fix the issue then I would go with this
> approach since it's simpler. But given that we're also cutting half the tests while
> retaining the coverage I think this approach is worth the extra code.

Well, I am actually not so keen into all those changes. These tests were
intentionally split, and yes with costs of a bit redundant (test case) code.
But they were cleanly build up on each other, from fundamental requirements
like whether it is possible to create a directory and file ... and then the
subsequent tests would become more and more demanding.

That way it was easier to review if somebody reports a test to fail, because
you could immediately see whether the preceding fundamental tests succeeded.

/Christian

Re: [PATCH for-9.0 1/3] qtest/virtio-9p-test.c: consolidate create dir, file and symlink tests

[Daniel Henrique Barboza]

On 3/27/24 07:14, Christian Schoenebeck wrote:
> On Wednesday, March 27, 2024 10:33:27 AM CET Daniel Henrique Barboza wrote:
>> On 3/27/24 05:47, Christian Schoenebeck wrote:
>>> On Tuesday, March 26, 2024 6:47:17 PM CET Daniel Henrique Barboza wrote:
>>>> On 3/26/24 14:05, Greg Kurz wrote:
>>>>> On Tue, 26 Mar 2024 10:26:04 -0300
>>>>> Daniel Henrique Barboza <dbarboza@ventanamicro.com> wrote:
>>>>>
>>>>>> The local 9p driver in virtio-9p-test.c its temporary dir right at the
>>>>>> start of qos-test (via virtio_9p_create_local_test_dir()) and only
>>>>>> deletes it after qos-test is finished (via
>>>>>> virtio_9p_remove_local_test_dir()).
>>>>>>
>>>>>> This means that any qos-test machine that ends up running virtio-9p-test local
>>>>>> tests more than once will end up re-using the same temp dir. This is
>>>>>> what's happening in [1] after we introduced the riscv machine nodes: if
>>>>>> we enable slow tests with the '-m slow' flag using qemu-system-riscv64,
>>>>>> this is what happens:
>>>>>>
>>>>>> - a temp dir is created, e.g. qtest-9p-local-WZLDL2;
>>>>>>
>>>>>> - virtio-9p-device tests will run virtio-9p-test successfully;
>>>>>>
>>>>>> - virtio-9p-pci tests will run virtio-9p-test, and fail right at the
>>>>>>      first slow test at fs_create_dir() because the "01" file was already
>>>>>>      created by fs_create_dir() test when running with the virtio-9p-device.
>>>>>>
>>>>>> We can fix it by making every test clean up their changes in the
>>>>>> filesystem after they're done. But we don't need every test either:
>>>>>> what fs_create_file() does is already exercised in fs_unlinkat_dir(),
>>>>>> i.e. a dir is created, verified to be created, and then removed. Fixing
>>>>>> fs_create_file() would turn it into fs_unlikat_dir(), so we don't need
>>>>>> both. The same theme follows every test in virtio-9p-test.c, where the
>>>>>> 'unlikat' variant does the same thing the 'create' does but with some
>>>>>> cleaning in the end.
>>>>>>
>>>>>> Consolide some tests as follows:
>>>>>>
>>>>>> - fs_create_dir() is removed. fs_unlinkat_dir() is renamed to
>>>>>>      fs_create_unlinkat_dir();
>>>>>>
>>>>>> - fs_create_file() is removed. fs_unlinkat_file() is renamed to
>>>>>>      fs_create_unlinkat_file(). The "04" dir it uses is now being removed;
>>>>>>
>>>>>> - fs_symlink_file() is removed. fs_unlinkat_symlink() is renamed to
>>>>>>      fs_create_unlinkat_symlink(). Both "real_file" and the "06" dir it
>>>>>>      creates is now being removed.
>>>>>>
>>>>>
>>>>> The  change looks good functionally but it breaks the legitimate assumption
>>>>> that files "06/*" come from test #6 and so on... I think you should consider
>>>>> renumbering to avoid confusion when debugging logs.
>>>>>
>>>>> Since this will bring more hunks, please split this in enough reviewable
>>>>> patches.
>>>>
>>>> Fair enough. Let me cook a v2. Thanks,
>>>
>>> Wouldn't it be much simpler to just change the name of the temporary
>>> directory, such that it contains the device name as well? Then these tests
>>> runs would run on independent directories and won't interfere with each other
>>> and that wouldn't need much changes I guess.
>>
>> That's true. If we were just trying to fix the issue then I would go with this
>> approach since it's simpler. But given that we're also cutting half the tests while
>> retaining the coverage I think this approach is worth the extra code.
> 
> Well, I am actually not so keen into all those changes. These tests were
> intentionally split, and yes with costs of a bit redundant (test case) code.
> But they were cleanly build up on each other, from fundamental requirements
> like whether it is possible to create a directory and file ... and then the
> subsequent tests would become more and more demanding.
> 
> That way it was easier to review if somebody reports a test to fail, because
> you could immediately see whether the preceding fundamental tests succeeded.

The current test design is flawed. It's based on a premise that doesn't happen, i.e.
a new temp dir will be created every time the test suit is executed. In reality the
temp dir is created only once in the constructor of the test, at the start of qos-test
(tests/qtest/qos-test.c, run_one_test()) and removed only once at the destructor
at the end of the run.

It's not possible to add a 'device name' in the created temp dir because we're too early
in the process, the tests didn't start at that point. So, with the current temp dir design,
the tests needs to clean themselves up after each run.

Here's the alternatives I'm willing to go for:

- what I just sent in v2;

- add cleanups in all existing tests. We can keep all of them, but the 'create' tests
will be carbon copies of the 'unlinkat' tests but with different names. Can be done;

- if we really want the tests untouched we can rework how the 'temp dir' is created/deleted.
The test dir will be created and removed after each test via the 'before' callback. To be
honest this seems like the best approach we can take, aside from what I did in v2, and
it's on par with how tests like vhost-user-test.c works.


Thanks,


Daniel

> 
> /Christian
> 
> 

Re: [PATCH for-9.0 1/3] qtest/virtio-9p-test.c: consolidate create dir, file and symlink tests

[Christian Schoenebeck]

On Wednesday, March 27, 2024 12:28:17 PM CET Daniel Henrique Barboza wrote:
> On 3/27/24 07:14, Christian Schoenebeck wrote:
> > On Wednesday, March 27, 2024 10:33:27 AM CET Daniel Henrique Barboza wrote:
> >> On 3/27/24 05:47, Christian Schoenebeck wrote:
> >>> On Tuesday, March 26, 2024 6:47:17 PM CET Daniel Henrique Barboza wrote:
> >>>> On 3/26/24 14:05, Greg Kurz wrote:
> >>>>> On Tue, 26 Mar 2024 10:26:04 -0300
> >>>>> Daniel Henrique Barboza <dbarboza@ventanamicro.com> wrote:
> >>>>>
> >>>>>> The local 9p driver in virtio-9p-test.c its temporary dir right at the
> >>>>>> start of qos-test (via virtio_9p_create_local_test_dir()) and only
> >>>>>> deletes it after qos-test is finished (via
> >>>>>> virtio_9p_remove_local_test_dir()).
> >>>>>>
> >>>>>> This means that any qos-test machine that ends up running virtio-9p-test local
> >>>>>> tests more than once will end up re-using the same temp dir. This is
> >>>>>> what's happening in [1] after we introduced the riscv machine nodes: if
> >>>>>> we enable slow tests with the '-m slow' flag using qemu-system-riscv64,
> >>>>>> this is what happens:
> >>>>>>
> >>>>>> - a temp dir is created, e.g. qtest-9p-local-WZLDL2;
> >>>>>>
> >>>>>> - virtio-9p-device tests will run virtio-9p-test successfully;
> >>>>>>
> >>>>>> - virtio-9p-pci tests will run virtio-9p-test, and fail right at the
> >>>>>>      first slow test at fs_create_dir() because the "01" file was already
> >>>>>>      created by fs_create_dir() test when running with the virtio-9p-device.
> >>>>>>
> >>>>>> We can fix it by making every test clean up their changes in the
> >>>>>> filesystem after they're done. But we don't need every test either:
> >>>>>> what fs_create_file() does is already exercised in fs_unlinkat_dir(),
> >>>>>> i.e. a dir is created, verified to be created, and then removed. Fixing
> >>>>>> fs_create_file() would turn it into fs_unlikat_dir(), so we don't need
> >>>>>> both. The same theme follows every test in virtio-9p-test.c, where the
> >>>>>> 'unlikat' variant does the same thing the 'create' does but with some
> >>>>>> cleaning in the end.
> >>>>>>
> >>>>>> Consolide some tests as follows:
> >>>>>>
> >>>>>> - fs_create_dir() is removed. fs_unlinkat_dir() is renamed to
> >>>>>>      fs_create_unlinkat_dir();
> >>>>>>
> >>>>>> - fs_create_file() is removed. fs_unlinkat_file() is renamed to
> >>>>>>      fs_create_unlinkat_file(). The "04" dir it uses is now being removed;
> >>>>>>
> >>>>>> - fs_symlink_file() is removed. fs_unlinkat_symlink() is renamed to
> >>>>>>      fs_create_unlinkat_symlink(). Both "real_file" and the "06" dir it
> >>>>>>      creates is now being removed.
> >>>>>>
> >>>>>
> >>>>> The  change looks good functionally but it breaks the legitimate assumption
> >>>>> that files "06/*" come from test #6 and so on... I think you should consider
> >>>>> renumbering to avoid confusion when debugging logs.
> >>>>>
> >>>>> Since this will bring more hunks, please split this in enough reviewable
> >>>>> patches.
> >>>>
> >>>> Fair enough. Let me cook a v2. Thanks,
> >>>
> >>> Wouldn't it be much simpler to just change the name of the temporary
> >>> directory, such that it contains the device name as well? Then these tests
> >>> runs would run on independent directories and won't interfere with each other
> >>> and that wouldn't need much changes I guess.
> >>
> >> That's true. If we were just trying to fix the issue then I would go with this
> >> approach since it's simpler. But given that we're also cutting half the tests while
> >> retaining the coverage I think this approach is worth the extra code.
> > 
> > Well, I am actually not so keen into all those changes. These tests were
> > intentionally split, and yes with costs of a bit redundant (test case) code.
> > But they were cleanly build up on each other, from fundamental requirements
> > like whether it is possible to create a directory and file ... and then the
> > subsequent tests would become more and more demanding.
> > 
> > That way it was easier to review if somebody reports a test to fail, because
> > you could immediately see whether the preceding fundamental tests succeeded.
> 
> The current test design is flawed. It's based on a premise that doesn't happen, i.e.
> a new temp dir will be created every time the test suit is executed. In reality the
> temp dir is created only once in the constructor of the test, at the start of qos-test
> (tests/qtest/qos-test.c, run_one_test()) and removed only once at the destructor
> at the end of the run.
> 
> It's not possible to add a 'device name' in the created temp dir because we're too early
> in the process, the tests didn't start at that point. So, with the current temp dir design,
> the tests needs to clean themselves up after each run.
> 
> Here's the alternatives I'm willing to go for:
> 
> - what I just sent in v2;
> 
> - add cleanups in all existing tests. We can keep all of them, but the 'create' tests
> will be carbon copies of the 'unlinkat' tests but with different names. Can be done;
> 
> - if we really want the tests untouched we can rework how the 'temp dir' is created/deleted.
> The test dir will be created and removed after each test via the 'before' callback. To be
> honest this seems like the best approach we can take, aside from what I did in v2, and
> it's on par with how tests like vhost-user-test.c works.

Yeah, the latter sounds like the best solution to me, too.

Don't get me wrong, I didn't want to burden you with more work. It's really
just that I think that restructuring all test cases is contra productive.

If you want I can also look into that. Just let me know.

Thanks!

/Christian

Re: [PATCH for-9.0 1/3] qtest/virtio-9p-test.c: consolidate create dir, file and symlink tests

[Greg Kurz]

On Wed, 27 Mar 2024 13:26:45 +0100
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> On Wednesday, March 27, 2024 12:28:17 PM CET Daniel Henrique Barboza wrote:
> > On 3/27/24 07:14, Christian Schoenebeck wrote:
> > > On Wednesday, March 27, 2024 10:33:27 AM CET Daniel Henrique Barboza wrote:
> > >> On 3/27/24 05:47, Christian Schoenebeck wrote:
> > >>> On Tuesday, March 26, 2024 6:47:17 PM CET Daniel Henrique Barboza wrote:
> > >>>> On 3/26/24 14:05, Greg Kurz wrote:
> > >>>>> On Tue, 26 Mar 2024 10:26:04 -0300
> > >>>>> Daniel Henrique Barboza <dbarboza@ventanamicro.com> wrote:
> > >>>>>
> > >>>>>> The local 9p driver in virtio-9p-test.c its temporary dir right at the
> > >>>>>> start of qos-test (via virtio_9p_create_local_test_dir()) and only
> > >>>>>> deletes it after qos-test is finished (via
> > >>>>>> virtio_9p_remove_local_test_dir()).
> > >>>>>>
> > >>>>>> This means that any qos-test machine that ends up running virtio-9p-test local
> > >>>>>> tests more than once will end up re-using the same temp dir. This is
> > >>>>>> what's happening in [1] after we introduced the riscv machine nodes: if
> > >>>>>> we enable slow tests with the '-m slow' flag using qemu-system-riscv64,
> > >>>>>> this is what happens:
> > >>>>>>
> > >>>>>> - a temp dir is created, e.g. qtest-9p-local-WZLDL2;
> > >>>>>>
> > >>>>>> - virtio-9p-device tests will run virtio-9p-test successfully;
> > >>>>>>
> > >>>>>> - virtio-9p-pci tests will run virtio-9p-test, and fail right at the
> > >>>>>>      first slow test at fs_create_dir() because the "01" file was already
> > >>>>>>      created by fs_create_dir() test when running with the virtio-9p-device.
> > >>>>>>
> > >>>>>> We can fix it by making every test clean up their changes in the
> > >>>>>> filesystem after they're done. But we don't need every test either:
> > >>>>>> what fs_create_file() does is already exercised in fs_unlinkat_dir(),
> > >>>>>> i.e. a dir is created, verified to be created, and then removed. Fixing
> > >>>>>> fs_create_file() would turn it into fs_unlikat_dir(), so we don't need
> > >>>>>> both. The same theme follows every test in virtio-9p-test.c, where the
> > >>>>>> 'unlikat' variant does the same thing the 'create' does but with some
> > >>>>>> cleaning in the end.
> > >>>>>>
> > >>>>>> Consolide some tests as follows:
> > >>>>>>
> > >>>>>> - fs_create_dir() is removed. fs_unlinkat_dir() is renamed to
> > >>>>>>      fs_create_unlinkat_dir();
> > >>>>>>
> > >>>>>> - fs_create_file() is removed. fs_unlinkat_file() is renamed to
> > >>>>>>      fs_create_unlinkat_file(). The "04" dir it uses is now being removed;
> > >>>>>>
> > >>>>>> - fs_symlink_file() is removed. fs_unlinkat_symlink() is renamed to
> > >>>>>>      fs_create_unlinkat_symlink(). Both "real_file" and the "06" dir it
> > >>>>>>      creates is now being removed.
> > >>>>>>
> > >>>>>
> > >>>>> The  change looks good functionally but it breaks the legitimate assumption
> > >>>>> that files "06/*" come from test #6 and so on... I think you should consider
> > >>>>> renumbering to avoid confusion when debugging logs.
> > >>>>>
> > >>>>> Since this will bring more hunks, please split this in enough reviewable
> > >>>>> patches.
> > >>>>
> > >>>> Fair enough. Let me cook a v2. Thanks,
> > >>>
> > >>> Wouldn't it be much simpler to just change the name of the temporary
> > >>> directory, such that it contains the device name as well? Then these tests
> > >>> runs would run on independent directories and won't interfere with each other
> > >>> and that wouldn't need much changes I guess.
> > >>
> > >> That's true. If we were just trying to fix the issue then I would go with this
> > >> approach since it's simpler. But given that we're also cutting half the tests while
> > >> retaining the coverage I think this approach is worth the extra code.
> > > 
> > > Well, I am actually not so keen into all those changes. These tests were
> > > intentionally split, and yes with costs of a bit redundant (test case) code.
> > > But they were cleanly build up on each other, from fundamental requirements
> > > like whether it is possible to create a directory and file ... and then the
> > > subsequent tests would become more and more demanding.
> > > 
> > > That way it was easier to review if somebody reports a test to fail, because
> > > you could immediately see whether the preceding fundamental tests succeeded.
> > 
> > The current test design is flawed. It's based on a premise that doesn't happen, i.e.
> > a new temp dir will be created every time the test suit is executed. In reality the
> > temp dir is created only once in the constructor of the test, at the start of qos-test
> > (tests/qtest/qos-test.c, run_one_test()) and removed only once at the destructor
> > at the end of the run.
> > 
> > It's not possible to add a 'device name' in the created temp dir because we're too early
> > in the process, the tests didn't start at that point. So, with the current temp dir design,
> > the tests needs to clean themselves up after each run.
> > 
> > Here's the alternatives I'm willing to go for:
> > 
> > - what I just sent in v2;
> > 
> > - add cleanups in all existing tests. We can keep all of them, but the 'create' tests
> > will be carbon copies of the 'unlinkat' tests but with different names. Can be done;
> > 
> > - if we really want the tests untouched we can rework how the 'temp dir' is created/deleted.
> > The test dir will be created and removed after each test via the 'before' callback. To be
> > honest this seems like the best approach we can take, aside from what I did in v2, and
> > it's on par with how tests like vhost-user-test.c works.
> 
> Yeah, the latter sounds like the best solution to me, too.
> 

+1

> Don't get me wrong, I didn't want to burden you with more work. It's really
> just that I think that restructuring all test cases is contra productive.
> 
> If you want I can also look into that. Just let me know.
> 
> Thanks!
> 
> /Christian
> 
> 



-- 
Greg

Re: [PATCH for-9.0 1/3] qtest/virtio-9p-test.c: consolidate create dir, file and symlink tests

[Daniel Henrique Barboza]

On 3/27/24 09:26, Christian Schoenebeck wrote:
> On Wednesday, March 27, 2024 12:28:17 PM CET Daniel Henrique Barboza wrote:
>> On 3/27/24 07:14, Christian Schoenebeck wrote:
>>> On Wednesday, March 27, 2024 10:33:27 AM CET Daniel Henrique Barboza wrote:
>>>> On 3/27/24 05:47, Christian Schoenebeck wrote:
>>>>> On Tuesday, March 26, 2024 6:47:17 PM CET Daniel Henrique Barboza wrote:
>>>>>> On 3/26/24 14:05, Greg Kurz wrote:
>>>>>>> On Tue, 26 Mar 2024 10:26:04 -0300
>>>>>>> Daniel Henrique Barboza <dbarboza@ventanamicro.com> wrote:
>>>>>>>
>>>>>>>> The local 9p driver in virtio-9p-test.c its temporary dir right at the
>>>>>>>> start of qos-test (via virtio_9p_create_local_test_dir()) and only
>>>>>>>> deletes it after qos-test is finished (via
>>>>>>>> virtio_9p_remove_local_test_dir()).
>>>>>>>>
>>>>>>>> This means that any qos-test machine that ends up running virtio-9p-test local
>>>>>>>> tests more than once will end up re-using the same temp dir. This is
>>>>>>>> what's happening in [1] after we introduced the riscv machine nodes: if
>>>>>>>> we enable slow tests with the '-m slow' flag using qemu-system-riscv64,
>>>>>>>> this is what happens:
>>>>>>>>
>>>>>>>> - a temp dir is created, e.g. qtest-9p-local-WZLDL2;
>>>>>>>>
>>>>>>>> - virtio-9p-device tests will run virtio-9p-test successfully;
>>>>>>>>
>>>>>>>> - virtio-9p-pci tests will run virtio-9p-test, and fail right at the
>>>>>>>>       first slow test at fs_create_dir() because the "01" file was already
>>>>>>>>       created by fs_create_dir() test when running with the virtio-9p-device.
>>>>>>>>
>>>>>>>> We can fix it by making every test clean up their changes in the
>>>>>>>> filesystem after they're done. But we don't need every test either:
>>>>>>>> what fs_create_file() does is already exercised in fs_unlinkat_dir(),
>>>>>>>> i.e. a dir is created, verified to be created, and then removed. Fixing
>>>>>>>> fs_create_file() would turn it into fs_unlikat_dir(), so we don't need
>>>>>>>> both. The same theme follows every test in virtio-9p-test.c, where the
>>>>>>>> 'unlikat' variant does the same thing the 'create' does but with some
>>>>>>>> cleaning in the end.
>>>>>>>>
>>>>>>>> Consolide some tests as follows:
>>>>>>>>
>>>>>>>> - fs_create_dir() is removed. fs_unlinkat_dir() is renamed to
>>>>>>>>       fs_create_unlinkat_dir();
>>>>>>>>
>>>>>>>> - fs_create_file() is removed. fs_unlinkat_file() is renamed to
>>>>>>>>       fs_create_unlinkat_file(). The "04" dir it uses is now being removed;
>>>>>>>>
>>>>>>>> - fs_symlink_file() is removed. fs_unlinkat_symlink() is renamed to
>>>>>>>>       fs_create_unlinkat_symlink(). Both "real_file" and the "06" dir it
>>>>>>>>       creates is now being removed.
>>>>>>>>
>>>>>>>
>>>>>>> The  change looks good functionally but it breaks the legitimate assumption
>>>>>>> that files "06/*" come from test #6 and so on... I think you should consider
>>>>>>> renumbering to avoid confusion when debugging logs.
>>>>>>>
>>>>>>> Since this will bring more hunks, please split this in enough reviewable
>>>>>>> patches.
>>>>>>
>>>>>> Fair enough. Let me cook a v2. Thanks,
>>>>>
>>>>> Wouldn't it be much simpler to just change the name of the temporary
>>>>> directory, such that it contains the device name as well? Then these tests
>>>>> runs would run on independent directories and won't interfere with each other
>>>>> and that wouldn't need much changes I guess.
>>>>
>>>> That's true. If we were just trying to fix the issue then I would go with this
>>>> approach since it's simpler. But given that we're also cutting half the tests while
>>>> retaining the coverage I think this approach is worth the extra code.
>>>
>>> Well, I am actually not so keen into all those changes. These tests were
>>> intentionally split, and yes with costs of a bit redundant (test case) code.
>>> But they were cleanly build up on each other, from fundamental requirements
>>> like whether it is possible to create a directory and file ... and then the
>>> subsequent tests would become more and more demanding.
>>>
>>> That way it was easier to review if somebody reports a test to fail, because
>>> you could immediately see whether the preceding fundamental tests succeeded.
>>
>> The current test design is flawed. It's based on a premise that doesn't happen, i.e.
>> a new temp dir will be created every time the test suit is executed. In reality the
>> temp dir is created only once in the constructor of the test, at the start of qos-test
>> (tests/qtest/qos-test.c, run_one_test()) and removed only once at the destructor
>> at the end of the run.
>>
>> It's not possible to add a 'device name' in the created temp dir because we're too early
>> in the process, the tests didn't start at that point. So, with the current temp dir design,
>> the tests needs to clean themselves up after each run.
>>
>> Here's the alternatives I'm willing to go for:
>>
>> - what I just sent in v2;
>>
>> - add cleanups in all existing tests. We can keep all of them, but the 'create' tests
>> will be carbon copies of the 'unlinkat' tests but with different names. Can be done;
>>
>> - if we really want the tests untouched we can rework how the 'temp dir' is created/deleted.
>> The test dir will be created and removed after each test via the 'before' callback. To be
>> honest this seems like the best approach we can take, aside from what I did in v2, and
>> it's on par with how tests like vhost-user-test.c works.
> 
> Yeah, the latter sounds like the best solution to me, too.

Fair enough. I have it ready here - I'll just wait to see how Gitlab likes it before
posting it.

Aside from creating multiple temp dirs at each run it's way more elegant indeed.

> 
> Don't get me wrong, I didn't want to burden you with more work. It's really
> just that I think that restructuring all test cases is contra productive.
> 
> If you want I can also look into that. Just let me know.

It's fine. It's breaking RISC-V stuff that I added during 9.0 so now it's my problem
too.


Thanks,

Daniel

> 
> Thanks!
> 
> /Christian
> 
> 

[PATCH for-9.0 2/3] qtest/virtio-9p-test.c: consolidate hardlink tests

[Daniel Henrique Barboza]

We need all virtio-9p-test.c tests to clean themselves up after being
executed to avoid an issue where multiple test runs of this same test
will find remains of the previous run. See [1] to see the side effects
of that.

Previous patch dealt with most tests in this file. We're now
consolidating the 'hardlink' tests:

- copy the asserts that checks if the created link is a hard link from
  fs_hardlink_file() to fs_unlinkat_hardlink(). This will make
  fs_unlinkat_hardlink() do exactly what fs_hardlink_file() is doing;

- remove fs_hardlink_file();

- fs_unlinkat_hardlink() is renamed to fs_create_unlinkat_hardlink().
  We'll also remove 'real_file' and the '08' dir it creates.

With this last change all local 9p tests of the file are not leaving
files or dirs behind in the temporary dir, allowing multiple runs in the
same session to succeed.

[1] https://mail.gnu.org/archive/html/qemu-devel/2024-03/msg05807.html

Reported-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 tests/qtest/virtio-9p-test.c | 55 +++++++++++++++---------------------
 1 file changed, 22 insertions(+), 33 deletions(-)

diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index cdbe3e78ea..9938516fe7 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -600,39 +600,13 @@ static void fs_create_unlinkat_symlink(void *obj, void *data,
     g_assert(stat(new_dir, &st) != 0);
 }
 
-static void fs_hardlink_file(void *obj, void *data, QGuestAllocator *t_alloc)
+static void fs_create_unlinkat_hardlink(void *obj, void *data,
+                                        QGuestAllocator *t_alloc)
 {
     QVirtio9P *v9p = obj;
     v9fs_set_allocator(t_alloc);
-    struct stat st_real, st_link;
-    g_autofree char *real_file = virtio_9p_test_path("07/real_file");
-    g_autofree char *hardlink_file = virtio_9p_test_path("07/hardlink_file");
-
-    tattach({ .client = v9p });
-    tmkdir({ .client = v9p, .atPath = "/", .name = "07" });
-    tlcreate({ .client = v9p, .atPath = "07", .name = "real_file" });
-    g_assert(stat(real_file, &st_real) == 0);
-    g_assert((st_real.st_mode & S_IFMT) == S_IFREG);
-
-    tlink({
-        .client = v9p, .atPath = "07", .name = "hardlink_file",
-        .toPath = "07/real_file"
-    });
-
-    /* check if link exists now ... */
-    g_assert(stat(hardlink_file, &st_link) == 0);
-    /* ... and it's a hard link, right? */
-    g_assert((st_link.st_mode & S_IFMT) == S_IFREG);
-    g_assert(st_link.st_dev == st_real.st_dev);
-    g_assert(st_link.st_ino == st_real.st_ino);
-}
-
-static void fs_unlinkat_hardlink(void *obj, void *data,
-                                 QGuestAllocator *t_alloc)
-{
-    QVirtio9P *v9p = obj;
-    v9fs_set_allocator(t_alloc);
-    struct stat st_real, st_link;
+    struct stat st_real, st_link, st;
+    g_autofree char *new_dir = virtio_9p_test_path("08");
     g_autofree char *real_file = virtio_9p_test_path("08/real_file");
     g_autofree char *hardlink_file = virtio_9p_test_path("08/hardlink_file");
 
@@ -646,13 +620,29 @@ static void fs_unlinkat_hardlink(void *obj, void *data,
         .client = v9p, .atPath = "08", .name = "hardlink_file",
         .toPath = "08/real_file"
     });
+
+    /* check if link exists now ... */
     g_assert(stat(hardlink_file, &st_link) == 0);
+    /* ... and it's a hard link, right? */
+    g_assert((st_link.st_mode & S_IFMT) == S_IFREG);
+    g_assert(st_link.st_dev == st_real.st_dev);
+    g_assert(st_link.st_ino == st_real.st_ino);
 
     tunlinkat({ .client = v9p, .atPath = "08", .name = "hardlink_file" });
     /* symlink should be gone now */
     g_assert(stat(hardlink_file, &st_link) != 0);
     /* and old file should still exist */
     g_assert(stat(real_file, &st_real) == 0);
+
+    /* cleanup: remove old file and dir */
+    tunlinkat({ .client = v9p, .atPath = "08", .name = "real_file" });
+    g_assert(stat(real_file, &st_real) != 0);
+
+    tunlinkat({
+        .client = v9p, .atPath = "/", .name = "08",
+        .flags = P9_DOTL_AT_REMOVEDIR
+    });
+    g_assert(stat(new_dir, &st) != 0);
 }
 
 static void *assign_9p_local_driver(GString *cmd_line, void *arg)
@@ -714,9 +704,8 @@ static void register_virtio_9p_test(void)
                  fs_create_unlinkat_file, &opts);
     qos_add_test("local/create_unlinkat_symlink", "virtio-9p",
                  fs_create_unlinkat_symlink, &opts);
-    qos_add_test("local/hardlink_file", "virtio-9p", fs_hardlink_file, &opts);
-    qos_add_test("local/unlinkat_hardlink", "virtio-9p", fs_unlinkat_hardlink,
-                 &opts);
+    qos_add_test("local/create_unlinkat_hardlink", "virtio-9p",
+                 fs_create_unlinkat_hardlink, &opts);
 }
 
 libqos_init(register_virtio_9p_test);
-- 
2.44.0

[PATCH for-9.0 3/3] qtest/virtio-9p-test.c: remove g_test_slow() gate

[Daniel Henrique Barboza]

Commit 558f5c42ef gated the local tests with g_test_slow() to skip them
in 'make check'. The reported issue back then was this following CI
problem:

https://lists.nongnu.org/archive/html/qemu-devel/2020-11/msg05510.html

This problem ended up being fixed when the recently added risc-v machine
nodes faced the same issue [1]. We're now able to run these tests with
'make check' in the CI.

This reverts commit 558f5c42efded3e0d0b20a90bce2a9a14580d824.

[1] https://mail.gnu.org/archive/html/qemu-devel/2024-03/msg05807.html

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 tests/qtest/virtio-9p-test.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index 9938516fe7..ff69eb334a 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -687,15 +687,6 @@ static void register_virtio_9p_test(void)
 
 
     /* 9pfs test cases using the 'local' filesystem driver */
-
-    /*
-     * XXX: Until we are sure that these tests can run everywhere,
-     * keep them as "slow" so that they aren't run with "make check".
-     */
-    if (!g_test_slow()) {
-        return;
-    }
-
     opts.before = assign_9p_local_driver;
     qos_add_test("local/config", "virtio-9p", pci_config,  &opts);
     qos_add_test("local/create_unlinkat_dir", "virtio-9p",
-- 
2.44.0

Re: [PATCH for-9.0 0/3] qtest/virtio-9p-test.c: fix slow tests

[Greg Kurz]

Bom dia Daniel !

On Tue, 26 Mar 2024 10:26:03 -0300
Daniel Henrique Barboza <dbarboza@ventanamicro.com> wrote:

> Hi,
> 
> Thomas reported in [1] a problem that happened with the RISC-V machine
> where some tests from virtio-9p-test.c were failing with '-m slow', i.e.
> enabling slow tests.
> 
> In the end it wasn't a RISC-V specific problem. It just so happens that
> the recently added riscv machine nodes runs the tests from
> virtio-9p-test two times for each qos-test run: one with the
> virtio-9p-device device and another with the virtio-9p-pci. The temp dir
> for these tests is being created at the start of qos-test and removed
> only at the end of qos-test, and the tests are leaving dirs and files
> behind. virtio-9-device tests run first, creates stuff in the temp dir,
> then when virtio-9p-pci tests runs again it'll fail because the previous
> run left created dirs and files in the same temp dir. Here's a run that
> exemplifies the problem:
> 
> $ MALLOC_PERTURB_=21 V=2 QTEST_QEMU_BINARY=./qemu-system-riscv64 ./tests/qtest/qos-test -m slow
> (...)
> # starting QEMU: exec ./qemu-system-riscv64 -qtest unix:/tmp/qtest-621710.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-621710.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -M virt,aclint=on,aia=aplic-imsic -fsdev local,id=fsdev0,path='/home/danielhb/work/qemu/build/qtest-9p-local-7E16K2',security_model=mapped-xattr -device virtio-9p-device,fsdev=fsdev0,mount_tag=qtest -accel qtest
> ( goes ok ...)
> # starting QEMU: exec ./qemu-system-riscv64 -qtest unix:/tmp/qtest-621710.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-621710.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -M virt,aclint=on,aia=aplic-imsic -fsdev local,id=fsdev0,path='/home/danielhb/work/qemu/build/qtest-9p-local-7E16K2',security_model=mapped-xattr -device virtio-9p-pci,fsdev=fsdev0,addr=04.0,mount_tag=qtest -accel qtest
> ok 168 /riscv64/virt/generic-pcihost/pci-bus-generic/pci-bus/virtio-9p-pci/virtio-9p/virtio-9p-tests/local/config
> Received response 7 (RLERROR) instead of 73 (RMKDIR)
> Rlerror has errno 17 (File exists)
> **
> ERROR:../tests/qtest/libqos/virtio-9p-client.c:275:v9fs_req_recv: assertion failed (hdr.id == id): (7 == 73)
> 
> As we can see we're running both 'virtio-9p-device' tests and 'virtio-9p-pci'
> tests using the same '/home/danielhb/work/qemu/build/qtest-9p-local-7E16K2'
> temp dir. 
> 


Good catch ! I'll try to find some time to review.

> The quick fix I came up with was to make each test clean themselves up
> after each run. The tests were also consolidated, i.e. fewer tests with the
> same coverage, because the 'unlikat' tests were doing the same thing the
> 'create' tests were doing but removing stuff after. Might as well keep just
> the 'unlikat' tests.
> 

As long as coverage is preserved, I'm fine with consolidation of the
checks. In any case, last call goes to Christian.

> I also went ahead and reverted 558f5c42efd ("tests/9pfs: Mark "local"
> tests as "slow"") after realizing that the problem I was fixing is also
> the same problem that this patch was trying to working around with the
> skip [2]. I validated this change in this Gitlab pipeline:
> 

Are you sure with that ? Issues look very similar indeed but not
exactly the same.

Cheers,

--
Greg

> https://gitlab.com/danielhb/qemu/-/pipelines/1227953967
> 
> [1] https://mail.gnu.org/archive/html/qemu-devel/2024-03/msg05807.html
> [2] https://lists.nongnu.org/archive/html/qemu-devel/2020-11/msg05510.html
> 
> Daniel Henrique Barboza (3):
>   qtest/virtio-9p-test.c: consolidate create dir, file and symlink tests
>   qtest/virtio-9p-test.c: consolidate hardlink tests
>   qtest/virtio-9p-test.c: remove g_test_slow() gate
> 
>  tests/qtest/virtio-9p-test.c | 155 +++++++++++------------------------
>  1 file changed, 48 insertions(+), 107 deletions(-)
> 

Re: [PATCH for-9.0 0/3] qtest/virtio-9p-test.c: fix slow tests

[Daniel Henrique Barboza]

On 3/26/24 12:55, Greg Kurz wrote:
> Bom dia Daniel !

Bonne après-midi !

> 
> On Tue, 26 Mar 2024 10:26:03 -0300
> Daniel Henrique Barboza <dbarboza@ventanamicro.com> wrote:
> 
>> Hi,
>>
>> Thomas reported in [1] a problem that happened with the RISC-V machine
>> where some tests from virtio-9p-test.c were failing with '-m slow', i.e.
>> enabling slow tests.
>>
>> In the end it wasn't a RISC-V specific problem. It just so happens that
>> the recently added riscv machine nodes runs the tests from
>> virtio-9p-test two times for each qos-test run: one with the
>> virtio-9p-device device and another with the virtio-9p-pci. The temp dir
>> for these tests is being created at the start of qos-test and removed
>> only at the end of qos-test, and the tests are leaving dirs and files
>> behind. virtio-9-device tests run first, creates stuff in the temp dir,
>> then when virtio-9p-pci tests runs again it'll fail because the previous
>> run left created dirs and files in the same temp dir. Here's a run that
>> exemplifies the problem:
>>
>> $ MALLOC_PERTURB_=21 V=2 QTEST_QEMU_BINARY=./qemu-system-riscv64 ./tests/qtest/qos-test -m slow
>> (...)
>> # starting QEMU: exec ./qemu-system-riscv64 -qtest unix:/tmp/qtest-621710.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-621710.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -M virt,aclint=on,aia=aplic-imsic -fsdev local,id=fsdev0,path='/home/danielhb/work/qemu/build/qtest-9p-local-7E16K2',security_model=mapped-xattr -device virtio-9p-device,fsdev=fsdev0,mount_tag=qtest -accel qtest
>> ( goes ok ...)
>> # starting QEMU: exec ./qemu-system-riscv64 -qtest unix:/tmp/qtest-621710.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-621710.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -M virt,aclint=on,aia=aplic-imsic -fsdev local,id=fsdev0,path='/home/danielhb/work/qemu/build/qtest-9p-local-7E16K2',security_model=mapped-xattr -device virtio-9p-pci,fsdev=fsdev0,addr=04.0,mount_tag=qtest -accel qtest
>> ok 168 /riscv64/virt/generic-pcihost/pci-bus-generic/pci-bus/virtio-9p-pci/virtio-9p/virtio-9p-tests/local/config
>> Received response 7 (RLERROR) instead of 73 (RMKDIR)
>> Rlerror has errno 17 (File exists)
>> **
>> ERROR:../tests/qtest/libqos/virtio-9p-client.c:275:v9fs_req_recv: assertion failed (hdr.id == id): (7 == 73)
>>
>> As we can see we're running both 'virtio-9p-device' tests and 'virtio-9p-pci'
>> tests using the same '/home/danielhb/work/qemu/build/qtest-9p-local-7E16K2'
>> temp dir.
>>
> 
> 
> Good catch ! I'll try to find some time to review.
> 
>> The quick fix I came up with was to make each test clean themselves up
>> after each run. The tests were also consolidated, i.e. fewer tests with the
>> same coverage, because the 'unlikat' tests were doing the same thing the
>> 'create' tests were doing but removing stuff after. Might as well keep just
>> the 'unlikat' tests.
>>
> 
> As long as coverage is preserved, I'm fine with consolidation of the
> checks. In any case, last call goes to Christian.
> 
>> I also went ahead and reverted 558f5c42efd ("tests/9pfs: Mark "local"
>> tests as "slow"") after realizing that the problem I was fixing is also
>> the same problem that this patch was trying to working around with the
>> skip [2]. I validated this change in this Gitlab pipeline:
>>
> 
> Are you sure with that ? Issues look very similar indeed but not
> exactly the same.

We can skip this revert if we're not sure about it. Gitlab passed with it but
perhaps this isn't evidence enough. I'll let you guys decide.


Thanks,

Daniel

> 
> Cheers,
> 
> --
> Greg
> 
>> https://gitlab.com/danielhb/qemu/-/pipelines/1227953967
>>
>> [1] https://mail.gnu.org/archive/html/qemu-devel/2024-03/msg05807.html
>> [2] https://lists.nongnu.org/archive/html/qemu-devel/2020-11/msg05510.html
>>
>> Daniel Henrique Barboza (3):
>>    qtest/virtio-9p-test.c: consolidate create dir, file and symlink tests
>>    qtest/virtio-9p-test.c: consolidate hardlink tests
>>    qtest/virtio-9p-test.c: remove g_test_slow() gate
>>
>>   tests/qtest/virtio-9p-test.c | 155 +++++++++++------------------------
>>   1 file changed, 48 insertions(+), 107 deletions(-)
>>

Re: [PATCH for-9.0 0/3] qtest/virtio-9p-test.c: fix slow tests

[Christian Schoenebeck]

On Tuesday, March 26, 2024 5:07:16 PM CET Daniel Henrique Barboza wrote:
> 
> On 3/26/24 12:55, Greg Kurz wrote:
> > Bom dia Daniel !
> 
> Bonne après-midi !
> 
> > 
> > On Tue, 26 Mar 2024 10:26:03 -0300
> > Daniel Henrique Barboza <dbarboza@ventanamicro.com> wrote:
> > 
> >> Hi,
> >>
> >> Thomas reported in [1] a problem that happened with the RISC-V machine
> >> where some tests from virtio-9p-test.c were failing with '-m slow', i.e.
> >> enabling slow tests.
> >>
> >> In the end it wasn't a RISC-V specific problem. It just so happens that
> >> the recently added riscv machine nodes runs the tests from
> >> virtio-9p-test two times for each qos-test run: one with the
> >> virtio-9p-device device and another with the virtio-9p-pci. The temp dir
> >> for these tests is being created at the start of qos-test and removed
> >> only at the end of qos-test, and the tests are leaving dirs and files
> >> behind. virtio-9-device tests run first, creates stuff in the temp dir,
> >> then when virtio-9p-pci tests runs again it'll fail because the previous
> >> run left created dirs and files in the same temp dir. Here's a run that
> >> exemplifies the problem:
> >>
> >> $ MALLOC_PERTURB_=21 V=2 QTEST_QEMU_BINARY=./qemu-system-riscv64 ./tests/qtest/qos-test -m slow
> >> (...)
> >> # starting QEMU: exec ./qemu-system-riscv64 -qtest unix:/tmp/qtest-621710.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-621710.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -M virt,aclint=on,aia=aplic-imsic -fsdev local,id=fsdev0,path='/home/danielhb/work/qemu/build/qtest-9p-local-7E16K2',security_model=mapped-xattr -device virtio-9p-device,fsdev=fsdev0,mount_tag=qtest -accel qtest
> >> ( goes ok ...)
> >> # starting QEMU: exec ./qemu-system-riscv64 -qtest unix:/tmp/qtest-621710.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-621710.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -M virt,aclint=on,aia=aplic-imsic -fsdev local,id=fsdev0,path='/home/danielhb/work/qemu/build/qtest-9p-local-7E16K2',security_model=mapped-xattr -device virtio-9p-pci,fsdev=fsdev0,addr=04.0,mount_tag=qtest -accel qtest
> >> ok 168 /riscv64/virt/generic-pcihost/pci-bus-generic/pci-bus/virtio-9p-pci/virtio-9p/virtio-9p-tests/local/config
> >> Received response 7 (RLERROR) instead of 73 (RMKDIR)
> >> Rlerror has errno 17 (File exists)
> >> **
> >> ERROR:../tests/qtest/libqos/virtio-9p-client.c:275:v9fs_req_recv: assertion failed (hdr.id == id): (7 == 73)
> >>
> >> As we can see we're running both 'virtio-9p-device' tests and 'virtio-9p-pci'
> >> tests using the same '/home/danielhb/work/qemu/build/qtest-9p-local-7E16K2'
> >> temp dir.
> >>
> > 
> > 
> > Good catch ! I'll try to find some time to review.
> > 
> >> The quick fix I came up with was to make each test clean themselves up
> >> after each run. The tests were also consolidated, i.e. fewer tests with the
> >> same coverage, because the 'unlikat' tests were doing the same thing the
> >> 'create' tests were doing but removing stuff after. Might as well keep just
> >> the 'unlikat' tests.
> >>
> > 
> > As long as coverage is preserved, I'm fine with consolidation of the
> > checks. In any case, last call goes to Christian.
> > 
> >> I also went ahead and reverted 558f5c42efd ("tests/9pfs: Mark "local"
> >> tests as "slow"") after realizing that the problem I was fixing is also
> >> the same problem that this patch was trying to working around with the
> >> skip [2]. I validated this change in this Gitlab pipeline:
> >>
> > 
> > Are you sure with that ? Issues look very similar indeed but not
> > exactly the same.
> 
> We can skip this revert if we're not sure about it. Gitlab passed with it but
> perhaps this isn't evidence enough. I'll let you guys decide.

I am a bit surprised because errnos were different (file exists vs. not
supported), but indeed, it did pass in your Gitlab pipeline. So I am fine with
bringing those tests back in on Gitlab.

/Christian

Re: [PATCH for-9.0 0/3] qtest/virtio-9p-test.c: fix slow tests

[Thomas Huth]

On 26/03/2024 14.26, Daniel Henrique Barboza wrote:
> Hi,
> 
> Thomas reported in [1] a problem that happened with the RISC-V machine
> where some tests from virtio-9p-test.c were failing with '-m slow', i.e.
> enabling slow tests.
> 
> In the end it wasn't a RISC-V specific problem. It just so happens that
> the recently added riscv machine nodes runs the tests from
> virtio-9p-test two times for each qos-test run: one with the
> virtio-9p-device device and another with the virtio-9p-pci. The temp dir
> for these tests is being created at the start of qos-test and removed
> only at the end of qos-test, and the tests are leaving dirs and files
> behind. virtio-9-device tests run first, creates stuff in the temp dir,
> then when virtio-9p-pci tests runs again it'll fail because the previous
> run left created dirs and files in the same temp dir. Here's a run that
> exemplifies the problem:
> 
> $ MALLOC_PERTURB_=21 V=2 QTEST_QEMU_BINARY=./qemu-system-riscv64 ./tests/qtest/qos-test -m slow
> (...)
> # starting QEMU: exec ./qemu-system-riscv64 -qtest unix:/tmp/qtest-621710.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-621710.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -M virt,aclint=on,aia=aplic-imsic -fsdev local,id=fsdev0,path='/home/danielhb/work/qemu/build/qtest-9p-local-7E16K2',security_model=mapped-xattr -device virtio-9p-device,fsdev=fsdev0,mount_tag=qtest -accel qtest
> ( goes ok ...)
> # starting QEMU: exec ./qemu-system-riscv64 -qtest unix:/tmp/qtest-621710.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-621710.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -M virt,aclint=on,aia=aplic-imsic -fsdev local,id=fsdev0,path='/home/danielhb/work/qemu/build/qtest-9p-local-7E16K2',security_model=mapped-xattr -device virtio-9p-pci,fsdev=fsdev0,addr=04.0,mount_tag=qtest -accel qtest
> ok 168 /riscv64/virt/generic-pcihost/pci-bus-generic/pci-bus/virtio-9p-pci/virtio-9p/virtio-9p-tests/local/config
> Received response 7 (RLERROR) instead of 73 (RMKDIR)
> Rlerror has errno 17 (File exists)
> **
> ERROR:../tests/qtest/libqos/virtio-9p-client.c:275:v9fs_req_recv: assertion failed (hdr.id == id): (7 == 73)
> 
> As we can see we're running both 'virtio-9p-device' tests and 'virtio-9p-pci'
> tests using the same '/home/danielhb/work/qemu/build/qtest-9p-local-7E16K2'
> temp dir.
> 
> The quick fix I came up with was to make each test clean themselves up
> after each run. The tests were also consolidated, i.e. fewer tests with the
> same coverage, because the 'unlikat' tests were doing the same thing the
> 'create' tests were doing but removing stuff after. Might as well keep just
> the 'unlikat' tests.
> 
> I also went ahead and reverted 558f5c42efd ("tests/9pfs: Mark "local"
> tests as "slow"") after realizing that the problem I was fixing is also
> the same problem that this patch was trying to working around with the
> skip [2]. I validated this change in this Gitlab pipeline:
> 
> https://gitlab.com/danielhb/qemu/-/pipelines/1227953967
> 
> [1] https://mail.gnu.org/archive/html/qemu-devel/2024-03/msg05807.html
> [2] https://lists.nongnu.org/archive/html/qemu-devel/2020-11/msg05510.html
> 
> Daniel Henrique Barboza (3):
>    qtest/virtio-9p-test.c: consolidate create dir, file and symlink tests
>    qtest/virtio-9p-test.c: consolidate hardlink tests

Thanks, these fix the "make check SPEED=slow" problems for me!

Tested-by: Thomas Huth <thuth@redhat.com>