[libvirt] [PATCH v4 0/8] tests: Introduce global mock library

diff to v3: - Well, I've found out that we have virmock.h that can be used just for what Peter suggested earlier: have LOAD_SYM shared. To my surprise, we already have a macro just for that = VIR_MOCK_REAL_INIT. - As agreed in v3, this feature is yet not enabled by default, but only if a env var is set. In order to do that, I have to write (hackish?) patch 5/8. Michal Privoznik (8): virpcimock: Adapt to virmock.h vircgroupmock: Adapt to virmock.h nssmock: Adapt to virmock.h securityselinuxhelper: Adapt to virmock.h virmock.h: Introduce VIR_MOCK_CALL_STAT tests: Introduce global mock library virtestmock: Print invalid file accesses into a file tests: Introduce check-file-access.pl .gitignore | 1 + HACKING | 10 ++ Makefile.am | 3 + cfg.mk | 2 +- docs/hacking.html.in | 13 +++ tests/Makefile.am | 28 ++++- tests/check-file-access.pl | 104 ++++++++++++++++++ tests/file_access_whitelist.txt | 23 ++++ tests/nssmock.c | 27 ++--- tests/securityselinuxhelper.c | 80 ++++++-------- tests/testutils.c | 30 ++++-- tests/testutils.h | 10 +- tests/vircgroupmock.c | 83 ++++++--------- tests/virmock.h | 33 ++++++ tests/virpcimock.c | 96 +++++++---------- tests/virtestmock.c | 226 ++++++++++++++++++++++++++++++++++++++++ 16 files changed, 583 insertions(+), 186 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

Instead of introducing our own wrapper for dlsym() we can use the one provided by virmock.h. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virmock.h | 10 ++++++ tests/virpcimock.c | 97 ++++++++++++++++++++++-------------------------------- 2 files changed, 50 insertions(+), 57 deletions(-) diff --git a/tests/virmock.h b/tests/virmock.h index a69c836..62a7c8f 100644 --- a/tests/virmock.h +++ b/tests/virmock.h @@ -292,4 +292,14 @@ } \ } while (0) +# define VIR_MOCK_REAL_INIT_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) + #endif /* __VIR_MOCK_H__ */ diff --git a/tests/virpcimock.c b/tests/virpcimock.c index cfe42a6..c7786e5 100644 --- a/tests/virpcimock.c +++ b/tests/virpcimock.c @@ -21,10 +21,7 @@ #include <config.h> #ifdef __linux__ -# include "internal.h" -# include <stdio.h> -# include <dlfcn.h> -# include <stdlib.h> +# include "virmock.h" # include <unistd.h> # include <fcntl.h> # include <sys/stat.h> @@ -35,15 +32,15 @@ # include "virfile.h" # include "dirname.h" -static int (*realaccess)(const char *path, int mode); -static int (*reallstat)(const char *path, struct stat *sb); -static int (*real__lxstat)(int ver, const char *path, struct stat *sb); -static int (*realstat)(const char *path, struct stat *sb); -static int (*real__xstat)(int ver, const char *path, struct stat *sb); -static char *(*realcanonicalize_file_name)(const char *path); -static int (*realopen)(const char *path, int flags, ...); -static int (*realclose)(int fd); -static DIR * (*realopendir)(const char *name); +static int (*real_access)(const char *path, int mode); +static int (*real_lstat)(const char *path, struct stat *sb); +static int (*real___lxstat)(int ver, const char *path, struct stat *sb); +static int (*real_stat)(const char *path, struct stat *sb); +static int (*real___xstat)(int ver, const char *path, struct stat *sb); +static char *(*real_canonicalize_file_name)(const char *path); +static int (*real_open)(const char *path, int flags, ...); +static int (*real_close)(int fd); +static DIR * (*real_opendir)(const char *name); /* Don't make static, since it causes problems with clang * when passed as an arg to virAsprintf() @@ -181,7 +178,7 @@ make_file(const char *path, if (virAsprintfQuiet(&filepath, "%s/%s", path, name) < 0) ABORT_OOM(); - if ((fd = realopen(filepath, O_CREAT|O_WRONLY, 0666)) < 0) + if ((fd = real_open(filepath, O_CREAT|O_WRONLY, 0666)) < 0) ABORT("Unable to open: %s", filepath); if (value && safewrite(fd, value, len) != len) @@ -207,7 +204,7 @@ pci_read_file(const char *path, goto cleanup; } - if ((fd = realopen(newpath, O_RDWR)) < 0) + if ((fd = real_open(newpath, O_RDWR)) < 0) goto cleanup; bzero(buf, buf_size); @@ -222,7 +219,7 @@ pci_read_file(const char *path, ret = 0; cleanup: VIR_FREE(newpath); - realclose(fd); + real_close(fd); return ret; } @@ -354,8 +351,8 @@ pci_device_new_from_stub(const struct pciDevice *data) /* If there is a config file for the device within virpcitestdata dir, * symlink it. Otherwise create a dummy config file. */ - if ((realstat && realstat(configSrc, &sb) == 0) || - (real__xstat && real__xstat(_STAT_VER, configSrc, &sb) == 0)) { + if ((real_stat && real_stat(configSrc, &sb) == 0) || + (real___xstat && real___xstat(_STAT_VER, configSrc, &sb) == 0)) { /* On success, copy @configSrc into the destination (a copy, * rather than a symlink, is required since we write into the * file, and parallel VPATH builds must not stomp on the @@ -772,30 +769,16 @@ pci_driver_handle_remove_id(const char *path) static void init_syms(void) { - if (realaccess) + if (real_access) return; -# define LOAD_SYM(name) \ - do { \ - if (!(real ## name = dlsym(RTLD_NEXT, #name))) \ - ABORT("Cannot find real '%s' symbol\n", #name); \ - } while (0) - -# define LOAD_SYM_ALT(name1, name2) \ - do { \ - if (!(real ## name1 = dlsym(RTLD_NEXT, #name1)) && \ - !(real ## name2 = dlsym(RTLD_NEXT, #name2))) \ - ABORT("Cannot find real '%s' or '%s' symbol\n", \ - #name1, #name2); \ - } while (0) - - LOAD_SYM(access); - LOAD_SYM_ALT(lstat, __lxstat); - LOAD_SYM_ALT(stat, __xstat); - LOAD_SYM(canonicalize_file_name); - LOAD_SYM(open); - LOAD_SYM(close); - LOAD_SYM(opendir); + VIR_MOCK_REAL_INIT(access); + VIR_MOCK_REAL_INIT_ALT(lstat, __lxstat); + VIR_MOCK_REAL_INIT_ALT(stat, __xstat); + VIR_MOCK_REAL_INIT(canonicalize_file_name); + VIR_MOCK_REAL_INIT(open); + VIR_MOCK_REAL_INIT(close); + VIR_MOCK_REAL_INIT(opendir); } static void @@ -865,10 +848,10 @@ access(const char *path, int mode) char *newpath; if (getrealpath(&newpath, path) < 0) return -1; - ret = realaccess(newpath, mode); + ret = real_access(newpath, mode); VIR_FREE(newpath); } else { - ret = realaccess(path, mode); + ret = real_access(path, mode); } return ret; } @@ -884,10 +867,10 @@ __lxstat(int ver, const char *path, struct stat *sb) char *newpath; if (getrealpath(&newpath, path) < 0) return -1; - ret = real__lxstat(ver, newpath, sb); + ret = real___lxstat(ver, newpath, sb); VIR_FREE(newpath); } else { - ret = real__lxstat(ver, path, sb); + ret = real___lxstat(ver, path, sb); } return ret; } @@ -903,10 +886,10 @@ lstat(const char *path, struct stat *sb) char *newpath; if (getrealpath(&newpath, path) < 0) return -1; - ret = reallstat(newpath, sb); + ret = real_lstat(newpath, sb); VIR_FREE(newpath); } else { - ret = reallstat(path, sb); + ret = real_lstat(path, sb); } return ret; } @@ -922,10 +905,10 @@ __xstat(int ver, const char *path, struct stat *sb) char *newpath; if (getrealpath(&newpath, path) < 0) return -1; - ret = real__xstat(ver, newpath, sb); + ret = real___xstat(ver, newpath, sb); VIR_FREE(newpath); } else { - ret = real__xstat(ver, path, sb); + ret = real___xstat(ver, path, sb); } return ret; } @@ -941,10 +924,10 @@ stat(const char *path, struct stat *sb) char *newpath; if (getrealpath(&newpath, path) < 0) return -1; - ret = realstat(newpath, sb); + ret = real_stat(newpath, sb); VIR_FREE(newpath); } else { - ret = realstat(path, sb); + ret = real_stat(path, sb); } return ret; } @@ -960,10 +943,10 @@ canonicalize_file_name(const char *path) char *newpath; if (getrealpath(&newpath, path) < 0) return NULL; - ret = realcanonicalize_file_name(newpath); + ret = real_canonicalize_file_name(newpath); VIR_FREE(newpath); } else { - ret = realcanonicalize_file_name(path); + ret = real_canonicalize_file_name(path); } return ret; } @@ -986,16 +969,16 @@ open(const char *path, int flags, ...) va_start(ap, flags); mode = va_arg(ap, mode_t); va_end(ap); - ret = realopen(newpath ? newpath : path, flags, mode); + ret = real_open(newpath ? newpath : path, flags, mode); } else { - ret = realopen(newpath ? newpath : path, flags); + ret = real_open(newpath ? newpath : path, flags); } /* Catch both: /sys/bus/pci/drivers/... and * /sys/bus/pci/device/.../driver/... */ if (ret >= 0 && STRPREFIX(path, SYSFS_PCI_PREFIX) && strstr(path, "driver") && add_fd(ret, path) < 0) { - realclose(ret); + real_close(ret); ret = -1; } @@ -1015,7 +998,7 @@ opendir(const char *path) getrealpath(&newpath, path) < 0) return NULL; - ret = realopendir(newpath ? newpath : path); + ret = real_opendir(newpath ? newpath : path); VIR_FREE(newpath); return ret; @@ -1026,7 +1009,7 @@ close(int fd) { if (remove_fd(fd) < 0) return -1; - return realclose(fd); + return real_close(fd); } #else /* Nothing to override on non-__linux__ platforms */ -- 2.8.1

