[libvirt] [PATCH 0/4] Introduce big test mock

What? Big test mock that checks all our file accesses whether we are touching anything outside $srcdir or $builddir. Why? Because it has happened in the past (and is happening even now as you'll see later) that our test suite produces nondeterministic results because of a file in live system being accessible or having some specific value. How? Well, this is merely introducing a mock library that every C program on Linux will use. I'm not aware of any approach for our shell tests. The mock library then collects all the paths outside $srcdir or $builddir and prints them into a file. The file is then checked against a white list, because some accesses might be desirable. Unfortunately, as you'll learn in the 4/4 commit message, I was unable to wire this up to 'make check' so this is accessible under 'make check-access' target. If there's some access outside aforementioned directories, the path will be printed out. So far there is plenty of them. For instance, in qemuxml2argvtest we are checking /proc/sys/crypto/fips_enabled for each test. Ouch. What's left to do? a) Enhance white list b) Wire this up to 'check' c) Test d) Review Michal Privoznik (4): seclabeltest: Update to use VIRT_TEST_MAIN tests: Introduce global mock library virtestmock: Print invalid file accesses into a file tests: Introduce check-file-access.pl .gitignore | 1 + HACKING | 11 ++ Makefile.am | 3 + cfg.mk | 8 +- docs/hacking.html.in | 15 +++ tests/Makefile.am | 28 +++- tests/check-file-access.pl | 106 +++++++++++++++ tests/file_access_whitelist.txt | 19 +++ tests/seclabeltest.c | 7 +- tests/testutils.c | 58 ++++++++- tests/testutils.h | 29 +---- tests/vircgroupmock.c | 6 +- tests/virpcimock.c | 6 +- tests/virtestmock.c | 279 ++++++++++++++++++++++++++++++++++++++++ 14 files changed, 535 insertions(+), 41 deletions(-) create mode 100755 tests/check-file-access.pl create mode 100644 tests/file_access_whitelist.txt create mode 100644 tests/virtestmock.c -- 2.7.3

Our tests should use either VIRT_TEST_MAIN() or VIRT_TEST_MAIN_PRELOAD() macros which create main() function and call the passed callback subsequently. This is important because the wrapper which calls the callback eventually does important stuff like setting logging based on env variables and such. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/Makefile.am | 2 +- tests/seclabeltest.c | 7 +++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/tests/Makefile.am b/tests/Makefile.am index db4f88b..67f597a 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -1182,7 +1182,7 @@ virauthconfigtest_SOURCES = \ virauthconfigtest_LDADD = $(LDADDS) seclabeltest_SOURCES = \ - seclabeltest.c + seclabeltest.c testutils.h testutils.c seclabeltest_LDADD = $(LDADDS) if WITH_SECDRIVER_SELINUX diff --git a/tests/seclabeltest.c b/tests/seclabeltest.c index 6b1e789..be6e79f 100644 --- a/tests/seclabeltest.c +++ b/tests/seclabeltest.c @@ -7,9 +7,10 @@ #include <errno.h> #include "security/security_driver.h" #include "virrandom.h" +#include "testutils.h" -int -main(int argc ATTRIBUTE_UNUSED, char **argv ATTRIBUTE_UNUSED) +static int +mymain(void) { virSecurityManagerPtr mgr; const char *doi, *model; @@ -41,3 +42,5 @@ main(int argc ATTRIBUTE_UNUSED, char **argv ATTRIBUTE_UNUSED) return 0; } + +VIRT_TEST_MAIN(mymain) -- 2.7.3

On 21/04/16 12:16, Michal Privoznik wrote:
Our tests should use either VIRT_TEST_MAIN() or VIRT_TEST_MAIN_PRELOAD() macros which create main() function and call the passed callback subsequently. This is important because the wrapper which calls the callback eventually does important stuff like setting logging based on env variables and such.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/Makefile.am | 2 +- tests/seclabeltest.c | 7 +++++-- 2 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/tests/Makefile.am b/tests/Makefile.am index db4f88b..67f597a 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -1182,7 +1182,7 @@ virauthconfigtest_SOURCES = \ virauthconfigtest_LDADD = $(LDADDS)
seclabeltest_SOURCES = \ - seclabeltest.c + seclabeltest.c testutils.h testutils.c seclabeltest_LDADD = $(LDADDS)
if WITH_SECDRIVER_SELINUX diff --git a/tests/seclabeltest.c b/tests/seclabeltest.c index 6b1e789..be6e79f 100644 --- a/tests/seclabeltest.c +++ b/tests/seclabeltest.c @@ -7,9 +7,10 @@ #include <errno.h> #include "security/security_driver.h" #include "virrandom.h" +#include "testutils.h"
-int -main(int argc ATTRIBUTE_UNUSED, char **argv ATTRIBUTE_UNUSED) +static int +mymain(void) { virSecurityManagerPtr mgr; const char *doi, *model; @@ -41,3 +42,5 @@ main(int argc ATTRIBUTE_UNUSED, char **argv ATTRIBUTE_UNUSED)
return 0; } + +VIRT_TEST_MAIN(mymain)
ACK Erik

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 | 33 +++++++++ tests/testutils.h | 29 ++------ tests/vircgroupmock.c | 6 +- tests/virpcimock.c | 6 +- tests/virtestmock.c | 180 ++++++++++++++++++++++++++++++++++++++++++++++++++ 7 files changed, 241 insertions(+), 28 deletions(-) create mode 100644 tests/virtestmock.c diff --git a/cfg.mk b/cfg.mk index 16da2b3..afee8e9 100644 --- a/cfg.mk +++ b/cfg.mk @@ -1153,7 +1153,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 67f597a..6412337 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) \ @@ -448,6 +452,7 @@ endif WITH_DBUS if WITH_LINUX test_libraries += virusbmock.la \ virnetdevbandwidthmock.la \ + virtestmock.la $(NULL) endif WITH_LINUX @@ -1134,9 +1139,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..403ab37 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -832,8 +832,35 @@ virTestSetEnvPath(void) return ret; } +static void +virTestLDPreload(char *const argv[], + const char *lib) +{ + const char *preload = getenv("LD_PRELOAD"); + + if (preload == NULL || strstr(preload, lib) == NULL) { + char *newenv; + + if (!virFileIsExecutable(lib)) { + fprintf(stderr, "%s is not executable\n", lib); + _exit(EXIT_FAILURE); + } + + if ((!preload && VIR_STRDUP(newenv, lib) < 0) || + (preload && virAsprintf(&newenv, "%s:%s", lib, preload) < 0)) { + fprintf(stderr, "Out of memory\n"); + _exit(EXIT_FAILURE); + } + setenv("LD_PRELOAD", newenv, 1); + execv(argv[0], argv); + } +} + +#define TEST_MOCK (abs_builddir "/.libs/virtestmock.so") + int virtTestMain(int argc, char **argv, + const char *lib, int (*func)(void)) { int ret; @@ -842,6 +869,12 @@ int virtTestMain(int argc, char *oomstr; #endif +#ifdef __linux__ + virTestLDPreload(argv, TEST_MOCK); +#endif + if (lib) + virTestLDPreload(argv, lib); + virFileActivateDirOverride(argv[0]); if (virTestSetEnvPath() < 0) diff --git a/tests/testutils.h b/tests/testutils.h index 0417a0b..215f349 100644 --- a/tests/testutils.h +++ b/tests/testutils.h @@ -102,33 +102,18 @@ 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_MAIN_PRELOAD(func, lib) \ - int main(int argc, char **argv) { \ - const char *preload = getenv("LD_PRELOAD"); \ - if (preload == NULL || strstr(preload, lib) == NULL) { \ - char *newenv; \ - if (!virFileIsExecutable(lib)) { \ - perror(lib); \ - return EXIT_FAILURE; \ - } \ - if (!preload) { \ - newenv = (char *) lib; \ - } else if (virAsprintf(&newenv, "%s:%s", lib, preload) < 0) { \ - perror("virAsprintf"); \ - return EXIT_FAILURE; \ - } \ - setenv("LD_PRELOAD", newenv, 1); \ - execv(argv[0], argv); \ - } \ - return virtTestMain(argc, argv, func); \ +# define VIRT_TEST_MAIN_PRELOAD(func, lib) \ + int main(int argc, char **argv) { \ + return virtTestMain(argc, argv, lib, func); \ } virCapsPtr virTestGenericCapsInit(void); diff --git a/tests/vircgroupmock.c b/tests/vircgroupmock.c index cfc51e8..395254b 100644 --- a/tests/vircgroupmock.c +++ b/tests/vircgroupmock.c @@ -416,8 +416,10 @@ static void init_syms(void) 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..ac8a665 100644 --- a/tests/virpcimock.c +++ b/tests/virpcimock.c @@ -790,8 +790,10 @@ init_syms(void) } 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..f138e98 --- /dev/null +++ b/tests/virtestmock.c @@ -0,0 +1,180 @@ +/* + * Copyright (C) 2014 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) +{ + 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); + } +} + + +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.7.3

