[libvirt] [PATCH] tests: fix mocking of stat() / lstat() functions

Daniel P. Berrangé posted 1 patch 4 years, 12 months ago
Test syntax-check passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20190401170329.32306-1-berrange@redhat.com
build-aux/mock-noinline.pl |   3 +
cfg.mk                     |   4 +-
tests/qemusecuritymock.c   | 131 +++++-----------
tests/vircgroupmock.c      | 146 +++--------------
tests/virfilewrapper.c     |  85 ++--------
tests/virmock.h            |  11 --
tests/virmockstathelpers.c | 310 +++++++++++++++++++++++++++++++++++++
tests/virpcimock.c         |  93 +----------
tests/virtestmock.c        | 140 +----------------
9 files changed, 396 insertions(+), 527 deletions(-)
create mode 100644 tests/virmockstathelpers.c
[libvirt] [PATCH] tests: fix mocking of stat() / lstat() functions
Posted by Daniel P. Berrangé 4 years, 12 months ago
Quite a few of the tests have a need to mock the stat() / lstat()
functions and they are taking somewhat different & inconsistent
approaches none of which are actually fully correct. This is shown
by fact that 'make check' fails on 32-bit hosts. Investigation
revealed that the code was calling into the native C library impl,
not getting intercepted by our mocks.

The POSIX stat() function might resolve to any number of different
symbols in the C library.

The may be an additional stat64() function exposed by the headers
too.

On 64-bit hosts the stat & stat64 functions are identical, always
refering to the 64-bit ABI.

On 32-bit hosts they refer to the 32-bit & 64-bit ABIs respectively.

Libvirt uses _FILE_OFFSET_BITS=64 on 32-bit hosts, which causes the
C library to transparently rewrite stat() calls to be stat64() calls.
Libvirt will never see the 32-bit ABI from the traditional stat()
call. We cannot assume this rewriting is done using a macro. It might
be, but on GLibC it is done with a magic __asm__ statement to apply
the rewrite at link time instead of at preprocessing.

In GLibC there may be two additional functions exposed by the headers,
__xstat() and __xstat64(). When these exist, stat() and stat64() are
transparently rewritten to call __xstat() and __xstat64() respectively.
The former symbols will not actally exist in the library at all, only
the header. The leading "__" indicates the symbols are a private impl
detail of the C library that applications should not care about.
Unfortunately, because we are trying to mock replace the C library,
we need to know about this internal impl detail.

With all this in mind the list of functions we have to mock will depend
on several factors

 - If _FILE_OFFSET_BITS is set, then we are on a 32-bit host, and we
   only need to mock stat64 and __xstat64. The other stat / __xstat
   functions exist, but we'll never call them so they can be ignored
   for mocking.

 - If _FILE_OFFSET_BITS is not set, then we are on a 64-bit host and
   we should mock stat, stat64, __xstat & __xstat64. Either may be
   called by app code.

 - If __xstat & __xstat64 exist, then stat & stat64 will not exist
   as symbols in the library, so the latter should not be mocked.

The same all applies to lstat()

These rules are complex enough that we don't want to duplicate them
across every mock file, so this centralizes all the logic in a helper
file virmockstathelper.c that should be #included when needed. The
code merely need to provide a filename rewriting callback called
virMockStatRedirect(). Optionally VIR_MOCK_STAT_HOOK can be defined
as a macro if further processing is needed inline.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 build-aux/mock-noinline.pl |   3 +
 cfg.mk                     |   4 +-
 tests/qemusecuritymock.c   | 131 +++++-----------
 tests/vircgroupmock.c      | 146 +++--------------
 tests/virfilewrapper.c     |  85 ++--------
 tests/virmock.h            |  11 --
 tests/virmockstathelpers.c | 310 +++++++++++++++++++++++++++++++++++++
 tests/virpcimock.c         |  93 +----------
 tests/virtestmock.c        | 140 +----------------
 9 files changed, 396 insertions(+), 527 deletions(-)
 create mode 100644 tests/virmockstathelpers.c

NB I have tested on Fedora 32-bit and 64-bit but have not yet
tested on other platforms, so they may break. This is to be
investigated before pushing so we don't break other platforms
more than they are already broken wrt mocking :-)

diff --git a/build-aux/mock-noinline.pl b/build-aux/mock-noinline.pl
index c6b40001c5..958e133885 100644
--- a/build-aux/mock-noinline.pl
+++ b/build-aux/mock-noinline.pl
@@ -6,6 +6,9 @@ my %mocked;
 # Functions in public header don't get the noinline annotation
 # so whitelist them here
 $noninlined{"virEventAddTimeout"} = 1;
