diff mbox series

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

Message ID 20180615194705.28019-13-alex.bennee@linaro.org
State Superseded
Headers show
Series fix building of tests/tcg - last chance to review! | expand

Commit Message

Alex Bennée June 15, 2018, 7:46 p.m. UTC
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 <alex.bennee@linaro.org>

---
 tests/tcg/multiarch/linux-test.c | 37 ++++++++++++++++----------------
 1 file changed, 19 insertions(+), 18 deletions(-)

-- 
2.17.1

Comments

Thomas Huth June 16, 2018, 7:46 p.m. UTC | #1
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 <alex.bennee@linaro.org>

> ---

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

 Thomas
Philippe Mathieu-Daudé June 16, 2018, 10:24 p.m. UTC | #2
Hi Alex,

On 06/15/2018 04:46 PM, 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 <alex.bennee@linaro.org>

> ---

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


Since /tmp doesn't always fit, can this be:

       char *tmpbase = getenv("TMPDIR");
       char *template = g_strdup_printf("%s/qemu-test-XXXXXX",
                                        tmpbase ? tmpbase : "/tmp");

> +    char *tmpdir = mkdtemp(template);


       g_free(template);

>  

> -    /* clean up, just in case */

> -    unlink(TESTPATH "/file1");

> -    unlink(TESTPATH "/file2");

> -    unlink(TESTPATH "/file3");

> -    rmdir(TESTPATH);

> +    chk_error(strlen(tmpdir));

>  

>      if (getcwd(cur_dir, sizeof(cur_dir)) == NULL)

>          error("getcwd");

>  

> -    chk_error(mkdir(TESTPATH, 0755));

> -

> -    chk_error(chdir(TESTPATH));

> +    chk_error(chdir(tmpdir));

>  

>      /* open/read/write/close/readv/writev/lseek */

>  

> @@ -163,7 +157,7 @@ static void test_file(void)

>          st.st_mtime != 1000)

>          error("stat time");

>  

> -    chk_error(stat(TESTPATH, &st));

> +    chk_error(stat(tmpdir, &st));

>      if (!S_ISDIR(st.st_mode))

>          error("stat mode");

>  

> @@ -185,7 +179,7 @@ static void test_file(void)

>          error("stat mode");

>  

>      /* getdents */

> -    dir = opendir(TESTPATH);

> +    dir = opendir(tmpdir);

>      if (!dir)

>          error("opendir");

>      len = 0;

> @@ -207,7 +201,7 @@ static void test_file(void)

>      chk_error(unlink("file3"));

>      chk_error(unlink("file2"));

>      chk_error(chdir(cur_dir));

> -    chk_error(rmdir(TESTPATH));

> +    chk_error(rmdir(tmpdir));

>  }

>  

>  static void test_fork(void)

> @@ -264,7 +258,7 @@ static int server_socket(void)

>      chk_error(setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &val, sizeof(val)));

>  

>      sockaddr.sin_family = AF_INET;

> -    sockaddr.sin_port = htons(TESTPORT);

> +    sockaddr.sin_port = htons(0); /* choose random ephemeral port) */

>      sockaddr.sin_addr.s_addr = 0;

>      chk_error(bind(fd, (struct sockaddr *)&sockaddr, sizeof(sockaddr)));

>      chk_error(listen(fd, 0));

> @@ -272,7 +266,7 @@ static int server_socket(void)

>  

>  }

>  

> -static int client_socket(void)

> +static int client_socket(uint16_t port)

