[PATCH for-9.0 v3 0/2] qtest/virtio-9p-test.c: fix slow tests

[PATCH for-9.0 v3 0/2] qtest/virtio-9p-test.c: fix slow tests

[Daniel Henrique Barboza]

Hi,

In this new version we took a different approach after the discussions
we had in [1]. The tests are now untouched, and we're addressing the root
cause directly: the fact that we have a single temp dir for all the test
execution in qos-test.

We're now creating and cleaning temp dirs for each individual test by
calling virtio_9p_create_local_test_dir() in the .before callback for
the local 9p tests (assign_9p_local_driver()). In this same callback we
queue the cleanup function that will erase the created temp dir. The
cleanup will run after the test ran successfully.

This approach is similar to what other qtests do (in fact this design was
taken from vhost-user-test.c) so it's not like we're doing something
novel.

I kept the revert of the slow test gate because Gitlab seems to approve
it:

https://gitlab.com/danielhb/qemu/-/pipelines/1229836634

Feel free to take just patch 1 if we're not sure about re-enabling these
tests in Gitlab.


Changes from v3:
- patches 1 to 6: dropped
- patch 1 (new):
  - create and remove temporary dirs on each test
- v2 link: https://mail.gnu.org/archive/html/qemu-devel/2024-03/msg06335.html

[1] https://mail.gnu.org/archive/html/qemu-devel/2024-03/msg06400.html

Daniel Henrique Barboza (2):
  qtest/virtio-9p-test.c: create/remove temp dirs after each test
  qtest/virtio-9p-test.c: remove g_test_slow() gate

 tests/qtest/virtio-9p-test.c | 32 +++++++++++---------------------
 1 file changed, 11 insertions(+), 21 deletions(-)

-- 
2.44.0

[PATCH for-9.0 v3 1/2] qtest/virtio-9p-test.c: create/remove temp dirs after each test

[Daniel Henrique Barboza]

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;

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

The root cause is that we're creating a single temporary dir, via the
construct/destruct callbacks, and this temp dir is kept for the entire
qos-test run.

We can change each test to clean after themselves. This approach would
make the 'create' tests obsolete since we would need to create and
delete dirs/files/symlinks for the cleanup, turning them into the
'unlinkat' tests that comes right after.

We chose a different approach that handles the root cause: do not use
constructor/destructor to create the temp dir. Create one temp dir for
each test, and remove it after the test is complete. This is the
approach taken for other qtests like vhost-user-test.c where each test
requires a setup() and a subsequent cleanup(), all of those instantiated
in the .before callback.

[1] https://mail.gnu.org/archive/html/qemu-devel/2024-03/msg05807.html

Reported-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 tests/qtest/virtio-9p-test.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index 65e69491e5..0179b3a394 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -693,9 +693,20 @@ static void fs_unlinkat_hardlink(void *obj, void *data,
     g_assert(stat(real_file, &st_real) == 0);
 }
 
+static void cleanup_9p_local_driver(void *data)
+{
+    /* remove previously created test dir when test is completed */
+    virtio_9p_remove_local_test_dir();
+}
+
 static void *assign_9p_local_driver(GString *cmd_line, void *arg)
 {
+    /* make sure test dir for the 'local' tests exists */
+    virtio_9p_create_local_test_dir();
+
     virtio_9p_assign_local_driver(cmd_line, "security_model=mapped-xattr");
+
+    g_test_queue_destroy(cleanup_9p_local_driver, NULL);
     return arg;
 }
 
@@ -759,15 +770,3 @@ static void register_virtio_9p_test(void)
 }
 
 libqos_init(register_virtio_9p_test);
-
-static void __attribute__((constructor)) construct_9p_test(void)
-{
-    /* make sure test dir for the 'local' tests exists */
-    virtio_9p_create_local_test_dir();
-}
-
-static void __attribute__((destructor)) destruct_9p_test(void)
-{
-    /* remove previously created test dir when test suite completed */
-    virtio_9p_remove_local_test_dir();
-}
-- 
2.44.0