+# This one confuses the script as its defined in the mock file
+# but is actually just a local helper
+$noninlined{"virMockStatRedirect"} = 1;
 
 foreach my $arg (@ARGV) {
     if ($arg =~ /\.h$/) {
diff --git a/cfg.mk b/cfg.mk
index a62d4db13b..2c473121a4 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -1272,10 +1272,10 @@ exclude_file_name_regexp--sc_prohibit_xmlURI = ^src/util/viruri\.c$$
 exclude_file_name_regexp--sc_prohibit_return_as_function = \.py$$
 
 exclude_file_name_regexp--sc_require_config_h = \
-	^(examples/|tools/virsh-edit\.c$$)
+	^(examples/|tools/virsh-edit\.c$$|tests/virmockstathelpers.c)
 
 exclude_file_name_regexp--sc_require_config_h_first = \
-	^(examples/|tools/virsh-edit\.c$$)
+	^(examples/|tools/virsh-edit\.c$$|tests/virmockstathelpers.c)
 
 exclude_file_name_regexp--sc_trailing_blank = \
   /sysinfodata/.*\.data|/virhostcpudata/.*\.cpuinfo|^gnulib/local/.*/.*diff$$
diff --git a/tests/qemusecuritymock.c b/tests/qemusecuritymock.c
index d1b17d8aa4..3fdc165fb1 100644
--- a/tests/qemusecuritymock.c
+++ b/tests/qemusecuritymock.c
@@ -50,10 +50,6 @@
 
 
 static int (*real_chown)(const char *path, uid_t uid, gid_t gid);
-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 int (*real_open)(const char *path, int flags, ...);
 static int (*real_close)(int fd);
 
@@ -106,8 +102,6 @@ init_syms(void)
         return;
 
     VIR_MOCK_REAL_INIT(chown);
-    VIR_MOCK_REAL_INIT_ALT(lstat, __lxstat);
-    VIR_MOCK_REAL_INIT_ALT(stat, __xstat);
     VIR_MOCK_REAL_INIT(open);
     VIR_MOCK_REAL_INIT(close);
 
@@ -211,36 +205,35 @@ int virFileRemoveXAttr(const char *path,
 }
 
 
-static int
-mock_stat(const char *path,
-          struct stat *sb)
-{
-    uint32_t *val;
-
-    virMutexLock(&m);
-    init_hash();
-
-    memset(sb, 0, sizeof(*sb));
-
-    sb->st_mode = S_IFREG | 0666;
-    sb->st_size = 123456;
-    sb->st_ino = 1;
-
-    if (!(val = virHashLookup(chown_paths, path))) {
-        /* New path. Set the defaults */
-        sb->st_uid = DEFAULT_UID;
-        sb->st_gid = DEFAULT_GID;
-    } else {
-        /* Known path. Set values passed to chown() earlier */
-        sb->st_uid = *val % 16;
-        sb->st_gid = *val >> 16;
-    }
-
-    virMutexUnlock(&m);
-
-    return 0;
-}
-
+#define VIR_MOCK_STAT_HOOK \
+    do { \
+        if (getenv(ENVVAR)) { \
+            uint32_t *val; \
+\
+            virMutexLock(&m); \
+            init_hash(); \
+\
+            memset(sb, 0, sizeof(*sb)); \
+\
+            sb->st_mode = S_IFREG | 0666; \
+            sb->st_size = 123456; \
+            sb->st_ino = 1; \
+\
+            if (!(val = virHashLookup(chown_paths, path))) { \
+                /* New path. Set the defaults */ \
+                sb->st_uid = DEFAULT_UID; \
+                sb->st_gid = DEFAULT_GID; \
+            } else { \
+                /* Known path. Set values passed to chown() earlier */ \
+                sb->st_uid = *val % 16; \
+                sb->st_gid = *val >> 16; \
+            } \
+\
+            virMutexUnlock(&m); \
+\
+            return 0; \
+        } \
+    } while (0)
 
 static int
 mock_chown(const char *path,
@@ -276,68 +269,12 @@ mock_chown(const char *path,
 }
 
 
-#ifdef HAVE___LXSTAT
-int
-__lxstat(int ver, const char *path, struct stat *sb)
-{
-    int ret;
-
-    init_syms();
-
-    if (getenv(ENVVAR))
-        ret = mock_stat(path, sb);
-    else
-        ret = real___lxstat(ver, path, sb);
-
-    return ret;
-}
-#endif /* HAVE___LXSTAT */
-
-int
-lstat(const char *path, struct stat *sb)
-{
-    int ret;
-
-    init_syms();
-
-    if (getenv(ENVVAR))
-        ret = mock_stat(path, sb);
-    else
-        ret = real_lstat(path, sb);
-
-    return ret;
-}
-
-#ifdef HAVE___XSTAT
-int
-__xstat(int ver, const char *path, struct stat *sb)
-{
-    int ret;
-
-    init_syms();
-
-    if (getenv(ENVVAR))
-        ret = mock_stat(path, sb);
-    else
-        ret = real___xstat(ver, path, sb);
-
-    return ret;
-}
-#endif /* HAVE___XSTAT */
+#include "virmockstathelpers.c"
 
-int
-stat(const char *path, struct stat *sb)
+static int
+virMockStatRedirect(const char *path ATTRIBUTE_UNUSED, char **newpath ATTRIBUTE_UNUSED)
 {
-    int ret;
-
-    init_syms();
-
-    if (getenv(ENVVAR))
-        ret = mock_stat(path, sb);
-    else
-        ret = real_stat(path, sb);
-
-    return ret;
+    return 0;
 }
 
 
@@ -386,6 +323,8 @@ close(int fd)
 {
     int ret;
 
+    init_syms();
+
     if (fd == 42 && getenv(ENVVAR))
         ret = 0;
     else
diff --git a/tests/vircgroupmock.c b/tests/vircgroupmock.c
index 9c67a44b0d..11a24035aa 100644
--- a/tests/vircgroupmock.c
+++ b/tests/vircgroupmock.c
@@ -38,10 +38,6 @@
 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
@@ -317,8 +313,6 @@ static void init_syms(void)
 
     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);
 }
@@ -508,139 +502,41 @@ int access(const char *path, int mode)
     return ret;
 }
 
-int __lxstat(int ver, const char *path, struct stat *sb)
-{
-    int ret;
-
-    init_syms();
-
-    if (STRPREFIX(path, SYSFS_CGROUP_PREFIX)) {
-        init_sysfs();
-        char *newpath;
-        if (asprintf(&newpath, "%s%s",
-                     fakesysfscgroupdir,
-                     path + strlen(SYSFS_CGROUP_PREFIX)) < 0) {
-            errno = ENOMEM;
-            return -1;
-        }
-        ret = real___lxstat(ver, newpath, sb);
-        free(newpath);
-    } else if (STRPREFIX(path, fakedevicedir0)) {
-        sb->st_mode = S_IFBLK;
-        sb->st_rdev = makedev(8, 0);
-        return 0;
-    } else if (STRPREFIX(path, fakedevicedir1)) {
-        sb->st_mode = S_IFBLK;
-        sb->st_rdev = makedev(9, 0);
-        return 0;
-    } else {
-        ret = real___lxstat(ver, path, sb);
-    }
-    return ret;
-}
-
-int lstat(const char *path, struct stat *sb)
-{
-    int ret;
-
-    init_syms();
-
-    if (STRPREFIX(path, SYSFS_CGROUP_PREFIX)) {
-        init_sysfs();
-        char *newpath;
-        if (asprintf(&newpath, "%s%s",
-                     fakesysfscgroupdir,
-                     path + strlen(SYSFS_CGROUP_PREFIX)) < 0) {
-            errno = ENOMEM;
-            return -1;
-        }
-        ret = real_lstat(newpath, sb);
-        free(newpath);
-    } else if (STRPREFIX(path, fakedevicedir0)) {
-        sb->st_mode = S_IFBLK;
-        sb->st_rdev = makedev(8, 0);
-        return 0;
-    } else if (STRPREFIX(path, fakedevicedir1)) {
-        sb->st_mode = S_IFBLK;
-        sb->st_rdev = makedev(9, 0);
-        return 0;
-    } else {
-        ret = real_lstat(path, sb);
-    }
-    return ret;
-}
-
-int __xstat(int ver, const char *path, struct stat *sb)
-{
-    int ret;
-
-    init_syms();
+# define VIR_MOCK_STAT_HOOK \
+    do { \
+        if (STRPREFIX(path, fakedevicedir0)) { \
+            sb->st_mode = S_IFBLK; \
+            sb->st_rdev = makedev(8, 0); \
+            return 0; \
+        } else if (STRPREFIX(path, fakedevicedir1)) { \
+            sb->st_mode = S_IFBLK; \
+            sb->st_rdev = makedev(9, 0); \
+            return 0; \
+        } \
+    } while (0)
 
-    if (STRPREFIX(path, SYSFS_CGROUP_PREFIX)) {
-        init_sysfs();
-        char *newpath;
-        if (asprintf(&newpath, "%s%s",
-                     fakesysfscgroupdir,
-                     path + strlen(SYSFS_CGROUP_PREFIX)) < 0) {
-            errno = ENOMEM;
-            return -1;
-        }
-        ret = real___xstat(ver, newpath, sb);
-        free(newpath);
-    } else if (STRPREFIX(path, fakedevicedir0)) {
-        sb->st_mode = S_IFBLK;
-        sb->st_rdev = makedev(8, 0);
-        return 0;
-    } else if (STRPREFIX(path, fakedevicedir1)) {
-        sb->st_mode = S_IFBLK;
-        sb->st_rdev = makedev(9, 0);
-        return 0;
-    } else {
-        ret = real___xstat(ver, path, sb);
-    }
-    return ret;
-}
+# include "virmockstathelpers.c"
 
-int stat(const char *path, struct stat *sb)
+static int
+virMockStatRedirect(const char *path, char **newpath)
 {
-    char *newpath = NULL;
-    int ret;
-
-    init_syms();
-
     if (STREQ(path, SYSFS_CPU_PRESENT)) {
         init_sysfs();
-        if (asprintf(&newpath, "%s/%s",
+        if (asprintf(newpath, "%s/%s",
                      fakesysfscgroupdir,
-                     SYSFS_CPU_PRESENT_MOCKED) < 0) {
-            errno = ENOMEM;
+                     SYSFS_CPU_PRESENT_MOCKED) < 0)
             return -1;
-        }
     } else if (STRPREFIX(path, SYSFS_CGROUP_PREFIX)) {
         init_sysfs();
-        if (asprintf(&newpath, "%s%s",
+        if (asprintf(newpath, "%s%s",
                      fakesysfscgroupdir,
-                     path + strlen(SYSFS_CGROUP_PREFIX)) < 0) {
-            errno = ENOMEM;
-            return -1;
-        }
-    } else if (STRPREFIX(path, fakedevicedir0)) {
-        sb->st_mode = S_IFBLK;
-        sb->st_rdev = makedev(8, 0);
-        return 0;
-    } else if (STRPREFIX(path, fakedevicedir1)) {
-        sb->st_mode = S_IFBLK;
-        sb->st_rdev = makedev(9, 0);
-        return 0;
-    } else {
-        if (!(newpath = strdup(path)))
+                     path + strlen(SYSFS_CGROUP_PREFIX)) < 0)
             return -1;
     }
-    ret = real_stat(newpath, sb);
-    free(newpath);
-    return ret;
+    return 0;
 }
 
+
 int mkdir(const char *path, mode_t mode)
 {
     int ret;
diff --git a/tests/virfilewrapper.c b/tests/virfilewrapper.c
index 88441331ce..067cb30657 100644
--- a/tests/virfilewrapper.c
+++ b/tests/virfilewrapper.c
@@ -39,15 +39,9 @@ static size_t nprefixes;
 static const char **prefixes;
 
 /* TODO: callbacks */
-
-
 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);
 static DIR *(*real_opendir)(const char *path);
 
@@ -58,8 +52,6 @@ static void init_syms(void)
 
     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);
     VIR_MOCK_REAL_INIT(opendir);