>  {

>      int fd;

>      struct sockaddr_in sockaddr;

> @@ -280,7 +274,7 @@ static int client_socket(void)

>      /* server socket */

>      fd = chk_error(socket(PF_INET, SOCK_STREAM, 0));

>      sockaddr.sin_family = AF_INET;

> -    sockaddr.sin_port = htons(TESTPORT);

> +    sockaddr.sin_port = htons(port);

>      inet_aton("127.0.0.1", &sockaddr.sin_addr);

>      chk_error(connect(fd, (struct sockaddr *)&sockaddr, sizeof(sockaddr)));

>      return fd;

> @@ -292,10 +286,17 @@ static void test_socket(void)

>  {

>      int server_fd, client_fd, fd, pid, ret, val;

>      struct sockaddr_in sockaddr;

> -    socklen_t len;

> +    struct sockaddr_in server_addr;

> +    socklen_t len, socklen;

> +    uint16_t server_port;

>      char buf[512];

>  

>      server_fd = server_socket();

> +    /* find out what port we got */

> +    socklen = sizeof(server_addr);

> +    ret = getsockname(server_fd, &server_addr, &socklen);

> +    chk_error(ret);

> +    server_port = ntohs(server_addr.sin_port);

>  

>      /* test a few socket options */

>      len = sizeof(val);

> @@ -305,7 +306,7 @@ static void test_socket(void)

>  

>      pid = chk_error(fork());

>      if (pid == 0) {

> -        client_fd = client_socket();

> +        client_fd = client_socket(server_port);

>          send(client_fd, socket_msg, sizeof(socket_msg), 0);

>          close(client_fd);

>          exit(0);

>
Alex Bennée June 17, 2018, 9:18 a.m. UTC | #3
Philippe Mathieu-Daudé <f4bug@amsat.org> writes:

> Hi Alex,

>

> On 06/15/2018 04:46 PM, 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 <alex.bennee@linaro.org>

>> ---

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

>

> Since /tmp doesn't always fit, can this be:

>

>        char *tmpbase = getenv("TMPDIR");

>        char *template = g_strdup_printf("%s/qemu-test-XXXXXX",

>                                         tmpbase ? tmpbase : "/tmp");


It depends if we want to honour TMPDIR, is /tmp not likely to be there?

Either way we can't use glib functions for these tests to keep the
compilation simple.

>

>> +    char *tmpdir = mkdtemp(template);

>

>        g_free(template);

>

>>

>> -    /* clean up, just in case */

>> -    unlink(TESTPATH "/file1");

>> -    unlink(TESTPATH "/file2");

>> -    unlink(TESTPATH "/file3");

>> -    rmdir(TESTPATH);

>> +    chk_error(strlen(tmpdir));

>>

>>      if (getcwd(cur_dir, sizeof(cur_dir)) == NULL)

>>          error("getcwd");

>>

>> -    chk_error(mkdir(TESTPATH, 0755));

>> -

>> -    chk_error(chdir(TESTPATH));

>> +    chk_error(chdir(tmpdir));

>>

>>      /* open/read/write/close/readv/writev/lseek */

>>

>> @@ -163,7 +157,7 @@ static void test_file(void)

>>          st.st_mtime != 1000)

>>          error("stat time");

>>

>> -    chk_error(stat(TESTPATH, &st));

>> +    chk_error(stat(tmpdir, &st));

>>      if (!S_ISDIR(st.st_mode))

>>          error("stat mode");

>>

>> @@ -185,7 +179,7 @@ static void test_file(void)

>>          error("stat mode");

>>

>>      /* getdents */

>> -    dir = opendir(TESTPATH);

>> +    dir = opendir(tmpdir);

>>      if (!dir)

>>          error("opendir");

>>      len = 0;

>> @@ -207,7 +201,7 @@ static void test_file(void)

>>      chk_error(unlink("file3"));

>>      chk_error(unlink("file2"));

>>      chk_error(chdir(cur_dir));

>> -    chk_error(rmdir(TESTPATH));

>> +    chk_error(rmdir(tmpdir));

>>  }

>>

>>  static void test_fork(void)

>> @@ -264,7 +258,7 @@ static int server_socket(void)

>>      chk_error(setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &val, sizeof(val)));

>>

>>      sockaddr.sin_family = AF_INET;

>> -    sockaddr.sin_port = htons(TESTPORT);

>> +    sockaddr.sin_port = htons(0); /* choose random ephemeral port) */

>>      sockaddr.sin_addr.s_addr = 0;

>>      chk_error(bind(fd, (struct sockaddr *)&sockaddr, sizeof(sockaddr)));

