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 <mpriv...@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.
