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