@@ -115,10 +107,11 @@ virFileWrapperClearPrefixes(void)
     VIR_FREE(overrides);
 }
 
-static char *
-virFileWrapperOverridePrefix(const char *path)
+# include "virmockstathelpers.c"
+
+int
+virMockStatRedirect(const char *path, char **newpath)
 {
-    char *ret = NULL;
     size_t i = 0;
 
     for (i = 0; i < noverrides; i++) {
@@ -127,16 +120,13 @@ virFileWrapperOverridePrefix(const char *path)
         if (!tmp)
             continue;
 
-        if (virAsprintfQuiet(&ret, "%s%s", overrides[i], tmp) < 0)
-            return NULL;
+        if (virAsprintfQuiet(newpath, "%s%s", overrides[i], tmp) < 0)
+            return -1;
 
         break;
     }
 
-    if (!ret)
-        ignore_value(VIR_STRDUP_QUIET(ret, path));
-
-    return ret;
+    return 0;
 }
 
 
@@ -144,8 +134,7 @@ virFileWrapperOverridePrefix(const char *path)
     do { \
         init_syms(); \
  \
-        newpath = virFileWrapperOverridePrefix(path); \
-        if (!newpath) \
+        if (virMockStatRedirect(path, &newpath) < 0) \
             abort(); \
     } while (0)
 
@@ -156,7 +145,7 @@ FILE *fopen(const char *path, const char *mode)
 
     PATH_OVERRIDE(newpath, path);
 
-    return real_fopen(newpath, mode);
+    return real_fopen(newpath ? newpath : path, mode);
 }
 
 int access(const char *path, int mode)
@@ -165,56 +154,7 @@ int access(const char *path, int mode)
 
     PATH_OVERRIDE(newpath, path);
 
-    return real_access(newpath, mode);
-}
-
-# ifdef HAVE___LXSTAT
-int __lxstat(int ver, const char *path, struct stat *sb)
-{
-    VIR_AUTOFREE(char *) newpath = NULL;
-
-    PATH_OVERRIDE(newpath, path);
-
-    return real___lxstat(ver, newpath, sb);
-}
-# endif /* HAVE___LXSTAT */
-
-int lstat(const char *path, struct stat *sb)
-{
-    VIR_AUTOFREE(char *) newpath = NULL;
-
-    PATH_OVERRIDE(newpath, path);
-
-    return real_lstat(newpath, sb);
-}
-
-# ifdef HAVE___XSTAT
-int __xstat(int ver, const char *path, struct stat *sb)
-{
-    VIR_AUTOFREE(char *) newpath = NULL;
-
-    PATH_OVERRIDE(newpath, path);
-
-    return real___xstat(ver, newpath, sb);
-}
-# endif /* HAVE___XSTAT */
-
-int stat(const char *path, struct stat *sb)
-{
-    VIR_AUTOFREE(char *) newpath = NULL;
-
-    PATH_OVERRIDE(newpath, path);
-
-    return real_stat(newpath, sb);
-}
-
-int mkdir(const char *path, mode_t mode)
-{
-    VIR_AUTOFREE(char *) newpath = NULL;
-
-    PATH_OVERRIDE(newpath, path);
-
-    return real_mkdir(newpath, mode);
+    return real_access(newpath ? newpath : path, mode);
 }
 
 int open(const char *path, int flags, ...)
@@ -234,7 +174,7 @@ int open(const char *path, int flags, ...)
         va_end(ap);
     }
 
-    return real_open(newpath, flags, mode);
+    return real_open(newpath ? newpath : path, flags, mode);
 }
 
 DIR *opendir(const char *path)
@@ -243,6 +183,7 @@ DIR *opendir(const char *path)
 
     PATH_OVERRIDE(newpath, path);
 
-    return real_opendir(newpath);
+    return real_opendir(newpath ? newpath : path);
 }
+
 #endif
diff --git a/tests/virmock.h b/tests/virmock.h
index 9c7ecf60ce..46631433c7 100644
--- a/tests/virmock.h
+++ b/tests/virmock.h
@@ -290,15 +290,4 @@
         } \
     } while (0)
 
