On 11/29/18 10:04 PM, Frederic Lecaille wrote:
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.

A remaining patch for varnishtest.

Fred.
>From bdcdde3744f26059dd2151f2c30ed2151500098e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20L=C3=A9caille?= <flecai...@haproxy.com>
Date: Fri, 30 Nov 2018 07:41:13 +0100
Subject: [PATCH 2/2] Support spaces in remaining filenames.

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

diff --git a/bin/varnishtest/vtc_haproxy.c b/bin/varnishtest/vtc_haproxy.c
index e361aad78..ec3006505 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);
@@ -498,7 +498,7 @@ haproxy_delete(struct haproxy *h)
 	vtc_logclose(h->vl);
 
 	if (!leave_temp) {
-		bprintf(buf, "rm -rf %s", h->workdir);
+		bprintf(buf, "rm -rf \"%s\"", h->workdir);
 		AZ(system(buf));
 	}
 
@@ -548,7 +548,7 @@ haproxy_start(struct haproxy *h)
 	vsb = VSB_new_auto();
 	AN(vsb);
 
-	VSB_printf(vsb, "exec %s", h->filename);
+	VSB_printf(vsb, "exec \"%s\"", h->filename);
 	if (h->opt_check_mode)
 		VSB_printf(vsb, " -c");
 	else if (h->opt_daemon)
@@ -567,7 +567,7 @@ haproxy_start(struct haproxy *h)
 		bprintf(buf, "%s/pid", h->workdir);
 		h->pid_fn = strdup(buf);
 		AN(h->pid_fn);
-		VSB_printf(vsb, " -p %s", h->pid_fn);
+		VSB_printf(vsb, " -p \"%s\"", h->pid_fn);
 	}
 
 	AZ(VSB_finish(vsb));
-- 
2.11.0

Reply via email to