Re: [Qemu-devel] [PATCH v7 12/54] tests/tcg/multiarch: don't hard code paths/ports for linux-test

[Daniel P . Berrangé]

On Mon, Jun 18, 2018 at 11:56:08AM +0100, Alex Bennée wrote:
> 
> Thomas Huth <address@hidden> writes:
> 
> > On 15.06.2018 21:46, Alex Bennée wrote:
> >> The fixed path and ports get in the way of running our tests and
> >> builds in parallel. Instead of using TESTPATH we use mkdtemp() and
> >> instead of a fixed port we allow the kernel to assign one and query it
> >> afterwards.
> >>
> >> Signed-off-by: Alex Bennée <address@hidden>
> >> ---
> >>  tests/tcg/multiarch/linux-test.c | 37 ++++++++++++++++----------------
> >>  1 file changed, 19 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/tests/tcg/multiarch/linux-test.c 
> >> b/tests/tcg/multiarch/linux-test.c
> >> index 6f2c531474..3f73b96420 100644
> >> --- a/tests/tcg/multiarch/linux-test.c
> >> +++ b/tests/tcg/multiarch/linux-test.c
> >> @@ -41,8 +41,6 @@
> >>  #include <setjmp.h>
> >>  #include <sys/shm.h>
> >>
> >> -#define TESTPATH "/tmp/linux-test.tmp"
> >> -#define TESTPORT 7654
> >>  #define STACK_SIZE 16384
> >>
> >>  static void error1(const char *filename, int line, const char *fmt, ...)
> >> @@ -85,19 +83,15 @@ static void test_file(void)
> >>      struct iovec vecs[2];
> >>      DIR *dir;
> >>      struct dirent *de;
> >> +    char template[] = "/tmp/linux-test-XXXXXX";
> >> +    char *tmpdir = mkdtemp(template);
> >>
> >> -    /* clean up, just in case */
> >> -    unlink(TESTPATH "/file1");
> >> -    unlink(TESTPATH "/file2");
> >> -    unlink(TESTPATH "/file3");
> >> -    rmdir(TESTPATH);
> >> +    chk_error(strlen(tmpdir));
> >
> > That line looks wrong to me. According to my man-page of mkdtemp(), it
> > returns either NULL or a pointer to the modified string.
> > In case of NULL, strlen(tmpdir) will simply crash. And even if it would
> > not crash, strlen() only returns values >= 0, so there is no way the
> > chk_error could ever report an error here.
> 
> As we only really want to check we did actually do a mkdtemp would:
> 
>   chk_error(tmpdir)
> 
> Be enough?

I feel like this is a common task across all our test cases, so we would
be well served by defining a helper program to give better semantics.

I feel like we should be creating temporary files in the build dir too
by default, rather than /tmp, since some of our test suites create quite
large files and /tmp is a limited size RAMFS on many distros. THis leads
to obscure errors when running many tests in parallel if space is exhausted.

So how about creating a shared function:

    char *qtest_tempdir(const char *basename) {
        char *here = g_get_current_dir();
        char *tmpl = g_strdup_printf("%s/%sXXXXXX", here, basename);
        g_free(here);
        char *res = g_mkstemp(tmpl);
        if (res == NULL) {
            error_report("Unable to create temporary dir: %s",
                         strerror(errno));
            abort();
        }
        return res;
    }

To be used as

     char *dir = qtest_tempdir("linux-test");


And all other test suites can be updated to use this over time too.

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