[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v2 2/3] virtestmock: Print invalid file accesses into a file



On 11.05.2016 17:15, Peter Krempa wrote:
> On Tue, May 10, 2016 at 17:24:13 +0200, Michal Privoznik wrote:
>> All the accesses to files outside our build or source directories
>> are now identified and appended into a file for later processing.
>> The location of the file that contains all the records can be
>> controlled via VIR_TEST_FILE_ACCESS env variable and defaults to
>> abs_builddir "/test_file_access.txt".
>>
>> The script that will process the access file is to be added in
>> next commit.
>>
>> Signed-off-by: Michal Privoznik <mprivozn redhat com>
>> ---
>>  .gitignore           |   1 +
>>  HACKING              |  11 ++++++
>>  cfg.mk               |   6 +--
>>  docs/hacking.html.in |  15 ++++++++
>>  tests/testutils.c    |  27 +++++++++----
>>  tests/virtestmock.c  | 105 +++++++++++++++++++++++++++++++++++++++++++++++++--
>>  6 files changed, 151 insertions(+), 14 deletions(-)
>>
> 
> [...]
> 
>> diff --git a/HACKING b/HACKING
>> index e308568..63ad327 100644
>> --- a/HACKING
>> +++ b/HACKING
>> @@ -152,6 +152,17 @@ There is also a "./run" script at the top level, to make it easier to run
>>  programs that have not yet been installed, as well as to wrap invocations of
>>  various tests under gdb or Valgrind.
>>  
>> +When running our test suite it may happen that nondeterministic result is
>> +produced because of the test suite relying on a particular file in the system
> 
> it may happen that the test result is nondeterministic
> 
>> +being accessible or having some specific value. This is a problem because if
>> +ran under different environment the test result may be different and a test
> 
> This whole sentence seems redundant with the first one.
> 
>> +can fail. To catch this kind of errors, the test suite has a module for that.
> 
> has a module that prints any path touched
> 
>> +The module prints any path touched that fulfils constraints described above
>> +into a file. Then "VIR_TEST_FILE_ACCESS" environment variable can alter
>> +location where the file is stored.
>> +
>> +  VIR_TEST_FILE_ACCESS="/tmp/file_access.txt" ./qemuxml2argvtest
>> +
>>  
>>  
>>  (9) The Valgrind test should produce similar output to "make check". If the output
>> diff --git a/cfg.mk b/cfg.mk
>> index c0aba57..1bf63ac 100644
>> --- a/cfg.mk
>> +++ b/cfg.mk
>> @@ -1181,10 +1181,10 @@ exclude_file_name_regexp--sc_prohibit_access_xok = \
>>  	^(cfg\.mk|src/util/virutil\.c)$$
>>  
>>  exclude_file_name_regexp--sc_prohibit_asprintf = \
>> -  ^(cfg\.mk|bootstrap.conf$$|src/util/virstring\.[ch]$$|tests/vircgroupmock\.c$$)
>> +  ^(cfg\.mk|bootstrap.conf$$|src/util/virstring\.[ch]$$|tests/vir(cgroup|test)mock\.c$$)
>>  
>>  exclude_file_name_regexp--sc_prohibit_strdup = \
>> -  ^(docs/|examples/|src/util/virstring\.c|tests/vir(netserverclient|cgroup)mock.c$$)
>> +  ^(docs/|examples/|src/util/virstring\.c|tests/vir(netserverclient|cgroup|test)mock.c$$)
>>  
>>  exclude_file_name_regexp--sc_prohibit_close = \
>>    (\.p[yl]$$|\.spec\.in$$|^docs/|^(src/util/virfile\.c|src/libvirt-stream\.c|tests/vir.+mock\.c)$$)
>> @@ -1211,7 +1211,7 @@ exclude_file_name_regexp--sc_prohibit_select = \
>>  	^cfg\.mk$$
>>  
>>  exclude_file_name_regexp--sc_prohibit_raw_allocation = \
>> -  ^(docs/hacking\.html\.in|src/util/viralloc\.[ch]|examples/.*|tests/(securityselinuxhelper|vircgroupmock)\.c|tools/wireshark/src/packet-libvirt\.c)$$
>> +  ^(docs/hacking\.html\.in|src/util/viralloc\.[ch]|examples/.*|tests/(securityselinuxhelper|vir(cgroup|test)mock)\.c|tools/wireshark/src/packet-libvirt\.c)$$
>>  
>>  exclude_file_name_regexp--sc_prohibit_readlink = \
>>    ^src/(util/virutil|lxc/lxc_container)\.c$$
> 
> If you statically link with our utils you could avoid any of those
> above. Is there a special reason to avoid our wrappers?
> 
>> diff --git a/docs/hacking.html.in b/docs/hacking.html.in
>> index 5cd23a2..63d26bd 100644
>> --- a/docs/hacking.html.in
>> +++ b/docs/hacking.html.in
>> @@ -189,6 +189,21 @@
>>            under gdb or Valgrind.
>>          </p>
>>  
>> +        <p>When running our test suite it may happen that
>> +        nondeterministic result is produced because of the test
>> +        suite relying on a particular file in the system being
>> +        accessible or having some specific value. This is a
>> +        problem because if ran under different environment the
>> +        test result may be different and a test can fail. To
>> +        catch this kind of errors, the test suite has a module
>> +        for that. The module prints any path touched that fulfils
>> +        constraints described above into a file. Then
>> +        <code>VIR_TEST_FILE_ACCESS</code> environment variable
>> +        can alter location where the file is stored.</p>
>> +<pre>
>> +  VIR_TEST_FILE_ACCESS="/tmp/file_access.txt" ./qemuxml2argvtest
>> +</pre>
> 
> See comments above.
> 
>> +
>>        </li>
>>        <li><p>The Valgrind test should produce similar output to
>>            <code>make check</code>. If the output has traces within libvirt
>> diff --git a/tests/testutils.c b/tests/testutils.c
>> index 595b64d..725398c 100644
>> --- a/tests/testutils.c
>> +++ b/tests/testutils.c
>> @@ -156,6 +156,13 @@ virtTestRun(const char *title,
>>  {
>>      int ret = 0;
>>  
>> +    /* Some test are fragile about environ settings.  If that's
>> +     * the case, don't poison it. All this means is that we will
> 
> You mean that the test cases actually purge the environment?

Yes. I don't recall the exact test off the top of my head, but there was
an issue that we dry ran a command and compared what it would be ran
like. Including all environ variables.

> 
>> +     * not see which test in particular is touching which file,
>> +     * but we are still able to tell which binary is doing so. */
> 
> I don't think this is accurate, since if you don't have _PROGNAME set,
> the logging code won't print anything.

Okay, I can remove it.

> 
>> +    if (getenv("VIR_TEST_MOCK_PROGNAME"))
>> +        setenv("VIR_TEST_MOCK_TESTNAME", title, 1);
> 
> [...]
> 
>> diff --git a/tests/virtestmock.c b/tests/virtestmock.c
>> index f138e98..c47eb0c 100644
>> --- a/tests/virtestmock.c
>> +++ b/tests/virtestmock.c
> 
> [...]
> 
>> @@ -64,17 +68,112 @@ static void init_syms(void)
>>      LOAD_SYM(access);
>>      LOAD_SYM_ALT(stat, __xstat);
>>      LOAD_SYM_ALT(lstat, __lxstat);
>> +
>> +}
>> +
>> +static void
>> +printFile(const char *output,
>> +          const char *file)
>> +{
>> +    FILE *fp;
>> +    const char *testname = getenv("VIR_TEST_MOCK_TESTNAME");
>> +
>> +    if (!progname) {
>> +        progname = getenv("VIR_TEST_MOCK_PROGNAME");
>> +
>> +        if (!progname)
>> +            return;
>> +    }
>> +
>> +    if (!(fp = realfopen(output, "a"))) {
>> +        fprintf(stderr, "Unable to open %s: %s\n", output, strerror(errno));
>> +        abort();
> 
> I'm a bit worried that if this wrapper fails the testsuite will abort.
> Couldn't we just skip creating/opening the file at this point? You can
> always error out in the phase where the file is checked.

But how would I know then that this has failed? Note that this should
also work in case a single test is ran. Why is aborting each test that
failed to open the file a bad thing anyway?

> 
>> +    }
>> +
>> +    if (flock(fileno(fp), LOCK_EX) < 0) {
>> +        fprintf(stderr, "Unable to lock %s: %s\n", output, strerror(errno));
>> +        fclose(fp);
>> +        abort();
> 
> Won't there be a possibility of collision if two tests are running in
> parallel?

I don't know what you mean. That's why I lock the file here so that
there's no collision.

> 
>> +    }
>> +
>> +    /* Now append the following line into the output file:
>> +     * $file: $progname $testname */
>> +
>> +    fprintf(fp, "%s: %s", file, progname);
>> +    if (testname)
>> +        fprintf(fp, ": %s", testname);
>> +
>> +    fputc('\n', fp);
>> +
>> +    flock(fileno(fp), LOCK_UN);
>> +    fclose(fp);
>> +}
>> +
>> +
>> +#define VIR_FILE_ACCESS_DEFAULT abs_builddir "/test_file_access.txt"
>> +
>> +static void
>> +cutOffLastComponent(char *path)
>> +{
>> +    char *base = path, *p = base;
>> +
>> +    while (*p) {
>> +        if (*p == '/')
>> +            base = p;
>> +        p++;
>> +    }
>> +
>> +    if (base != p)
>> +        *base = '\0';
> 
> We already have a function like this:
> virStorageFileRemoveLastPathComponent.
> 
> Also 'strrchr' instead of the while loop?

Hm.. okay. I can expose the function. I've looked around virfile.c and
haven't find anything. I didn't check storage driver sources though.

> 
>>  }
>>  
>>  static void
>>  checkPath(const char *path)
>>  {
>> +    char *fullPath = NULL;
>> +    char *relPath = NULL;
>> +    char *crippledPath = NULL;
>> +
>> +    if (path[0] != '/' &&
>> +        asprintf(&relPath, "./%s", path) < 0)
>> +        goto error;
>> +
>> +    /* Le sigh. Both canonicalize_file_name() and realpath()
>> +     * expect @path to exist otherwise they return an error. So
>> +     * if we are called over an non-existent file, this could
>> +     * return an error. In that case do our best and hope we will
>> +     * catch possible error. */
> 
> You can always stat the file to check if it exists before trying to get
> it's canonical path.

Why? if stat() succeeds so does canonicalize_file_name() and vice versa.
Therefore I think calling stat() would be just redundant.

> 
>> +    if ((fullPath = canonicalize_file_name(relPath ? relPath : path))) {
>> +        path = fullPath;
>> +    } else {
>> +        /* Yeah, our worst nightmares just became true. Path does
>> +         * not exist. Cut off the last component and retry. */
>> +        if (!(crippledPath = strdup(relPath ? relPath : path)))
>> +            goto error;
> 
> So if the parrent directory won't exist either you will attempt to do
> the checking in the non-canonical path?
> 
> Maybe you could try to match the paths according to getcwd() to resolve
> relative paths at first or maybe do this in a loop until you find an
> existing object. (I'm mostly worried about paths like
> ../../../../../../something/that/does/not/exist where at least the root
> directory should be accessible)

I think this is overcomplicated approach.

> 
>> +
>> +        cutOffLastComponent(crippledPath);
>> +
>> +        if ((fullPath = canonicalize_file_name(crippledPath)))
>> +            path = fullPath;
>> +    }
>> +
>> +
>>      if (!STRPREFIX(path, abs_topsrcdir) &&
>>          !STRPREFIX(path, abs_topbuilddir)) {
>> -        /* Okay, this is a dummy check and spurious print.
>> -         * But this is going to be replaced soon. */
>> -        fprintf(stderr, "*** %s ***\n", path);
>> +        const char *output = getenv("VIR_TEST_FILE_ACCESS");
> 
> I think you could cache this too  as you've did with 'progname'

Okay.

> 
>> +        if (!output)
>> +            output = VIR_FILE_ACCESS_DEFAULT;
>> +        printFile(output, path);
>>      }
>> +
>> +    free(crippledPath);
>> +    free(relPath);
>> +    free(fullPath); 
>> +
>> +    return;
>> + error:
>> +    fprintf(stderr, "Out of memory\n");
>> +    abort();
>>  }
> 

Michal


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]