On 11/29/18 5:36 AM, Willy Tarreau wrote:
Hi guys,

On Wed, Nov 28, 2018 at 11:17:22AM +0100, Frederic Lecaille wrote:
Perhaps we should "chmod +x" this script.

Good point, done here.

However I'm now seeing this when starting it :

########################## Starting varnishtest ##########################
Testing with haproxy version: 1.8-dev0-3b0a6d-66
Assert error in start_test(), vtc_main.c line 283:
   Condition((mkdir(tmpdir, 0711)) == 0) not true.
   errno = 13 (Permission denied)
./scripts/run-regtests.sh: line 299: 21484 Aborted                 
$VARNISHTEST_PROGRAM $varnishtestparams $verbose $jobcount -l -k -t 10 $testlist
########################## Gathering failed results ##########################
find: `/proc/tty/driver': Permission denied
find: `/proc/1/task/1/fd': Permission denied
find: `/proc/1/task/1/fdinfo': Permission denied
find: `/proc/1/task/1/ns': Permission denied
find: `/proc/1/fd': Permission denied
find: `/proc/1/map_files': Permission denied
(...)

Then I stopped it using Ctrl-C.

I'm seeing a few minor issues we must still address :

   - haproxy is started from the path, which means that on all systems
     where it's installed, it will be the one provided by the system and
     not the just built one which will be tested (as happened above),
     which is confusing. I think we shouldn't search for haproxy in the
     path but use $PWD/haproxy or ./haproxy or something like this.

   - it seems there are some issues in the way TMPDIR/TESTDIR are created,
     as it ended up with an empty TESTDIR. In my case, TMPDIR is not set,
     so TESTDIR was set to /tmp/varnishtest_haproxy. mkdir worked (I now
     have this directory), but a test is missing on this mkdir.

   - the way the sub-directory is created is problematic on shared
     development machines, as only the first user will own the directory
     and will thus prevent other users from creating their own overthere,
     thus it's probably preferable not to create an intermediary directory
     in the end.

   - in my case it's mktemp which failed :

     ++ date +%Y-%m-%d_%H-%M-%S
     + TESTRUNDATETIME=2018-11-29_05-03-43
     + TESTDIR=/tmp/varnishtest_haproxy
     + mkdir -p /tmp/varnishtest_haproxy
     ++ mktemp -d /tmp/varnishtest_haproxy/2018-11-29_05-03-43.XXXX
     mktemp: cannot make temp dir 
/tmp/varnishtest_haproxy/2018-11-29_05-03-43.XXXX: Invalid argument
     + TESTDIR=

     I haven't checked why yet, but we definitely need to test the status
     code for success as well.

   - in my opinion the script is still missing a number of quotes when using
     string variables, especially in the directory names. If someone is crazy
     enough to have TMPDIR="/tmp/Temp Dir", then he'll have some fun.

   - I'm seeing a linux-specific "rm -d" at the end, it's be better to
     replace it with rmdir.

   - there's "/usr/bin/env sh" at the top of the shell script. /bin/sh is
     the portable one, I've spent lots of time in the past editing scripts
     where env was forced to /usr/bin while it was placed in /bin on some
     systems, so I'm pretty certain that this one is not portable at all :-)

Here is a patch for haproxy (named 0001-REGTEST*) to fix these issues.

Note the other patch for varnishtest which is also required (sent to PHK).

Fred.
>From 7d316b74e65be4d75802feefe3d17b37afddf04e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20L=C3=A9caille?= <flecai...@haproxy.com>
Date: Thu, 29 Nov 2018 21:51:55 +0100
Subject: [PATCH] Add the support for spaces in filenames for the temporaty
 working directory.

---
 bin/varnishtest/vtc_haproxy.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/bin/varnishtest/vtc_haproxy.c b/bin/varnishtest/vtc_haproxy.c
index 9d12ed671..e361aad78 100644
--- a/bin/varnishtest/vtc_haproxy.c
+++ b/bin/varnishtest/vtc_haproxy.c
@@ -477,7 +477,7 @@ haproxy_new(const char *name)
 	h->cli = haproxy_cli_new(h);
 	AN(h->cli);
 
-	bprintf(buf, "rm -rf %s ; mkdir -p %s", h->workdir, h->workdir);
+	bprintf(buf, "rm -rf %s ; mkdir -p \"%s\"", h->workdir, h->workdir);
 	AZ(system(buf));
 
 	VTAILQ_INSERT_TAIL(&haproxies, h, list);
@@ -561,7 +561,7 @@ haproxy_start(struct haproxy *h)
 
 	VSB_printf(vsb, " %s", VSB_data(h->args));
 