On 21/04/16 12:16, Michal Privoznik wrote:
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 | 33 +++++++++ tests/testutils.h | 29 ++------ tests/vircgroupmock.c | 6 +- tests/virpcimock.c | 6 +- tests/virtestmock.c | 180 ++++++++++++++++++++++++++++++++++++++++++++++++++ 7 files changed, 241 insertions(+), 28 deletions(-) create mode 100644 tests/virtestmock.c
...
virCapsPtr virTestGenericCapsInit(void); diff --git a/tests/vircgroupmock.c b/tests/vircgroupmock.c index cfc51e8..395254b 100644 --- a/tests/vircgroupmock.c +++ b/tests/vircgroupmock.c @@ -416,8 +416,10 @@ static void init_syms(void)
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); }
I wondered why this change ^^ was necessary, but then realized the tests would crash for some reason that is unknown to me.
diff --git a/tests/virtestmock.c b/tests/virtestmock.c new file mode 100644 index 0000000..f138e98 --- /dev/null +++ b/tests/virtestmock.c @@ -0,0 +1,180 @@ +/* + * Copyright (C) 2014 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) +{ + 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); + } +} + + +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); +}
The patch works, it also looks reasonable to me, but I'm far from being a master in this matter, so let's wait if someone would like to chime in and express their opinion. If not, I say ACK. Erik

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/.gitignore b/.gitignore index 381db69..a681416 100644 --- a/.gitignore +++ b/.gitignore @@ -174,6 +174,7 @@ /tests/objectlocking.cm[ix] /tests/reconnect /tests/ssh +/tests/test_file_access.txt /tests/test_conf /tools/*.[18] /tools/libvirt-guests.init 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 +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 "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 afee8e9..ccd0036 100644 --- a/cfg.mk +++ b/cfg.mk @@ -1170,10 +1170,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)$$) @@ -1200,7 +1200,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$$ 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> + </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 403ab37..e8dd39a 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 + * not see which test in particular is touching which file, + * but we are still able to tell which binary is doing so. */ + if (getenv("VIR_TEST_MOCK_PROGNAME")) + setenv("VIR_TEST_MOCK_TESTNAME", title, 1); + if (testCounter == 0 && !virTestGetVerbose()) fprintf(stderr, " "); @@ -280,6 +287,7 @@ virtTestRun(const char *title, } #endif /* TEST_OOM */ + unsetenv("VIR_TEST_MOCK_TESTNAME"); return ret; } @@ -875,17 +883,20 @@ int virtTestMain(int argc, if (lib) virTestLDPreload(argv, 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 f138e98..c47eb0c 100644 --- a/tests/virtestmock.c +++ b/tests/virtestmock.c @@ -25,6 +25,8 @@ #include <dlfcn.h> #include <sys/stat.h> #include <fcntl.h> +#include <execinfo.h> +#include <sys/file.h> #include "internal.h" #include "configmake.h" @@ -37,6 +39,8 @@ 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; + static void init_syms(void) { if (realopen) @@ -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(); + } + + 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); +} + + +#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'; } 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. */ + 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; + + 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"); + 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(); } -- 2.7.3