-# define VIR_MOCK_REAL_INIT_ALT(name1, name2) \
-    do { \
-        real_ ## name1 = dlsym(RTLD_NEXT, #name1); \
-        real_ ## name2 = dlsym(RTLD_NEXT, #name2); \
-        if (!real_##name1 && !real_##name2) { \
-            fprintf(stderr, "Cannot find real '%s' or '%s' symbol\n", \
-                    #name1, #name2); \
-            abort(); \
-        } \
-    } while (0)
-
 #endif /* LIBVIRT_VIRMOCK_H */
diff --git a/tests/virmockstathelpers.c b/tests/virmockstathelpers.c
new file mode 100644
index 0000000000..745937113b
--- /dev/null
+++ b/tests/virmockstathelpers.c
@@ -0,0 +1,310 @@
+/*
+ * Copyright (C) 2019 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/>.
+ *
+ * Helpers for dealing with the many variants of stat(). This
+ * C file should be included from any file that wants to mock
+ * stat() correctly.
+ */
+
+#include "virmock.h"
+#include "viralloc.h"
+
+#include <sys/stat.h>
+#include <unistd.h>
+
+/*
+ * The POSIX stat() function might resolve to any number of different
+ * symbols in the C library.
+ *
+ * The may be an additional stat64() function exposed by the headers
+ * too.
+ *
+ * On 64-bit hosts the stat & stat64 functions are identical, always
+ * refering to the 64-bit ABI.
+ *
+ * On 32-bit hosts they refer to the 32-bit & 64-bit ABIs respectively.
+ *
+ * Libvirt uses _FILE_OFFSET_BITS=64 on 32-bit hosts, which causes the
+ * C library to transparently rewrite stat() calls to be stat64() calls.
+ * Libvirt will never see the 32-bit ABI from the traditional stat()
+ * call. We cannot assume this rewriting is done using a macro. It might
+ * be, but on GLibC it is done with a magic __asm__ statement to apply
+ * the rewrite at link time instead of at preprocessing.
+ *
+ * In GLibC there may be two additional functions exposed by the headers,
+ * __xstat() and __xstat64(). When these exist, stat() and stat64() are
+ * transparently rewritten to call __xstat() and __xstat64() respectively.
+ * The former symbols will not actally exist in the library at all, only
+ * the header. The leading "__" indicates the symbols are a private impl
+ * detail of the C library that applications should not care about.
+ * Unfortunately, because we are trying to mock replace the C library,
+ * we need to know about this internal impl detail.
+ *
+ * With all this in mind the list of functions we have to mock will depend
+ * on several factors
+ *
+ *  - If _FILE_OFFSET_BITS is set, then we are on a 32-bit host, and we
+ *    only need to mock stat64 and __xstat64. The other stat / __xstat
+ *    functions exist, but we'll never call them so they can be ignored
+ *    for mocking.
+ *
+ *  - If _FILE_OFFSET_BITS is not set, then we are on a 64-bit host and
+ *    we should mock stat, stat64, __xstat & __xstat64. Either may be
+ *    called by app code.
+ *
+ *  - If __xstat & __xstat64 exist, then stat & stat64 will not exist
+ *    as symbols in the library, so the latter should not be mocked.
+ *
+ * The same all applies to lstat()
+ */
+
+
+
+#if defined(HAVE_STAT) && !defined(HAVE___XSTAT) && !defined(_FILE_OFFSET_BITS)
+# define MOCK_STAT
+#endif
+#if defined(HAVE_STAT64) && !defined(HAVE___XSTAT64)
+# define MOCK_STAT64
+#endif
+#if defined(HAVE___XSTAT) && !defined(_FILE_OFFSET_BITS)
+# define MOCK___XSTAT
+#endif
+#if defined(HAVE___XSTAT64)
+# define MOCK___XSTAT64
+#endif
+#if defined(HAVE_LSTAT) && !defined(HAVE___LXSTAT) && !defined(_FILE_OFFSET_BITS)
+# define MOCK_LSTAT
+#endif
+#if defined(HAVE_LSTAT64) && !defined(HAVE___LXSTAT64)
+# define MOCK_LSTAT64
+#endif
+#if defined(HAVE___LXSTAT) && !defined(_FILE_OFFSET_BITS)
+# define MOCK___LXSTAT
+#endif
+#if defined(HAVE___LXSTAT64)
+# define MOCK___LXSTAT64
+#endif
+
+#ifdef MOCK_STAT
+static int (*real_stat)(const char *path, struct stat *sb);
+#endif
+#ifdef MOCK_STAT64
+static int (*real_stat64)(const char *path, struct stat64 *sb);
+#endif
+#ifdef MOCK___XSTAT
+static int (*real___xstat)(int ver, const char *path, struct stat *sb);
+#endif
+#ifdef MOCK___XSTAT64
+static int (*real___xstat64)(int ver, const char *path, struct stat64 *sb);
+#endif
+#ifdef MOCK_LSTAT
+static int (*real_lstat)(const char *path, struct stat *sb);
+#endif
+#ifdef MOCK_LSTAT64
+static int (*real_lstat64)(const char *path, struct stat64 *sb);
+#endif
+#ifdef MOCK___LXSTAT
+static int (*real___lxstat)(int ver, const char *path, struct stat *sb);
+#endif
+#ifdef MOCK___LXSTAT64
+static int (*real___lxstat64)(int ver, const char *path, struct stat64 *sb);
+#endif
+
+
+static void virMockStatInit(void)
+{
+#ifdef MOCK_STAT
+    VIR_MOCK_REAL_INIT(stat);
+#endif
+#ifdef MOCK_STAT64
+    VIR_MOCK_REAL_INIT(stat64);
+#endif
+#ifdef MOCK___XSTAT
+    VIR_MOCK_REAL_INIT(__xstat);
+#endif
+#ifdef MOCK___XSTAT64
+    VIR_MOCK_REAL_INIT(__xstat64);
+#endif
+#ifdef MOCK_LSTAT
+    VIR_MOCK_REAL_INIT(lstat);
+#endif
+#ifdef MOCK_LSTAT64
+    VIR_MOCK_REAL_INIT(lstat64);
+#endif
+#ifdef MOCK___LXSTAT
+    VIR_MOCK_REAL_INIT(__lxstat);
+#endif
+#ifdef MOCK___LXSTAT64
+    VIR_MOCK_REAL_INIT(__lxstat64);
+#endif
+}
+
+/*
+ * @stat: the path being queried
+ * @newpath: fill with redirected path, or leave NULL to use orig path
+ *
+ * Return 0 on success, -1 on allocation error
+ */
+static int virMockStatRedirect(const char *path, char **newpath);
+
+#define fdebug(msg, ...) do { if (getenv("VIR_STAT_MOCK_DEBUG")) fprintf(stderr, msg, __VA_ARGS__); } while (0)
+
+#ifndef VIR_MOCK_STAT_HOOK
+# define VIR_MOCK_STAT_HOOK do { } while (0)
+#endif
+
+#ifdef MOCK_STAT
+int stat(const char *path, struct stat *sb)
+{
+    VIR_AUTOFREE(char *) newpath = NULL;
+
+    virMockStatInit();
+
+    if (virMockStatRedirect(path, &newpath) < 0)
+        abort();
+    fdebug("stat redirect %s to %s sb=%p\n", path, newpath ? newpath : path, sb);
+
+    VIR_MOCK_STAT_HOOK;
+
+    return real_stat(newpath ? newpath : path, sb);
+}
+#endif
+
+#ifdef MOCK_STAT64
+int stat64(const char *path, struct stat64 *sb)
+{
+    VIR_AUTOFREE(char *) newpath = NULL;
+
+    virMockStatInit();
+
+    if (virMockStatRedirect(path, &newpath) < 0)
+        abort();
+    fdebug("stat64 redirect %s to %s sb=%p\n", path, newpath ? newpath : path, sb);
+
+    VIR_MOCK_STAT_HOOK;
+
+    return real_stat64(newpath ? newpath : path, sb);
+}
+#endif
+
+#ifdef MOCK___XSTAT
+int
+__xstat(int ver, const char *path, struct stat *sb)
+{
+    VIR_AUTOFREE(char *) newpath = NULL;
+
+    virMockStatInit();
+
+    if (virMockStatRedirect(path, &newpath) < 0)
+        abort();
+    fdebug("__xstat redirect %s to %s sb=%p\n", path, newpath ? newpath : path, sb);
+
+    VIR_MOCK_STAT_HOOK;
+
+    return real___xstat(ver, newpath ? newpath : path, sb);
+}
+#endif
+
+#ifdef MOCK___XSTAT64
+int
+__xstat64(int ver, const char *path, struct stat64 *sb)
+{
+    VIR_AUTOFREE(char *) newpath = NULL;
+
+    virMockStatInit();
+
+    if (virMockStatRedirect(path, &newpath) < 0)
+        abort();
+    fdebug("__xstat64 redirect %s to %s sb=%p\n", path, newpath ? newpath : path, sb);
+
+    VIR_MOCK_STAT_HOOK;
+
+    return real___xstat64(ver, newpath ? newpath : path, sb);
+}
+#endif
+
+#ifdef MOCK_LSTAT
+int
+lstat(const char *path, struct stat *sb)
+{
+    VIR_AUTOFREE(char *) newpath = NULL;
+
+    virMockStatInit();
+
+    if (virMockStatRedirect(path, &newpath) < 0)
+        abort();
+    fdebug("lstat redirect %s to %s sb=%p\n", path, newpath ? newpath : path, sb);
+
+    VIR_MOCK_STAT_HOOK;
+
+    return real_lstat(newpath ? newpath : path, sb);
+}
+#endif
+
+#ifdef MOCK_LSTAT64
+int
+lstat64(const char *path, struct stat64 *sb)
+{
+    VIR_AUTOFREE(char *) newpath = NULL;
+
+    virMockStatInit();
+
+    if (virMockStatRedirect(path, &newpath) < 0)
+        abort();
+    fdebug("lstat64 redirect %s to %s sb=%p\n", path, newpath ? newpath : path, sb);
+
+    VIR_MOCK_STAT_HOOK;
+
+    return real_lstat64(newpath ? newpath : path, sb);
+}
+#endif
+
+#ifdef MOCK___LXSTAT
+int
+__lxstat(int ver, const char *path, struct stat *sb)
+{
+    VIR_AUTOFREE(char *) newpath = NULL;
+
+    virMockStatInit();
+
+    if (virMockStatRedirect(path, &newpath) < 0)
+        abort();
+    fdebug("__lxstat redirect %s to %s sb=%p\n", path, newpath ? newpath : path, sb);
+
+    VIR_MOCK_STAT_HOOK;
+
+    return real___lxstat(ver, newpath ? newpath : path, sb);
+}
+#endif
+
+#ifdef MOCK___LXSTAT64
+int
+__lxstat64(int ver, const char *path, struct stat64 *sb)
+{
+    VIR_AUTOFREE(char *) newpath = NULL;
+
+    virMockStatInit();
+
+    if (virMockStatRedirect(path, &newpath) < 0)
+        abort();
+    fdebug("__lxstat64 redirect %s to %s sb=%p\n", path, newpath ? newpath : path, sb);
+
+    VIR_MOCK_STAT_HOOK;
+
+    return real___lxstat64(ver, newpath ? newpath : path, sb);
+}
+#endif
diff --git a/tests/virpcimock.c b/tests/virpcimock.c
index ce8176cbec..7f9cdaa9b8 100644
--- a/tests/virpcimock.c
+++ b/tests/virpcimock.c
@@ -31,10 +31,6 @@
 # include "dirname.h"
 
 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 int (*real_open)(const char *path, int flags, ...);
 static int (*real_close)(int fd);
 static DIR * (*real_opendir)(const char *name);
@@ -365,15 +361,9 @@ pci_device_new_from_stub(const struct pciDevice *data)
     if (virFileMakePath(devpath) < 0)
         ABORT("Unable to create: %s", devpath);
 
-    if (real_stat && real_stat(configSrc, &sb) == 0)
+    if (stat(configSrc, &sb) == 0)
         configSrcExists = true;
 
-# ifdef HAVE___XSTAT
-    if (!configSrcExists &&
-        real___xstat && real___xstat(_STAT_VER, configSrc, &sb) == 0)
-        configSrcExists = true;
-# endif
-
     /* If there is a config file for the device within virpcitestdata dir,
      * symlink it. Otherwise create a dummy config file. */
     if (configSrcExists) {
@@ -813,8 +803,6 @@ init_syms(void)
         return;
 
     VIR_MOCK_REAL_INIT(access);
-    VIR_MOCK_REAL_INIT_ALT(lstat, __lxstat);
-    VIR_MOCK_REAL_INIT_ALT(stat, __xstat);
     VIR_MOCK_REAL_INIT(open);
     VIR_MOCK_REAL_INIT(close);
     VIR_MOCK_REAL_INIT(opendir);
@@ -896,85 +884,17 @@ access(const char *path, int mode)
     return ret;
 }
 
-# ifdef HAVE___LXSTAT
-int
-__lxstat(int ver, const char *path, struct stat *sb)
-{
-    int ret;
-
-    init_syms();
-
-    if (STRPREFIX(path, SYSFS_PCI_PREFIX)) {
-        char *newpath;
-        if (getrealpath(&newpath, path) < 0)
-            return -1;
-        ret = real___lxstat(ver, newpath, sb);
-        VIR_FREE(newpath);
-    } else {
-        ret = real___lxstat(ver, path, sb);
-    }
-    return ret;
-}
-# endif /* HAVE___LXSTAT */
-
-int
-lstat(const char *path, struct stat *sb)
-{
-    int ret;
-
-    init_syms();
-
-    if (STRPREFIX(path, SYSFS_PCI_PREFIX)) {
-        char *newpath;
-        if (getrealpath(&newpath, path) < 0)
-            return -1;
-        ret = real_lstat(newpath, sb);
-        VIR_FREE(newpath);
-    } else {
-        ret = real_lstat(path, sb);
-    }
-    return ret;
-}
 
-# ifdef HAVE___XSTAT
-int
-__xstat(int ver, const char *path, struct stat *sb)
+static int
+virMockStatRedirect(const char *path, char **newpath)
 {
-    int ret;
-
-    init_syms();
-
     if (STRPREFIX(path, SYSFS_PCI_PREFIX)) {
-        char *newpath;
-        if (getrealpath(&newpath, path) < 0)
+        if (getrealpath(newpath, path) < 0)
             return -1;
-        ret = real___xstat(ver, newpath, sb);
-        VIR_FREE(newpath);
-    } else {
-        ret = real___xstat(ver, path, sb);
     }
-    return ret;
+    return 0;
 }
-# endif /* HAVE___XSTAT */
 
-int
-stat(const char *path, struct stat *sb)
-{
-    int ret;
-
-    init_syms();
-
-    if (STRPREFIX(path, SYSFS_PCI_PREFIX)) {
-        char *newpath;
-        if (getrealpath(&newpath, path) < 0)
-            return -1;
-        ret = real_stat(newpath, sb);
-        VIR_FREE(newpath);
-    } else {
-        ret = real_stat(path, sb);
-    }
-    return ret;
-}
 
 int
 open(const char *path, int flags, ...)
@@ -1058,6 +978,9 @@ virFileCanonicalizePath(const char *path)
 
     return ret;
 }
