[PATCH 0/6] libxl: Misc cleanups and improvements
My main objective with this series was to decompose libxlDomainStart a bit, as it had become quite unwieldy. Along the way I made a few other small improvements to the code. Jim Fehlig (6): libxl: Drop unused 'cfg' parameter from libxlDomainSaveImageOpen libxl: Move managed save logic to libxlDomainStartNew libxl: Add a helper function to unprepare network devices libxl: Introduce libxlDomainStartPrepare libxl: Introduce libxlDomainStartPerform libxl: Add helper function for running the hook script src/libxl/libxl_domain.c | 384 ++++++++++++++++++------------------ src/libxl/libxl_domain.h | 11 +- src/libxl/libxl_driver.c | 30 +-- src/libxl/libxl_migration.c | 15 +- 4 files changed, 218 insertions(+), 222 deletions(-) -- 2.31.1
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_domain.c | 4 +--- src/libxl/libxl_domain.h | 3 +-- src/libxl/libxl_driver.c | 4 +--- 3 files changed, 3 insertions(+), 8 deletions(-) diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index af20434843..336b67b129 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -764,7 +764,6 @@ libxlDomainManagedSavePath(libxlDriverPrivate *driver, virDomainObj *vm) */ int libxlDomainSaveImageOpen(libxlDriverPrivate *driver, - libxlDriverConfig *cfg G_GNUC_UNUSED, const char *from, virDomainDef **ret_def, libxlSavefileHeader *ret_hdr) @@ -1260,8 +1259,7 @@ libxlDomainStart(libxlDriverPrivate *driver, if (virFileExists(managed_save_path)) { - managed_save_fd = libxlDomainSaveImageOpen(driver, cfg, - managed_save_path, + managed_save_fd = libxlDomainSaveImageOpen(driver, managed_save_path, &def, &hdr); if (managed_save_fd < 0) goto cleanup; diff --git a/src/libxl/libxl_domain.h b/src/libxl/libxl_domain.h index 928ee8f5cd..526c8e7332 100644 --- a/src/libxl/libxl_domain.h +++ b/src/libxl/libxl_domain.h @@ -103,11 +103,10 @@ libxlDomainManagedSavePath(libxlDriverPrivate *driver, int libxlDomainSaveImageOpen(libxlDriverPrivate *driver, - libxlDriverConfig *cfg, const char *from, virDomainDef **ret_def, libxlSavefileHeader *ret_hdr) - ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5); + ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4); int libxlDomainDestroyInternal(libxlDriverPrivate *driver, diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index c97b2fb485..838747a1e3 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -1942,7 +1942,6 @@ libxlDomainRestoreFlags(virConnectPtr conn, const char *from, const char *dxml, unsigned int flags) { libxlDriverPrivate *driver = conn->privateData; - libxlDriverConfig *cfg = libxlDriverConfigGet(driver); virDomainObj *vm = NULL; virDomainDef *def = NULL; libxlSavefileHeader hdr; @@ -1961,7 +1960,7 @@ libxlDomainRestoreFlags(virConnectPtr conn, const char *from, return -1; } - fd = libxlDomainSaveImageOpen(driver, cfg, from, &def, &hdr); + fd = libxlDomainSaveImageOpen(driver, from, &def, &hdr); if (fd < 0) goto cleanup; @@ -1995,7 +1994,6 @@ libxlDomainRestoreFlags(virConnectPtr conn, const char *from, virReportSystemError(errno, "%s", _("cannot close file")); virDomainDefFree(def); virDomainObjEndAPI(&vm); - virObjectUnref(cfg); return ret; } -- 2.31.1
the logic to check for existence of a managed save image and use it to start the VM can be moved to libxlDomainStartNew. libxlDomainStart has become unwieldy and this is a small step to make it more manageable. Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_domain.c | 97 ++++++++++++++++++++-------------------- 1 file changed, 49 insertions(+), 48 deletions(-) diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 336b67b129..e906495b64 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -1230,14 +1230,10 @@ libxlDomainStart(libxlDriverPrivate *driver, uint32_t restore_ver) { libxl_domain_config d_config; - virDomainDef *def = NULL; virObjectEvent *event = NULL; - libxlSavefileHeader hdr; int ret = -1; uint32_t domid = 0; g_autofree char *dom_xml = NULL; - g_autofree char *managed_save_path = NULL; - int managed_save_fd = -1; libxlDomainObjPrivate *priv = vm->privateData; g_autoptr(libxlDriverConfig) cfg = libxlDriverConfigGet(driver); virHostdevManager *hostdev_mgr = driver->hostdevMgr; @@ -1250,47 +1246,6 @@ libxlDomainStart(libxlDriverPrivate *driver, libxl_domain_config_init(&d_config); - /* If there is a managed saved state restore it instead of starting - * from scratch. The old state is removed once the restoring succeeded. */ - if (restore_fd < 0) { - managed_save_path = libxlDomainManagedSavePath(driver, vm); - if (managed_save_path == NULL) - goto cleanup; - - if (virFileExists(managed_save_path)) { - - managed_save_fd = libxlDomainSaveImageOpen(driver, managed_save_path, - &def, &hdr); - if (managed_save_fd < 0) - goto cleanup; - - restore_fd = managed_save_fd; - restore_ver = hdr.version; - - if (STRNEQ(vm->def->name, def->name) || - memcmp(vm->def->uuid, def->uuid, VIR_UUID_BUFLEN)) { - char vm_uuidstr[VIR_UUID_STRING_BUFLEN]; - char def_uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(vm->def->uuid, vm_uuidstr); - virUUIDFormat(def->uuid, def_uuidstr); - virReportError(VIR_ERR_OPERATION_FAILED, - _("cannot restore domain '%s' uuid %s from a file" - " which belongs to domain '%s' uuid %s"), - vm->def->name, vm_uuidstr, def->name, def_uuidstr); - goto cleanup; - } - - virDomainObjAssignDef(vm, def, true, NULL); - def = NULL; - - if (unlink(managed_save_path) < 0) - VIR_WARN("Failed to remove the managed state %s", - managed_save_path); - - vm->hasManagedSave = false; - } - } - if (virDomainObjSetDefTransient(driver->xmlopt, vm, NULL) < 0) goto cleanup; @@ -1476,8 +1431,6 @@ libxlDomainStart(libxlDriverPrivate *driver, cleanup: libxl_domain_config_dispose(&d_config); - virDomainDefFree(def); - VIR_FORCE_CLOSE(managed_save_fd); return ret; } @@ -1486,7 +1439,55 @@ libxlDomainStartNew(libxlDriverPrivate *driver, virDomainObj *vm, bool start_paused) { - return libxlDomainStart(driver, vm, start_paused, -1, LIBXL_SAVE_VERSION); + g_autofree char *managed_save_path = NULL; + int restore_fd = -1; + virDomainDef *def = NULL; + libxlSavefileHeader hdr; + uint32_t restore_ver = LIBXL_SAVE_VERSION; + int ret = -1; + + /* If there is a managed saved state restore it instead of starting + * from scratch. The old state is removed once the restoring succeeded. */ + managed_save_path = libxlDomainManagedSavePath(driver, vm); + if (managed_save_path == NULL) + return -1; + + if (virFileExists(managed_save_path)) { + restore_fd = libxlDomainSaveImageOpen(driver, managed_save_path, + &def, &hdr); + if (restore_fd < 0) + goto cleanup; + + restore_ver = hdr.version; + + if (STRNEQ(vm->def->name, def->name) || + memcmp(vm->def->uuid, def->uuid, VIR_UUID_BUFLEN)) { + char vm_uuidstr[VIR_UUID_STRING_BUFLEN]; + char def_uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(vm->def->uuid, vm_uuidstr); + virUUIDFormat(def->uuid, def_uuidstr); + virReportError(VIR_ERR_OPERATION_FAILED, + _("cannot restore domain '%s' uuid %s from a file which belongs to domain '%s' uuid %s"), + vm->def->name, vm_uuidstr, def->name, def_uuidstr); + goto cleanup; + } + + virDomainObjAssignDef(vm, def, true, NULL); + def = NULL; + + if (unlink(managed_save_path) < 0) + VIR_WARN("Failed to remove the managed state %s", + managed_save_path); + + vm->hasManagedSave = false; + } + + ret = libxlDomainStart(driver, vm, start_paused, restore_fd, restore_ver); + + cleanup: + virDomainDefFree(def); + VIR_FORCE_CLOSE(restore_fd); + return ret; } int -- 2.31.1
Move network device cleanup code from libxlDomainCleanup to a helper function for use in a subsequent patch. Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_domain.c | 49 +++++++++++++++++++++++----------------- 1 file changed, 28 insertions(+), 21 deletions(-) diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index e906495b64..f957c29d0d 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -826,6 +826,33 @@ libxlDomainSaveImageOpen(libxlDriverPrivate *driver, return -1; } +static void +libxlNetworkUnwindDevices(virDomainDef *def) +{ + if (def->nnets) { + size_t i; + + for (i = 0; i < def->nnets; i++) { + virDomainNetDef *net = def->nets[i]; + + if (net->ifname && + STRPREFIX(net->ifname, LIBXL_GENERATED_PREFIX_XEN)) + VIR_FREE(net->ifname); + + /* cleanup actual device */ + virDomainNetRemoveHostdev(def, net); + if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) { + g_autoptr(virConnect) conn = virGetConnectNetwork(); + + if (conn) + virDomainNetReleaseActualDevice(conn, def, net); + else + VIR_WARN("Unable to release network device '%s'", NULLSTR(net->ifname)); + } + } + } +} + /* * Internal domain destroy function. * @@ -923,29 +950,9 @@ libxlDomainCleanup(libxlDriverPrivate *driver, } } - if ((vm->def->nnets)) { - size_t i; - - for (i = 0; i < vm->def->nnets; i++) { - virDomainNetDef *net = vm->def->nets[i]; - - if (net->ifname && - STRPREFIX(net->ifname, LIBXL_GENERATED_PREFIX_XEN)) - VIR_FREE(net->ifname); - - /* cleanup actual device */ - virDomainNetRemoveHostdev(vm->def, net); - if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) { - if (conn || (conn = virGetConnectNetwork())) - virDomainNetReleaseActualDevice(conn, vm->def, net); - else - VIR_WARN("Unable to release network device '%s'", NULLSTR(net->ifname)); - } - } - } + libxlNetworkUnwindDevices(vm->def); file = g_strdup_printf("%s/%s.xml", cfg->stateDir, vm->def->name); - if (unlink(file) < 0 && errno != ENOENT && errno != ENOTDIR) VIR_DEBUG("Failed to remove domain XML for %s", vm->def->name); VIR_FREE(file); -- 2.31.1
On a %A in %Y, Jim Fehlig wrote:
Move network device cleanup code from libxlDomainCleanup to a helper function for use in a subsequent patch.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_domain.c | 49 +++++++++++++++++++++++----------------- 1 file changed, 28 insertions(+), 21 deletions(-)
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index e906495b64..f957c29d0d 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c
@@ -923,29 +950,9 @@ libxlDomainCleanup(libxlDriverPrivate *driver, } }
- if ((vm->def->nnets)) { - size_t i; - - for (i = 0; i < vm->def->nnets; i++) { - virDomainNetDef *net = vm->def->nets[i]; - - if (net->ifname && - STRPREFIX(net->ifname, LIBXL_GENERATED_PREFIX_XEN)) - VIR_FREE(net->ifname); - - /* cleanup actual device */ - virDomainNetRemoveHostdev(vm->def, net); - if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) { - if (conn || (conn = virGetConnectNetwork())) - virDomainNetReleaseActualDevice(conn, vm->def, net);
This removes the last use of 'conn' from this function: ../src/libxl/libxl_domain.c:919:27: error: unused variable 'conn' [-Werror,-Wunused-variable] g_autoptr(virConnect) conn = NULL; ^ Jano
- else - VIR_WARN("Unable to release network device '%s'", NULLSTR(net->ifname)); - } - } - } + libxlNetworkUnwindDevices(vm->def);
file = g_strdup_printf("%s/%s.xml", cfg->stateDir, vm->def->name); - if (unlink(file) < 0 && errno != ENOENT && errno != ENOTDIR) VIR_DEBUG("Failed to remove domain XML for %s", vm->def->name); VIR_FREE(file);
Introduce libxlDomainStartPrepare as part of decomposing libxlDomainStart. Perform all prepratory operations such as hostdevs, network devs, etc. Also ensure all such operations are properly unwound on error. Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_domain.c | 99 ++++++++++++++++++++++------------------ 1 file changed, 55 insertions(+), 44 deletions(-) diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index f957c29d0d..d564165807 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -1224,6 +1224,55 @@ libxlDomainCreateChannelPTY(virDomainDef *def, libxl_ctx *ctx) libxl_device_channel_dispose(&x_channels[i]); } +static int +libxlDomainStartPrepare(libxlDriverPrivate *driver, + virDomainObj *vm) +{ + virHostdevManager *hostdev_mgr = driver->hostdevMgr; + unsigned int hostdev_flags = VIR_HOSTDEV_SP_PCI | VIR_HOSTDEV_SP_USB; + + if (virDomainObjSetDefTransient(driver->xmlopt, vm, NULL) < 0) + return -1; + + /* Run an early hook to set-up missing devices */ + if (virHookPresent(VIR_HOOK_DRIVER_LIBXL)) { + g_autofree char *xml = virDomainDefFormat(vm->def, driver->xmlopt, 0); + int hookret; + + hookret = virHookCall(VIR_HOOK_DRIVER_LIBXL, vm->def->name, + VIR_HOOK_LIBXL_OP_PREPARE, VIR_HOOK_SUBOP_BEGIN, + NULL, xml, NULL); + /* + * If the script raised an error abort the launch + */ + if (hookret < 0) + goto error; + } + + if (virDomainLockProcessStart(driver->lockManager, + "xen:///system", + vm, + true, + NULL) < 0) + goto error; + + if (libxlNetworkPrepareDevices(vm->def) < 0) + goto error; + + if (virHostdevPrepareDomainDevices(hostdev_mgr, LIBXL_DRIVER_INTERNAL_NAME, + vm->def, hostdev_flags) < 0) + goto error; + + return 0; + + error: + libxlNetworkUnwindDevices(vm->def); + virHostdevReAttachDomainDevices(hostdev_mgr, LIBXL_DRIVER_INTERNAL_NAME, + vm->def, hostdev_flags); + virDomainObjRemoveTransientDef(vm); + return -1; +} + /* * Start a domain through libxenlight. * @@ -1243,46 +1292,15 @@ libxlDomainStart(libxlDriverPrivate *driver, g_autofree char *dom_xml = NULL; libxlDomainObjPrivate *priv = vm->privateData; g_autoptr(libxlDriverConfig) cfg = libxlDriverConfigGet(driver); - virHostdevManager *hostdev_mgr = driver->hostdevMgr; libxl_asyncprogress_how aop_console_how; libxl_domain_restore_params params; - unsigned int hostdev_flags = VIR_HOSTDEV_SP_PCI; g_autofree char *config_json = NULL; - hostdev_flags |= VIR_HOSTDEV_SP_USB; + if (libxlDomainStartPrepare(driver, vm) < 0) + return -1; libxl_domain_config_init(&d_config); - if (virDomainObjSetDefTransient(driver->xmlopt, vm, NULL) < 0) - goto cleanup; - - /* Run an early hook to set-up missing devices */ - if (virHookPresent(VIR_HOOK_DRIVER_LIBXL)) { - char *xml = virDomainDefFormat(vm->def, driver->xmlopt, 0); - int hookret; - - hookret = virHookCall(VIR_HOOK_DRIVER_LIBXL, vm->def->name, - VIR_HOOK_LIBXL_OP_PREPARE, VIR_HOOK_SUBOP_BEGIN, - NULL, xml, NULL); - VIR_FREE(xml); - - /* - * If the script raised an error abort the launch - */ - if (hookret < 0) - goto cleanup_dom; - } - - if (virDomainLockProcessStart(driver->lockManager, - "xen:///system", - vm, - true, - NULL) < 0) - goto cleanup; - - if (libxlNetworkPrepareDevices(vm->def) < 0) - goto cleanup_dom; - if (libxlBuildDomainConfig(driver->reservedGraphicsPorts, vm->def, cfg, &d_config) < 0) goto cleanup_dom; @@ -1290,10 +1308,6 @@ libxlDomainStart(libxlDriverPrivate *driver, if (cfg->autoballoon && libxlDomainFreeMem(cfg->ctx, &d_config) < 0) goto cleanup_dom; - if (virHostdevPrepareDomainDevices(hostdev_mgr, LIBXL_DRIVER_INTERNAL_NAME, - vm->def, hostdev_flags) < 0) - goto cleanup_dom; - /* now that we know it is about to start call the hook if present */ if (virHookPresent(VIR_HOOK_DRIVER_LIBXL)) { char *xml = virDomainDefFormat(vm->def, driver->xmlopt, 0); @@ -1415,7 +1429,7 @@ libxlDomainStart(libxlDriverPrivate *driver, * If the script raised an error abort the launch */ if (hookret < 0) - goto cleanup_dom; + goto destroy_dom; } event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STARTED, @@ -1424,21 +1438,18 @@ libxlDomainStart(libxlDriverPrivate *driver, VIR_DOMAIN_EVENT_STARTED_RESTORED); virObjectEventStateQueue(driver->domainEventState, event); - ret = 0; - goto cleanup; + libxl_domain_config_dispose(&d_config); + return 0; destroy_dom: - ret = -1; libxlDomainDestroyInternal(driver, vm); vm->def->id = -1; virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF, VIR_DOMAIN_SHUTOFF_FAILED); cleanup_dom: libxlDomainCleanup(driver, vm); - - cleanup: libxl_domain_config_dispose(&d_config); - return ret; + return -1; } int -- 2.31.1
Introduce libxlDomainStartPerform as part of decomposing libxlDomainStart. Perform all operations that are part of starting a domain. On error the domain is destroyed from libxl's perspective, but the operations perfomed in libxlDomainStartPrepare must be unwound by libxlDomainStart. Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_domain.c | 87 ++++++++++++++++++++++++---------------- 1 file changed, 52 insertions(+), 35 deletions(-) diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index d564165807..c656cba3a3 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -1273,21 +1273,16 @@ libxlDomainStartPrepare(libxlDriverPrivate *driver, return -1; } -/* - * Start a domain through libxenlight. - * - * virDomainObj *must be locked and a job acquired on invocation - */ static int -libxlDomainStart(libxlDriverPrivate *driver, - virDomainObj *vm, - bool start_paused, - int restore_fd, - uint32_t restore_ver) +libxlDomainStartPerform(libxlDriverPrivate *driver, + virDomainObj *vm, + bool start_paused, + int restore_fd, + uint32_t restore_ver) { libxl_domain_config d_config; - virObjectEvent *event = NULL; int ret = -1; + int libxlret = -1; uint32_t domid = 0; g_autofree char *dom_xml = NULL; libxlDomainObjPrivate *priv = vm->privateData; @@ -1296,17 +1291,14 @@ libxlDomainStart(libxlDriverPrivate *driver, libxl_domain_restore_params params; g_autofree char *config_json = NULL; - if (libxlDomainStartPrepare(driver, vm) < 0) - return -1; - libxl_domain_config_init(&d_config); if (libxlBuildDomainConfig(driver->reservedGraphicsPorts, vm->def, cfg, &d_config) < 0) - goto cleanup_dom; + goto cleanup; if (cfg->autoballoon && libxlDomainFreeMem(cfg->ctx, &d_config) < 0) - goto cleanup_dom; + goto cleanup; /* now that we know it is about to start call the hook if present */ if (virHookPresent(VIR_HOOK_DRIVER_LIBXL)) { @@ -1322,7 +1314,7 @@ libxlDomainStart(libxlDriverPrivate *driver, * If the script raised an error abort the launch */ if (hookret < 0) - goto cleanup_dom; + goto cleanup; } if (priv->hookRun) { @@ -1341,19 +1333,19 @@ libxlDomainStart(libxlDriverPrivate *driver, aop_console_how.for_callback = vm; aop_console_how.callback = libxlConsoleCallback; if (restore_fd < 0) { - ret = libxl_domain_create_new(cfg->ctx, &d_config, + libxlret = libxl_domain_create_new(cfg->ctx, &d_config, &domid, NULL, &aop_console_how); } else { libxl_domain_restore_params_init(¶ms); params.stream_version = restore_ver; - ret = libxlDomainCreateRestoreWrapper(cfg->ctx, &d_config, &domid, + libxlret = libxlDomainCreateRestoreWrapper(cfg->ctx, &d_config, &domid, restore_fd, ¶ms, &aop_console_how); libxl_domain_restore_params_dispose(¶ms); } virObjectLock(vm); - if (ret) { + if (libxlret) { if (restore_fd < 0) virReportError(VIR_ERR_INTERNAL_ERROR, _("libxenlight failed to create new domain '%s'"), @@ -1362,7 +1354,7 @@ libxlDomainStart(libxlDriverPrivate *driver, virReportError(VIR_ERR_INTERNAL_ERROR, _("libxenlight failed to restore domain '%s'"), d_config.c_info.name); - goto cleanup_dom; + goto cleanup; } /* @@ -1412,9 +1404,6 @@ libxlDomainStart(libxlDriverPrivate *driver, if (virDomainObjSave(vm, driver->xmlopt, cfg->stateDir) < 0) goto destroy_dom; - if (g_atomic_int_add(&driver->nactive, 1) == 0 && driver->inhibitCallback) - driver->inhibitCallback(true, driver->inhibitOpaque); - /* finally we can call the 'started' hook script if any */ if (virHookPresent(VIR_HOOK_DRIVER_LIBXL)) { char *xml = virDomainDefFormat(vm->def, driver->xmlopt, 0); @@ -1432,24 +1421,52 @@ libxlDomainStart(libxlDriverPrivate *driver, goto destroy_dom; } - event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STARTED, - restore_fd < 0 ? - VIR_DOMAIN_EVENT_STARTED_BOOTED : - VIR_DOMAIN_EVENT_STARTED_RESTORED); - virObjectEventStateQueue(driver->domainEventState, event); - - libxl_domain_config_dispose(&d_config); - return 0; + ret = 0; + goto cleanup; destroy_dom: libxlDomainDestroyInternal(driver, vm); vm->def->id = -1; virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF, VIR_DOMAIN_SHUTOFF_FAILED); - cleanup_dom: - libxlDomainCleanup(driver, vm); + cleanup: libxl_domain_config_dispose(&d_config); - return -1; + return ret; +} + +/* + * Start a domain through libxenlight. + * + * virDomainObj must be locked and a job acquired on invocation + */ +static int +libxlDomainStart(libxlDriverPrivate *driver, + virDomainObj *vm, + bool start_paused, + int restore_fd, + uint32_t restore_ver) +{ + virObjectEvent *event = NULL; + + if (libxlDomainStartPrepare(driver, vm) < 0) + return -1; + + if (libxlDomainStartPerform(driver, vm, start_paused, + restore_fd, restore_ver) < 0) { + libxlDomainCleanup(driver, vm); + return -1; + } + + if (g_atomic_int_add(&driver->nactive, 1) == 0 && driver->inhibitCallback) + driver->inhibitCallback(true, driver->inhibitOpaque); + + event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STARTED, + restore_fd < 0 ? + VIR_DOMAIN_EVENT_STARTED_BOOTED : + VIR_DOMAIN_EVENT_STARTED_RESTORED); + virObjectEventStateQueue(driver->domainEventState, event); + + return 0; } int -- 2.31.1
The same pattern of retrieving the domXML, running the hook script, and checking for error is used throughout the libxl driver. Remove some repetitive code by adding a helper function to perform these tasks. Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_domain.c | 102 ++++++++++++++---------------------- src/libxl/libxl_domain.h | 8 +++ src/libxl/libxl_driver.c | 26 +++------ src/libxl/libxl_migration.c | 15 ++---- 4 files changed, 58 insertions(+), 93 deletions(-) diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index c656cba3a3..e699c1daa9 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -853,6 +853,25 @@ libxlNetworkUnwindDevices(virDomainDef *def) } } +int +libxlDomainHookRun(libxlDriverPrivate *driver, + virDomainDef *def, + unsigned int def_fmtflags, + int hookop, + int hooksubop, + char **output) +{ + g_autofree char *xml = NULL; + + if (!virHookPresent(VIR_HOOK_DRIVER_LIBXL)) + return 0; + + xml = virDomainDefFormat(def, driver->xmlopt, def_fmtflags); + return virHookCall(VIR_HOOK_DRIVER_LIBXL, def->name, + hookop, hooksubop, + NULL, xml, output); +} + /* * Internal domain destroy function. * @@ -904,16 +923,10 @@ libxlDomainCleanup(libxlDriverPrivate *driver, hostdev_flags |= VIR_HOSTDEV_SP_USB; - /* now that we know it's stopped call the hook if present */ - if (virHookPresent(VIR_HOOK_DRIVER_LIBXL)) { - char *xml = virDomainDefFormat(vm->def, driver->xmlopt, 0); - - /* we can't stop the operation even if the script raised an error */ - ignore_value(virHookCall(VIR_HOOK_DRIVER_LIBXL, vm->def->name, - VIR_HOOK_LIBXL_OP_STOPPED, VIR_HOOK_SUBOP_END, - NULL, xml, NULL)); - VIR_FREE(xml); - } + /* Call hook with stopped operation. Ignore error and continue with cleanup */ + ignore_value(libxlDomainHookRun(driver, vm->def, 0, + VIR_HOOK_LIBXL_OP_STOPPED, + VIR_HOOK_SUBOP_END, NULL)); virHostdevReAttachDomainDevices(hostdev_mgr, LIBXL_DRIVER_INTERNAL_NAME, vm->def, hostdev_flags); @@ -957,16 +970,10 @@ libxlDomainCleanup(libxlDriverPrivate *driver, VIR_DEBUG("Failed to remove domain XML for %s", vm->def->name); VIR_FREE(file); - /* The "release" hook cleans up additional resources */ - if (virHookPresent(VIR_HOOK_DRIVER_LIBXL)) { - char *xml = virDomainDefFormat(vm->def, driver->xmlopt, 0); - - /* we can't stop the operation even if the script raised an error */ - ignore_value(virHookCall(VIR_HOOK_DRIVER_LIBXL, vm->def->name, - VIR_HOOK_LIBXL_OP_RELEASE, VIR_HOOK_SUBOP_END, - NULL, xml, NULL)); - VIR_FREE(xml); - } + /* Call hook with release operation. Ignore error and continue with cleanup */ + ignore_value(libxlDomainHookRun(driver, vm->def, 0, + VIR_HOOK_LIBXL_OP_RELEASE, + VIR_HOOK_SUBOP_END, NULL)); virDomainObjRemoveTransientDef(vm); } @@ -1235,19 +1242,10 @@ libxlDomainStartPrepare(libxlDriverPrivate *driver, return -1; /* Run an early hook to set-up missing devices */ - if (virHookPresent(VIR_HOOK_DRIVER_LIBXL)) { - g_autofree char *xml = virDomainDefFormat(vm->def, driver->xmlopt, 0); - int hookret; - - hookret = virHookCall(VIR_HOOK_DRIVER_LIBXL, vm->def->name, - VIR_HOOK_LIBXL_OP_PREPARE, VIR_HOOK_SUBOP_BEGIN, - NULL, xml, NULL); - /* - * If the script raised an error abort the launch - */ - if (hookret < 0) - goto error; - } + if (libxlDomainHookRun(driver, vm->def, 0, + VIR_HOOK_LIBXL_OP_PREPARE, + VIR_HOOK_SUBOP_BEGIN, NULL) < 0) + goto error; if (virDomainLockProcessStart(driver->lockManager, "xen:///system", @@ -1301,21 +1299,10 @@ libxlDomainStartPerform(libxlDriverPrivate *driver, goto cleanup; /* now that we know it is about to start call the hook if present */ - if (virHookPresent(VIR_HOOK_DRIVER_LIBXL)) { - char *xml = virDomainDefFormat(vm->def, driver->xmlopt, 0); - int hookret; - - hookret = virHookCall(VIR_HOOK_DRIVER_LIBXL, vm->def->name, - VIR_HOOK_LIBXL_OP_START, VIR_HOOK_SUBOP_BEGIN, - NULL, xml, NULL); - VIR_FREE(xml); - - /* - * If the script raised an error abort the launch - */ - if (hookret < 0) - goto cleanup; - } + if (libxlDomainHookRun(driver, vm->def, 0, + VIR_HOOK_LIBXL_OP_START, + VIR_HOOK_SUBOP_BEGIN, NULL) < 0) + goto cleanup; if (priv->hookRun) { char uuidstr[VIR_UUID_STRING_BUFLEN]; @@ -1405,21 +1392,10 @@ libxlDomainStartPerform(libxlDriverPrivate *driver, goto destroy_dom; /* finally we can call the 'started' hook script if any */ - if (virHookPresent(VIR_HOOK_DRIVER_LIBXL)) { - char *xml = virDomainDefFormat(vm->def, driver->xmlopt, 0); - int hookret; - - hookret = virHookCall(VIR_HOOK_DRIVER_LIBXL, vm->def->name, - VIR_HOOK_LIBXL_OP_STARTED, VIR_HOOK_SUBOP_BEGIN, - NULL, xml, NULL); - VIR_FREE(xml); - - /* - * If the script raised an error abort the launch - */ - if (hookret < 0) - goto destroy_dom; - } + if (libxlDomainHookRun(driver, vm->def, 0, + VIR_HOOK_LIBXL_OP_STARTED, + VIR_HOOK_SUBOP_BEGIN, NULL) < 0) + goto destroy_dom; ret = 0; goto cleanup; diff --git a/src/libxl/libxl_domain.h b/src/libxl/libxl_domain.h index 526c8e7332..1618c47ed5 100644 --- a/src/libxl/libxl_domain.h +++ b/src/libxl/libxl_domain.h @@ -108,6 +108,14 @@ libxlDomainSaveImageOpen(libxlDriverPrivate *driver, libxlSavefileHeader *ret_hdr) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4); +int +libxlDomainHookRun(libxlDriverPrivate *driver, + virDomainDef *def, + unsigned int def_fmtflags, + int hookop, + int hooksubop, + char **output); + int libxlDomainDestroyInternal(libxlDriverPrivate *driver, virDomainObj *vm); diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 838747a1e3..0c3c53c1d1 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -450,26 +450,12 @@ libxlReconnectDomain(virDomainObj *vm, if (virDomainObjSave(vm, driver->xmlopt, cfg->stateDir) < 0) VIR_WARN("Cannot update XML for running Xen guest %s", vm->def->name); - /* now that we know it's reconnected call the hook if present */ - if (virHookPresent(VIR_HOOK_DRIVER_LIBXL) && - STRNEQ("Domain-0", vm->def->name)) { - char *xml = virDomainDefFormat(vm->def, driver->xmlopt, 0); - int hookret; - - /* we can't stop the operation even if the script raised an error */ - hookret = virHookCall(VIR_HOOK_DRIVER_LIBXL, vm->def->name, - VIR_HOOK_LIBXL_OP_RECONNECT, VIR_HOOK_SUBOP_BEGIN, - NULL, xml, NULL); - VIR_FREE(xml); - if (hookret < 0) { - /* Stop the domain if the hook failed */ - if (virDomainObjIsActive(vm)) { - libxlDomainDestroyInternal(driver, vm); - virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF, VIR_DOMAIN_SHUTOFF_FAILED); - } - goto error; - } - } + /* now that we know it's reconnected call the hook */ + if (STRNEQ("Domain-0", vm->def->name) && + (libxlDomainHookRun(driver, vm->def, 0, + VIR_HOOK_LIBXL_OP_RECONNECT, + VIR_HOOK_SUBOP_BEGIN, NULL) < 0)) + goto error; ret = 0; diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c index 0af6e0d09a..4677f798fc 100644 --- a/src/libxl/libxl_migration.c +++ b/src/libxl/libxl_migration.c @@ -490,18 +490,13 @@ libxlDomainMigrationPrepareAny(virConnectPtr dconn, /* Let migration hook filter domain XML */ if (virHookPresent(VIR_HOOK_DRIVER_LIBXL)) { - char *xml; int hookret; - if (!(xml = virDomainDefFormat(*def, driver->xmlopt, - VIR_DOMAIN_XML_SECURE | - VIR_DOMAIN_XML_MIGRATABLE))) - return -1; - - hookret = virHookCall(VIR_HOOK_DRIVER_LIBXL, (*def)->name, - VIR_HOOK_LIBXL_OP_MIGRATE, VIR_HOOK_SUBOP_BEGIN, - NULL, xml, xmlout); - VIR_FREE(xml); + hookret = libxlDomainHookRun(driver, *def, + VIR_DOMAIN_XML_SECURE | VIR_DOMAIN_XML_MIGRATABLE, + VIR_HOOK_LIBXL_OP_MIGRATE, + VIR_HOOK_SUBOP_BEGIN, + xmlout); if (hookret < 0) { return -1; -- 2.31.1
On a %A in %Y, Jim Fehlig wrote:
My main objective with this series was to decompose libxlDomainStart a bit, as it had become quite unwieldy. Along the way I made a few other small improvements to the code.
Jim Fehlig (6): libxl: Drop unused 'cfg' parameter from libxlDomainSaveImageOpen libxl: Move managed save logic to libxlDomainStartNew libxl: Add a helper function to unprepare network devices libxl: Introduce libxlDomainStartPrepare libxl: Introduce libxlDomainStartPerform libxl: Add helper function for running the hook script
src/libxl/libxl_domain.c | 384 ++++++++++++++++++------------------ src/libxl/libxl_domain.h | 11 +- src/libxl/libxl_driver.c | 30 +-- src/libxl/libxl_migration.c | 15 +- 4 files changed, 218 insertions(+), 222 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Jano Tomko -
Jim Fehlig