[PATCH for-9.0 v3 2/2] qtest/virtio-9p-test.c: remove g_test_slow() gate

[Daniel Henrique Barboza]

Commit 558f5c42ef gated the local tests with g_test_slow() to skip them
in 'make check'. The reported issue back then was this following CI
problem:

https://lists.nongnu.org/archive/html/qemu-devel/2020-11/msg05510.html

This problem ended up being fixed after it was detected with the
recently added risc-v machine nodes [1]. virtio-9p-test.c is now
creating and removing temporary dirs for each test run, instead of
creating a single dir for the entire qos-test scope.

We're now able to run these tests with 'make check' in the CI, so let's
go ahead and re-enable them.

This reverts commit 558f5c42efded3e0d0b20a90bce2a9a14580d824.

[1] https://mail.gnu.org/archive/html/qemu-devel/2024-03/msg05807.html

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 tests/qtest/virtio-9p-test.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index 0179b3a394..3c8cd235cf 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -746,15 +746,6 @@ static void register_virtio_9p_test(void)
 
 
     /* 9pfs test cases using the 'local' filesystem driver */
-
-    /*
-     * XXX: Until we are sure that these tests can run everywhere,
-     * keep them as "slow" so that they aren't run with "make check".
-     */
-    if (!g_test_slow()) {
-        return;
-    }
-
     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);
-- 
2.44.0

Re: [PATCH for-9.0 v3 0/2] qtest/virtio-9p-test.c: fix slow tests

[Greg Kurz]

On Wed, 27 Mar 2024 11:20:09 -0300
Daniel Henrique Barboza <dbarboza@ventanamicro.com> wrote:

> Hi,
> 
> In this new version we took a different approach after the discussions
> we had in [1]. The tests are now untouched, and we're addressing the root
> cause directly: the fact that we have a single temp dir for all the test
> execution in qos-test.
> 
> We're now creating and cleaning temp dirs for each individual test by
> calling virtio_9p_create_local_test_dir() in the .before callback for
> the local 9p tests (assign_9p_local_driver()). In this same callback we
> queue the cleanup function that will erase the created temp dir. The
> cleanup will run after the test ran successfully.
> 
> This approach is similar to what other qtests do (in fact this design was
> taken from vhost-user-test.c) so it's not like we're doing something
> novel.
> 
> I kept the revert of the slow test gate because Gitlab seems to approve
> it:
> 
> https://gitlab.com/danielhb/qemu/-/pipelines/1229836634
> 
> Feel free to take just patch 1 if we're not sure about re-enabling these
> tests in Gitlab.
> 
> 
> Changes from v3:
> - patches 1 to 6: dropped
> - patch 1 (new):
>   - create and remove temporary dirs on each test
> - v2 link: https://mail.gnu.org/archive/html/qemu-devel/2024-03/msg06335.html
> 
> [1] https://mail.gnu.org/archive/html/qemu-devel/2024-03/msg06400.html
> 
> Daniel Henrique Barboza (2):
>   qtest/virtio-9p-test.c: create/remove temp dirs after each test
>   qtest/virtio-9p-test.c: remove g_test_slow() gate
> 
>  tests/qtest/virtio-9p-test.c | 32 +++++++++++---------------------
>  1 file changed, 11 insertions(+), 21 deletions(-)
> 

Definitely better !

Full series,

Reviewed-by:Greg Kurz <groug@kaod.org>

Cheers,

-- 
Greg

Re: [PATCH for-9.0 v3 0/2] qtest/virtio-9p-test.c: fix slow tests

[Christian Schoenebeck]