>>      chk_error(listen(fd, 0));

>> @@ -272,7 +266,7 @@ static int server_socket(void)

>>

>>  }

>>

>> -static int client_socket(void)

>> +static int client_socket(uint16_t port)

>>  {

>>      int fd;

>>      struct sockaddr_in sockaddr;

>> @@ -280,7 +274,7 @@ static int client_socket(void)

>>      /* server socket */

>>      fd = chk_error(socket(PF_INET, SOCK_STREAM, 0));

>>      sockaddr.sin_family = AF_INET;

>> -    sockaddr.sin_port = htons(TESTPORT);

>> +    sockaddr.sin_port = htons(port);

>>      inet_aton("127.0.0.1", &sockaddr.sin_addr);

>>      chk_error(connect(fd, (struct sockaddr *)&sockaddr, sizeof(sockaddr)));

>>      return fd;

>> @@ -292,10 +286,17 @@ static void test_socket(void)

>>  {

>>      int server_fd, client_fd, fd, pid, ret, val;

>>      struct sockaddr_in sockaddr;

>> -    socklen_t len;

>> +    struct sockaddr_in server_addr;

>> +    socklen_t len, socklen;

>> +    uint16_t server_port;

>>      char buf[512];

>>

>>      server_fd = server_socket();

>> +    /* find out what port we got */

>> +    socklen = sizeof(server_addr);

>> +    ret = getsockname(server_fd, &server_addr, &socklen);

>> +    chk_error(ret);

>> +    server_port = ntohs(server_addr.sin_port);

>>

>>      /* test a few socket options */

>>      len = sizeof(val);

>> @@ -305,7 +306,7 @@ static void test_socket(void)

>>

>>      pid = chk_error(fork());

>>      if (pid == 0) {

>> -        client_fd = client_socket();

>> +        client_fd = client_socket(server_port);

>>          send(client_fd, socket_msg, sizeof(socket_msg), 0);

>>          close(client_fd);

>>          exit(0);

>>



--
Alex Bennée
Alex Bennée June 18, 2018, 10:56 a.m. UTC | #4
Thomas Huth <thuth@redhat.com> 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 <alex.bennee@linaro.org>

>> ---

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

>

>  Thomas



--
Alex Bennée
Daniel P. Berrangé June 18, 2018, 11:08 a.m. UTC | #5
On Mon, Jun 18, 2018 at 11:56:08AM +0100, Alex Bennée wrote:
> 

> Thomas Huth <thuth@redhat.com> 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 <alex.bennee@linaro.org>

> >> ---

> >>  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 :|
Thomas Huth June 18, 2018, 11:08 a.m. UTC | #6
On 18.06.2018 12:56, Alex Bennée wrote:
> 

> Thomas Huth <thuth@redhat.com> 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 <alex.bennee@linaro.org>

>>> ---

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


chk_error() checks for negative values, so I guess you rather should
assert(tmpdir) here instead.

 Thomas
Thomas Huth June 18, 2018, 11:09 a.m. UTC | #7
On 18.06.2018 13:08, Daniel P. Berrangé wrote:
> On Mon, Jun 18, 2018 at 11:56:08AM +0100, Alex Bennée wrote:

>>

>> Thomas Huth <thuth@redhat.com> 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 <alex.bennee@linaro.org>

>>>> ---

>>>>  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) {


We're talking about a tcg test here, not a qtest.

 Thomas
Daniel P. Berrangé June 18, 2018, 11:14 a.m. UTC | #8
On Mon, Jun 18, 2018 at 01:09:54PM +0200, Thomas Huth wrote:
> On 18.06.2018 13:08, Daniel P. Berrangé wrote:

> > On Mon, Jun 18, 2018 at 11:56:08AM +0100, Alex Bennée wrote:

> >>

> >> Thomas Huth <thuth@redhat.com> 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 <alex.bennee@linaro.org>

> >>>> ---

> >>>>  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) {

> 

> We're talking about a tcg test here, not a qtest.


The code I illustrated has nothing tieing it to libqtest. It can be used
anywhere. libqtest simply stole the 'qtest_' naming convention.

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 :|
Alex Bennée June 18, 2018, 12:04 p.m. UTC | #9
Daniel P. Berrangé <berrange@redhat.com> writes:

> On Mon, Jun 18, 2018 at 11:56:08AM +0100, Alex Bennée wrote:

>>

>> Thomas Huth <thuth@redhat.com> 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 <alex.bennee@linaro.org>

>> >> ---

>> >>  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
Thomas Huth June 18, 2018, 12:40 p.m. UTC | #10
On 18.06.2018 14:04, Alex Bennée wrote:
> 

> Daniel P. Berrangé <berrange@redhat.com> writes:

> 

>> On Mon, Jun 18, 2018 at 11:56:08AM +0100, Alex Bennée wrote:

>>>

>>> Thomas Huth <thuth@redhat.com> 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 <alex.bennee@linaro.org>

>>>>> ---

>>>>>  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
Philippe Mathieu-Daudé June 18, 2018, 3:18 p.m. UTC | #11
On 06/17/2018 06:18 AM, Alex Bennée wrote:
> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:

> 

>> Hi Alex,

>>

>> On 06/15/2018 04:46 PM, 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 <alex.bennee@linaro.org>

>>> ---

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

>>

>> Since /tmp doesn't always fit, can this be:

>>

>>        char *tmpbase = getenv("TMPDIR");

>>        char *template = g_strdup_printf("%s/qemu-test-XXXXXX",

>>                                         tmpbase ? tmpbase : "/tmp");

> 

> It depends if we want to honour TMPDIR, is /tmp not likely to be there?


My /tmp is a not huge tmpfs and I had troubles running iotests which let
some dangling big files on failure. Now I prefer run tests with
TMPDIR=/scratch where I have plenty of slower space.
Shouldn't be a problem here however.

> 

> Either way we can't use glib functions for these tests to keep the

> compilation simple.


char template[PATH_MAX] + snprintf() :)

> 

>>

>>> +    char *tmpdir = mkdtemp(template);

>>

>>        g_free(template);

>>

>>>

>>> -    /* clean up, just in case */

>>> -    unlink(TESTPATH "/file1");

>>> -    unlink(TESTPATH "/file2");

>>> -    unlink(TESTPATH "/file3");

>>> -    rmdir(TESTPATH);

>>> +    chk_error(strlen(tmpdir));

>>>

>>>      if (getcwd(cur_dir, sizeof(cur_dir)) == NULL)

>>>          error("getcwd");

>>>

>>> -    chk_error(mkdir(TESTPATH, 0755));

>>> -

>>> -    chk_error(chdir(TESTPATH));

>>> +    chk_error(chdir(tmpdir));

>>>

>>>      /* open/read/write/close/readv/writev/lseek */

>>>

>>> @@ -163,7 +157,7 @@ static void test_file(void)

>>>          st.st_mtime != 1000)

>>>          error("stat time");

>>>

>>> -    chk_error(stat(TESTPATH, &st));

>>> +    chk_error(stat(tmpdir, &st));

>>>      if (!S_ISDIR(st.st_mode))

>>>          error("stat mode");

>>>

>>> @@ -185,7 +179,7 @@ static void test_file(void)

>>>          error("stat mode");

>>>

>>>      /* getdents */

>>> -    dir = opendir(TESTPATH);

>>> +    dir = opendir(tmpdir);

>>>      if (!dir)

>>>          error("opendir");

>>>      len = 0;

>>> @@ -207,7 +201,7 @@ static void test_file(void)

>>>      chk_error(unlink("file3"));

>>>      chk_error(unlink("file2"));

>>>      chk_error(chdir(cur_dir));

>>> -    chk_error(rmdir(TESTPATH));

>>> +    chk_error(rmdir(tmpdir));

>>>  }