+
+# include "virmockstathelpers.c"
+
 #else
 /* Nothing to override on this platform */
 #endif
diff --git a/tests/virtestmock.c b/tests/virtestmock.c
index 3049c90789..bc62312444 100644
--- a/tests/virtestmock.c
+++ b/tests/virtestmock.c
@@ -36,33 +36,9 @@
 #include "viralloc.h"
 #include "virfile.h"
 
-/* stat can be a macro as follows:
- *
- *   #define stat stat64
- *
- * This wouldn't fly with our mock. Make sure that the macro (and
- * all its friends) are undefined. We don't want anybody mangling
- * our code. */
-#undef stat
-#undef stat64
-#undef __xstat
-#undef __xstat64
-#undef lstat
-#undef lstat64
-#undef __lxstat
-#undef __lxstat64
-
 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_stat64)(const char *path, void *sb);
-static int (*real___xstat)(int ver, const char *path, struct stat *sb);
-static int (*real___xstat64)(int ver, const char *path, void *sb);
-static int (*real_lstat)(const char *path, struct stat *sb);
-static int (*real_lstat64)(const char *path, void *sb);
-static int (*real___lxstat)(int ver, const char *path, struct stat *sb);
-static int (*real___lxstat64)(int ver, const char *path, void *sb);
 static int (*real_connect)(int fd, const struct sockaddr *addr, socklen_t addrlen);
 
 static const char *progname;
