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