On Wednesday, March 27, 2024 3:20:09 PM CET Daniel Henrique Barboza wrote:
> Hi,
> 
> In this new version we took a different approach after the discussions
> we had in [1]. The tests are now untouched, and we're addressing the root
> cause directly: the fact that we have a single temp dir for all the test
> execution in qos-test.
> 
> We're now creating and cleaning temp dirs for each individual test by
> calling virtio_9p_create_local_test_dir() in the .before callback for
> the local 9p tests (assign_9p_local_driver()). In this same callback we
> queue the cleanup function that will erase the created temp dir. The
> cleanup will run after the test ran successfully.
> 
> This approach is similar to what other qtests do (in fact this design was
> taken from vhost-user-test.c) so it's not like we're doing something
> novel.
> 
> I kept the revert of the slow test gate because Gitlab seems to approve
> it:
> 
> https://gitlab.com/danielhb/qemu/-/pipelines/1229836634
> 
> Feel free to take just patch 1 if we're not sure about re-enabling these
> tests in Gitlab.
> 
> 
> Changes from v3:
> - patches 1 to 6: dropped
> - patch 1 (new):
>   - create and remove temporary dirs on each test
> - v2 link: https://mail.gnu.org/archive/html/qemu-devel/2024-03/msg06335.html
> 
> [1] https://mail.gnu.org/archive/html/qemu-devel/2024-03/msg06400.html
> 
> Daniel Henrique Barboza (2):
>   qtest/virtio-9p-test.c: create/remove temp dirs after each test
>   qtest/virtio-9p-test.c: remove g_test_slow() gate
> 
>  tests/qtest/virtio-9p-test.c | 32 +++++++++++---------------------
>  1 file changed, 11 insertions(+), 21 deletions(-)
> 
> 

Awesome!

Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com>

Are the riscv patches already on master? I.e. should I push those two patches
through my queue?

/Christian

Re: [PATCH for-9.0 v3 0/2] qtest/virtio-9p-test.c: fix slow tests

[Daniel Henrique Barboza]

On 3/27/24 15:53, Christian Schoenebeck wrote:
> On Wednesday, March 27, 2024 3:20:09 PM CET Daniel Henrique Barboza wrote:
>> Hi,
>>
>> In this new version we took a different approach after the discussions
>> we had in [1]. The tests are now untouched, and we're addressing the root
>> cause directly: the fact that we have a single temp dir for all the test
>> execution in qos-test.
>>
>> We're now creating and cleaning temp dirs for each individual test by
>> calling virtio_9p_create_local_test_dir() in the .before callback for
>> the local 9p tests (assign_9p_local_driver()). In this same callback we
>> queue the cleanup function that will erase the created temp dir. The
>> cleanup will run after the test ran successfully.
>>
>> This approach is similar to what other qtests do (in fact this design was
>> taken from vhost-user-test.c) so it's not like we're doing something
>> novel.
>>
>> I kept the revert of the slow test gate because Gitlab seems to approve
>> it:
>>
>> https://gitlab.com/danielhb/qemu/-/pipelines/1229836634
>>
>> Feel free to take just patch 1 if we're not sure about re-enabling these
>> tests in Gitlab.
>>
>>
>> Changes from v3:
>> - patches 1 to 6: dropped
>> - patch 1 (new):
>>    - create and remove temporary dirs on each test
>> - v2 link: https://mail.gnu.org/archive/html/qemu-devel/2024-03/msg06335.html
>>
>> [1] https://mail.gnu.org/archive/html/qemu-devel/2024-03/msg06400.html
>>
>> Daniel Henrique Barboza (2):
>>    qtest/virtio-9p-test.c: create/remove temp dirs after each test
>>    qtest/virtio-9p-test.c: remove g_test_slow() gate
>>
>>   tests/qtest/virtio-9p-test.c | 32 +++++++++++---------------------
>>   1 file changed, 11 insertions(+), 21 deletions(-)
>>
>>
> 
> Awesome!
> 
> Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> 
> Are the riscv patches already on master? I.e. should I push those two patches
> through my queue?

