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