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@xxxxxxxxxx>
---
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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
