Re: [PATCH for-9.0 1/3] qtest/virtio-9p-test.c: consolidate create dir, file and symlink tests

[Greg Kurz]

On Wed, 27 Mar 2024 13:26:45 +0100
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> On Wednesday, March 27, 2024 12:28:17 PM CET Daniel Henrique Barboza wrote:
> > On 3/27/24 07:14, Christian Schoenebeck wrote:
> > > 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 <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.
> > > 
> > > 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.
> > 
> > The current test design is flawed. It's based on a premise that doesn't 
> > happen, i.e.
> > a new temp dir will be created every time the test suit is executed. In 
> > reality the
> > temp dir is created only once in the constructor of the test, at the start 
> > of qos-test
> > (tests/qtest/qos-test.c, run_one_test()) and removed only once at the 
> > destructor
> > at the end of the run.
> > 
> > It's not possible to add a 'device name' in the created temp dir because 
> > we're too early
> > in the process, the tests didn't start at that point. So, with the current 
> > temp dir design,
> > the tests needs to clean themselves up after each run.
> > 
> > Here's the alternatives I'm willing to go for:
> > 
> > - what I just sent in v2;
> > 
> > - add cleanups in all existing tests. We can keep all of them, but the 
> > 'create' tests
> > will be carbon copies of the 'unlinkat' tests but with different names. Can 
> > be done;
> > 
> > - if we really want the tests untouched we can rework how the 'temp dir' is 
> > created/deleted.
> > The test dir will be created and removed after each test via the 'before' 
> > callback. To be
> > honest this seems like the best approach we can take, aside from what I did 
> > in v2, and
> > it's on par with how tests like vhost-user-test.c works.
> 
> Yeah, the latter sounds like the best solution to me, too.
> 

+1

> Don't get me wrong, I didn't want to burden you with more work. It's really
> just that I think that restructuring all test cases is contra productive.
> 
> If you want I can also look into that. Just let me know.
> 
> Thanks!
> 
> /Christian
> 
> 



-- 
Greg