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