-	VSB_printf(vsb, " -f %s ", h->cfg_fn);
+	VSB_printf(vsb, " -f \"%s\" ", h->cfg_fn);
 
 	if (h->opt_worker || h->opt_daemon) {
 		bprintf(buf, "%s/pid", h->workdir);
@@ -754,7 +754,7 @@ haproxy_write_conf(const struct haproxy *h, const char *cfg, int auto_be)
 	vsb2 = VSB_new_auto();
 	AN(vsb2);
 
-	VSB_printf(vsb, "    global\n\tstats socket %s "
+	VSB_printf(vsb, "    global\n\tstats socket \"%s\" "
 		   "level admin mode 600\n", h->cli_fn);
 	VSB_printf(vsb, "    stats socket \"fd@${cli}\" level admin\n");
 	AZ(VSB_cat(vsb, cfg));
-- 
2.11.0

>From bd6bc4956f3bd005af7c27cf83e78c8cd14aea02 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20L=C3=A9caille?= <flecai...@haproxy.com>
Date: Thu, 29 Nov 2018 21:41:42 +0100
Subject: [PATCH] REGTEST: Fix several issues.

Use #!bin/sh more portable shebang.
Support filenames with spaces.
Set HAPROXY_PROGRAM environment variable value to ${PWD}/haproxy.
exit(1) if we could not creat the higher level temporary working directory
or create its sub-directory with mktemp utility.
As defined by POSIX, use six characters for the mktemp template.
---
 scripts/run-regtests.sh | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/scripts/run-regtests.sh b/scripts/run-regtests.sh
index 5add8541..d05d8deb 100755
--- a/scripts/run-regtests.sh
+++ b/scripts/run-regtests.sh
@@ -1,4 +1,4 @@
-#!/usr/bin/env sh
+#!/bin/sh
 
 if [ "$1" = "--help" ]; then
   cat << EOF
@@ -180,8 +180,8 @@ _version() {
 echo ""
 echo "########################## Preparing to run tests ##########################"
 
-HAPROXY_PROGRAM=${HAPROXY_PROGRAM:-haproxy}
-VARNISHTEST_PROGRAM=${VARNISHTEST_PROGRAM:-varnishtest}
+HAPROXY_PROGRAM="${HAPROXY_PROGRAM:-${PWD}/haproxy}"
+VARNISHTEST_PROGRAM="${VARNISHTEST_PROGRAM:-varnishtest}"
 
 preparefailed=
 if ! [ -x "$(command -v $HAPROXY_PROGRAM)" ]; then
@@ -205,12 +205,12 @@ echo "Testing with haproxy version: $HAPROXY_VERSION"
 
 TESTRUNDATETIME="$(date '+%Y-%m-%d_%H-%M-%S')"
 
-TESTDIR=${TMPDIR:-/tmp}/varnishtest_haproxy
-mkdir -p "$TESTDIR"
-TESTDIR=$(mktemp -d $TESTDIR/$TESTRUNDATETIME.XXXX)
+TESTDIR="${TMPDIR:-/tmp}"
+mkdir -p "$TESTDIR" || exit 1
+TESTDIR=$(mktemp -d "$TESTDIR/$TESTRUNDATETIME.XXXXXX") || exit 1
 
-export TMPDIR=$TESTDIR
-export HAPROXY_PROGRAM=$HAPROXY_PROGRAM
+export TMPDIR="$TESTDIR"
+export HAPROXY_PROGRAM="$HAPROXY_PROGRAM"
 
 # Mimic implicit build options from haproxy MakeFile that are present for each target:
 
@@ -300,18 +300,18 @@ fi
 if [ $_vtresult != 0 ]
 then
   echo "########################## Gathering failed results ##########################"
-  for i in $(find $TESTDIR/ -type d -name "vtc.*");
-  do
-    cat <<- EOF | tee $TESTDIR/failedtests.log
-$(echo "###### $(cat $i/INFO) ######")
-$(echo "## test results in: $i")
-$(grep -- ---- $i/LOG)
-
+  export TESTDIR
+  find "$TESTDIR" -type d -name "vtc.*" -exec sh -c 'for i; do
+    if [ ! -e "$i/LOG" ] ; then continue; fi
+    cat <<- EOF | tee -a "$TESTDIR/failedtests.log"
+$(echo "###### $(cat "$i/INFO") ######")
+$(echo "## test results in: \"$i\"")
+$(grep -- ---- "$i/LOG")
 EOF
-  done
+  done' sh {} +
   exit 1
 else
   # all tests were succesfull, removing tempdir (the last part.)
-  rm -d $TESTDIR
+  rmdir "$TESTDIR"
 fi
 exit 0
-- 
2.11.0

Reply via email to