Re: [PATCH for-9.0 1/3] qtest/virtio-9p-test.c: consolidate create dir, file and symlink tests
On 3/27/24 09:26, Christian Schoenebeck 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 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. Fair enough. I have it ready here - I'll just wait to see how Gitlab likes it before post
Re: [PATCH for-9.0 1/3] qtest/virtio-9p-test.c: consolidate create dir, file and symlink tests
On Wed, 27 Mar 2024 13:26:45 +0100 Christian Schoenebeck 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 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 r
Re: [PATCH for-9.0 1/3] qtest/virtio-9p-test.c: consolidate create dir, file and symlink tests
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 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 th
Re: [PATCH for-9.0 1/3] qtest/virtio-9p-test.c: consolidate create dir, file and symlink tests
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 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. Thanks, Daniel /Christian
Re: [PATCH for-9.0 1/3] qtest/virtio-9p-test.c: consolidate create dir, file and symlink tests
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 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
Re: [PATCH for-9.0 1/3] qtest/virtio-9p-test.c: consolidate create dir, file and symlink tests
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 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
Re: [PATCH for-9.0 1/3] qtest/virtio-9p-test.c: consolidate create dir, file and symlink tests
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 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. /Christian
Re: [PATCH for-9.0 1/3] qtest/virtio-9p-test.c: consolidate create dir, file and symlink tests
On 3/26/24 14:05, Greg Kurz wrote:
On Tue, 26 Mar 2024 10:26:04 -0300
Daniel Henrique Barboza 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
Signed-off-by: Daniel Henrique Barboza
---
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/d
Re: [PATCH for-9.0 1/3] qtest/virtio-9p-test.c: consolidate create dir, file and symlink tests
On Tue, 26 Mar 2024 10:26:04 -0300
Daniel Henrique Barboza 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
> Signed-off-by: Daniel Henrique Barboza
> ---
> 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_a
[PATCH for-9.0 1/3] qtest/virtio-9p-test.c: consolidate create dir, file and symlink tests
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.
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
Signed-off-by: Daniel Henrique Barboza
---
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 =