>>>

>>>  static void test_fork(void)

>>> @@ -264,7 +258,7 @@ static int server_socket(void)

>>>      chk_error(setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &val, sizeof(val)));

>>>

>>>      sockaddr.sin_family = AF_INET;

>>> -    sockaddr.sin_port = htons(TESTPORT);

>>> +    sockaddr.sin_port = htons(0); /* choose random ephemeral port) */

>>>      sockaddr.sin_addr.s_addr = 0;

>>>      chk_error(bind(fd, (struct sockaddr *)&sockaddr, sizeof(sockaddr)));

>>>      chk_error(listen(fd, 0));

>>> @@ -272,7 +266,7 @@ static int server_socket(void)

>>>

>>>  }

>>>

>>> -static int client_socket(void)

>>> +static int client_socket(uint16_t port)

>>>  {

>>>      int fd;

>>>      struct sockaddr_in sockaddr;

>>> @@ -280,7 +274,7 @@ static int client_socket(void)

>>>      /* server socket */

>>>      fd = chk_error(socket(PF_INET, SOCK_STREAM, 0));

>>>      sockaddr.sin_family = AF_INET;

>>> -    sockaddr.sin_port = htons(TESTPORT);

>>> +    sockaddr.sin_port = htons(port);

>>>      inet_aton("127.0.0.1", &sockaddr.sin_addr);

>>>      chk_error(connect(fd, (struct sockaddr *)&sockaddr, sizeof(sockaddr)));

>>>      return fd;

>>> @@ -292,10 +286,17 @@ static void test_socket(void)

>>>  {

>>>      int server_fd, client_fd, fd, pid, ret, val;

>>>      struct sockaddr_in sockaddr;

>>> -    socklen_t len;

>>> +    struct sockaddr_in server_addr;

>>> +    socklen_t len, socklen;

>>> +    uint16_t server_port;

>>>      char buf[512];

>>>

>>>      server_fd = server_socket();

>>> +    /* find out what port we got */

>>> +    socklen = sizeof(server_addr);

>>> +    ret = getsockname(server_fd, &server_addr, &socklen);

>>> +    chk_error(ret);

>>> +    server_port = ntohs(server_addr.sin_port);

>>>

>>>      /* test a few socket options */

>>>      len = sizeof(val);

>>> @@ -305,7 +306,7 @@ static void test_socket(void)

>>>

>>>      pid = chk_error(fork());

>>>      if (pid == 0) {

>>> -        client_fd = client_socket();

>>> +        client_fd = client_socket(server_port);

>>>          send(client_fd, socket_msg, sizeof(socket_msg), 0);

>>>          close(client_fd);

>>>          exit(0);

>>>

> 

> 

> --

> Alex Bennée

>
Philippe Mathieu-Daudé June 18, 2018, 3:23 p.m. UTC | #12
On 06/18/2018 12:18 PM, Philippe Mathieu-Daudé wrote:
> On 06/17/2018 06:18 AM, Alex Bennée wrote:

>> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:

>>

>>> Hi Alex,

>>>

>>> On 06/15/2018 04:46 PM, 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 <alex.bennee@linaro.org>

>>>> ---

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

>>>

>>> Since /tmp doesn't always fit, can this be:

>>>

>>>        char *tmpbase = getenv("TMPDIR");

>>>        char *template = g_strdup_printf("%s/qemu-test-XXXXXX",

>>>                                         tmpbase ? tmpbase : "/tmp");

>>

>> It depends if we want to honour TMPDIR, is /tmp not likely to be there?

> 

> My /tmp is a not huge tmpfs and I had troubles running iotests which let

> some dangling big files on failure. Now I prefer run tests with

> TMPDIR=/scratch where I have plenty of slower space.

> Shouldn't be a problem here however.


Thus is something that could be addressed in later patch, or not.

> 

>>

>> Either way we can't use glib functions for these tests to keep the

>> compilation simple.

> 

> char template[PATH_MAX] + snprintf() :)

> 

>>

>>>

>>>> +    char *tmpdir = mkdtemp(template);

>>>

>>>        g_free(template);

>>>

>>>>

>>>> -    /* clean up, just in case */

>>>> -    unlink(TESTPATH "/file1");

>>>> -    unlink(TESTPATH "/file2");

>>>> -    unlink(TESTPATH "/file3");

>>>> -    rmdir(TESTPATH);

>>>> +    chk_error(strlen(tmpdir));

>>>>

>>>>      if (getcwd(cur_dir, sizeof(cur_dir)) == NULL)

>>>>          error("getcwd");

>>>>

>>>> -    chk_error(mkdir(TESTPATH, 0755));

>>>> -

>>>> -    chk_error(chdir(TESTPATH));

>>>> +    chk_error(chdir(tmpdir));

>>>>

>>>>      /* open/read/write/close/readv/writev/lseek */

>>>>

>>>> @@ -163,7 +157,7 @@ static void test_file(void)

>>>>          st.st_mtime != 1000)

>>>>          error("stat time");

>>>>

>>>> -    chk_error(stat(TESTPATH, &st));

>>>> +    chk_error(stat(tmpdir, &st));

>>>>      if (!S_ISDIR(st.st_mode))

>>>>          error("stat mode");

>>>>

>>>> @@ -185,7 +179,7 @@ static void test_file(void)

>>>>          error("stat mode");

>>>>

>>>>      /* getdents */

>>>> -    dir = opendir(TESTPATH);

>>>> +    dir = opendir(tmpdir);

>>>>      if (!dir)

>>>>          error("opendir");

>>>>      len = 0;

>>>> @@ -207,7 +201,7 @@ static void test_file(void)

>>>>      chk_error(unlink("file3"));

>>>>      chk_error(unlink("file2"));

>>>>      chk_error(chdir(cur_dir));

>>>> -    chk_error(rmdir(TESTPATH));

>>>> +    chk_error(rmdir(tmpdir));

>>>>  }

