7:33 a.m.
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(a)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