On Fri, May 13, 2016 at 14:32:02 +0200, Michal Privoznik wrote:
Instead of introducing our own wrapper for dlsym() we can use the one provided by virmock.h.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virmock.h | 10 ++++++ tests/virpcimock.c | 97 ++++++++++++++++++++++-------------------------------- 2 files changed, 50 insertions(+), 57 deletions(-)
[...]
diff --git a/tests/virpcimock.c b/tests/virpcimock.c index cfe42a6..c7786e5 100644 --- a/tests/virpcimock.c +++ b/tests/virpcimock.c @@ -21,10 +21,7 @@ #include <config.h>
#ifdef __linux__ -# include "internal.h" -# include <stdio.h> -# include <dlfcn.h> -# include <stdlib.h>
stdio.h (fprintf) and stdlib.h (abort) are still used by this file so you probably should not remove the headers.
+# include "virmock.h" # include <unistd.h> # include <fcntl.h> # include <sys/stat.h>
ACK to the rest.

Instead of introducing our own wrapper for dlsym() we can use the one provided by virmock.h. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/vircgroupmock.c | 83 +++++++++++++++++++-------------------------------- 1 file changed, 31 insertions(+), 52 deletions(-) diff --git a/tests/vircgroupmock.c b/tests/vircgroupmock.c index cfc51e8..10fe24d 100644 --- a/tests/vircgroupmock.c +++ b/tests/vircgroupmock.c @@ -21,11 +21,7 @@ #include <config.h> #ifdef __linux__ -# include "internal.h" - -# include <stdio.h> -# include <dlfcn.h> -# include <stdlib.h> +# include "virmock.h" # include <unistd.h> # include <fcntl.h> # include <sys/stat.h> @@ -41,14 +37,14 @@ # include "virstring.h" # include "virfile.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 int (*realmkdir)(const char *path, mode_t mode); +static int (*real_open)(const char *path, int flags, ...); +static FILE *(*real_fopen)(const char *path, const char *mode); +static int (*real_access)(const char *path, int mode); +static int (*real_stat)(const char *path, struct stat *sb); +static int (*real___xstat)(int ver, const char *path, struct stat *sb); +static int (*real_lstat)(const char *path, struct stat *sb); +static int (*real___lxstat)(int ver, const char *path, struct stat *sb); +static int (*real_mkdir)(const char *path, mode_t mode); /* Don't make static, since it causes problems with clang * when passed as an arg to asprintf() @@ -173,7 +169,7 @@ static int make_file(const char *path, if (asprintf(&filepath, "%s/%s", path, name) < 0) return -1; - if ((fd = realopen(filepath, O_CREAT|O_WRONLY, 0600)) < 0) + if ((fd = real_open(filepath, O_CREAT|O_WRONLY, 0600)) < 0) goto cleanup; if (write(fd, value, strlen(value)) != strlen(value)) @@ -204,7 +200,7 @@ static int make_controller(const char *path, mode_t mode) if (STREQ(controller, "cpuacct")) return symlink("cpu,cpuacct", path); - if (realmkdir(path, mode) < 0) + if (real_mkdir(path, mode) < 0) goto cleanup; # define MAKE_FILE(name, value) \ @@ -394,32 +390,15 @@ static int make_controller(const char *path, mode_t mode) static void init_syms(void) { - if (realfopen) + if (real_fopen) 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(fopen); - LOAD_SYM(access); - LOAD_SYM_ALT(lstat, __lxstat); - LOAD_SYM_ALT(stat, __xstat); - LOAD_SYM(mkdir); - LOAD_SYM(open); + VIR_MOCK_REAL_INIT(fopen); + VIR_MOCK_REAL_INIT(access); + VIR_MOCK_REAL_INIT_ALT(lstat, __lxstat); + VIR_MOCK_REAL_INIT_ALT(stat, __xstat); + VIR_MOCK_REAL_INIT(mkdir); + VIR_MOCK_REAL_INIT(open); } static void init_sysfs(void) @@ -527,7 +506,7 @@ FILE *fopen(const char *path, const char *mode) } } - return realfopen(path, mode); + return real_fopen(path, mode); } int access(const char *path, int mode) @@ -545,7 +524,7 @@ int access(const char *path, int mode) errno = ENOMEM; return -1; } - ret = realaccess(newpath, mode); + ret = real_access(newpath, mode); free(newpath); } else if (STREQ(path, "/proc/cgroups") || STREQ(path, "/proc/self/cgroup") || @@ -557,7 +536,7 @@ int access(const char *path, int mode) * a symlink to /proc/self/mounts. */ ret = 0; } else { - ret = realaccess(path, mode); + ret = real_access(path, mode); } return ret; } @@ -577,7 +556,7 @@ int __lxstat(int ver, const char *path, struct stat *sb) errno = ENOMEM; return -1; } - ret = real__lxstat(ver, newpath, sb); + ret = real___lxstat(ver, newpath, sb); free(newpath); } else if (STRPREFIX(path, fakedevicedir0)) { sb->st_mode = S_IFBLK; @@ -588,7 +567,7 @@ int __lxstat(int ver, const char *path, struct stat *sb) sb->st_rdev = makedev(9, 0); return 0; } else { - ret = real__lxstat(ver, path, sb); + ret = real___lxstat(ver, path, sb); } return ret; } @@ -608,7 +587,7 @@ int lstat(const char *path, struct stat *sb) errno = ENOMEM; return -1; } - ret = reallstat(newpath, sb); + ret = real_lstat(newpath, sb); free(newpath); } else if (STRPREFIX(path, fakedevicedir0)) { sb->st_mode = S_IFBLK; @@ -619,7 +598,7 @@ int lstat(const char *path, struct stat *sb) sb->st_rdev = makedev(9, 0); return 0; } else { - ret = reallstat(path, sb); + ret = real_lstat(path, sb); } return ret; } @@ -639,7 +618,7 @@ int __xstat(int ver, const char *path, struct stat *sb) errno = ENOMEM; return -1; } - ret = real__xstat(ver, newpath, sb); + ret = real___xstat(ver, newpath, sb); free(newpath); } else if (STRPREFIX(path, fakedevicedir0)) { sb->st_mode = S_IFBLK; @@ -650,7 +629,7 @@ int __xstat(int ver, const char *path, struct stat *sb) sb->st_rdev = makedev(9, 0); return 0; } else { - ret = real__xstat(ver, path, sb); + ret = real___xstat(ver, path, sb); } return ret; } @@ -690,7 +669,7 @@ int stat(const char *path, struct stat *sb) if (!(newpath = strdup(path))) return -1; } - ret = realstat(newpath, sb); + ret = real_stat(newpath, sb); free(newpath); return ret; } @@ -713,7 +692,7 @@ int mkdir(const char *path, mode_t mode) ret = make_controller(newpath, mode); free(newpath); } else { - ret = realmkdir(path, mode); + ret = real_mkdir(path, mode); } return ret; } @@ -750,9 +729,9 @@ int open(const char *path, int flags, ...) va_start(ap, flags); mode = va_arg(ap, mode_t); va_end(ap); - ret = realopen(newpath ? newpath : path, flags, mode); + ret = real_open(newpath ? newpath : path, flags, mode); } else { - ret = realopen(newpath ? newpath : path, flags); + ret = real_open(newpath ? newpath : path, flags); } free(newpath); return ret; -- 2.8.1

