qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v7 12/54] tests/tcg/multiarch: don't hard code p


From: Alex Bennée
Subject: Re: [Qemu-devel] [PATCH v7 12/54] tests/tcg/multiarch: don't hard code paths/ports for linux-test
Date: Mon, 18 Jun 2018 13:04:57 +0100
User-agent: mu4e 1.1.0; emacs 26.1.50

Daniel P. Berrangé <address@hidden> writes:

> 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;
>     }

We could do, although we can't use glib due to the potential vagaries
of cross compile setups. However my personal feeling is to keep the TCG
tests as self contained as possible for the time being.

>
> To be used as
>
>      char *dir = qtest_tempdir("linux-test");
>
>
> And all other test suites can be updated to use this over time too.

Is this something that could be addressed in later patches?

>
> Regards,
> Daniel


--
Alex Bennée



reply via email to

[Prev in Thread] Current Thread [Next in Thread]