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