[libvirt] [PATCH v2 0/3] Introduce big test mock
v2 of: https://www.redhat.com/archives/libvir-list/2016-April/msg01410.html diff to v1: - I've pushed the first patch of the original series - Rebased onto current master (as Jirka's introduction of qemucapsprobe made my patches not applicable). Michal Privoznik (3): 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 | 26 +++- tests/check-file-access.pl | 106 +++++++++++++++ tests/file_access_whitelist.txt | 23 ++++ tests/testutils.c | 34 ++++- tests/testutils.h | 10 +- tests/vircgroupmock.c | 6 +- tests/virpcimock.c | 6 +- tests/virtestmock.c | 279 ++++++++++++++++++++++++++++++++++++++++ 13 files changed, 507 insertions(+), 21 deletions(-) create mode 100755 tests/check-file-access.pl create mode 100644 tests/file_access_whitelist.txt create mode 100644 tests/virtestmock.c -- 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 | 6 +- tests/virpcimock.c | 6 +- tests/virtestmock.c | 180 ++++++++++++++++++++++++++++++++++++++++++++++++++ 7 files changed, 215 insertions(+), 11 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..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.8.1
On Tue, May 10, 2016 at 17:24:12 +0200, 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 | 9 +++ tests/testutils.h | 10 +-- tests/vircgroupmock.c | 6 +- tests/virpcimock.c | 6 +- tests/virtestmock.c | 180 ++++++++++++++++++++++++++++++++++++++++++++++++++ 7 files changed, 215 insertions(+), 11 deletions(-) create mode 100644 tests/virtestmock.c
[...] I'll have to look whether this is really necessary.
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); }
LOAD_SYM_ALT is unused after this. Additionally this could be aggregated into a single header so that every mock library doesn't have to reimplement these helpers.
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);
Same as above in regards of LOAD_SYM_ALT.
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.
2016?
+ * + * 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)
This would too benefit from not having to reimplement these.
+ + 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);
I'd rather do nothing at this place. Leave it empty until you implement it in the next patch.
+ } +}
ACK if you remove contents of checkPath and remove or refactor the symbol extractors. Peter
On 11.05.2016 16:50, Peter Krempa wrote:
On Tue, May 10, 2016 at 17:24:12 +0200, 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 | 9 +++ tests/testutils.h | 10 +-- tests/vircgroupmock.c | 6 +- tests/virpcimock.c | 6 +- tests/virtestmock.c | 180 ++++++++++++++++++++++++++++++++++++++++++++++++++ 7 files changed, 215 insertions(+), 11 deletions(-) create mode 100644 tests/virtestmock.c
[...]
I'll have to look whether this is really necessary.
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); }
LOAD_SYM_ALT is unused after this. Additionally this could be aggregated into a single header so that every mock library doesn't have to reimplement these helpers.
Oh, you mean that LOAD_SYM macro should go somewhere into a header file? Well I can do that, but I'd rather do that in a separate patch, since that would be touching more files than there are in this patch. Morevoer, we have to think twice where to put it because as I was testing this approach, I've encountered the following deadlock: Thread 1 (Thread 0x7f2f61527800 (LWP 31448)): #0 0x00007f2f5dbbbcac in __lll_lock_wait () from /lib64/libpthread.so.0 #1 0x00007f2f5dbb5aa5 in pthread_mutex_lock () from /lib64/libpthread.so.0 #2 0x00007f2f60c20885 in virMutexLock (m=0x7f2f61137920 <virLogMutex>) at util/virthread.c:89 #3 0x00007f2f60be5e2f in virLogLock () at util/virlog.c:143 #4 0x00007f2f60be692e in virLogSourceUpdate (source=0x7f2f611291f0 <virLogSelf>) at util/virlog.c:473 #5 0x00007f2f60be6bae in virLogVMessage (source=0x7f2f611291f0 <virLogSelf>, priority=VIR_LOG_ERROR, filename=0x7f2f60e534da "util/virfile.c", linenr=3157, funcname=0x7f2f60e544a8 <__FUNCTION__.16218> "virFilePrintf", metadata=0x7fffd2f20730, fmt=0x7f2f60e5140a "%s", vargs=0x7fffd2f20560) at util/virlog.c:574 #6 0x00007f2f60be6ad8 in virLogMessage (source=0x7f2f611291f0 <virLogSelf>, priority=VIR_LOG_ERROR, filename=0x7f2f60e534da "util/virfile.c", linenr=3157, funcname=0x7f2f60e544a8 <__FUNCTION__.16218> "virFilePrintf", metadata=0x7fffd2f20730, fmt=0x7f2f60e5140a "%s") at util/virlog.c:519 #7 0x00007f2f60bc68eb in virRaiseErrorLog (filename=0x7f2f60e534da "util/virfile.c", funcname=0x7f2f60e544a8 <__FUNCTION__.16218> "virFilePrintf", linenr=3157, err=0x62bdc0, meta=0x7fffd2f20730) at util/virerror.c:671 #8 0x00007f2f60bc6c46 in virRaiseErrorFull (filename=0x7f2f60e534da "util/virfile.c", funcname=0x7f2f60e544a8 <__FUNCTION__.16218> "virFilePrintf", linenr=3157, domain=0, code=38, level=VIR_ERR_ERROR, str1=0x7f2f60e5140a "%s", str2=0x7fffd2f20d00 "Could not write to stream: Bad file descriptor", str3=0x0, int1=9, int2=-1, fmt=0x7f2f60e5140a "%s") at util/virerror.c:766 #9 0x00007f2f60bc885c in virReportSystemErrorFull (domcode=0, theerrno=9, filename=0x7f2f60e534da "util/virfile.c", funcname=0x7f2f60e544a8 <__FUNCTION__.16218> "virFilePrintf", linenr=3157, fmt=0x7f2f60e5354f "%s") at util/virerror.c:1512 #10 0x00007f2f60bd16e3 in virFilePrintf (fp=0x7f2f5dba7680 <_IO_2_1_stderr_>, msg=0x7f2f6114bb1d "*** %s ***\n") at util/virfile.c:3156 #11 0x00007f2f6113b7c7 in checkPath (path=0x7f2f595a0011 "/etc/hosts") at virtestmock.c:60 #12 0x00007f2f6113b960 in fopen (path=0x7f2f595a0011 "/etc/hosts", mode=0x7f2f5959fff0 "rce") at virtestmock.c:90 #13 0x00007f2f5959b713 in internal_setent () from /lib64/libnss_files.so.2 #14 0x00007f2f5959c107 in _nss_files_gethostbyname4_r () from /lib64/libnss_files.so.2 #15 0x00007f2f5d8dea80 in gaih_inet () from /lib64/libc.so.6 #16 0x00007f2f5d8e0a1c in getaddrinfo () from /lib64/libc.so.6 #17 0x00007f2f60c29f5e in virGetHostnameImpl (quiet=true) at util/virutil.c:703 #18 0x00007f2f60c2a134 in virGetHostnameQuiet () at util/virutil.c:745 #19 0x00007f2f60be6848 in virLogHostnameString (rawmsg=0x7fffd2f21dc8, msg=0x7fffd2f21dd0) at util/virlog.c:449 #20 0x00007f2f60be6d7c in virLogVMessage (source=0x7f2f61129250 <virLogSelf>, priority=VIR_LOG_WARN, filename=0x7f2f60e534da "util/virfile.c", linenr=95, funcname=0x7f2f60e53f70 <__func__.15542> "virFileClose", metadata=0x0, fmt=0x7f2f60e534e9 "Tried to close invalid fd %d", vargs=0x7fffd2f21e70) at util/virlog.c:610 #21 0x00007f2f60be6ad8 in virLogMessage (source=0x7f2f61129250 <virLogSelf>, priority=VIR_LOG_WARN, filename=0x7f2f60e534da "util/virfile.c", linenr=95, funcname=0x7f2f60e53f70 <__func__.15542> "virFileClose", metadata=0x0, fmt=0x7f2f60e534e9 "Tried to close invalid fd %d") at util/virlog.c:519 #22 0x00007f2f60bcb124 in virFileClose (fdptr=0x7fffd2f223b8, flags=VIR_FILE_CLOSE_PRESERVE_ERRNO) at util/virfile.c:95 #23 0x0000000000404bca in virtTestCaptureProgramExecChild (argv=0x7fffd2f224d0, pipefd=5) at testutils.c:373 #24 0x0000000000404ce1 in virtTestCaptureProgramOutput (argv=0x7fffd2f224d0, buf=0x7fffd2f22490, maxlen=4096) at testutils.c:404 #25 0x0000000000402738 in testCompareOutputLit (expectData=0x406948 " Id Name", ' ' <repeats 27 times>, "State\n", '-' <repeats 52 times>, "\n 1 test", ' ' <repeats 27 times>, "running\n\n", filter=0x0, argv=0x7fffd2f224d0) at virshtest.c:68 #26 0x000000000040281f in testCompareListDefault (data=0x0) at virshtest.c:105 #27 0x00000000004045f4 in virtTestRun (title=0x406d14 "virsh list (default)", body=0x4027bf <testCompareListDefault>, data=0x0) at testutils.c:175 #28 0x000000000040307d in mymain () at virshtest.c:261 #29 0x0000000000405f69 in virtTestMain (argc=1, argv=0x7fffd2f227a8, lib=0x0, func=0x402fff <mymain>) at testutils.c:967 #30 0x0000000000404485 in main (argc=1, argv=0x7fffd2f227a8) at virshtest.c:422 Guess what, fprintf is replaced by virFilePrintf(). I mean WAT?!
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);
Same as above in regards of LOAD_SYM_ALT.
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.
2016?
Ooops.
+ * + * 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)
This would too benefit from not having to reimplement these.
+ + 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);
I'd rather do nothing at this place. Leave it empty until you implement it in the next patch.
Okay. Michal
On Thu, May 12, 2016 at 14:33:19 +0200, Michal Privoznik wrote:
On 11.05.2016 16:50, Peter Krempa wrote:
On Tue, May 10, 2016 at 17:24:12 +0200, 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 | 9 +++ tests/testutils.h | 10 +-- tests/vircgroupmock.c | 6 +- tests/virpcimock.c | 6 +- tests/virtestmock.c | 180 ++++++++++++++++++++++++++++++++++++++++++++++++++ 7 files changed, 215 insertions(+), 11 deletions(-) create mode 100644 tests/virtestmock.c
[...]
I'll have to look whether this is really necessary.
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); }
LOAD_SYM_ALT is unused after this. Additionally this could be aggregated into a single header so that every mock library doesn't have to reimplement these helpers.
Yes I meant in a separate patch. All the tests using mocking reimplement those.
Oh, you mean that LOAD_SYM macro should go somewhere into a header file? Well I can do that, but I'd rather do that in a separate patch, since that would be touching more files than there are in this patch. Morevoer, we have to think twice where to put it because as I was testing this approach, I've encountered the following deadlock:
I would opt for a totally separate header file just for the mocking helper code.a Peter
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 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..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 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$$ 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 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 + * 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; } @@ -851,17 +859,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 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.8.1
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
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@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
On Thu, May 12, 2016 at 14:33:08 +0200, Michal Privoznik wrote:
On 11.05.2016 17:15, Peter Krempa wrote:
On Tue, May 10, 2016 at 17:24:13 +0200, Michal Privoznik wrote:
[...]
+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?
In such case it's more than likely that the file will fail to be opened even for the analysis.
+ } + + 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.
Never mind. I re-read the man page for flock and noticed that LOCK_EX whithout LOCK_NB will actually block until the lock is released. [...]
+ +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.
src/util/virstoragefile.c AFAIK
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.
Hmm, fair enough.
+ 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.
Hmm, let's see if it will bite in the future then.
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 | 23 +++++++++ 4 files changed, 145 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 c52a4cb..da07e6c 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 da68f2e..7cd641d 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -451,6 +451,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..850b285 --- /dev/null +++ b/tests/file_access_whitelist.txt @@ -0,0 +1,23 @@ +# 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. + +/bin/cat: sysinfotest +/bin/dirname: sysinfotest: x86 sysinfo +/bin/sleep: commandtest +/bin/true: commandtest +/dev/null +/dev/urandom +/etc/hosts +/proc/\d+/status + +# This is just a dummy example, DO NOT USE IT LIKE THAT! +.*: nonexistent-test-touching-everything -- 2.8.1
On Tue, May 10, 2016 at 17:24:14 +0200, Michal Privoznik wrote:
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 | 23 +++++++++ 4 files changed, 145 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 c52a4cb..da07e6c 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 da68f2e..7cd641d 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -451,6 +451,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
I added '| sort -u' while trying this. There's a lot of multiplicated lines.
+ +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
Will this file ever contain comments?
+ } 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;
perl complains that the above line is useless. Also the script doesn't return failure if it finds files out of the build path.
diff --git a/tests/file_access_whitelist.txt b/tests/file_access_whitelist.txt new file mode 100644 index 0000000..850b285 --- /dev/null +++ b/tests/file_access_whitelist.txt @@ -0,0 +1,23 @@ +# 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. + +/bin/cat: sysinfotest +/bin/dirname: sysinfotest: x86 sysinfo +/bin/sleep: commandtest +/bin/true: commandtest +/dev/null +/dev/urandom +/etc/hosts +/proc/\d+/status
My test run showed that there is a looot of stuff to add unfortunately. Looks good to me, but my perl knowledge is rather poor.
On 12.05.2016 09:09, Peter Krempa wrote:
On Tue, May 10, 2016 at 17:24:14 +0200, Michal Privoznik wrote:
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 | 23 +++++++++ 4 files changed, 145 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 c52a4cb..da07e6c 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 da68f2e..7cd641d 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -451,6 +451,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
I added '| sort -u' while trying this. There's a lot of multiplicated lines.
Okay.
+ +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
Will this file ever contain comments?
I guess not. I can remove it.
+ } 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;
perl complains that the above line is useless. Also the script doesn't return failure if it finds files out of the build path.
diff --git a/tests/file_access_whitelist.txt b/tests/file_access_whitelist.txt new file mode 100644 index 0000000..850b285 --- /dev/null +++ b/tests/file_access_whitelist.txt @@ -0,0 +1,23 @@ +# 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. + +/bin/cat: sysinfotest +/bin/dirname: sysinfotest: x86 sysinfo +/bin/sleep: commandtest +/bin/true: commandtest +/dev/null +/dev/urandom +/etc/hosts +/proc/\d+/status
My test run showed that there is a looot of stuff to add unfortunately.
Looks good to me, but my perl knowledge is rather poor.
Correct. There's still plenty of rules to add. That's why the perl is not returning any error. Not just yet. Michal
participants (2)
-
Michal Privoznik -
Peter Krempa