Re: [PATCH 2/2] tests: Turn testQemuPrepareHostBackendChardevOne() into proper mock
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(a)redhat.com>
---
[...]
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index ed41b7a7a2..e10bb61785 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -34,6 +34,9 @@
# define LIBVIRT_QEMU_CAPSPRIV_H_ALLOW
# include "qemu/qemu_capspriv.h"
+# define LIBVIRT_QEMU_PROCESSPRIV_H_ALLOW
+# include "qemu/qemu_processpriv.h"
+
# include "testutilsqemu.h"
# define VIR_FROM_THIS VIR_FROM_QEMU
@@ -393,12 +396,7 @@ testCompareXMLToArgvCreateArgs(virQEMUDriver *drv,
VIR_QEMU_PROCESS_START_COLD) < 0)
return NULL;
- if (qemuDomainDeviceBackendChardevForeach(vm->def,
- testQemuPrepareHostBackendChardevOne,
- vm) < 0)
- return NULL;
-
- if (testQemuPrepareHostBackendChardevOne(NULL, priv->monConfig, vm) < 0)
The main idea behind functions having "qemuProcessPrepareHost" prefix is
that they are specifically _not_ called when doing any testing or other
code path (xml2argv conversion) that is not resulting in starting a VM.
In case when tests need specific preparation steps they re-implement
them so that we don't get any form of bad behaviour when the main
function touching the host gets changed.
Breaking this assumption could be bad for further changes.
+ if (qemuProcessPrepareHostBackendChardev(vm) < 0)
return NULL;
for (i = 0; i < vm->def->ndisks; i++) {
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.