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

[Michal Privoznik]

On 18.03.2016 10:42, Ján Tomko wrote:
> 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.

Well, it's not necessary now, but it will be later. Thing is, if a
program would re-exec itself, the path would be there twice.

> 
>> +        (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.
> 

Okay.

Pushed. Thanks.

Michal