Re: [PULL 00/13] 9p queue 2020-10-23

[Christian Schoenebeck]

On Donnerstag, 29. Oktober 2020 16:04:03 CET Daniel P. Berrangé wrote:
> On Thu, Oct 29, 2020 at 02:52:16PM +0000, Peter Maydell wrote:
> > On Thu, 29 Oct 2020 at 14:31, Christian Schoenebeck
> > 
> > <qemu_oss@crudebyte.com> wrote:
> > > On Donnerstag, 29. Oktober 2020 15:15:19 CET Peter Maydell wrote:
> > > > On Thu, 29 Oct 2020 at 14:06, Christian Schoenebeck
> > > > 
> > > > <qemu_oss@crudebyte.com> wrote:
> > > > > Ok, I'll use mkdtemp() instead, that avoids other potential parallel
> > > > > config
> > > > > colissions that I may not have considered yet.
> > > > > 
> > > > > The original motivation against mkdtemp() was that /tmp is isually a
> > > > > ramfs,
> > > > > hence very limited regarding large file tests. But that's not an
> > > > > issue
> > > > > right now.
> > > > 
> > > > How large is "large" here ?
> > > 
> > > E.g. ~10 GiB which might be a problem for cloud based CI platforms.
> > 
> > Yeah, 10GB is too big by an order of magnitude for anything in the
> > standard "make check" set. It could go in an optional "I'm the 9p
> > maintainer and I want to run a wider set of tests" target though.
> 
> I think as a rough rule of thumb, tests should not create files
> that are larger than the size of the QEMU build dir, and if it is
> creating non-trivially sized files, then they should be created in
> the build dir, not /tmp.  This probably puts 1 GB as a upper bound
> on size but bear in mind tests can run in parallel, so ideally biggest
> file sizes should be more in the 100s of MB range
> 
> Regards,
> Daniel

I definitely won't run such large-file tests uncaged, so it would not run by
default for sure. But sooner or later it would make sense to streamline test
case options in QEMU in general, so that certain tests would automatically run
or not depending on runner's capabilities, limits and preferences.

The TCG tests for instance periodically trigger test failures as they exceed
Travis-CI's runtime limit of 40mins once in a while.

Back to the actual 9p test failure issue, I think I will go for something like
this:

diff --git a/tests/qtest/libqos/virtio-9p.c b/tests/qtest/libqos/virtio-9p.c
index d43647b3b7..ebbacd5896 100644
--- a/tests/qtest/libqos/virtio-9p.c
+++ b/tests/qtest/libqos/virtio-9p.c
@@ -35,7 +35,12 @@ static char *concat_path(const char* a, const char* b)
 static void init_local_test_path(void)
 {
     char *pwd = g_get_current_dir();
-    local_test_path = concat_path(pwd, "qtest-9p-local");
+    char *template = concat_path(pwd, "qtest-9p-local-XXXXXX");
+    local_test_path = mkdtemp(template);
+    if (!local_test_path) {
+        g_test_message("mkdtemp('%s') failed: %s", template, strerror(errno));
+    }
+    g_assert(local_test_path);
     g_free(pwd);
 }
 
@@ -246,11 +251,6 @@ static void virtio_9p_register_nodes(void)
     const char *str_simple = "fsdev=fsdev0,mount_tag=" MOUNT_TAG;
     const char *str_addr = "fsdev=fsdev0,addr=04.0,mount_tag=" MOUNT_TAG;
 
-    /* make sure test dir for the 'local' tests exists and is clean */
-    init_local_test_path();
-    remove_local_test_dir();
-    create_local_test_dir();
-
     QPCIAddress addr = {
         .devfn = QPCI_DEVFN(4, 0),
     };
@@ -278,3 +278,14 @@ static void virtio_9p_register_nodes(void)
 }
 
 libqos_init(virtio_9p_register_nodes);
+
+static void __attribute__((constructor)) construct_virtio_9p(void) {
+    /* make sure test dir for the 'local' tests exists */
+    init_local_test_path();
+    create_local_test_dir();
+}
+
+static void __attribute__((destructor)) destruct_virtio_9p(void) {
+    /* remove previously created test dir when test suite completed */
+    remove_local_test_dir();
+}

So it would still use the current directory instead of /tmp/ and would
create and remove the test dir on process start/end. I wanted to avoid
using constructor/destructor atttributes, as some compilers don't
support them, but it seems it's already used in the code base and there
are currently no setup/teardown callbacks in libqos.

Another downside: with 'make check -jN' this will temporarily create a
bunch of unused, empty dirs. But I hope that's Ok to fix in QEMU 6
(by adding setup/teardown callbacks to libqos later).

Just hoping there are no glib versions that use threads instead of
processes.

Best regards,
Christian Schoenebeck