On Tue, 26 Mar 2024 10:26:04 -0300 Daniel Henrique Barboza <dbarb...@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 <th...@redhat.com> > Signed-off-by: Daniel Henrique Barboza <dbarb...@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
