[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 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?

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

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

> +    }
> +
> +    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?

> +    }
> +
> +    /* 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?

>  }
>  
>  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.

> +    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)

> +
> +        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'

> +        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();
>  }

Peter

Attachment: signature.asc
Description: Digital signature


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