Yes, they were pushed to master almost 2 months ago and Thomas got wind of this problem
at the start of this week.


Thanks,

Daniel

> 
> /Christian
> 
> 

Re: [PATCH for-9.0 v3 0/2] qtest/virtio-9p-test.c: fix slow tests

[Thomas Huth]

On 27/03/2024 15.20, Daniel Henrique Barboza wrote:
> Hi,
> 
> In this new version we took a different approach after the discussions
> we had in [1]. The tests are now untouched, and we're addressing the root
> cause directly: the fact that we have a single temp dir for all the test
> execution in qos-test.
> 
> We're now creating and cleaning temp dirs for each individual test by
> calling virtio_9p_create_local_test_dir() in the .before callback for
> the local 9p tests (assign_9p_local_driver()). In this same callback we
> queue the cleanup function that will erase the created temp dir. The
> cleanup will run after the test ran successfully.
> 
> This approach is similar to what other qtests do (in fact this design was
> taken from vhost-user-test.c) so it's not like we're doing something
> novel.
> 
> I kept the revert of the slow test gate because Gitlab seems to approve
> it:
> 
> https://gitlab.com/danielhb/qemu/-/pipelines/1229836634
> 
> Feel free to take just patch 1 if we're not sure about re-enabling these
> tests in Gitlab.
> 
> 
> Changes from v3:
> - patches 1 to 6: dropped
> - patch 1 (new):
>    - create and remove temporary dirs on each test
> - v2 link: https://mail.gnu.org/archive/html/qemu-devel/2024-03/msg06335.html
> 
> [1] https://mail.gnu.org/archive/html/qemu-devel/2024-03/msg06400.html
> 
> Daniel Henrique Barboza (2):
>    qtest/virtio-9p-test.c: create/remove temp dirs after each test
>    qtest/virtio-9p-test.c: remove g_test_slow() gate
> 
>   tests/qtest/virtio-9p-test.c | 32 +++++++++++---------------------
>   1 file changed, 11 insertions(+), 21 deletions(-)


Works for me, too!

Tested-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH for-9.0 v3 0/2] qtest/virtio-9p-test.c: fix slow tests

[Christian Schoenebeck]

On Wednesday, March 27, 2024 3:20:09 PM CET Daniel Henrique Barboza wrote:
> Hi,
> 
> In this new version we took a different approach after the discussions
> we had in [1]. The tests are now untouched, and we're addressing the root
> cause directly: the fact that we have a single temp dir for all the test
> execution in qos-test.
> 
> We're now creating and cleaning temp dirs for each individual test by
> calling virtio_9p_create_local_test_dir() in the .before callback for
> the local 9p tests (assign_9p_local_driver()). In this same callback we
> queue the cleanup function that will erase the created temp dir. The
> cleanup will run after the test ran successfully.
> 
> This approach is similar to what other qtests do (in fact this design was
> taken from vhost-user-test.c) so it's not like we're doing something
> novel.
> 
> I kept the revert of the slow test gate because Gitlab seems to approve
> it:
> 
> https://gitlab.com/danielhb/qemu/-/pipelines/1229836634
> 
> Feel free to take just patch 1 if we're not sure about re-enabling these
> tests in Gitlab.
> 
> 
> Changes from v3:
> - patches 1 to 6: dropped
> - patch 1 (new):
>   - create and remove temporary dirs on each test
> - v2 link: https://mail.gnu.org/archive/html/qemu-devel/2024-03/msg06335.html
> 
> [1] https://mail.gnu.org/archive/html/qemu-devel/2024-03/msg06400.html
> 
> Daniel Henrique Barboza (2):
>   qtest/virtio-9p-test.c: create/remove temp dirs after each test
>   qtest/virtio-9p-test.c: remove g_test_slow() gate
> 
>  tests/qtest/virtio-9p-test.c | 32 +++++++++++---------------------
>  1 file changed, 11 insertions(+), 21 deletions(-)

