On 4/14/22 18:03, Peter Krempa wrote: > On Thu, Apr 14, 2022 at 16:44:25 +0200, Michal Privoznik wrote: >> Commit v8.0.0-409-gad81aa8ad0 added another function into >> qemuhotplugmock.c. However, it did so in a bit clumsy way: the >> function calls testQemuPrepareHostBackendChardevOne() which is >> not exported and is a part of libtest_utils_qemu.a static >> library. Fortunately, qemuhotplugtest links with it and thus we >> did not see any trouble at runtime as the symbol was resolved >> into something in the binary. The problem arose when the test is >> ran under valgrind. There the symbol is not resolved (although I >> don't fully understand why). >> >> Nevertheless, the testQemuPrepareHostBackendChardevOne() function >> can be turned into a proper mock of >> qemuProcessPrepareHostBackendChardev() (since they former was >> heavily inspired by the latter). >> >> Moreover, if the QEMU stub driver config is changed so that >> stdioLogD is false, then more code can be cleaned up: > > But we actually want to primarily test the case when virtlogd is > actually used thus 'stdioLogD' shall be true in the tests. > >> >> 1) qemuProcessPrepareHostBackendChardevHotplug() mock from >> qemuhotplugmock.c can be dropped (effectively reverting the >> original commit), > > The commit doing this was specifically part of a series where we didn't > honour the use of virtlogd when hotplugging sockets thus bypassing the > protections which virtlogd is supposed to provide. > >> >> 2) testCompareXMLToArgvCreateArgs() can call full blown >> qemuProcessPrepareHostBackendChardev() instead of open coding >> it. >> >> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx> >> --- > > [...] >> diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c >> index 105b41cbeb..e65d0cf64e 100644 >> --- a/tests/testutilsqemu.c >> +++ b/tests/testutilsqemu.c >> @@ -578,6 +578,8 @@ int qemuTestDriverInit(virQEMUDriver *driver) >> if (!driver->config) >> goto error; >> >> + driver->config->stdioLogD = false; >> + > > I don't think this change is wanted and also it's not properly > justified. We want to be testing any potential side effects of having > logd enabled as it's the default configuration even if it at this point > doesn't have impact on what we test. > Alright, so you want driver->config->stdioLogD = true to stay; fair enough. However, clearly testQemuPrepareHostBackendChardevOne() (and also it's qemu_process.c origin: qemuProcessPrepareHostBackendChardevOne()) have affect on generated cmd line. So PrepareHost infix is a bit misleading. But what I'm trying to fix is: _build/tests $ valgrind --trace-children=yes ./qemuhotplugtest ==13118== Memcheck, a memory error detector ==13118== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al. ==13118== Using Valgrind-3.18.1 and LibVEX; rerun with -h for copyright info ==13118== Command: ./qemuhotplugtest ==13118== valgrind: symbol lookup error: /.../libvirt.git/_build/tests/libqemuhotplugmock.so: undefined symbol: testQemuPrepareHostBackendChardevOne So I guess I'll need to try something else. Michal