@@ -78,10 +54,6 @@ static void init_syms(void)
     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(stat64, __xstat64);
-    VIR_MOCK_REAL_INIT_ALT(lstat, __lxstat);
-    VIR_MOCK_REAL_INIT_ALT(lstat64, __lxstat64);
     VIR_MOCK_REAL_INIT(connect);
 }
 
@@ -217,119 +189,15 @@ int access(const char *path, int mode)
     return real_access(path, mode);
 }
 
-/* Okay, the following ifdef rain forest may look messy at a
- * first glance. But here's the thing: during run time linking of
- * a binary, stat() may not be actually needing symbol stat. It
- * might as well not had been stat() in the first place (see the
- * reasoning at the beginning of this file). However, if we would
- * expose stat symbol here, we will poison the well and trick
- * dynamic linker into thinking we are some old binary that still
- * uses the symbol. So whenever code from upper layers calls
- * stat(), the control would get here, but real_stat can actually
- * be a NULL pointer because newer glibc have __xstat instead.
- * Worse, it can have __xstat64 instead __xstat.
- *
- * Anyway, these ifdefs are there to implement the following
- * preference function:
- *
- * stat < stat64 < __xstat < __xstat64
- *
- * It's the same story with lstat.
- * Also, I feel sorry for you that you had to read this.
- */
-#if defined(HAVE_STAT) && !defined(HAVE___XSTAT)
-int stat(const char *path, struct stat *sb)
-{
-    init_syms();
 
-    checkPath(path, "stat");
+#define VIR_MOCK_STAT_HOOK CHECK_PATH(path)
 
-    return real_stat(path, sb);
-}
-#endif
+#include "virmockstathelpers.c"
 
-#if defined(HAVE_STAT64) && !defined(HAVE___XSTAT64)
-int stat64(const char *path, struct stat64 *sb)
+static int virMockStatRedirect(const char *path ATTRIBUTE_UNUSED, char **newpath ATTRIBUTE_UNUSED)
 {
-    init_syms();
-
-    checkPath(path, "stat");
-
-    return real_stat64(path, sb);
+    return 0;
 }
-#endif
-
-#if defined(HAVE___XSTAT) && !defined(HAVE___XSTAT64)
-int
-__xstat(int ver, const char *path, struct stat *sb)
-{
-    init_syms();
-
-    checkPath(path, "stat");
-
-    return real___xstat(ver, path, sb);
-}
-#endif
-
-#if defined(HAVE___XSTAT64)
-int
-__xstat64(int ver, const char *path, struct stat64 *sb)
-{
-    init_syms();
-
-    checkPath(path, "stat");
-
-    return real___xstat64(ver, path, sb);
-}
-#endif
-
-#if defined(HAVE_LSTAT) && !defined(HAVE___LXSTAT)
-int
-lstat(const char *path, struct stat *sb)
-{
-    init_syms();
-
-    checkPath(path, "lstat");
-
-    return real_lstat(path, sb);
-}
-#endif
-
-#if defined(HAVE_LSTAT64) && !defined(HAVE___LXSTAT64)
-int
-lstat64(const char *path, struct stat64 *sb)
-{
-    init_syms();
-
-    checkPath(path, "lstat");
-
-    return real_lstat64(path, sb);
-}
-#endif
-
-#if defined(HAVE___LXSTAT) && !defined(HAVE___LXSTAT64)
-int
-__lxstat(int ver, const char *path, struct stat *sb)
-{
-    init_syms();
-
-    checkPath(path, "lstat");
-
-    return real___lxstat(ver, path, sb);
-}
-#endif
-
-#if defined(HAVE___LXSTAT64)
-int
-__lxstat64(int ver, const char *path, struct stat64 *sb)
-{
-    init_syms();
-
-    checkPath(path, "lstat");
-
-    return real___lxstat64(ver, path, sb);
-}
-#endif
 
 
 int connect(int sockfd, const struct sockaddr *addr, socklen_t addrlen)
-- 
2.20.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] tests: fix mocking of stat() / lstat() functions
Posted by Daniel P. Berrangé 4 years, 12 months ago
On Mon, Apr 01, 2019 at 06:03:29PM +0100, Daniel P. Berrangé wrote:
> Quite a few of the tests have a need to mock the stat() / lstat()
> functions and they are taking somewhat different & inconsistent
> approaches none of which are actually fully correct. This is shown
> by fact that 'make check' fails on 32-bit hosts. Investigation
> revealed that the code was calling into the native C library impl,
> not getting intercepted by our mocks.
> 
> The POSIX stat() function might resolve to any number of different
> symbols in the C library.
> 
> The may be an additional stat64() function exposed by the headers
> too.
> 
> On 64-bit hosts the stat & stat64 functions are identical, always
> refering to the 64-bit ABI.
> 
> On 32-bit hosts they refer to the 32-bit & 64-bit ABIs respectively.
> 
> Libvirt uses _FILE_OFFSET_BITS=64 on 32-bit hosts, which causes the
> C library to transparently rewrite stat() calls to be stat64() calls.
> Libvirt will never see the 32-bit ABI from the traditional stat()
> call. We cannot assume this rewriting is done using a macro. It might
> be, but on GLibC it is done with a magic __asm__ statement to apply
> the rewrite at link time instead of at preprocessing.
> 
> In GLibC there may be two additional functions exposed by the headers,
> __xstat() and __xstat64(). When these exist, stat() and stat64() are
> transparently rewritten to call __xstat() and __xstat64() respectively.
> The former symbols will not actally exist in the library at all, only
> the header. The leading "__" indicates the symbols are a private impl
> detail of the C library that applications should not care about.
> Unfortunately, because we are trying to mock replace the C library,
> we need to know about this internal impl detail.
> 
> With all this in mind the list of functions we have to mock will depend
> on several factors
> 
>  - If _FILE_OFFSET_BITS is set, then we are on a 32-bit host, and we
>    only need to mock stat64 and __xstat64. The other stat / __xstat
>    functions exist, but we'll never call them so they can be ignored
>    for mocking.
> 
>  - If _FILE_OFFSET_BITS is not set, then we are on a 64-bit host and
>    we should mock stat, stat64, __xstat & __xstat64. Either may be
>    called by app code.
> 
>  - If __xstat & __xstat64 exist, then stat & stat64 will not exist
>    as symbols in the library, so the latter should not be mocked.
> 
> The same all applies to lstat()
> 
> These rules are complex enough that we don't want to duplicate them
> across every mock file, so this centralizes all the logic in a helper
> file virmockstathelper.c that should be #included when needed. The
> code merely need to provide a filename rewriting callback called
> virMockStatRedirect(). Optionally VIR_MOCK_STAT_HOOK can be defined
> as a macro if further processing is needed inline.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  build-aux/mock-noinline.pl |   3 +
>  cfg.mk                     |   4 +-
>  tests/qemusecuritymock.c   | 131 +++++-----------
>  tests/vircgroupmock.c      | 146 +++--------------
>  tests/virfilewrapper.c     |  85 ++--------
>  tests/virmock.h            |  11 --
>  tests/virmockstathelpers.c | 310 +++++++++++++++++++++++++++++++++++++
>  tests/virpcimock.c         |  93 +----------
>  tests/virtestmock.c        | 140 +----------------
>  9 files changed, 396 insertions(+), 527 deletions(-)
>  create mode 100644 tests/virmockstathelpers.c
> 
> NB I have tested on Fedora 32-bit and 64-bit but have not yet
> tested on other platforms, so they may break. This is to be
> investigated before pushing so we don't break other platforms
> more than they are already broken wrt mocking :-)

