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

[Thomas Huth]

On 18.06.2018 14:04, Alex Bennée wrote:
> 
> 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.

I agree.

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

Yes, I think that's something for a later patch series. Or maybe even a
BiteSizeTask?

 Thomas