Queued on 9p.next:
https://github.com/cschoenebeck/qemu/commits/9p.next

Thanks!

/Christian

Re: [PATCH for-9.0 v3 2/2] qtest/virtio-9p-test.c: remove g_test_slow() gate

[Michael Tokarev]

27.03.2024 17:20, Daniel Henrique Barboza :
> Commit 558f5c42ef gated the local tests with g_test_slow() to skip them
> in 'make check'. The reported issue back then was this following CI
> problem:
> 
> https://lists.nongnu.org/archive/html/qemu-devel/2020-11/msg05510.html
> 
> This problem ended up being fixed after it was detected with the
> recently added risc-v machine nodes [1]. virtio-9p-test.c is now
> creating and removing temporary dirs for each test run, instead of
> creating a single dir for the entire qos-test scope.
> 
> We're now able to run these tests with 'make check' in the CI, so let's
> go ahead and re-enable them.
> 
> This reverts commit 558f5c42efded3e0d0b20a90bce2a9a14580d824.
> 
> [1] https://mail.gnu.org/archive/html/qemu-devel/2024-03/msg05807.html

This makes tests being unable to complete on a tmpfs.  It looks like
9pfs tests needs another tweak here.

# starting QEMU: exec ./qemu-system-x86_64 -qtest unix:/tmp/qtest-798502.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-798502.qmp,id=char0 
-mon chardev=char0,mode=control -display none -audio none -M pc  -fsdev 
local,id=fsdev0,path='/tmp/q/master/qtest-9p-local-9LHRL2',security_model=mapped-xattr -device virtio-9p-pci,fsdev=fsdev0,addr=04.0,mount_tag=qtest 
-accel qtest
Received response 7 (RLERROR) instead of 73 (RMKDIR)
Rlerror has errno 95 (Operation not supported)
**
ERROR:../../../build/qemu/master/tests/qtest/libqos/virtio-9p-client.c:275:v9fs_req_recv: assertion failed (hdr.id == id): (7 == 73)

This is when I build it on /tmp/ which is a tmpfs.  When I build
it on a real filesystem, it works fine.

Apparently xattrs aren't supported on a tmpfs.

/mjt

Re: [PATCH for-9.0 v3 2/2] qtest/virtio-9p-test.c: remove g_test_slow() gate

[Daniel Henrique Barboza]

On 4/16/24 16:54, Michael Tokarev wrote:
> 27.03.2024 17:20, Daniel Henrique Barboza :
>> Commit 558f5c42ef gated the local tests with g_test_slow() to skip them
>> in 'make check'. The reported issue back then was this following CI
>> problem:
>>
>> https://lists.nongnu.org/archive/html/qemu-devel/2020-11/msg05510.html
>>
>> This problem ended up being fixed after it was detected with the
>> recently added risc-v machine nodes [1]. virtio-9p-test.c is now
>> creating and removing temporary dirs for each test run, instead of
>> creating a single dir for the entire qos-test scope.
>>
>> We're now able to run these tests with 'make check' in the CI, so let's
>> go ahead and re-enable them.
>>
>> This reverts commit 558f5c42efded3e0d0b20a90bce2a9a14580d824.
>>
>> [1] https://mail.gnu.org/archive/html/qemu-devel/2024-03/msg05807.html
> 
> This makes tests being unable to complete on a tmpfs.  It looks like
> 9pfs tests needs another tweak here.
> 
> # starting QEMU: exec ./qemu-system-x86_64 -qtest unix:/tmp/qtest-798502.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-798502.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -M pc  -fsdev local,id=fsdev0,path='/tmp/q/master/qtest-9p-local-9LHRL2',security_model=mapped-xattr -device virtio-9p-pci,fsdev=fsdev0,addr=04.0,mount_tag=qtest -accel qtest
> Received response 7 (RLERROR) instead of 73 (RMKDIR)
> Rlerror has errno 95 (Operation not supported)
> **
> ERROR:../../../build/qemu/master/tests/qtest/libqos/virtio-9p-client.c:275:v9fs_req_recv: assertion failed (hdr.id == id): (7 == 73)
> 
> This is when I build it on /tmp/ which is a tmpfs.  When I build
> it on a real filesystem, it works fine.
> 
> Apparently xattrs aren't supported on a tmpfs.