It passes on FreeBSD 11, but fails on RHEL-7, so need a v2 of this.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] tests: fix mocking of stat() / lstat() functions
Posted by Michal Privoznik 4 years, 12 months ago
On 4/2/19 10:58 AM, Daniel P. Berrangé wrote:
> On Mon, Apr 01, 2019 at 06:03:29PM +0100, Daniel P. Berrangé wrote:
>> Quite a few of the tests have a need to mock the stat() / lstat()
>> functions and they are taking somewhat different & inconsistent
>> approaches none of which are actually fully correct. This is shown
>> by fact that 'make check' fails on 32-bit hosts. Investigation
>> revealed that the code was calling into the native C library impl,
>> not getting intercepted by our mocks.
>>
>> The POSIX stat() function might resolve to any number of different
>> symbols in the C library.
>>
>> The may be an additional stat64() function exposed by the headers
>> too.
>>
>> On 64-bit hosts the stat & stat64 functions are identical, always
>> refering to the 64-bit ABI.
>>
>> On 32-bit hosts they refer to the 32-bit & 64-bit ABIs respectively.
>>
>> Libvirt uses _FILE_OFFSET_BITS=64 on 32-bit hosts, which causes the
>> C library to transparently rewrite stat() calls to be stat64() calls.
>> Libvirt will never see the 32-bit ABI from the traditional stat()
>> call. We cannot assume this rewriting is done using a macro. It might
>> be, but on GLibC it is done with a magic __asm__ statement to apply
>> the rewrite at link time instead of at preprocessing.
>>
>> In GLibC there may be two additional functions exposed by the headers,
>> __xstat() and __xstat64(). When these exist, stat() and stat64() are
>> transparently rewritten to call __xstat() and __xstat64() respectively.
>> The former symbols will not actally exist in the library at all, only
>> the header. The leading "__" indicates the symbols are a private impl
>> detail of the C library that applications should not care about.
>> Unfortunately, because we are trying to mock replace the C library,
>> we need to know about this internal impl detail.
>>
>> With all this in mind the list of functions we have to mock will depend
>> on several factors
>>
>>   - If _FILE_OFFSET_BITS is set, then we are on a 32-bit host, and we
>>     only need to mock stat64 and __xstat64. The other stat / __xstat
>>     functions exist, but we'll never call them so they can be ignored
>>     for mocking.
>>
>>   - If _FILE_OFFSET_BITS is not set, then we are on a 64-bit host and
>>     we should mock stat, stat64, __xstat & __xstat64. Either may be
>>     called by app code.
>>
>>   - If __xstat & __xstat64 exist, then stat & stat64 will not exist
>>     as symbols in the library, so the latter should not be mocked.
>>
>> The same all applies to lstat()
>>
>> These rules are complex enough that we don't want to duplicate them
>> across every mock file, so this centralizes all the logic in a helper
>> file virmockstathelper.c that should be #included when needed. The
>> code merely need to provide a filename rewriting callback called
>> virMockStatRedirect(). Optionally VIR_MOCK_STAT_HOOK can be defined
>> as a macro if further processing is needed inline.
>>
>> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>> ---
>>   build-aux/mock-noinline.pl |   3 +
>>   cfg.mk                     |   4 +-
>>   tests/qemusecuritymock.c   | 131 +++++-----------
>>   tests/vircgroupmock.c      | 146 +++--------------
>>   tests/virfilewrapper.c     |  85 ++--------
>>   tests/virmock.h            |  11 --
>>   tests/virmockstathelpers.c | 310 +++++++++++++++++++++++++++++++++++++
>>   tests/virpcimock.c         |  93 +----------
>>   tests/virtestmock.c        | 140 +----------------
>>   9 files changed, 396 insertions(+), 527 deletions(-)
>>   create mode 100644 tests/virmockstathelpers.c
>>
>> NB I have tested on Fedora 32-bit and 64-bit but have not yet
>> tested on other platforms, so they may break. This is to be
>> investigated before pushing so we don't break other platforms
>> more than they are already broken wrt mocking :-)
> 
> It passes on FreeBSD 11, but fails on RHEL-7, so need a v2 of this.
> 

I've just finished testing on all 32bit systems I have (fedora i686 and gentoo on rpi). It passes on all of them.


On rhel7 I see qemufirmwaretest and qemuxml2argvtest failing and I've tracked it down to qemuFirmwareBuildFileList(). When it's iterating over given directory, it can see all those json files but ent->d_type is DT_UNKNOWN:

qemuFirmwareBuildFileList (files=0x777de0, dir=0x50df88 "/usr/share/qemu/firmware") at qemu/qemu_firmware.c:912

