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

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(a)redhat.com&gt; >> --- >> 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(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list