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