928             if (ent->d_type != DT_REG && ent->d_type != DT_LNK)
qemuFirmwareBuildFileList 1 $ p *ent
$1 = {d_ino = 961973, d_off = 9, d_reclen = 32, d_type = 0 '\000', d_name = "40-bios.json\000\212\002\017\000\000\000\000\000\r\000\000\000\000\000\000\000(\000\000\065\060-ovmf-sb-keys.json\000\262\034\017\000\000\000\000\000\021\000\000\000\000\000\000\000(\000\000\066

So the actual bug might be in the code we're testing because it's not handling DT_UKNOWN as manpage says. This looks like a fix:



diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c
index 4ce010c..7d58122 100644
--- a/src/qemu/qemu_firmware.c
+++ b/src/qemu/qemu_firmware.c
@@ -924,8 +924,9 @@ qemuFirmwareBuildFileList(virHashTablePtr files, const char *dir)
     while ((rc = virDirRead(dirp, &ent, dir)) > 0) {
         VIR_AUTOFREE(char *) filename = NULL;
         VIR_AUTOFREE(char *) path = NULL;
+        struct stat sb;
 
-        if (ent->d_type != DT_REG && ent->d_type != DT_LNK)
+        if (ent->d_type != DT_REG && ent->d_type != DT_LNK && ent->d_type != DT_UNKNOWN)
             continue;
 
         if (STRPREFIX(ent->d_name, "."))
@@ -937,6 +938,11 @@ qemuFirmwareBuildFileList(virHashTablePtr files, const char *dir)
         if (virAsprintf(&path, "%s/%s", dir, filename) < 0)
             goto cleanup;
 
+        if (ent->d_type == DT_UNKNOWN &&
+            stat(path, &sb) >= 0 &&
+            ((sb.st_mode & S_IFMT) != S_IFREG && (sb.st_mode & S_IFMT) != S_IFLNK))
+            continue;
+
         if (virHashUpdateEntry(files, filename, path) < 0)
             goto cleanup;


So how about I send this as a separate patch and ACK yours? There are more places where DT_UNKNOWN is not handled though.


BTW: squash this into your patch:

diff --git i/tests/Makefile.am w/tests/Makefile.am
index d3cdbff8bb..1319c3b12c 100644
--- i/tests/Makefile.am
+++ w/tests/Makefile.am
@@ -146,6 +146,7 @@ EXTRA_DIST = \
        virjsondata \
        virmacmaptestdata \
        virmock.h \
+       virmockstathelpers.c \
        virnetdaemondata \
        virnetdevtestdata \
        virnwfilterbindingxml2xmldata \

Thanks for looking into this.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] tests: fix mocking of stat() / lstat() functions
Posted by Jim Fehlig 4 years, 11 months ago
On 4/2/19 3:42 AM, Michal Privoznik wrote:
> On 4/2/19 10:58 AM, Daniel P. Berrangé wrote:
>> On Mon, Apr 01, 2019 at 06:03:29PM +0100, Daniel P. Berrangé wrote:
>>> Quite a few of the tests have a need to mock the stat() / lstat()
>>> functions and they are taking somewhat different & inconsistent
>>> approaches none of which are actually fully correct. This is shown
>>> by fact that 'make check' fails on 32-bit hosts. Investigation
>>> revealed that the code was calling into the native C library impl,
>>> not getting intercepted by our mocks.
>>>
>>> The POSIX stat() function might resolve to any number of different
>>> symbols in the C library.
>>>
>>> The may be an additional stat64() function exposed by the headers
>>> too.
>>>
>>> On 64-bit hosts the stat & stat64 functions are identical, always
>>> refering to the 64-bit ABI.
>>>
>>> On 32-bit hosts they refer to the 32-bit & 64-bit ABIs respectively.
>>>
>>> Libvirt uses _FILE_OFFSET_BITS=64 on 32-bit hosts, which causes the
>>> C library to transparently rewrite stat() calls to be stat64() calls.
>>> Libvirt will never see the 32-bit ABI from the traditional stat()
>>> call. We cannot assume this rewriting is done using a macro. It might
>>> be, but on GLibC it is done with a magic __asm__ statement to apply
>>> the rewrite at link time instead of at preprocessing.
>>>
>>> In GLibC there may be two additional functions exposed by the headers,
>>> __xstat() and __xstat64(). When these exist, stat() and stat64() are
>>> transparently rewritten to call __xstat() and __xstat64() respectively.
>>> The former symbols will not actally exist in the library at all, only
>>> the header. The leading "__" indicates the symbols are a private impl
>>> detail of the C library that applications should not care about.
>>> Unfortunately, because we are trying to mock replace the C library,
>>> we need to know about this internal impl detail.
>>>
>>> With all this in mind the list of functions we have to mock will depend
>>> on several factors
>>>
>>>    - If _FILE_OFFSET_BITS is set, then we are on a 32-bit host, and we
>>>      only need to mock stat64 and __xstat64. The other stat / __xstat
>>>      functions exist, but we'll never call them so they can be ignored
>>>      for mocking.
>>>
>>>    - If _FILE_OFFSET_BITS is not set, then we are on a 64-bit host and
>>>      we should mock stat, stat64, __xstat & __xstat64. Either may be
>>>      called by app code.
>>>
>>>    - If __xstat & __xstat64 exist, then stat & stat64 will not exist
>>>      as symbols in the library, so the latter should not be mocked.
>>>
>>> The same all applies to lstat()
>>>
>>> These rules are complex enough that we don't want to duplicate them
>>> across every mock file, so this centralizes all the logic in a helper
>>> file virmockstathelper.c that should be #included when needed. The
>>> code merely need to provide a filename rewriting callback called
>>> virMockStatRedirect(). Optionally VIR_MOCK_STAT_HOOK can be defined
>>> as a macro if further processing is needed inline.
>>>
>>> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>>> ---
>>>    build-aux/mock-noinline.pl |   3 +
>>>    cfg.mk                     |   4 +-
>>>    tests/qemusecuritymock.c   | 131 +++++-----------
>>>    tests/vircgroupmock.c      | 146 +++--------------
>>>    tests/virfilewrapper.c     |  85 ++--------
>>>    tests/virmock.h            |  11 --
>>>    tests/virmockstathelpers.c | 310 +++++++++++++++++++++++++++++++++++++
>>>    tests/virpcimock.c         |  93 +----------
>>>    tests/virtestmock.c        | 140 +----------------
>>>    9 files changed, 396 insertions(+), 527 deletions(-)
>>>    create mode 100644 tests/virmockstathelpers.c
>>>
>>> NB I have tested on Fedora 32-bit and 64-bit but have not yet
>>> tested on other platforms, so they may break. This is to be
>>> investigated before pushing so we don't break other platforms
>>> more than they are already broken wrt mocking :-)
>>
>> It passes on FreeBSD 11, but fails on RHEL-7, so need a v2 of this.
>>
> 
> I've just finished testing on all 32bit systems I have (fedora i686 and gentoo on rpi). It passes on all of them.
> 
> 
> On rhel7 I see qemufirmwaretest and qemuxml2argvtest failing and I've tracked it down to qemuFirmwareBuildFileList(). When it's iterating over given directory, it can see all those json files but ent->d_type is DT_UNKNOWN:

FTR, I was also seeing the qemufirmwaretest and qemuxml2argvtest failures (on 
i586 and armv7l) and can verify they are resolved with Daniel's patch and your 
changes to qemu_firmware.c.

Regards,
Jim

> 
> qemuFirmwareBuildFileList (files=0x777de0, dir=0x50df88 "/usr/share/qemu/firmware") at qemu/qemu_firmware.c:912
> 
> 928             if (ent->d_type != DT_REG && ent->d_type != DT_LNK)
> qemuFirmwareBuildFileList 1 $ p *ent
> $1 = {d_ino = 961973, d_off = 9, d_reclen = 32, d_type = 0 '\000', d_name = "40-bios.json\000\212\002\017\000\000\000\000\000\r\000\000\000\000\000\000\000(\000\000\065\060-ovmf-sb-keys.json\000\262\034\017\000\000\000\000\000\021\000\000\000\000\000\000\000(\000\000\066
> 
> So the actual bug might be in the code we're testing because it's not handling DT_UKNOWN as manpage says. This looks like a fix:
> 
> 
> 
> diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c
> index 4ce010c..7d58122 100644
> --- a/src/qemu/qemu_firmware.c
> +++ b/src/qemu/qemu_firmware.c
> @@ -924,8 +924,9 @@ qemuFirmwareBuildFileList(virHashTablePtr files, const char *dir)
>       while ((rc = virDirRead(dirp, &ent, dir)) > 0) {
>           VIR_AUTOFREE(char *) filename = NULL;
>           VIR_AUTOFREE(char *) path = NULL;
> +        struct stat sb;
>   
> -        if (ent->d_type != DT_REG && ent->d_type != DT_LNK)
> +        if (ent->d_type != DT_REG && ent->d_type != DT_LNK && ent->d_type != DT_UNKNOWN)
>               continue;
>   
>           if (STRPREFIX(ent->d_name, "."))
> @@ -937,6 +938,11 @@ qemuFirmwareBuildFileList(virHashTablePtr files, const char *dir)
>           if (virAsprintf(&path, "%s/%s", dir, filename) < 0)
>               goto cleanup;
>   
> +        if (ent->d_type == DT_UNKNOWN &&
> +            stat(path, &sb) >= 0 &&
> +            ((sb.st_mode & S_IFMT) != S_IFREG && (sb.st_mode & S_IFMT) != S_IFLNK))
> +            continue;
> +
>           if (virHashUpdateEntry(files, filename, path) < 0)
>               goto cleanup;
> 
> 
> So how about I send this as a separate patch and ACK yours? There are more places where DT_UNKNOWN is not handled though.
> 
> 
> BTW: squash this into your patch:
> 
> diff --git i/tests/Makefile.am w/tests/Makefile.am
> index d3cdbff8bb..1319c3b12c 100644
> --- i/tests/Makefile.am
> +++ w/tests/Makefile.am
> @@ -146,6 +146,7 @@ EXTRA_DIST = \
>          virjsondata \
>          virmacmaptestdata \
>          virmock.h \
> +       virmockstathelpers.c \
>          virnetdaemondata \
>          virnetdevtestdata \
>          virnwfilterbindingxml2xmldata \
> 
> Thanks for looking into this.
> 
> Michal
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list