Currently, there are two (at least) issues in virBhyveProcessStop(). Before going into details, a quick overview of the bhyve shutdown process. It is a two stage process*: first, the main bhyve process gets destroyed (either via an external command or within the guest), then the resources need to be cleaned up using the bhyvectl(8) tool.
The first issue is that if virCommandRun() for bhyvectl(8) fails, virBhyveProcessStop() jumps to the 'cleanup' label and misses cleaning of some resources. The second issue is more serious. Currently, monitor is closed only after running of the bhyvectl(8) command. That means that the monitor could catch the domain destroy event and try to run virBhyveProcessStop() on the same domain again, resulting in trying to release already released resources, such as the monitor itself. Address by: * Making virCommandRun() on bhyvectl(8) non-critical. Even if it fails, we try to clean up all resources. We consider the function failed (return value 1) though. * Close monitor before running bhyvectl(8) Additionally, do not verify that virBhyveProcessBuildDestroyCmd() returns non-NULL, there could be only allocation errors. Reported-by: Peter Krempa <[email protected]> Signed-off-by: Roman Bogorodskiy <[email protected]> --- src/bhyve/bhyve_process.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/bhyve/bhyve_process.c b/src/bhyve/bhyve_process.c index 26320200c5..842ff0d6fc 100644 --- a/src/bhyve/bhyve_process.c +++ b/src/bhyve/bhyve_process.c @@ -515,7 +515,7 @@ virBhyveProcessStop(struct _bhyveConn *driver, virDomainObj *vm, virDomainShutoffReason reason) { - int ret = -1; + int ret = 0; g_autoptr(virCommand) cmd = NULL; bhyveDomainObjPrivate *priv = vm->privateData; @@ -531,15 +531,19 @@ virBhyveProcessStop(struct _bhyveConn *driver, return -1; } - if (!(cmd = virBhyveProcessBuildDestroyCmd(driver, vm->def))) - return -1; - - if (virCommandRun(cmd, NULL) < 0) - goto cleanup; - + /* Destroy monitor before running the actual destroy command to prevent + * it from detecting VM shutdown and entering this cleanup routine again */ if ((priv != NULL) && (priv->mon != NULL)) bhyveMonitorClose(priv->mon); + cmd = virBhyveProcessBuildDestroyCmd(driver, vm->def); + if (virCommandRun(cmd, NULL) < 0) { + /* Only failure of the actual destroy command warrants unsuccessful return code, + * other failures are not considered critical */ + ret = -1; + VIR_WARN("Failed to run the domain destroy command"); + } + bhyveProcessStopHook(driver, vm, VIR_HOOK_BHYVE_OP_STOPPED); /* Cleanup network interfaces */ @@ -554,8 +558,6 @@ virBhyveProcessStop(struct _bhyveConn *driver, } } - ret = 0; - virCloseCallbacksDomainRemove(vm, NULL, bhyveProcessAutoDestroy); virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF, reason); @@ -563,8 +565,6 @@ virBhyveProcessStop(struct _bhyveConn *driver, vm->def->id = -1; bhyveProcessStopHook(driver, vm, VIR_HOOK_BHYVE_OP_RELEASE); - - cleanup: virPidFileDelete(BHYVE_STATE_DIR, vm->def->name); bhyveProcessRemoveDomainStatus(BHYVE_STATE_DIR, vm->def->name); -- 2.52.0
