Re: [libvirt] [PATCH] tests: Set PATH in each test

On Thu, Mar 17, 2016 at 02:31:49PM +0100, Michal Privoznik wrote: > Currently we spawn couple of binaries in our test suite. > Moreover, we provide some spoofed versions of system binaries > hoping that those will be executed instead of the system ones. > For instance, for testing SSH socket we have written our own ssh > binary for producing predictable results. We certainly don't want > to execute the system ssh binary. > However, in order to prefer our binaries over system ones, we > need to set PATH environment variable. But this is done only at > the Makefile level. So if anybody runs a test by hand that > expects our spoofed binary, the test ends up executing real > system binaries. This is not good. In fact, it's terribly wrong. What a tragedy! > The fix lies in a small trick - putting our build directory at > the beginning of the PATH environment variable in each test. > Hopefully, since every test has this VIRT_TEST_MAIN* wrapper, we > can fix this at a single place. > Moreover, while this removes setting PATH for our tests written > in bash, it's safe as we are not calling anything ours that would > require PATH change there. > > Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; > --- > tests/Makefile.am | 3 --- > tests/testutils.c | 21 ++++++++++++++++++++- > 2 files changed, 20 insertions(+), 4 deletions(-) ACK > diff --git a/tests/Makefile.am b/tests/Makefile.am > index 90981dc..74f7f5a 100644 > --- a/tests/Makefile.am > +++ b/tests/Makefile.am > @@ -462,8 +462,6 @@ TESTS = $(test_programs) \ > # intermediate shell variable, but must do all the expansion in make > > lv_abs_top_builddir=$(shell cd '$(top_builddir)' && pwd) > -path_add = $(subst :,$(PATH_SEPARATOR),\ > - $(subst !,$(lv_abs_top_builddir)/,!daemon:!tools:!tests)) > > VIR_TEST_EXPENSIVE ?= $(VIR_TEST_EXPENSIVE_DEFAULT) > TESTS_ENVIRONMENT = \ > @@ -472,7 +470,6 @@ TESTS_ENVIRONMENT = \ > abs_builddir=$(abs_builddir) \ > abs_srcdir=$(abs_srcdir) \ > CONFIG_HEADER="$(lv_abs_top_builddir)/config.h" \ > - PATH="$(path_add)$(PATH_SEPARATOR)$$PATH" \ > SHELL="$(SHELL)" \ > LIBVIRT_DRIVER_DIR="$(lv_abs_top_builddir)/src/.libs" \ > LIBVIRT_AUTOSTART=0 \ > diff --git a/tests/testutils.c b/tests/testutils.c > index 88c4d4b..9211b94 100644 > --- a/tests/testutils.c > +++ b/tests/testutils.c > @@ -806,6 +806,24 @@ virTestGetRegenerate(void) > return testRegenerate; > } > > +static int > +virTestSetEnvPath(void) > +{ > + int ret = -1; > + const char *path = getenv("PATH"); > + char *new_path = NULL; > + > + if (strstr(path, abs_builddir) != path && Is the strstr call really necessary? I would rather prepend it unconditionally.

> + (virAsprintf(&new_path, "%s:%s", abs_builddir, path) < 0 || > + setenv("PATH", new_path, 1) < 0)) > + goto cleanup; > + > + ret = 0; > + cleanup: > + VIR_FREE(new_path); > + return ret; > +} > + > int virtTestMain(int argc, > char **argv, > int (*func)(void)) > @@ -818,7 +836,8 @@ int virtTestMain(int argc, > > virFileActivateDirOverride(argv[0]); > > - if (!virFileExists(abs_srcdir)) > + if (virTestSetEnvPath() < 0 || These are not related, both deserve their separate ifs.