On Fri, May 13, 2016 at 14:32:03 +0200, Michal Privoznik wrote:
Instead of introducing our own wrapper for dlsym() we can use the one provided by virmock.h.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/vircgroupmock.c | 83 +++++++++++++++++++-------------------------------- 1 file changed, 31 insertions(+), 52 deletions(-)
diff --git a/tests/vircgroupmock.c b/tests/vircgroupmock.c index cfc51e8..10fe24d 100644 --- a/tests/vircgroupmock.c +++ b/tests/vircgroupmock.c @@ -21,11 +21,7 @@ #include <config.h>
#ifdef __linux__ -# include "internal.h" - -# include <stdio.h> -# include <dlfcn.h> -# include <stdlib.h> +# include "virmock.h"
Same comment as in previous patch. ACK

Instead of introducing our own wrapper for dlsym() we can use the one provided by virmock.h. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/nssmock.c | 27 +++++++++------------------ 1 file changed, 9 insertions(+), 18 deletions(-) diff --git a/tests/nssmock.c b/tests/nssmock.c index 31b1177..ee1d948 100644 --- a/tests/nssmock.c +++ b/tests/nssmock.c @@ -21,21 +21,18 @@ #include <config.h> #ifdef NSS -# include <stdio.h> -# include <stdlib.h> -# include <dlfcn.h> +# include "virmock.h" # include <sys/types.h> # include <dirent.h> # include <sys/stat.h> # include <fcntl.h> # include "configmake.h" -# include "internal.h" # include "virstring.h" # include "viralloc.h" -static int (*realopen)(const char *path, int flags, ...); -static DIR * (*realopendir)(const char *name); +static int (*real_open)(const char *path, int flags, ...); +static DIR * (*real_opendir)(const char *name); # define LEASEDIR LOCALSTATEDIR "/lib/libvirt/dnsmasq/" @@ -59,17 +56,11 @@ static DIR * (*realopendir)(const char *name); static void init_syms(void) { - if (realopen) + if (real_open) return; -# define LOAD_SYM(name) \ - do { \ - if (!(real ## name = dlsym(RTLD_NEXT, #name))) \ - ABORT("Cannot find real '%s' symbol\n", #name); \ - } while (0) - - LOAD_SYM(open); - LOAD_SYM(opendir); + VIR_MOCK_REAL_INIT(open); + VIR_MOCK_REAL_INIT(opendir); } static int @@ -109,9 +100,9 @@ open(const char *path, int flags, ...) va_start(ap, flags); mode = va_arg(ap, int); va_end(ap); - ret = realopen(newpath ? newpath : path, flags, mode); + ret = real_open(newpath ? newpath : path, flags, mode); } else { - ret = realopen(newpath ? newpath : path, flags); + ret = real_open(newpath ? newpath : path, flags); } VIR_FREE(newpath); @@ -130,7 +121,7 @@ opendir(const char *path) getrealpath(&newpath, path) < 0) return NULL; - ret = realopendir(newpath ? newpath : path); + ret = real_opendir(newpath ? newpath : path); VIR_FREE(newpath); return ret; -- 2.8.1