>>>>

>>>>  static void test_fork(void)

>>>> @@ -264,7 +258,7 @@ static int server_socket(void)

>>>>      chk_error(setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &val, sizeof(val)));

>>>>

>>>>      sockaddr.sin_family = AF_INET;

>>>> -    sockaddr.sin_port = htons(TESTPORT);

>>>> +    sockaddr.sin_port = htons(0); /* choose random ephemeral port) */

>>>>      sockaddr.sin_addr.s_addr = 0;

>>>>      chk_error(bind(fd, (struct sockaddr *)&sockaddr, sizeof(sockaddr)));

>>>>      chk_error(listen(fd, 0));

>>>> @@ -272,7 +266,7 @@ static int server_socket(void)

>>>>

>>>>  }

>>>>

>>>> -static int client_socket(void)

>>>> +static int client_socket(uint16_t port)

>>>>  {

>>>>      int fd;

>>>>      struct sockaddr_in sockaddr;

>>>> @@ -280,7 +274,7 @@ static int client_socket(void)

>>>>      /* server socket */

>>>>      fd = chk_error(socket(PF_INET, SOCK_STREAM, 0));

>>>>      sockaddr.sin_family = AF_INET;

>>>> -    sockaddr.sin_port = htons(TESTPORT);

>>>> +    sockaddr.sin_port = htons(port);

>>>>      inet_aton("127.0.0.1", &sockaddr.sin_addr);

>>>>      chk_error(connect(fd, (struct sockaddr *)&sockaddr, sizeof(sockaddr)));

>>>>      return fd;

>>>> @@ -292,10 +286,17 @@ static void test_socket(void)

>>>>  {

>>>>      int server_fd, client_fd, fd, pid, ret, val;

>>>>      struct sockaddr_in sockaddr;

>>>> -    socklen_t len;

>>>> +    struct sockaddr_in server_addr;

>>>> +    socklen_t len, socklen;

>>>> +    uint16_t server_port;

>>>>      char buf[512];

>>>>

>>>>      server_fd = server_socket();

>>>> +    /* find out what port we got */

>>>> +    socklen = sizeof(server_addr);

>>>> +    ret = getsockname(server_fd, &server_addr, &socklen);

>>>> +    chk_error(ret);

>>>> +    server_port = ntohs(server_addr.sin_port);

>>>>

>>>>      /* test a few socket options */

>>>>      len = sizeof(val);

>>>> @@ -305,7 +306,7 @@ static void test_socket(void)

>>>>

>>>>      pid = chk_error(fork());

>>>>      if (pid == 0) {

>>>> -        client_fd = client_socket();

>>>> +        client_fd = client_socket(server_port);

>>>>          send(client_fd, socket_msg, sizeof(socket_msg), 0);

>>>>          close(client_fd);

>>>>          exit(0);

>>>>

>>

>>

>> --

>> Alex Bennée

>>
diff mbox series

Patch

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));
 
     if (getcwd(cur_dir, sizeof(cur_dir)) == NULL)
         error("getcwd");
 
-    chk_error(mkdir(TESTPATH, 0755));
-
-    chk_error(chdir(TESTPATH));
+    chk_error(chdir(tmpdir));
 
     /* open/read/write/close/readv/writev/lseek */
 
@@ -163,7 +157,7 @@  static void test_file(void)
         st.st_mtime != 1000)
         error("stat time");
 
