[libvirt] [PATCH] tests: fix mocking of stat() / lstat() functions
Jim Fehlig
jfehlig at suse.com
Fri Apr 5 19:24:54 UTC 2019
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 at 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 at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
More information about the libvir-list
mailing list