This script will check output generated by virtestmock against a white list. All non matching records found are printed out. So far, the white list is rather sparse at the moment. This test should be ran only after all other tests finished, and should cleanup the temporary file before their execution. Because I'm unable to reflect these requirements in Makefile.am correctly, I've introduced new target 'check-access' under which this test is available. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- Makefile.am | 3 ++ tests/Makefile.am | 13 +++++ tests/check-file-access.pl | 106 ++++++++++++++++++++++++++++++++++++++++ tests/file_access_whitelist.txt | 19 +++++++ 4 files changed, 141 insertions(+) create mode 100755 tests/check-file-access.pl create mode 100644 tests/file_access_whitelist.txt diff --git a/Makefile.am b/Makefile.am index d6f09ba..986916c 100644 --- a/Makefile.am +++ b/Makefile.am @@ -67,6 +67,9 @@ rpm: clean check-local: all tests +check-access: + @($(MAKE) $(AM_MAKEFLAGS) -C tests check-access) + cov: clean-cov $(MKDIR_P) $(top_builddir)/coverage $(LCOV) -c -o $(top_builddir)/coverage/libvirt.info.tmp \ diff --git a/tests/Makefile.am b/tests/Makefile.am index 6412337..f65fc97 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -456,6 +456,19 @@ test_libraries += virusbmock.la \ $(NULL) endif WITH_LINUX +if WITH_LINUX +check-access: file-access-clean + $(MAKE) $(AM_MAKEFLAGS) check + $(PERL) check-file-access.pl + +file-access-clean: + > test_file_access.txt +endif WITH_LINUX + +EXTRA_DIST += \ + check-file-access.pl \ + file_access_whitelist.txt + if WITH_TESTS noinst_PROGRAMS = $(test_programs) $(test_helpers) noinst_LTLIBRARIES = $(test_libraries) diff --git a/tests/check-file-access.pl b/tests/check-file-access.pl new file mode 100755 index 0000000..6e59201 --- /dev/null +++ b/tests/check-file-access.pl @@ -0,0 +1,106 @@ +#!/usr/bin/perl -w +# +# 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/>. +# +# This script is supposed to check test_file_access.txt file and +# warn about file accesses outside our working tree. +# +# + +use strict; +use warnings; + +my $access_file = "test_file_access.txt"; +my $whitelist_file = "file_access_whitelist.txt"; + +my @files; +my @whitelist; + +open FILE, "<", $access_file or die "Unable to open $access_file: $!"; +while (<FILE>) { + chomp; + if (/^\s*#.*$/) { + # comment + } elsif (/^(\S*):\s*(\S*)(\s*:\s*(.*))?$/) { + my %rec; + ${rec}{path} = $1; + ${rec}{progname} = $2; + if (defined $4) { + ${rec}{testname} = $4; + } + push (@files, \%rec); + } else { + die "Malformed line $_"; + } +} +close FILE; + +open FILE, "<", $whitelist_file or die "Unable to open $whitelist_file: $!"; +while (<FILE>) { + chomp; + if (/^\s*#.*$/) { + # comment + } elsif (/^(\S*)(:\s*(\S*)(\s*:\s*(.*))?)?$/) { + my %rec; + ${rec}{path} = $1; + if (defined $3) { + ${rec}{progname} = $3; + } + if (defined $5) { + ${rec}{testname} = $5; + } + push (@whitelist, \%rec); + } else { + die "Malformed line $_"; + } +} +close FILE; + +# Now we should check if %traces is included in $whitelist. For +# now checking just keys is sufficient +my $error = 0; +for my $file (@files) { + my $match = 0; + + for my $rule (@whitelist) { + if (not %${file}{path} =~ m/^$rule->{path}$/) { + next; + } + + if (defined %${rule}{progname} and + not %${file}{progname} =~ m/^$rule->{progname}$/) { + next; + } + + if (defined %${rule}{testname} and + defined %${file}{testname} and + not %${file}{testname} =~ m/^$rule->{testname}$/) { + next; + } + + $match = 1; + } + + if (not $match) { + $error = 1; + print "$file->{path}: $file->{progname}"; + print ": $file->{testname}" if defined %${file}{testname}; + print "\n"; + } +} + +$error; diff --git a/tests/file_access_whitelist.txt b/tests/file_access_whitelist.txt new file mode 100644 index 0000000..0e853f7 --- /dev/null +++ b/tests/file_access_whitelist.txt @@ -0,0 +1,19 @@ +# This is a whitelist that allows accesses to files not in our +# build directory nor source directory. The records are in the +# following format: +# +# $path: $progname: $testname +# +# All these three are evaluated as perl RE. So to allow /dev/sda +# and /dev/sdb, you can just '/dev/sd[a-b]', or to allow +# /proc/$pid/status you can '/proc/\d+/status' and so on. +# Moreover, $progname and $testname can be empty, in which which +# case $path is allowed for all tests. + +/etc/hosts +/proc/\d+/status +/bin/cat: sysinfotest +/bin/dirname: sysinfotest: x86 sysinfo + +# This is just a dummy example, DO NOT USE IT LIKE THAT! +.*: nonexistent-test-touching-everything -- 2.7.3
participants (2)
-
Erik Skultety
-
Michal Privoznik