-    chk_error(stat(TESTPATH, &st));
+    chk_error(stat(tmpdir, &st));
     if (!S_ISDIR(st.st_mode))
         error("stat mode");
 
@@ -185,7 +179,7 @@  static void test_file(void)
         error("stat mode");
 
     /* getdents */
-    dir = opendir(TESTPATH);
+    dir = opendir(tmpdir);
     if (!dir)
         error("opendir");
     len = 0;
@@ -207,7 +201,7 @@  static void test_file(void)
     chk_error(unlink("file3"));
     chk_error(unlink("file2"));
     chk_error(chdir(cur_dir));
-    chk_error(rmdir(TESTPATH));
+    chk_error(rmdir(tmpdir));
 }
 
 static void test_fork(void)
@@ -264,7 +258,7 @@  static int server_socket(void)
     chk_error(setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &val, sizeof(val)));
 
     sockaddr.sin_family = AF_INET;
-    sockaddr.sin_port = htons(TESTPORT);
+    sockaddr.sin_port = htons(0); /* choose random ephemeral port) */
     sockaddr.sin_addr.s_addr = 0;
     chk_error(bind(fd, (struct sockaddr *)&sockaddr, sizeof(sockaddr)));
     chk_error(listen(fd, 0));
@@ -272,7 +266,7 @@  static int server_socket(void)
 
 }
 
