[libvirt PATCH 0/7] tests: libxl: clean up test mocking
Refactor libxlDriverConfigNew to remove the need for mocking virFilePath and add libxlDomainGetEmulatorType to the mock to remove the need to invoke a binary for nearly every domain we parse Ján Tomko (7): testutilsxen: error out on initialization failure libxl: conf: move default keepalive settings to libxlDriverConfigNew libxl: StateInitialize: use g_autofree libxl: split out DriverConfigInit out of DriverConfigNew libxl: do not mock virFileMakePath tests: link the libxl tests with libxltestdriver.la tests: libxl: do not run the emulator src/libxl/libxl_capabilities.h | 3 +- src/libxl/libxl_conf.c | 85 ++++++++++++++++++---------------- src/libxl/libxl_conf.h | 2 + src/libxl/libxl_driver.c | 7 +-- tests/Makefile.am | 9 ++-- tests/libxlmock.c | 18 +++---- tests/testutilsxen.c | 9 +++- 7 files changed, 75 insertions(+), 58 deletions(-) -- 2.24.1
libxlDriverConfigNew can possibly fail on wrong firmware values (unlikely) or on failure to create the log directory (possible if you're debugging tests with VIR_FILE_ACCESS) Signed-off-by: Ján Tomko <jtomko@redhat.com> Fixes: 4a4132b4625778cf80acb9c92d06351b44468ac3 --- tests/testutilsxen.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/testutilsxen.c b/tests/testutilsxen.c index b73c79581d..76da33826c 100644 --- a/tests/testutilsxen.c +++ b/tests/testutilsxen.c @@ -94,7 +94,8 @@ libxlDriverPrivatePtr testXLInitDriver(void) return NULL; } - driver->config = libxlDriverConfigNew(); + if (!(driver->config = libxlDriverConfigNew())) + return NULL; driver->config->caps = testXLInitCaps(); -- 2.24.1
These hardcoded defaults do not need to be read from the file. Move them out of libxlDriverConfigLoadFile. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/libxl/libxl_conf.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 56deca7b7c..9c722342ba 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -1763,6 +1763,10 @@ libxlDriverConfigNew(void) goto error; cfg->firmwares[cfg->nfirmwares - 1]->name = g_strdup(LIBXL_FIRMWARE_DIR "/hvmloader"); + /* defaults for keepalive messages */ + cfg->keepAliveInterval = 5; + cfg->keepAliveCount = 5; + return cfg; error: @@ -1786,10 +1790,6 @@ int libxlDriverConfigLoadFile(libxlDriverConfigPtr cfg, { g_autoptr(virConf) conf = NULL; - /* defaults for keepalive messages */ - cfg->keepAliveInterval = 5; - cfg->keepAliveCount = 5; - /* Check the file is readable before opening it, otherwise * libvirt emits an error. */ -- 2.24.1
Use g_autofree to free the driver config file path. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/libxl/libxl_driver.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 2b06f1be1e..0b88c9f9d9 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -652,7 +652,7 @@ libxlStateInitialize(bool privileged, void *opaque) { libxlDriverConfigPtr cfg; - char *driverConf = NULL; + g_autofree char *driverConf = NULL; char ebuf[1024]; bool autostart = true; @@ -706,7 +706,6 @@ libxlStateInitialize(bool privileged, if (libxlDriverConfigLoadFile(cfg, driverConf) < 0) goto error; - VIR_FREE(driverConf); /* Register the callbacks providing access to libvirt's event loop */ libxl_osevent_register_hooks(cfg->ctx, &libxl_osevent_callbacks, cfg->ctx); @@ -822,7 +821,6 @@ libxlStateInitialize(bool privileged, return VIR_DRV_STATE_INIT_COMPLETE; error: - VIR_FREE(driverConf); libxlStateCleanup(); return VIR_DRV_STATE_INIT_ERROR; } -- 2.24.1
Take the parts affected by the host state out of DriverConfigNew and put them into a separate function. Adjust all the callers to call both functions. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/libxl/libxl_conf.c | 81 ++++++++++++++++++++++------------------ src/libxl/libxl_conf.h | 2 + src/libxl/libxl_driver.c | 3 ++ tests/testutilsxen.c | 3 ++ 4 files changed, 52 insertions(+), 37 deletions(-) diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 9c722342ba..907df525c5 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -1686,8 +1686,6 @@ libxlDriverConfigPtr libxlDriverConfigNew(void) { libxlDriverConfigPtr cfg; - char ebuf[1024]; - unsigned int free_mem; if (libxlConfigInitialize() < 0) return NULL; @@ -1705,41 +1703,6 @@ libxlDriverConfigNew(void) cfg->autoDumpDir = g_strdup(LIBXL_DUMP_DIR); cfg->channelDir = g_strdup(LIBXL_CHANNEL_DIR); - if (virFileMakePath(cfg->logDir) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("failed to create log dir '%s': %s"), - cfg->logDir, - virStrerror(errno, ebuf, sizeof(ebuf))); - goto error; - } - - cfg->logger = libxlLoggerNew(cfg->logDir, virLogGetDefaultPriority()); - if (!cfg->logger) { - VIR_ERROR(_("cannot create logger for libxenlight, disabling driver")); - goto error; - } - - if (libxl_ctx_alloc(&cfg->ctx, LIBXL_VERSION, 0, (xentoollog_logger *)cfg->logger)) { - VIR_ERROR(_("cannot initialize libxenlight context, probably not " - "running in a Xen Dom0, disabling driver")); - goto error; - } - - if ((cfg->verInfo = libxl_get_version_info(cfg->ctx)) == NULL) { - VIR_ERROR(_("cannot version information from libxenlight, " - "disabling driver")); - goto error; - } - cfg->version = (cfg->verInfo->xen_version_major * 1000000) + - (cfg->verInfo->xen_version_minor * 1000); - - /* This will fill xenstore info about free and dom0 memory if missing, - * should be called before starting first domain */ - if (libxl_get_free_memory(cfg->ctx, &free_mem)) { - VIR_ERROR(_("Unable to configure libxl's memory management parameters")); - goto error; - } - #ifdef DEFAULT_LOADER_NVRAM if (virFirmwareParseList(DEFAULT_LOADER_NVRAM, &cfg->firmwares, @@ -1774,6 +1737,50 @@ libxlDriverConfigNew(void) return NULL; } +int +libxlDriverConfigInit(libxlDriverConfigPtr cfg) +{ + char ebuf[1024]; + unsigned int free_mem; + + if (virFileMakePath(cfg->logDir) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("failed to create log dir '%s': %s"), + cfg->logDir, + virStrerror(errno, ebuf, sizeof(ebuf))); + return -1; + } + + cfg->logger = libxlLoggerNew(cfg->logDir, virLogGetDefaultPriority()); + if (!cfg->logger) { + VIR_ERROR(_("cannot create logger for libxenlight, disabling driver")); + return -1; + } + + if (libxl_ctx_alloc(&cfg->ctx, LIBXL_VERSION, 0, (xentoollog_logger *)cfg->logger)) { + VIR_ERROR(_("cannot initialize libxenlight context, probably not " + "running in a Xen Dom0, disabling driver")); + return -1; + } + + if ((cfg->verInfo = libxl_get_version_info(cfg->ctx)) == NULL) { + VIR_ERROR(_("cannot version information from libxenlight, " + "disabling driver")); + return -1; + } + cfg->version = (cfg->verInfo->xen_version_major * 1000000) + + (cfg->verInfo->xen_version_minor * 1000); + + /* This will fill xenstore info about free and dom0 memory if missing, + * should be called before starting first domain */ + if (libxl_get_free_memory(cfg->ctx, &free_mem)) { + VIR_ERROR(_("Unable to configure libxl's memory management parameters")); + return -1; + } + + return 0; +} + libxlDriverConfigPtr libxlDriverConfigGet(libxlDriverPrivatePtr driver) { diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h index b61c52f1a5..07b3373170 100644 --- a/src/libxl/libxl_conf.h +++ b/src/libxl/libxl_conf.h @@ -164,6 +164,8 @@ struct _libxlSavefileHeader { libxlDriverConfigPtr libxlDriverConfigNew(void); +int +libxlDriverConfigInit(libxlDriverConfigPtr cfg); libxlDriverConfigPtr libxlDriverConfigGet(libxlDriverPrivatePtr driver); diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 0b88c9f9d9..d10529ac5b 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -707,6 +707,9 @@ libxlStateInitialize(bool privileged, if (libxlDriverConfigLoadFile(cfg, driverConf) < 0) goto error; + if (libxlDriverConfigInit(cfg) < 0) + goto error; + /* Register the callbacks providing access to libvirt's event loop */ libxl_osevent_register_hooks(cfg->ctx, &libxl_osevent_callbacks, cfg->ctx); diff --git a/tests/testutilsxen.c b/tests/testutilsxen.c index 76da33826c..b1260dcebf 100644 --- a/tests/testutilsxen.c +++ b/tests/testutilsxen.c @@ -97,6 +97,9 @@ libxlDriverPrivatePtr testXLInitDriver(void) if (!(driver->config = libxlDriverConfigNew())) return NULL; + if (libxlDriverConfigInit(driver->config) < 0) + return NULL; + driver->config->caps = testXLInitCaps(); driver->xmlopt = libxlCreateXMLConf(driver); -- 2.24.1
Point the logDir to abs_builddir instead. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tests/libxlmock.c | 11 ----------- tests/testutilsxen.c | 3 +++ 2 files changed, 3 insertions(+), 11 deletions(-) diff --git a/tests/libxlmock.c b/tests/libxlmock.c index b995b34cc5..60e6b78129 100644 --- a/tests/libxlmock.c +++ b/tests/libxlmock.c @@ -94,17 +94,6 @@ VIR_MOCK_STUB_RET_ARGS(bind, const struct sockaddr *, addr, socklen_t, addrlen) -VIR_MOCK_IMPL_RET_ARGS(virFileMakePath, int, - const char *, path) -{ - /* replace log path with a writable directory */ - if (strstr(path, "/log/")) { - g_snprintf((char*)path, strlen(path), "."); - return 0; - } - return real_virFileMakePath(path); -} - VIR_MOCK_IMPL_RET_ARGS(__xstat, int, int, ver, const char *, path, diff --git a/tests/testutilsxen.c b/tests/testutilsxen.c index b1260dcebf..d50c3003da 100644 --- a/tests/testutilsxen.c +++ b/tests/testutilsxen.c @@ -97,6 +97,9 @@ libxlDriverPrivatePtr testXLInitDriver(void) if (!(driver->config = libxlDriverConfigNew())) return NULL; + g_free(driver->config->logDir); + driver->config->logDir = g_strdup(abs_builddir); + if (libxlDriverConfigInit(driver->config) < 0) return NULL; -- 2.24.1
This lets us mock functions from the libxl driver. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tests/Makefile.am | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/tests/Makefile.am b/tests/Makefile.am index ed5255b62d..761c989e86 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -505,17 +505,20 @@ libxltestdriver_la_LIBADD = $(libxl_LDADDS) xlconfigtest_SOURCES = \ xlconfigtest.c testutilsxen.c testutilsxen.h \ testutils.c testutils.h -xlconfigtest_LDADD =$(libxl_LDADDS) +xlconfigtest_LDADD = libxltestdriver.la \ + $(libxl_LDADDS) xmconfigtest_SOURCES = \ xmconfigtest.c testutilsxen.c testutilsxen.h \ testutils.c testutils.h -xmconfigtest_LDADD = $(libxl_LDADDS) +xmconfigtest_LDADD = libxltestdriver.la \ + $(libxl_LDADDS) libxlxml2domconfigtest_SOURCES = \ libxlxml2domconfigtest.c testutilsxen.c testutilsxen.h \ testutils.c testutils.h -libxlxml2domconfigtest_LDADD = $(libxl_LDADDS) $(LIBXML_LIBS) +libxlxml2domconfigtest_LDADD = libxltestdriver.la \ + $(libxl_LDADDS) $(LIBXML_LIBS) libxlmock_la_SOURCES = \ libxlmock.c -- 2.24.1
Ever since commit c5a00350 the libxl parser invokes the emulator to probe which device model to use. Commit b90c4b5 introduced a workaround that used a stable path which was very likely to result in the answer matching the default. However the test is still affected by the host state and the binary gets invoked if present. Mock the libxlDomainGetEmulatorType function to stop wasting CPU cycles every time a 'make check' is run on a system with xen installed. For example xlconfigtest gets faster by 90 % Signed-off-by: Ján Tomko <jtomko@redhat.com> Fixes: b90c4b5f505698d600303c5b4f03f5d229b329dd --- src/libxl/libxl_capabilities.h | 3 ++- tests/libxlmock.c | 7 +++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/src/libxl/libxl_capabilities.h b/src/libxl/libxl_capabilities.h index 9a7c8bf636..9efb836429 100644 --- a/src/libxl/libxl_capabilities.h +++ b/src/libxl/libxl_capabilities.h @@ -50,4 +50,5 @@ libxlMakeDomainCapabilities(virDomainCapsPtr domCaps, size_t nfirmwares); int -libxlDomainGetEmulatorType(const virDomainDef *def); +libxlDomainGetEmulatorType(const virDomainDef *def) + G_GNUC_NO_INLINE; diff --git a/tests/libxlmock.c b/tests/libxlmock.c index 60e6b78129..a36ca135f6 100644 --- a/tests/libxlmock.c +++ b/tests/libxlmock.c @@ -30,6 +30,7 @@ # include "virfile.h" # include "virsocket.h" +# include "libxl/libxl_capabilities.h" VIR_MOCK_IMPL_RET_VOID(xs_daemon_open, struct xs_handle *) @@ -123,4 +124,10 @@ VIR_MOCK_IMPL_RET_ARGS(stat, int, return real_stat(path, sb); } +int +libxlDomainGetEmulatorType(const virDomainDef *def G_GNUC_UNUSED) +{ + return LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN; +} + #endif /* WITH_LIBXL && WITH_YAJL */ -- 2.24.1
On 2/22/20 7:25 AM, Ján Tomko wrote:
Ever since commit c5a00350 the libxl parser invokes the emulator to probe which device model to use.
Commit b90c4b5 introduced a workaround that used a stable path which was very likely to result in the answer matching the default. However the test is still affected by the host state and the binary gets invoked if present.
Mock the libxlDomainGetEmulatorType function to stop wasting CPU cycles every time a 'make check' is run on a system with xen installed.
For example xlconfigtest gets faster by 90 %
Signed-off-by: Ján Tomko <jtomko@redhat.com> Fixes: b90c4b5f505698d600303c5b4f03f5d229b329dd --- src/libxl/libxl_capabilities.h | 3 ++- tests/libxlmock.c | 7 +++++++ 2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/src/libxl/libxl_capabilities.h b/src/libxl/libxl_capabilities.h index 9a7c8bf636..9efb836429 100644 --- a/src/libxl/libxl_capabilities.h +++ b/src/libxl/libxl_capabilities.h @@ -50,4 +50,5 @@ libxlMakeDomainCapabilities(virDomainCapsPtr domCaps, size_t nfirmwares);
int -libxlDomainGetEmulatorType(const virDomainDef *def); +libxlDomainGetEmulatorType(const virDomainDef *def) + G_GNUC_NO_INLINE;
Ah, I think the lack of this change caused me grief in the past while fiddling with the mocking. Regards, Jim
diff --git a/tests/libxlmock.c b/tests/libxlmock.c index 60e6b78129..a36ca135f6 100644 --- a/tests/libxlmock.c +++ b/tests/libxlmock.c @@ -30,6 +30,7 @@
# include "virfile.h" # include "virsocket.h" +# include "libxl/libxl_capabilities.h"
VIR_MOCK_IMPL_RET_VOID(xs_daemon_open, struct xs_handle *) @@ -123,4 +124,10 @@ VIR_MOCK_IMPL_RET_ARGS(stat, int, return real_stat(path, sb); }
+int +libxlDomainGetEmulatorType(const virDomainDef *def G_GNUC_UNUSED) +{ + return LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN; +} + #endif /* WITH_LIBXL && WITH_YAJL */
On 2/22/20 7:25 AM, Ján Tomko wrote:
Refactor libxlDriverConfigNew to remove the need for mocking virFilePath and add libxlDomainGetEmulatorType to the mock to remove the need to invoke a binary for nearly every domain we parse
Ján Tomko (7): testutilsxen: error out on initialization failure libxl: conf: move default keepalive settings to libxlDriverConfigNew libxl: StateInitialize: use g_autofree libxl: split out DriverConfigInit out of DriverConfigNew libxl: do not mock virFileMakePath tests: link the libxl tests with libxltestdriver.la tests: libxl: do not run the emulator
src/libxl/libxl_capabilities.h | 3 +- src/libxl/libxl_conf.c | 85 ++++++++++++++++++---------------- src/libxl/libxl_conf.h | 2 + src/libxl/libxl_driver.c | 7 +-- tests/Makefile.am | 9 ++-- tests/libxlmock.c | 18 +++---- tests/testutilsxen.c | 9 +++- 7 files changed, 75 insertions(+), 58 deletions(-)
For the series Reviewed-by: Jim Fehlig <jfehlig@suse.com> Regards, Jim
participants (2)
-
Jim Fehlig -
Ján Tomko