Move some parts of virStorageFileRemoveLastPathComponent into a separate function so they can be reused. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virfile.c | 17 +++++++++++++++++ src/util/virfile.h | 1 + src/util/virstoragefile.c | 6 +----- 4 files changed, 20 insertions(+), 5 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a980a32..fb24808 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1517,6 +1517,7 @@ virFileReadHeaderFD; virFileReadLimFD; virFileRelLinkPointsTo; virFileRemove; +virFileRemoveLastComponent; virFileResolveAllLinks; virFileResolveLink; virFileRewrite; diff --git a/src/util/virfile.c b/src/util/virfile.c index 4d7b510..9d460b9 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -3132,6 +3132,23 @@ virFileSanitizePath(const char *path) return cleanpath; } +/** + * virFileRemoveLastComponent: + * + * For given path cut off the last component. If there's no dir + * separator (whole path is one file name), @path is turned into + * an empty string. + */ +void +virFileRemoveLastComponent(char *path) +{ + char *tmp; + + if ((tmp = strrchr(path, VIR_FILE_DIR_SEPARATOR))) + tmp[1] = '\0'; + else + path[0] = '\0'; +} /** * virFilePrintf: diff --git a/src/util/virfile.h b/src/util/virfile.h index dc62eab..dae234e 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -268,6 +268,7 @@ bool virFileIsAbsPath(const char *path); int virFileAbsPath(const char *path, char **abspath) ATTRIBUTE_RETURN_CHECK; const char *virFileSkipRoot(const char *path); +void virFileRemoveLastComponent(char *path); int virFileOpenTty(int *ttymaster, char **ttyName, diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index d4e61ca..d2da9e7 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -2868,16 +2868,12 @@ virStorageFileCanonicalizePath(const char *path, static char * virStorageFileRemoveLastPathComponent(const char *path) { - char *tmp; char *ret; if (VIR_STRDUP(ret, path ? path : "") < 0) return NULL; - if ((tmp = strrchr(ret, '/'))) - tmp[1] = '\0'; - else - ret[0] = '\0'; + virFileRemoveLastComponent(ret); return ret; } -- 2.8.1

The intent is that this library is going to be called every time to check if we are not touching anything outside srcdir or builddir. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- cfg.mk | 2 +- tests/Makefile.am | 13 +++- tests/testutils.c | 9 +++ tests/testutils.h | 10 +-- tests/vircgroupmock.c | 15 ++--- tests/virpcimock.c | 14 ++-- tests/virtestmock.c | 175 ++++++++++++++++++++++++++++++++++++++++++++++++++ 7 files changed, 210 insertions(+), 28 deletions(-) create mode 100644 tests/virtestmock.c diff --git a/cfg.mk b/cfg.mk index b277db1..c0aba57 100644 --- a/cfg.mk +++ b/cfg.mk @@ -1164,7 +1164,7 @@ exclude_file_name_regexp--sc_copyright_usage = \ ^COPYING(|\.LESSER)$$ exclude_file_name_regexp--sc_flags_usage = \ - ^(cfg\.mk|docs/|src/util/virnetdevtap\.c$$|tests/(vir(cgroup|pci|usb)|nss|qemuxml2argv)mock\.c$$) + ^(cfg\.mk|docs/|src/util/virnetdevtap\.c$$|tests/(vir(cgroup|pci|test|usb)|nss|qemuxml2argv)mock\.c$$) exclude_file_name_regexp--sc_libvirt_unmarked_diagnostics = \ ^(src/rpc/gendispatch\.pl$$|tests/) diff --git a/tests/Makefile.am b/tests/Makefile.am index 75efb90..da68f2e 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -18,7 +18,9 @@ # old automake does not provide abs_{src,build}dir variables abs_builddir = $(shell pwd) +abs_topbuilddir = $(shell cd .. && pwd) abs_srcdir = $(shell cd $(srcdir) && pwd) +abs_topsrcdir = $(shell cd $(top_srcdir) && pwd) SHELL = $(PREFERABLY_POSIX_SHELL) @@ -33,7 +35,9 @@ INCLUDES = \ AM_CFLAGS = \ -Dabs_builddir="\"$(abs_builddir)\"" \ + -Dabs_topbuilddir="\"$(abs_topbuilddir)\"" \ -Dabs_srcdir="\"$(abs_srcdir)\"" \ + -Dabs_topsrcdir="\"$(abs_topsrcdir)\"" \ $(LIBXML_CFLAGS) \ $(LIBNL_CFLAGS) \ $(GNUTLS_CFLAGS) \ @@ -443,6 +447,7 @@ endif WITH_DBUS if WITH_LINUX test_libraries += virusbmock.la \ virnetdevbandwidthmock.la \ + virtestmock.la $(NULL) endif WITH_LINUX @@ -1142,9 +1147,15 @@ virnetdevbandwidthmock_la_CFLAGS = $(AM_CFLAGS) virnetdevbandwidthmock_la_LDFLAGS = $(MOCKLIBS_LDFLAGS) virnetdevbandwidthmock_la_LIBADD = $(MOCKLIBS_LIBS) +virtestmock_la_SOURCES = \ + virtestmock.c +virtestmock_la_CFLAGS = $(AM_CFLAGS) +virtestmock_la_LDFLAGS = $(MOCKLIBS_LDFLAGS) +virtestmock_la_LIBADD = $(MOCKLIBS_LIBS) else ! WITH_LINUX EXTRA_DIST += virusbtest.c virusbmock.c \ - virnetdevbandwidthtest.c virnetdevbandwidthmock.c + virnetdevbandwidthtest.c virnetdevbandwidthmock.c \ + virtestmock.c endif ! WITH_LINUX if WITH_DBUS diff --git a/tests/testutils.c b/tests/testutils.c index 79d0763..595b64d 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -832,8 +832,11 @@ virTestSetEnvPath(void) return ret; } +#define TEST_MOCK (abs_builddir "/.libs/virtestmock.so") + int virtTestMain(int argc, char **argv, + const char *lib, int (*func)(void)) { int ret; @@ -842,6 +845,12 @@ int virtTestMain(int argc, char *oomstr; #endif +#ifdef __linux__ + VIRT_TEST_PRELOAD(TEST_MOCK); +#endif + if (lib) + VIRT_TEST_PRELOAD(lib); + virFileActivateDirOverride(argv[0]); if (virTestSetEnvPath() < 0) diff --git a/tests/testutils.h b/tests/testutils.h index 123a4fb..d1caf20 100644 --- a/tests/testutils.h +++ b/tests/testutils.h @@ -102,12 +102,13 @@ const char *virtTestCounterNext(void); int virtTestMain(int argc, char **argv, + const char *lib, int (*func)(void)); /* Setup, then call func() */ -# define VIRT_TEST_MAIN(func) \ - int main(int argc, char **argv) { \ - return virtTestMain(argc, argv, func); \ +# define VIRT_TEST_MAIN(func) \ + int main(int argc, char **argv) { \ + return virtTestMain(argc, argv, NULL, func); \ } # define VIRT_TEST_PRELOAD(lib) \ @@ -132,8 +133,7 @@ int virtTestMain(int argc, # define VIRT_TEST_MAIN_PRELOAD(func, lib) \ int main(int argc, char **argv) { \ - VIRT_TEST_PRELOAD(lib); \ - return virtTestMain(argc, argv, func); \ + return virtTestMain(argc, argv, lib, func); \ } virCapsPtr virTestGenericCapsInit(void); diff --git a/tests/vircgroupmock.c b/tests/vircgroupmock.c index cfc51e8..cd0eb34 100644 --- a/tests/vircgroupmock.c +++ b/tests/vircgroupmock.c @@ -405,19 +405,12 @@ static void init_syms(void) } \ } while (0) -# define LOAD_SYM_ALT(name1, name2) \ - do { \ - if (!(real ## name1 = dlsym(RTLD_NEXT, #name1)) && \ - !(real ## name2 = dlsym(RTLD_NEXT, #name2))) { \ - fprintf(stderr, "Cannot find real '%s' or '%s' symbol\n", #name1, #name2); \ - abort(); \ - } \ - } while (0) - LOAD_SYM(fopen); LOAD_SYM(access); - LOAD_SYM_ALT(lstat, __lxstat); - LOAD_SYM_ALT(stat, __xstat); + LOAD_SYM(lstat); + LOAD_SYM(__lxstat); + LOAD_SYM(stat); + LOAD_SYM(__xstat); LOAD_SYM(mkdir); LOAD_SYM(open); } diff --git a/tests/virpcimock.c b/tests/virpcimock.c index cfe42a6..48a6e7f 100644 --- a/tests/virpcimock.c +++ b/tests/virpcimock.c @@ -781,17 +781,11 @@ init_syms(void) ABORT("Cannot find real '%s' symbol\n", #name); \ } while (0) -# define LOAD_SYM_ALT(name1, name2) \ - do { \ - if (!(real ## name1 = dlsym(RTLD_NEXT, #name1)) && \ - !(real ## name2 = dlsym(RTLD_NEXT, #name2))) \ - ABORT("Cannot find real '%s' or '%s' symbol\n", \ - #name1, #name2); \ - } while (0) - LOAD_SYM(access); - LOAD_SYM_ALT(lstat, __lxstat); - LOAD_SYM_ALT(stat, __xstat); + LOAD_SYM(lstat); + LOAD_SYM(__lxstat); + LOAD_SYM(stat); + LOAD_SYM(__xstat); LOAD_SYM(canonicalize_file_name); LOAD_SYM(open); LOAD_SYM(close); diff --git a/tests/virtestmock.c b/tests/virtestmock.c new file mode 100644 index 0000000..5c39b89 --- /dev/null +++ b/tests/virtestmock.c @@ -0,0 +1,175 @@ +/* + * Copyright (C) 2016 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Author: Michal Privoznik <mprivozn@redhat.com> + */ + +#include <config.h> +#include <unistd.h> +#include <sys/types.h> +#include <stdio.h> +#include <dlfcn.h> +#include <sys/stat.h> +#include <fcntl.h> + +#include "internal.h" +#include "configmake.h" + +static int (*realopen)(const char *path, int flags, ...); +static FILE *(*realfopen)(const char *path, const char *mode); +static int (*realaccess)(const char *path, int mode); +static int (*realstat)(const char *path, struct stat *sb); +static int (*real__xstat)(int ver, const char *path, struct stat *sb); +static int (*reallstat)(const char *path, struct stat *sb); +static int (*real__lxstat)(int ver, const char *path, struct stat *sb); + +static void init_syms(void) +{ + if (realopen) + return; + +#define LOAD_SYM(name) \ + do { \ + if (!(real ## name = dlsym(RTLD_NEXT, #name))) { \ + fprintf(stderr, "Cannot find real '%s' symbol\n", #name); \ + abort(); \ + } \ + } while (0) + +#define LOAD_SYM_ALT(name1, name2) \ + do { \ + if (!(real ## name1 = dlsym(RTLD_NEXT, #name1)) && \ + !(real ## name2 = dlsym(RTLD_NEXT, #name2))) { \ + fprintf(stderr, "Cannot find real '%s' or '%s' symbol\n", #name1, #name2); \ + abort(); \ + } \ + } while (0) + + LOAD_SYM(open); + LOAD_SYM(fopen); + LOAD_SYM(access); + LOAD_SYM_ALT(stat, __xstat); + LOAD_SYM_ALT(lstat, __lxstat); +} + +static void +checkPath(const char *path ATTRIBUTE_UNUSED) +{ + /* Nada */ +} + + +int open(const char *path, int flags, ...) +{ + int ret; + + init_syms(); + + checkPath(path); + + if (flags & O_CREAT) { + va_list ap; + mode_t mode; + va_start(ap, flags); + mode = va_arg(ap, mode_t); + va_end(ap); + ret = realopen(path, flags, mode); + } else { + ret = realopen(path, flags); + } + return ret; +} + +FILE *fopen(const char *path, const char *mode) +{ + init_syms(); + + checkPath(path); + + return realfopen(path, mode); +} + + +int access(const char *path, int mode) +{ + init_syms(); + + checkPath(path); + + return realaccess(path, mode); +} + +int stat(const char *path, struct stat *sb) +{ + init_syms(); + + checkPath(path); + + if (!realstat) + return real__xstat(1, path, sb); + + return realstat(path, sb); +} + +int +__xstat(int ver, const char *path, struct stat *sb) +{ + init_syms(); + + checkPath(path); + if (!real__xstat) { + if (ver == 1) { + return realstat(path, sb); + } else { + fprintf(stderr, "Not handled __xstat(ver=%d)", ver); + abort(); + } + } + + return real__xstat(ver, path, sb); +} + +int +lstat(const char *path, struct stat *sb) +{ + init_syms(); + + checkPath(path); + + if (!reallstat) + return real__lxstat(1, path, sb); + + return reallstat(path, sb); +} + +int +__lxstat(int ver, const char *path, struct stat *sb) +{ + init_syms(); + + checkPath(path); + if (!real__lxstat) { + if (ver == 1) { + return reallstat(path, sb); + } else { + fprintf(stderr, "Not handled __lxstat(ver=%d)", ver); + abort(); + } + } + + return real__lxstat(ver, path, sb); +} -- 2.8.1

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 | 9 +++++ docs/hacking.html.in | 11 ++++++ tests/Makefile.am | 4 ++- tests/testutils.c | 25 +++++++++----- tests/virtestmock.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++++++-- 6 files changed, 134 insertions(+), 11 deletions(-) diff --git a/.gitignore b/.gitignore index fc21bad..ee89787 100644 --- a/.gitignore +++ b/.gitignore @@ -169,6 +169,7 @@ /tests/objectlocking.cm[ix] /tests/reconnect /tests/ssh +/tests/test_file_access.txt /tests/test_conf /tools/libvirt-guests.sh /tools/virt-login-shell diff --git a/HACKING b/HACKING index e308568..bd33532 100644 --- a/HACKING +++ b/HACKING @@ -152,6 +152,15 @@ 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 the test result is +nondeterministic because of the test suite relying on a particular file in the +system being accessible or having some specific value. To catch this kind of +errors, the test suite has a module for that 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/docs/hacking.html.in b/docs/hacking.html.in index 5cd23a2..34632ee 100644 --- a/docs/hacking.html.in +++ b/docs/hacking.html.in @@ -189,6 +189,17 @@ under gdb or Valgrind. </p> + <p>When running our test suite it may happen that the test result is + nondeterministic because of the test suite relying on a particular file + in the system being accessible or having some specific value. To catch + this kind of errors, the test suite has a module for that 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> + </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/Makefile.am b/tests/Makefile.am index da68f2e..ca3c8c3 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -1151,7 +1151,9 @@ virtestmock_la_SOURCES = \ virtestmock.c virtestmock_la_CFLAGS = $(AM_CFLAGS) virtestmock_la_LDFLAGS = $(MOCKLIBS_LDFLAGS) -virtestmock_la_LIBADD = $(MOCKLIBS_LIBS) +virtestmock_la_LIBADD = \ + $(MOCKLIBS_LIBS) \ + ../src/libvirt_util.la else ! WITH_LINUX EXTRA_DIST += virusbtest.c virusbmock.c \ virnetdevbandwidthtest.c virnetdevbandwidthmock.c \ diff --git a/tests/testutils.c b/tests/testutils.c index 595b64d..e0a0d56 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -156,6 +156,11 @@ virtTestRun(const char *title, { int ret = 0; + /* Some test are fragile about environ settings. If that's + * the case, don't poison it. */ + if (getenv("VIR_TEST_MOCK_PROGNAME")) + setenv("VIR_TEST_MOCK_TESTNAME", title, 1); + if (testCounter == 0 && !virTestGetVerbose()) fprintf(stderr, " "); @@ -280,6 +285,7 @@ virtTestRun(const char *title, } #endif /* TEST_OOM */ + unsetenv("VIR_TEST_MOCK_TESTNAME"); return ret; } @@ -851,17 +857,20 @@ int virtTestMain(int argc, if (lib) VIRT_TEST_PRELOAD(lib); - virFileActivateDirOverride(argv[0]); - - if (virTestSetEnvPath() < 0) - return EXIT_AM_HARDFAIL; - - if (!virFileExists(abs_srcdir)) - return EXIT_AM_HARDFAIL; - progname = last_component(argv[0]); if (STRPREFIX(progname, "lt-")) progname += 3; + + setenv("VIR_TEST_MOCK_PROGNAME", progname, 1); + + virFileActivateDirOverride(argv[0]); + + if (virTestSetEnvPath() < 0) + return EXIT_AM_HARDFAIL; + + if (!virFileExists(abs_srcdir)) + return EXIT_AM_HARDFAIL; + if (argc > 1) { fprintf(stderr, "Usage: %s\n", argv[0]); fputs("effective environment variables:\n" diff --git a/tests/virtestmock.c b/tests/virtestmock.c index 5c39b89..c97b6bd 100644 --- a/tests/virtestmock.c +++ b/tests/virtestmock.c @@ -25,9 +25,14 @@ #include <dlfcn.h> #include <sys/stat.h> #include <fcntl.h> +#include <execinfo.h> +#include <sys/file.h> #include "internal.h" #include "configmake.h" +#include "virstring.h" +#include "viralloc.h" +#include "virfile.h" static int (*realopen)(const char *path, int flags, ...); static FILE *(*realfopen)(const char *path, const char *mode); @@ -37,6 +42,11 @@ static int (*real__xstat)(int ver, const char *path, struct stat *sb); static int (*reallstat)(const char *path, struct stat *sb); static int (*real__lxstat)(int ver, const char *path, struct stat *sb); +static const char *progname; +const char *output; + +#define VIR_FILE_ACCESS_DEFAULT abs_builddir "/test_file_access.txt" + static void init_syms(void) { if (realopen) @@ -67,9 +77,90 @@ static void init_syms(void) } static void -checkPath(const char *path ATTRIBUTE_UNUSED) +printFile(const char *file) { - /* Nada */ + FILE *fp; + const char *testname = getenv("VIR_TEST_MOCK_TESTNAME"); + + if (!progname) { + progname = getenv("VIR_TEST_MOCK_PROGNAME"); + + if (!progname) + return; + + output = getenv("VIR_TEST_FILE_ACCESS"); + if (!output) + output = VIR_FILE_ACCESS_DEFAULT; + } + + if (!(fp = realfopen(output, "a"))) { + fprintf(stderr, "Unable to open %s: %s\n", output, strerror(errno)); + abort(); + } + + if (flock(fileno(fp), LOCK_EX) < 0) { + fprintf(stderr, "Unable to lock %s: %s\n", output, strerror(errno)); + fclose(fp); + abort(); + } + + /* 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); +} + +static void +checkPath(const char *path) +{ + char *fullPath = NULL; + char *relPath = NULL; + char *crippledPath = NULL; + + if (path[0] != '/' && + virAsprintfQuiet(&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. */ + 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 (VIR_STRDUP_QUIET(crippledPath, relPath ? relPath : path) < 0) + goto error; + + virFileRemoveLastComponent(crippledPath); + + if ((fullPath = canonicalize_file_name(crippledPath))) + path = fullPath; + } + + + if (!STRPREFIX(path, abs_topsrcdir) && + !STRPREFIX(path, abs_topbuilddir)) { + printFile(path); + } + + VIR_FREE(crippledPath); + VIR_FREE(relPath); + VIR_FREE(fullPath); + + return; + error: + fprintf(stderr, "Out of memory\n"); + abort(); } -- 2.8.1