Hmmm not sure how to proceed here since I'm not a 9p expert by any means. I'll
let Christian decide what to do.

If we can't figure it out we might need to re-introduce the gate again. Thanks,


Daniel

> 
> /mjt

Re: [PATCH for-9.0 v3 2/2] qtest/virtio-9p-test.c: remove g_test_slow() gate

[Christian Schoenebeck]

On Wednesday, April 17, 2024 1:16:02 AM CEST Daniel Henrique Barboza wrote:
> 
> On 4/16/24 16:54, Michael Tokarev wrote:
> > 27.03.2024 17:20, Daniel Henrique Barboza :
> >> Commit 558f5c42ef gated the local tests with g_test_slow() to skip them
> >> in 'make check'. The reported issue back then was this following CI
> >> problem:
> >>
> >> https://lists.nongnu.org/archive/html/qemu-devel/2020-11/msg05510.html
> >>
> >> This problem ended up being fixed after it was detected with the
> >> recently added risc-v machine nodes [1]. virtio-9p-test.c is now
> >> creating and removing temporary dirs for each test run, instead of
> >> creating a single dir for the entire qos-test scope.
> >>
> >> We're now able to run these tests with 'make check' in the CI, so let's
> >> go ahead and re-enable them.
> >>
> >> This reverts commit 558f5c42efded3e0d0b20a90bce2a9a14580d824.
> >>
> >> [1] https://mail.gnu.org/archive/html/qemu-devel/2024-03/msg05807.html
> > 
> > This makes tests being unable to complete on a tmpfs.  It looks like
> > 9pfs tests needs another tweak here.
> > 
> > # starting QEMU: exec ./qemu-system-x86_64 -qtest unix:/tmp/qtest-798502.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-798502.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -M pc  -fsdev local,id=fsdev0,path='/tmp/q/master/qtest-9p-local-9LHRL2',security_model=mapped-xattr -device virtio-9p-pci,fsdev=fsdev0,addr=04.0,mount_tag=qtest -accel qtest
> > Received response 7 (RLERROR) instead of 73 (RMKDIR)
> > Rlerror has errno 95 (Operation not supported)
> > **
> > ERROR:../../../build/qemu/master/tests/qtest/libqos/virtio-9p-client.c:275:v9fs_req_recv: assertion failed (hdr.id == id): (7 == 73)
> > 
> > This is when I build it on /tmp/ which is a tmpfs.  When I build
> > it on a real filesystem, it works fine.
> > 
> > Apparently xattrs aren't supported on a tmpfs.
> 
> Hmmm not sure how to proceed here since I'm not a 9p expert by any means. I'll
> let Christian decide what to do.
> 
> If we can't figure it out we might need to re-introduce the gate again. Thanks,

It's not that tmpfs exactly doesn't support xattrs. It supports the trusted.*
and security.* namespaces since 2011, so tmpfs was limited to those two. For
the 9p 'local' backend however we also need the user.* namespace which was
just added in Linux 6.6 last year (commit 2daf18a).

Unfortunately the respective kernel option TMPFS_XATTR is still off by default
(linux/fs/Kconfig).

Back then, when we added that 'slow' gate for the 9p 'local' tests, things
were a bit different. They simply did not run in the gitlab pipeline (for
reasons described above). Now they do.

So obviously it would make sense to preserve these tests for the gitlab
pipeline this time, e.g. by skipping these tests only if the underlying test
directory does not support *.user xattrs. I'm just not sure yet where exactly
such kind of *active* check would fit best into the glib test layout, as this
can be a bit tricky with glib.