-static int client_socket(void)
+static int client_socket(uint16_t port)
 {
     int fd;
     struct sockaddr_in sockaddr;
@@ -280,7 +274,7 @@  static int client_socket(void)
     /* server socket */
     fd = chk_error(socket(PF_INET, SOCK_STREAM, 0));
     sockaddr.sin_family = AF_INET;
-    sockaddr.sin_port = htons(TESTPORT);
+    sockaddr.sin_port = htons(port);
     inet_aton("127.0.0.1", &sockaddr.sin_addr);
     chk_error(connect(fd, (struct sockaddr *)&sockaddr, sizeof(sockaddr)));
     return fd;
@@ -292,10 +286,17 @@  static void test_socket(void)
 {
     int server_fd, client_fd, fd, pid, ret, val;
     struct sockaddr_in sockaddr;
-    socklen_t len;
+    struct sockaddr_in server_addr;
+    socklen_t len, socklen;
+    uint16_t server_port;
     char buf[512];
 
     server_fd = server_socket();
+    /* find out what port we got */
+    socklen = sizeof(server_addr);
+    ret = getsockname(server_fd, &server_addr, &socklen);
+    chk_error(ret);
+    server_port = ntohs(server_addr.sin_port);
 
     /* test a few socket options */
     len = sizeof(val);
@@ -305,7 +306,7 @@  static void test_socket(void)
 
     pid = chk_error(fork());
     if (pid == 0) {
-        client_fd = client_socket();
+        client_fd = client_socket(server_port);
         send(client_fd, socket_msg, sizeof(socket_msg), 0);
         close(client_fd);
         exit(0);