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

[Ján Tomko]

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 at redhat.com>
> ---
>  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.

Jan

> +        !virFileExists(abs_srcdir))
>          return EXIT_AM_HARDFAIL;
>  
>      progname = last_component(argv[0]);