/Christian

Re: [PATCH for-9.0 v3 2/2] qtest/virtio-9p-test.c: remove g_test_slow() gate

[Daniel P. Berrangé]

On Wed, Apr 17, 2024 at 01:52:24PM +0200, Christian Schoenebeck wrote:
> On Wednesday, April 17, 2024 1:16:02 AM CEST Daniel Henrique Barboza wrote:
> > 
> > On 4/16/24 16:54, Michael Tokarev wrote:
> > > 27.03.2024 17:20, Daniel Henrique Barboza :
> > >> Commit 558f5c42ef gated the local tests with g_test_slow() to skip them
> > >> in 'make check'. The reported issue back then was this following CI
> > >> problem:
> > >>
> > >> https://lists.nongnu.org/archive/html/qemu-devel/2020-11/msg05510.html
> > >>
> > >> This problem ended up being fixed after it was detected with the
> > >> recently added risc-v machine nodes [1]. virtio-9p-test.c is now
> > >> creating and removing temporary dirs for each test run, instead of
> > >> creating a single dir for the entire qos-test scope.
> > >>
> > >> We're now able to run these tests with 'make check' in the CI, so let's
> > >> go ahead and re-enable them.
> > >>
> > >> This reverts commit 558f5c42efded3e0d0b20a90bce2a9a14580d824.
> > >>
> > >> [1] https://mail.gnu.org/archive/html/qemu-devel/2024-03/msg05807.html
> > > 
> > > This makes tests being unable to complete on a tmpfs.  It looks like
> > > 9pfs tests needs another tweak here.
> > > 
> > > # starting QEMU: exec ./qemu-system-x86_64 -qtest unix:/tmp/qtest-798502.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-798502.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -M pc  -fsdev local,id=fsdev0,path='/tmp/q/master/qtest-9p-local-9LHRL2',security_model=mapped-xattr -device virtio-9p-pci,fsdev=fsdev0,addr=04.0,mount_tag=qtest -accel qtest
> > > Received response 7 (RLERROR) instead of 73 (RMKDIR)
> > > Rlerror has errno 95 (Operation not supported)
> > > **
> > > ERROR:../../../build/qemu/master/tests/qtest/libqos/virtio-9p-client.c:275:v9fs_req_recv: assertion failed (hdr.id == id): (7 == 73)
> > > 
> > > This is when I build it on /tmp/ which is a tmpfs.  When I build
> > > it on a real filesystem, it works fine.
> > > 
> > > Apparently xattrs aren't supported on a tmpfs.
> > 
> > Hmmm not sure how to proceed here since I'm not a 9p expert by any means. I'll
> > let Christian decide what to do.
> > 
> > If we can't figure it out we might need to re-introduce the gate again. Thanks,
> 
> It's not that tmpfs exactly doesn't support xattrs. It supports the trusted.*
> and security.* namespaces since 2011, so tmpfs was limited to those two. For
> the 9p 'local' backend however we also need the user.* namespace which was
> just added in Linux 6.6 last year (commit 2daf18a).
> 
> Unfortunately the respective kernel option TMPFS_XATTR is still off by default
> (linux/fs/Kconfig).
> 
> Back then, when we added that 'slow' gate for the 9p 'local' tests, things
> were a bit different. They simply did not run in the gitlab pipeline (for
> reasons described above). Now they do.
> 
> So obviously it would make sense to preserve these tests for the gitlab
> pipeline this time, e.g. by skipping these tests only if the underlying test
> directory does not support *.user xattrs. I'm just not sure yet where exactly
> such kind of *active* check would fit best into the glib test layout, as this
> can be a bit tricky with glib

You should run a method which checks ability to use '.user' xattrs, and
if it reports failure, then skip calling g_test_add. IOW, you can put
the xattr test in the same place as the old g_test_slow() check was.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|