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?= <[email protected]>
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