On Fri, May 13, 2016 at 14:32:04 +0200, Michal Privoznik wrote:
Instead of introducing our own wrapper for dlsym() we can use the one provided by virmock.h.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/nssmock.c | 27 +++++++++------------------ 1 file changed, 9 insertions(+), 18 deletions(-)
diff --git a/tests/nssmock.c b/tests/nssmock.c index 31b1177..ee1d948 100644 --- a/tests/nssmock.c +++ b/tests/nssmock.c @@ -21,21 +21,18 @@ #include <config.h>
#ifdef NSS -# include <stdio.h> -# include <stdlib.h> -# include <dlfcn.h> +# include "virmock.h"
This one is good, but ...
# include <sys/types.h> # include <dirent.h> # include <sys/stat.h> # include <fcntl.h>
# include "configmake.h" -# include "internal.h" # include "virstring.h" # include "viralloc.h"
-static int (*realopen)(const char *path, int flags, ...); -static DIR * (*realopendir)(const char *name); +static int (*real_open)(const char *path, int flags, ...); +static DIR * (*real_opendir)(const char *name);
# define LEASEDIR LOCALSTATEDIR "/lib/libvirt/dnsmasq/"
@@ -59,17 +56,11 @@ static DIR * (*realopendir)(const char *name); static void init_syms(void) { - if (realopen) + if (real_open) return;
-# define LOAD_SYM(name) \ - do { \ - if (!(real ## name = dlsym(RTLD_NEXT, #name))) \ - ABORT("Cannot find real '%s' symbol\n", #name); \ - } while (0)
With this you can also kill ABORT_OOM, ABORT and STDERR macros. ACK ^^

Instead of introducing our own wrapper for dlsym() we can use the one provided by virmock.h. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/securityselinuxhelper.c | 80 ++++++++++++++++++------------------------- 1 file changed, 34 insertions(+), 46 deletions(-) diff --git a/tests/securityselinuxhelper.c b/tests/securityselinuxhelper.c index 2cfa43f..22b6709 100644 --- a/tests/securityselinuxhelper.c +++ b/tests/securityselinuxhelper.c @@ -22,7 +22,7 @@ /* This file is only compiled on Linux, and only if xattr support was * detected. */ -#include <dlfcn.h> +#include "virmock.h" #include <errno.h> #if HAVE_LINUX_MAGIC_H # include <linux/magic.h> @@ -31,8 +31,6 @@ #if HAVE_SELINUX_LABEL_H # include <selinux/label.h> #endif -#include <stdio.h> -#include <stdlib.h> #include <string.h> #include <sys/vfs.h> #include <unistd.h> @@ -47,24 +45,24 @@ #include "viralloc.h" #include "virstring.h" -static int (*realstatfs)(const char *path, struct statfs *buf); -static int (*realsecurity_get_boolean_active)(const char *name); -static int (*realis_selinux_enabled)(void); +static int (*real_statfs)(const char *path, struct statfs *buf); +static int (*real_security_get_boolean_active)(const char *name); +static int (*real_is_selinux_enabled)(void); -static const char *(*realselinux_virtual_domain_context_path)(void); -static const char *(*realselinux_virtual_image_context_path)(void); +static const char *(*real_selinux_virtual_domain_context_path)(void); +static const char *(*real_selinux_virtual_image_context_path)(void); #ifdef HAVE_SELINUX_LXC_CONTEXTS_PATH -static const char *(*realselinux_lxc_contexts_path)(void); +static const char *(*real_selinux_lxc_contexts_path)(void); #endif #if HAVE_SELINUX_LABEL_H -static struct selabel_handle *(*realselabel_open)(unsigned int backend, +static struct selabel_handle *(*real_selabel_open)(unsigned int backend, VIR_SELINUX_OPEN_CONST struct selinux_opt *opts, unsigned nopts); -static void (*realselabel_close)(struct selabel_handle *handle); -static int (*realselabel_lookup_raw)(struct selabel_handle *handle, +static void (*real_selabel_close)(struct selabel_handle *handle); +static int (*real_selabel_lookup_raw)(struct selabel_handle *handle, security_context_t *con, const char *key, int type); @@ -72,35 +70,25 @@ static int (*realselabel_lookup_raw)(struct selabel_handle *handle, static void init_syms(void) { - if (realstatfs) + if (real_statfs) 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) + VIR_MOCK_REAL_INIT(statfs); + VIR_MOCK_REAL_INIT(security_get_boolean_active); + VIR_MOCK_REAL_INIT(is_selinux_enabled); - LOAD_SYM(statfs); - LOAD_SYM(security_get_boolean_active); - LOAD_SYM(is_selinux_enabled); - - LOAD_SYM(selinux_virtual_domain_context_path); - LOAD_SYM(selinux_virtual_image_context_path); + VIR_MOCK_REAL_INIT(selinux_virtual_domain_context_path); + VIR_MOCK_REAL_INIT(selinux_virtual_image_context_path); #ifdef HAVE_SELINUX_LXC_CONTEXTS_PATH - LOAD_SYM(selinux_lxc_contexts_path); + VIR_MOCK_REAL_INIT(selinux_lxc_contexts_path); #endif #if HAVE_SELINUX_LABEL_H - LOAD_SYM(selabel_open); - LOAD_SYM(selabel_close); - LOAD_SYM(selabel_lookup_raw); + VIR_MOCK_REAL_INIT(selabel_open); + VIR_MOCK_REAL_INIT(selabel_close); + VIR_MOCK_REAL_INIT(selabel_lookup_raw); #endif - -#undef LOAD_SYM } @@ -224,7 +212,7 @@ int statfs(const char *path, struct statfs *buf) init_syms(); - ret = realstatfs(path, buf); + ret = real_statfs(path, buf); if (!ret && STREQ(path, abs_builddir "/securityselinuxlabeldata/nfs")) buf->f_type = NFS_SUPER_MAGIC; return ret; @@ -269,15 +257,15 @@ int security_get_boolean_active(const char *name) return 0; init_syms(); - return realsecurity_get_boolean_active(name); + return real_security_get_boolean_active(name); } const char *selinux_virtual_domain_context_path(void) { init_syms(); - if (realis_selinux_enabled()) - return realselinux_virtual_domain_context_path(); + if (real_is_selinux_enabled()) + return real_selinux_virtual_domain_context_path(); return abs_srcdir "/securityselinuxhelperdata/virtual_domain_context"; } @@ -286,8 +274,8 @@ const char *selinux_virtual_image_context_path(void) { init_syms(); - if (realis_selinux_enabled()) - return realselinux_virtual_image_context_path(); + if (real_is_selinux_enabled()) + return real_selinux_virtual_image_context_path(); return abs_srcdir "/securityselinuxhelperdata/virtual_image_context"; } @@ -297,8 +285,8 @@ const char *selinux_lxc_contexts_path(void) { init_syms(); - if (realis_selinux_enabled()) - return realselinux_lxc_contexts_path(); + if (real_is_selinux_enabled()) + return real_selinux_lxc_contexts_path(); return abs_srcdir "/securityselinuxhelperdata/lxc_contexts"; } @@ -314,8 +302,8 @@ selabel_open(unsigned int backend, init_syms(); - if (realis_selinux_enabled()) - return realselabel_open(backend, opts, nopts); + if (real_is_selinux_enabled()) + return real_selabel_open(backend, opts, nopts); /* struct selabel_handle is opaque; fake it */ if (VIR_ALLOC(fake_handle) < 0) @@ -327,8 +315,8 @@ void selabel_close(struct selabel_handle *handle) { init_syms(); - if (realis_selinux_enabled()) - return realselabel_close(handle); + if (real_is_selinux_enabled()) + return real_selabel_close(handle); VIR_FREE(handle); } @@ -340,8 +328,8 @@ int selabel_lookup_raw(struct selabel_handle *handle, { init_syms(); - if (realis_selinux_enabled()) - return realselabel_lookup_raw(handle, con, key, type); + if (real_is_selinux_enabled()) + return real_selabel_lookup_raw(handle, con, key, type); /* Unimplemented */ errno = ENOENT; -- 2.8.1

On Fri, May 13, 2016 at 14:32:05 +0200, Michal Privoznik wrote:
Instead of introducing our own wrapper for dlsym() we can use the one provided by virmock.h.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/securityselinuxhelper.c | 80 ++++++++++++++++++------------------------- 1 file changed, 34 insertions(+), 46 deletions(-)
ACK

There is some magic going on when it comes to stat() or lstat(). Basically, stat() can either be a regular function, an inline function that calls __xstat(_STAT_VER, ...) or a macro that does the same as the inline func. Don't ask why is that, just read the documentation in sys/stat.h and make sure you have a bucket next to you. Anyway, currently there will not be both stat and __xstat symbols at the same time, as one of them gets overwritten to the other one during compilation. But this is not true anymore once we start chaining our mocking libraries. Therefore we need a wrapper that calls desired function from glibc. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/vircgroupmock.c | 14 +++++++------- tests/virmock.h | 23 +++++++++++++++++++++++ tests/virpcimock.c | 19 +++++++++---------- 3 files changed, 39 insertions(+), 17 deletions(-) diff --git a/tests/vircgroupmock.c b/tests/vircgroupmock.c index 10fe24d..287bd46 100644 --- a/tests/vircgroupmock.c +++ b/tests/vircgroupmock.c @@ -556,7 +556,7 @@ int __lxstat(int ver, const char *path, struct stat *sb) errno = ENOMEM; return -1; } - ret = real___lxstat(ver, newpath, sb); + ret = VIR_MOCK_CALL_LSTAT(ver, newpath, sb); free(newpath); } else if (STRPREFIX(path, fakedevicedir0)) { sb->st_mode = S_IFBLK; @@ -567,7 +567,7 @@ int __lxstat(int ver, const char *path, struct stat *sb) sb->st_rdev = makedev(9, 0); return 0; } else { - ret = real___lxstat(ver, path, sb); + ret = VIR_MOCK_CALL_LSTAT(ver, path, sb); } return ret; } @@ -587,7 +587,7 @@ int lstat(const char *path, struct stat *sb) errno = ENOMEM; return -1; } - ret = real_lstat(newpath, sb); + ret = VIR_MOCK_CALL_LSTAT(_STAT_VER, newpath, sb); free(newpath); } else if (STRPREFIX(path, fakedevicedir0)) { sb->st_mode = S_IFBLK; @@ -598,7 +598,7 @@ int lstat(const char *path, struct stat *sb) sb->st_rdev = makedev(9, 0); return 0; } else { - ret = real_lstat(path, sb); + ret = VIR_MOCK_CALL_LSTAT(_STAT_VER, path, sb); } return ret; } @@ -618,7 +618,7 @@ int __xstat(int ver, const char *path, struct stat *sb) errno = ENOMEM; return -1; } - ret = real___xstat(ver, newpath, sb); + ret = VIR_MOCK_CALL_STAT(ver, newpath, sb); free(newpath); } else if (STRPREFIX(path, fakedevicedir0)) { sb->st_mode = S_IFBLK; @@ -629,7 +629,7 @@ int __xstat(int ver, const char *path, struct stat *sb) sb->st_rdev = makedev(9, 0); return 0; } else { - ret = real___xstat(ver, path, sb); + ret = VIR_MOCK_CALL_STAT(ver, path, sb); } return ret; } @@ -669,7 +669,7 @@ int stat(const char *path, struct stat *sb) if (!(newpath = strdup(path))) return -1; } - ret = real_stat(newpath, sb); + ret = VIR_MOCK_CALL_STAT(_STAT_VER, newpath, sb); free(newpath); return ret; } diff --git a/tests/virmock.h b/tests/virmock.h index 62a7c8f..27c03ba 100644 --- a/tests/virmock.h +++ b/tests/virmock.h @@ -27,6 +27,7 @@ # endif # include <stdlib.h> # include <stdio.h> +# include <sys/stat.h> # include "internal.h" @@ -254,6 +255,28 @@ static void (*real_##name)(void); \ void name(void) +static inline int +callStat(int (*realStat)(const char *, struct stat *), + int (*realXstat)(int, const char *, struct stat *), + int __ver, const char *__filename, struct stat *__stat_buf) +{ + if (!realXstat) { + if (__ver == _STAT_VER) { + return realStat(__filename, __stat_buf); + } else { + fprintf(stderr, "Not handled __xstat(ver=%d)", __ver); + abort(); + } + } + + return realXstat(__ver, __filename, __stat_buf); +} + +# define VIR_MOCK_CALL_STAT(__ver, __filename, __stat_buf) \ + callStat(real_stat, real___xstat, __ver, __filename, __stat_buf) +# define VIR_MOCK_CALL_LSTAT(__ver, __filename, __stat_buf) \ + callStat(real_lstat, real___lxstat, __ver, __filename, __stat_buf) + /* * The VIR_MOCK_WRAP_NNN_MMM() macros are intended for use in the * individual test suites. The define a stub implementation of diff --git a/tests/virpcimock.c b/tests/virpcimock.c index c7786e5..f8a9fac 100644 --- a/tests/virpcimock.c +++ b/tests/virpcimock.c @@ -351,8 +351,7 @@ pci_device_new_from_stub(const struct pciDevice *data) /* If there is a config file for the device within virpcitestdata dir, * symlink it. Otherwise create a dummy config file. */ - if ((real_stat && real_stat(configSrc, &sb) == 0) || - (real___xstat && real___xstat(_STAT_VER, configSrc, &sb) == 0)) { + if (VIR_MOCK_CALL_STAT(_STAT_VER, configSrc, &sb) == 0) { /* On success, copy @configSrc into the destination (a copy, * rather than a symlink, is required since we write into the * file, and parallel VPATH builds must not stomp on the @@ -867,10 +866,10 @@ __lxstat(int ver, const char *path, struct stat *sb) char *newpath; if (getrealpath(&newpath, path) < 0) return -1; - ret = real___lxstat(ver, newpath, sb); + ret = VIR_MOCK_CALL_LSTAT(ver, newpath, sb); VIR_FREE(newpath); } else { - ret = real___lxstat(ver, path, sb); + ret = VIR_MOCK_CALL_LSTAT(ver, path, sb); } return ret; } @@ -886,10 +885,10 @@ lstat(const char *path, struct stat *sb) char *newpath; if (getrealpath(&newpath, path) < 0) return -1; - ret = real_lstat(newpath, sb); + ret = VIR_MOCK_CALL_LSTAT(_STAT_VER, newpath, sb); VIR_FREE(newpath); } else { - ret = real_lstat(path, sb); + ret = VIR_MOCK_CALL_LSTAT(_STAT_VER, path, sb); } return ret; } @@ -905,10 +904,10 @@ __xstat(int ver, const char *path, struct stat *sb) char *newpath; if (getrealpath(&newpath, path) < 0) return -1; - ret = real___xstat(ver, newpath, sb); + ret = VIR_MOCK_CALL_STAT(ver, newpath, sb); VIR_FREE(newpath); } else { - ret = real___xstat(ver, path, sb); + ret = VIR_MOCK_CALL_STAT(ver, path, sb); } return ret; } @@ -924,10 +923,10 @@ stat(const char *path, struct stat *sb) char *newpath; if (getrealpath(&newpath, path) < 0) return -1; - ret = real_stat(newpath, sb); + ret = VIR_MOCK_CALL_STAT(_STAT_VER, newpath, sb); VIR_FREE(newpath); } else { - ret = real_stat(path, sb); + ret = VIR_MOCK_CALL_STAT(_STAT_VER, path, sb); } return ret; } -- 2.8.1

On Fri, May 13, 2016 at 14:32:06 +0200, Michal Privoznik wrote:
There is some magic going on when it comes to stat() or lstat(). Basically, stat() can either be a regular function, an inline function that calls __xstat(_STAT_VER, ...) or a macro that does the same as the inline func. Don't ask why is that, just read the documentation in sys/stat.h and make sure you have a bucket next to you. Anyway, currently there will not be both stat and __xstat symbols at the same time, as one of them gets overwritten to the other one during compilation. But this is not true anymore once we start chaining our mocking libraries. Therefore we need a wrapper that calls desired function from glibc.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/vircgroupmock.c | 14 +++++++------- tests/virmock.h | 23 +++++++++++++++++++++++ tests/virpcimock.c | 19 +++++++++---------- 3 files changed, 39 insertions(+), 17 deletions(-)
[...]
diff --git a/tests/virmock.h b/tests/virmock.h index 62a7c8f..27c03ba 100644 --- a/tests/virmock.h +++ b/tests/virmock.h @@ -254,6 +255,28 @@ static void (*real_##name)(void); \ void name(void)
+static inline int +callStat(int (*realStat)(const char *, struct stat *), + int (*realXstat)(int, const char *, struct stat *),
This is a bit ugly, but it's in the mock code and I don't know of a better option. ACK

Michal Privoznik wrote:
There is some magic going on when it comes to stat() or lstat(). Basically, stat() can either be a regular function, an inline function that calls __xstat(_STAT_VER, ...) or a macro that does the same as the inline func. Don't ask why is that, just read the documentation in sys/stat.h and make sure you have a bucket next to you. Anyway, currently there will not be both stat and __xstat symbols at the same time, as one of them gets overwritten to the other one during compilation. But this is not true anymore once we start chaining our mocking libraries. Therefore we need a wrapper that calls desired function from glibc.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/vircgroupmock.c | 14 +++++++------- tests/virmock.h | 23 +++++++++++++++++++++++ tests/virpcimock.c | 19 +++++++++---------- 3 files changed, 39 insertions(+), 17 deletions(-)
...
diff --git a/tests/virmock.h b/tests/virmock.h index 62a7c8f..27c03ba 100644 --- a/tests/virmock.h +++ b/tests/virmock.h @@ -27,6 +27,7 @@ # endif # include <stdlib.h> # include <stdio.h> +# include <sys/stat.h>
# include "internal.h"
@@ -254,6 +255,28 @@ static void (*real_##name)(void); \ void name(void)
+static inline int +callStat(int (*realStat)(const char *, struct stat *), + int (*realXstat)(int, const char *, struct stat *), + int __ver, const char *__filename, struct stat *__stat_buf) +{ + if (!realXstat) { + if (__ver == _STAT_VER) { + return realStat(__filename, __stat_buf); + } else { + fprintf(stderr, "Not handled __xstat(ver=%d)", __ver); + abort(); + } + } + + return realXstat(__ver, __filename, __stat_buf); +}
Hm, this fails on FreeBSD with: CC nssmock_la-nssmock.lo In file included from nssmock.c:24: ./virmock.h:264:22: error: use of undeclared identifier '_STAT_VER' if (__ver == _STAT_VER) { ^ 1 error generated. Makefile:4932: recipe for target 'nssmock_la-nssmock.lo' failed gmake: *** [nssmock_la-nssmock.lo] Error 1 Roman Bogorodskiy

Roman Bogorodskiy wrote:
Michal Privoznik wrote:
There is some magic going on when it comes to stat() or lstat(). Basically, stat() can either be a regular function, an inline function that calls __xstat(_STAT_VER, ...) or a macro that does the same as the inline func. Don't ask why is that, just read the documentation in sys/stat.h and make sure you have a bucket next to you. Anyway, currently there will not be both stat and __xstat symbols at the same time, as one of them gets overwritten to the other one during compilation. But this is not true anymore once we start chaining our mocking libraries. Therefore we need a wrapper that calls desired function from glibc.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/vircgroupmock.c | 14 +++++++------- tests/virmock.h | 23 +++++++++++++++++++++++ tests/virpcimock.c | 19 +++++++++---------- 3 files changed, 39 insertions(+), 17 deletions(-)
...
diff --git a/tests/virmock.h b/tests/virmock.h index 62a7c8f..27c03ba 100644 --- a/tests/virmock.h +++ b/tests/virmock.h @@ -27,6 +27,7 @@ # endif # include <stdlib.h> # include <stdio.h> +# include <sys/stat.h>
# include "internal.h"
@@ -254,6 +255,28 @@ static void (*real_##name)(void); \ void name(void)
+static inline int +callStat(int (*realStat)(const char *, struct stat *), + int (*realXstat)(int, const char *, struct stat *), + int __ver, const char *__filename, struct stat *__stat_buf) +{ + if (!realXstat) { + if (__ver == _STAT_VER) { + return realStat(__filename, __stat_buf); + } else { + fprintf(stderr, "Not handled __xstat(ver=%d)", __ver); + abort(); + } + } + + return realXstat(__ver, __filename, __stat_buf); +}
Hm, this fails on FreeBSD with:
CC nssmock_la-nssmock.lo In file included from nssmock.c:24: ./virmock.h:264:22: error: use of undeclared identifier '_STAT_VER' if (__ver == _STAT_VER) { ^ 1 error generated. Makefile:4932: recipe for target 'nssmock_la-nssmock.lo' failed gmake: *** [nssmock_la-nssmock.lo] Error 1
I was to "fix" that by trivially adding +# ifndef _STAT_VER +# define _STAT_VER 0 +# endif + I'm not quite sure that's a right way to go though, because I haven't had a chance to wrap my brain around this new mock thing yet. Roman Bogorodskiy

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/virtestmock.c | 135 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 148 insertions(+), 2 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/virtestmock.c b/tests/virtestmock.c new file mode 100644 index 0000000..0877956 --- /dev/null +++ b/tests/virtestmock.c @@ -0,0 +1,135 @@ +/* + * Copyright (C) 2016 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Author: Michal Privoznik <mprivozn@redhat.com> + */ + +#include <config.h> + +#include "virmock.h" +#include <unistd.h> +#include <sys/types.h> +#include <fcntl.h> + +#include "internal.h" +#include "configmake.h" + +static int (*real_open)(const char *path, int flags, ...); +static FILE *(*real_fopen)(const char *path, const char *mode); +static int (*real_access)(const char *path, int mode); +static int (*real_stat)(const char *path, struct stat *sb); +static int (*real___xstat)(int ver, const char *path, struct stat *sb); +static int (*real_lstat)(const char *path, struct stat *sb); +static int (*real___lxstat)(int ver, const char *path, struct stat *sb); + +static void init_syms(void) +{ + if (real_open) + return; + + VIR_MOCK_REAL_INIT(open); + VIR_MOCK_REAL_INIT(fopen); + VIR_MOCK_REAL_INIT(access); + VIR_MOCK_REAL_INIT_ALT(stat, __xstat); + VIR_MOCK_REAL_INIT_ALT(lstat, __lxstat); +} + +static void +checkPath(const char *path ATTRIBUTE_UNUSED) +{ + /* Nada */ +} + + +int open(const char *path, int flags, ...) +{ + int ret; + + init_syms(); + + checkPath(path); + + if (flags & O_CREAT) { + va_list ap; + mode_t mode; + va_start(ap, flags); + mode = va_arg(ap, mode_t); + va_end(ap); + ret = real_open(path, flags, mode); + } else { + ret = real_open(path, flags); + } + return ret; +} + +FILE *fopen(const char *path, const char *mode) +{ + init_syms(); + + checkPath(path); + + return real_fopen(path, mode); +} + + +int access(const char *path, int mode) +{ + init_syms(); + + checkPath(path); + + return real_access(path, mode); +} + +int stat(const char *path, struct stat *sb) +{ + init_syms(); + + checkPath(path); + + return VIR_MOCK_CALL_STAT(_STAT_VER, path, sb); +} + +int +__xstat(int ver, const char *path, struct stat *sb) +{ + init_syms(); + + checkPath(path); + + return VIR_MOCK_CALL_STAT(ver, path, sb); +} + +int +lstat(const char *path, struct stat *sb) +{ + init_syms(); + + checkPath(path); + + return VIR_MOCK_CALL_LSTAT(_STAT_VER, path, sb); +} + +int +__lxstat(int ver, const char *path, struct stat *sb) +{ + init_syms(); + + checkPath(path); + + return VIR_MOCK_CALL_LSTAT(ver, path, sb); +} -- 2.8.1

On Fri, May 13, 2016 at 14:32:07 +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/virtestmock.c | 135 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 148 insertions(+), 2 deletions(-) create mode 100644 tests/virtestmock.c
[...]
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)
These are used in later patches.
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) \
These too. ACK if you move it to the corresponding patch. 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 | 10 ++++++ docs/hacking.html.in | 13 +++++++ tests/Makefile.am | 4 ++- tests/testutils.c | 30 +++++++++++++---- tests/testutils.h | 10 +++--- tests/virtestmock.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++++++-- 7 files changed, 149 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..918130e 100644 --- a/HACKING +++ b/HACKING @@ -152,6 +152,16 @@ There is also a "./run" script at the top level, to make it easier to run programs that have not yet been installed, as well as to wrap invocations of various tests under gdb or Valgrind. +When running our test suite it may happen that the test result is +nondeterministic because of the test suite relying on a particular file in the +system being accessible or having some specific value. To catch this kind of +errors, the test suite has a module for that prints any path touched that +fulfils constraints described above into a file. To enable it just set +"VIR_TEST_FILE_ACCESS" environment variable. Then "VIR_TEST_FILE_ACCESS" +environment variable can alter location where the file is stored. + + VIR_TEST_FILE_ACCESS=1 VIR_TEST_FILE_ACCESS_OUTPUT="/tmp/file_access.txt" ./qemuxml2argvtest + (9) The Valgrind test should produce similar output to "make check". If the output diff --git a/docs/hacking.html.in b/docs/hacking.html.in index 5cd23a2..2522a12 100644 --- a/docs/hacking.html.in +++ b/docs/hacking.html.in @@ -189,6 +189,19 @@ under gdb or Valgrind. </p> + <p>When running our test suite it may happen that the test result is + nondeterministic because of the test suite relying on a particular file + in the system being accessible or having some specific value. To catch + this kind of errors, the test suite has a module for that prints any + path touched that fulfils constraints described above + into a file. To enable it just set + <code>VIR_TEST_FILE_ACCESS</code> environment variable. + Then <code>VIR_TEST_FILE_ACCESS</code> environment + variable can alter location where the file is stored.</p> +<pre> + VIR_TEST_FILE_ACCESS=1 VIR_TEST_FILE_ACCESS_OUTPUT="/tmp/file_access.txt" ./qemuxml2argvtest +</pre> + </li> <li><p>The Valgrind test should produce similar output to <code>make check</code>. If the output has traces within libvirt diff --git a/tests/Makefile.am b/tests/Makefile.am index da68f2e..ca3c8c3 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -1151,7 +1151,9 @@ virtestmock_la_SOURCES = \ virtestmock.c virtestmock_la_CFLAGS = $(AM_CFLAGS) virtestmock_la_LDFLAGS = $(MOCKLIBS_LDFLAGS) -virtestmock_la_LIBADD = $(MOCKLIBS_LIBS) +virtestmock_la_LIBADD = \ + $(MOCKLIBS_LIBS) \ + ../src/libvirt_util.la else ! WITH_LINUX EXTRA_DIST += virusbtest.c virusbmock.c \ virnetdevbandwidthtest.c virnetdevbandwidthmock.c \ diff --git a/tests/testutils.c b/tests/testutils.c index 79d0763..9180e86 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -156,6 +156,11 @@ virtTestRun(const char *title, { int ret = 0; + /* Some test are fragile about environ settings. If that's + * the case, don't poison it. */ + if (getenv("VIR_TEST_MOCK_PROGNAME")) + setenv("VIR_TEST_MOCK_TESTNAME", title, 1); + if (testCounter == 0 && !virTestGetVerbose()) fprintf(stderr, " "); @@ -280,6 +285,7 @@ virtTestRun(const char *title, } #endif /* TEST_OOM */ + unsetenv("VIR_TEST_MOCK_TESTNAME"); return ret; } @@ -832,8 +838,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,17 +851,26 @@ int virtTestMain(int argc, char *oomstr; #endif - virFileActivateDirOverride(argv[0]); + if (getenv("VIR_TEST_FILE_ACCESS")) + VIRT_TEST_PRELOAD(TEST_MOCK); - if (virTestSetEnvPath() < 0) - return EXIT_AM_HARDFAIL; - - if (!virFileExists(abs_srcdir)) - return EXIT_AM_HARDFAIL; + if (lib) + VIRT_TEST_PRELOAD(lib); 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/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/virtestmock.c b/tests/virtestmock.c index 0877956..59ca5e5 100644 --- a/tests/virtestmock.c +++ b/tests/virtestmock.c @@ -24,9 +24,14 @@ #include <unistd.h> #include <sys/types.h> #include <fcntl.h> +#include <execinfo.h> +#include <sys/file.h> #include "internal.h" #include "configmake.h" +#include "virstring.h" +#include "viralloc.h" +#include "virfile.h" static int (*real_open)(const char *path, int flags, ...); static FILE *(*real_fopen)(const char *path, const char *mode); @@ -36,6 +41,11 @@ static int (*real___xstat)(int ver, const char *path, struct stat *sb); static int (*real_lstat)(const char *path, struct stat *sb); static int (*real___lxstat)(int ver, const char *path, struct stat *sb); +static const char *progname; +const char *output; + +#define VIR_FILE_ACCESS_DEFAULT abs_builddir "/test_file_access.txt" + static void init_syms(void) { if (real_open) @@ -49,9 +59,90 @@ static void init_syms(void) } static void -checkPath(const char *path ATTRIBUTE_UNUSED) +printFile(const char *file) { - /* Nada */ + FILE *fp; + const char *testname = getenv("VIR_TEST_MOCK_TESTNAME"); + + if (!progname) { + progname = getenv("VIR_TEST_MOCK_PROGNAME"); + + if (!progname) + return; + + output = getenv("VIR_TEST_FILE_ACCESS_OUTPUT"); + if (!output) + output = VIR_FILE_ACCESS_DEFAULT; + } + + if (!(fp = real_fopen(output, "a"))) { + fprintf(stderr, "Unable to open %s: %s\n", output, strerror(errno)); + abort(); + } + + if (flock(fileno(fp), LOCK_EX) < 0) { + fprintf(stderr, "Unable to lock %s: %s\n", output, strerror(errno)); + fclose(fp); + abort(); + } + + /* Now append the following line into the output file: + * $file: $progname $testname */ + + fprintf(fp, "%s: %s", file, progname); + if (testname) + fprintf(fp, ": %s", testname); + + fputc('\n', fp); + + flock(fileno(fp), LOCK_UN); + fclose(fp); +} + +static void +checkPath(const char *path) +{ + char *fullPath = NULL; + char *relPath = NULL; + char *crippledPath = NULL; + + if (path[0] != '/' && + virAsprintfQuiet(&relPath, "./%s", path) < 0) + goto error; + + /* Le sigh. Both canonicalize_file_name() and realpath() + * expect @path to exist otherwise they return an error. So + * if we are called over an non-existent file, this could + * return an error. In that case do our best and hope we will + * catch possible error. */ + if ((fullPath = canonicalize_file_name(relPath ? relPath : path))) { + path = fullPath; + } else { + /* Yeah, our worst nightmares just became true. Path does + * not exist. Cut off the last component and retry. */ + if (VIR_STRDUP_QUIET(crippledPath, relPath ? relPath : path) < 0) + goto error; + + virFileRemoveLastComponent(crippledPath); + + if ((fullPath = canonicalize_file_name(crippledPath))) + path = fullPath; + } + + + if (!STRPREFIX(path, abs_topsrcdir) && + !STRPREFIX(path, abs_topbuilddir)) { + printFile(path); + } + + VIR_FREE(crippledPath); + VIR_FREE(relPath); + VIR_FREE(fullPath); + + return; + error: + fprintf(stderr, "Out of memory\n"); + abort(); } -- 2.8.1

On Fri, May 13, 2016 at 14:32:08 +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 | 10 ++++++ docs/hacking.html.in | 13 +++++++ tests/Makefile.am | 4 ++- tests/testutils.c | 30 +++++++++++++---- tests/testutils.h | 10 +++--- tests/virtestmock.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++++++-- 7 files changed, 149 insertions(+), 14 deletions(-)
[...]
diff --git a/HACKING b/HACKING index e308568..918130e 100644 --- a/HACKING +++ b/HACKING @@ -152,6 +152,16 @@ There is also a "./run" script at the top level, to make it easier to run programs that have not yet been installed, as well as to wrap invocations of various tests under gdb or Valgrind.
+When running our test suite it may happen that the test result is +nondeterministic because of the test suite relying on a particular file in the +system being accessible or having some specific value. To catch this kind of +errors, the test suite has a module for that prints any path touched that +fulfils constraints described above into a file. To enable it just set +"VIR_TEST_FILE_ACCESS" environment variable. Then "VIR_TEST_FILE_ACCESS"
VIR_TEST_FILE_ACCESS_OUTPUT in the second place.
+environment variable can alter location where the file is stored. + + VIR_TEST_FILE_ACCESS=1 VIR_TEST_FILE_ACCESS_OUTPUT="/tmp/file_access.txt" ./qemuxml2argvtest +
(9) The Valgrind test should produce similar output to "make check". If the output diff --git a/docs/hacking.html.in b/docs/hacking.html.in index 5cd23a2..2522a12 100644 --- a/docs/hacking.html.in +++ b/docs/hacking.html.in @@ -189,6 +189,19 @@ under gdb or Valgrind. </p>
+ <p>When running our test suite it may happen that the test result is + nondeterministic because of the test suite relying on a particular file + in the system being accessible or having some specific value. To catch + this kind of errors, the test suite has a module for that prints any + path touched that fulfils constraints described above + into a file. To enable it just set + <code>VIR_TEST_FILE_ACCESS</code> environment variable. + Then <code>VIR_TEST_FILE_ACCESS</code> environment
Same here.
+ variable can alter location where the file is stored.</p> +<pre> + VIR_TEST_FILE_ACCESS=1 VIR_TEST_FILE_ACCESS_OUTPUT="/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
ACK ^^

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 | 104 ++++++++++++++++++++++++++++++++++++++++ tests/file_access_whitelist.txt | 23 +++++++++ 4 files changed, 143 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 ca3c8c3..238f6da 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 + VIR_TEST_FILE_ACCESS=1 $(MAKE) $(AM_MAKEFLAGS) check + $(PERL) check-file-access.pl | sort -u + +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..0c75ae9 --- /dev/null +++ b/tests/check-file-access.pl @@ -0,0 +1,104 @@ +#!/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*):\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"; + } +} + +exit $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 Fri, May 13, 2016 at 14:32:09 +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 | 104 ++++++++++++++++++++++++++++++++++++++++ tests/file_access_whitelist.txt | 23 +++++++++ 4 files changed, 143 insertions(+) create mode 100755 tests/check-file-access.pl create mode 100644 tests/file_access_whitelist.txt
ACK. And now we need to fix the findings :)

On 13.05.2016 18:29, Peter Krempa wrote:
On Fri, May 13, 2016 at 14:32:09 +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 | 104 ++++++++++++++++++++++++++++++++++++++++ tests/file_access_whitelist.txt | 23 +++++++++ 4 files changed, 143 insertions(+) create mode 100755 tests/check-file-access.pl create mode 100644 tests/file_access_whitelist.txt
ACK. And now we need to fix the findings :)
Thank you; I've fixed all the issues you've raised and pushed this. Michal
participants (3)
-
Michal Privoznik
-
Peter Krempa
-
Roman Bogorodskiy