[PATCH 0/3] bhyve: improve domain startup error handling
Roman Bogorodskiy (3): bhyve: error handling for monitor opening bhyve: improve domain startup error handling bhyve: clear monitor reference on close src/bhyve/bhyve_driver.c | 2 +- src/bhyve/bhyve_monitor.c | 6 ++-- src/bhyve/bhyve_process.c | 73 ++++++++++++++++++++++++--------------- src/bhyve/bhyve_process.h | 3 +- 4 files changed, 51 insertions(+), 33 deletions(-) -- 2.52.0
The virBhyveProcessStartImpl() function which implements domain start does not check if bhyve monitor open is successful. Check that and jump to the cleanup routines if it was not. Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com> --- src/bhyve/bhyve_process.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/bhyve/bhyve_process.c b/src/bhyve/bhyve_process.c index 2221d8700a..4d93ba8fac 100644 --- a/src/bhyve/bhyve_process.c +++ b/src/bhyve/bhyve_process.c @@ -411,7 +411,8 @@ virBhyveProcessStartImpl(struct _bhyveConn *driver, vm->def->id = vm->pid; virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, reason); - priv->mon = bhyveMonitorOpen(vm, driver); + if ((priv->mon = bhyveMonitorOpen(vm, driver)) == NULL) + goto cleanup; if (virBhyveDomainObjStartWorker(vm) < 0) goto cleanup; -- 2.52.0
On Sun, Jul 12, 2026 at 09:32:29 +0200, Roman Bogorodskiy wrote:
The virBhyveProcessStartImpl() function which implements domain start does not check if bhyve monitor open is successful. Check that and jump to the cleanup routines if it was not.
Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com> --- src/bhyve/bhyve_process.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Current cleanup routine for the domain start errors misses quite a few important steps. It does not set the proper domain state, it does not clean up sockets and so forth. The only thing it does is destroying of the actual domain and performing network cleanup. To properly clean all resources, replace running virBhyveProcessBuildDestroyCmd() command with executing virBhyveProcessStop() which already does the necessary steps. Extend virBhyveProcessStop() with the forceCleanup boolean argument which instructs it not to error out on some checks to account for inconsistent state after the partial startup. Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com> --- src/bhyve/bhyve_driver.c | 2 +- src/bhyve/bhyve_monitor.c | 6 ++-- src/bhyve/bhyve_process.c | 68 ++++++++++++++++++++++++--------------- src/bhyve/bhyve_process.h | 3 +- 4 files changed, 48 insertions(+), 31 deletions(-) diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c index 668e46ad37..a0d97d3f86 100644 --- a/src/bhyve/bhyve_driver.c +++ b/src/bhyve/bhyve_driver.c @@ -1019,7 +1019,7 @@ bhyveDomainDestroyFlags(virDomainPtr dom, unsigned int flags) if (virDomainObjCheckActive(vm) < 0) goto cleanup; - ret = virBhyveProcessStop(privconn, vm, VIR_DOMAIN_SHUTOFF_DESTROYED); + ret = virBhyveProcessStop(privconn, vm, VIR_DOMAIN_SHUTOFF_DESTROYED, false); event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_DESTROYED); diff --git a/src/bhyve/bhyve_monitor.c b/src/bhyve/bhyve_monitor.c index 8cf4b41ca3..a24696cad5 100644 --- a/src/bhyve/bhyve_monitor.c +++ b/src/bhyve/bhyve_monitor.c @@ -153,7 +153,7 @@ bhyveMonitorIO(int watch, int kq, int events G_GNUC_UNUSED, void *opaque) virReportError(VIR_ERR_INTERNAL_ERROR, _("Guest %1$s got signal %2$d and crashed"), name, WTERMSIG(status)); - virBhyveProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_CRASHED); + virBhyveProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_CRASHED, false); } else if (WIFEXITED(status)) { if (WEXITSTATUS(status) == 0 || mon->reboot) { /* 0 - reboot */ @@ -162,11 +162,11 @@ bhyveMonitorIO(int watch, int kq, int events G_GNUC_UNUSED, void *opaque) } else if (WEXITSTATUS(status) < 3) { /* 1 - shutdown, 2 - halt, 3 - triple fault. others - error */ VIR_INFO("Guest %s shut itself down; destroying domain.", name); - virBhyveProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_SHUTDOWN); + virBhyveProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_SHUTDOWN, false); } else { VIR_INFO("Guest %s had an error and exited with status %d; destroying domain.", name, WEXITSTATUS(status)); - virBhyveProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_UNKNOWN); + virBhyveProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_UNKNOWN, false); } } } diff --git a/src/bhyve/bhyve_process.c b/src/bhyve/bhyve_process.c index 4d93ba8fac..d501aa8bcc 100644 --- a/src/bhyve/bhyve_process.c +++ b/src/bhyve/bhyve_process.c @@ -66,7 +66,7 @@ bhyveProcessAutoDestroy(virDomainObj *vm, bhyveDomainObjPrivate *priv = vm->privateData; struct _bhyveConn *driver = priv->driver; - virBhyveProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_DESTROYED); + virBhyveProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_DESTROYED, false); if (!vm->persistent) virDomainObjListRemove(driver->domains, vm); @@ -437,18 +437,9 @@ virBhyveProcessStartImpl(struct _bhyveConn *driver, devmap_file); } - if (ret < 0) { - int exitstatus; /* Needed to avoid logging non-zero status */ - g_autoptr(virCommand) destroy_cmd = NULL; - if ((destroy_cmd = virBhyveProcessBuildDestroyCmd(driver, - vm->def)) != NULL) { - virCommandSetOutputFD(load_cmd, &logfd); - virCommandSetErrorFD(load_cmd, &logfd); - ignore_value(virCommandRun(destroy_cmd, &exitstatus)); - } - - bhyveNetCleanup(vm); - } + if (ret < 0) + ignore_value(virBhyveProcessStop(driver, vm, + VIR_DOMAIN_SHUTOFF_FAILED, true)); return ret; } @@ -634,26 +625,49 @@ bhyveProcessRemoveDomainStatus(const char *statusDir, } } +/** + * @driver: bhyve driver + * @vm: domain object + * @reason: shutoff reason + * @forceCleanup: boolean controlling cleanup + * + * Stops the domain and cleans up its resources. + * It could be used whether as a direct call or as a cleanup routine. + * In the latter case, @forceCleanup should be set to `true`, so it + * tries to clean up all resources instead of exiting early if the domain + * is not active for example. + * + * Does not run the shutdown hooks if the domain PID was not obtained. + * + * Returns 0 on success, -1 on error. + */ int virBhyveProcessStop(struct _bhyveConn *driver, virDomainObj *vm, - virDomainShutoffReason reason) + virDomainShutoffReason reason, + bool forceCleanup) { int ret = 0; size_t i = 0; g_autoptr(virCommand) cmd = NULL; bhyveDomainObjPrivate *priv = vm->privateData; + bool vm_started = false; - if (!virDomainObjIsActive(vm)) { - VIR_DEBUG("VM '%s' not active", vm->def->name); - return 0; - } + if (vm->pid != 0) + vm_started = true; - if (vm->pid == 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Invalid PID %1$d for VM"), - (int)vm->pid); - return -1; + if (!forceCleanup) { + if (!virDomainObjIsActive(vm)) { + VIR_DEBUG("VM '%s' not active", vm->def->name); + return 0; + } + + if (!vm_started) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid PID %1$d for VM"), + (int)vm->pid); + return -1; + } } /* Destroy monitor before running the actual destroy command to prevent @@ -669,7 +683,8 @@ virBhyveProcessStop(struct _bhyveConn *driver, VIR_WARN("Failed to run the domain destroy command"); } - bhyveProcessStopHook(driver, vm, VIR_HOOK_BHYVE_OP_STOPPED); + if (vm_started) + bhyveProcessStopHook(driver, vm, VIR_HOOK_BHYVE_OP_STOPPED); /* Cleanup network interfaces */ bhyveNetCleanup(vm); @@ -702,7 +717,8 @@ virBhyveProcessStop(struct _bhyveConn *driver, vm->pid = 0; vm->def->id = -1; - bhyveProcessStopHook(driver, vm, VIR_HOOK_BHYVE_OP_RELEASE); + if (vm_started) + bhyveProcessStopHook(driver, vm, VIR_HOOK_BHYVE_OP_RELEASE); virPidFileDelete(BHYVE_STATE_DIR, vm->def->name); bhyveProcessRemoveDomainStatus(BHYVE_STATE_DIR, vm->def->name); @@ -736,7 +752,7 @@ int virBhyveProcessRestart(struct _bhyveConn *driver, virDomainObj *vm) { - if (virBhyveProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_SHUTDOWN) < 0) + if (virBhyveProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_SHUTDOWN, false) < 0) return -1; if (virBhyveProcessStartImpl(driver, vm, VIR_DOMAIN_RUNNING_BOOTED) < 0) diff --git a/src/bhyve/bhyve_process.h b/src/bhyve/bhyve_process.h index bf82f748a6..339e5d429a 100644 --- a/src/bhyve/bhyve_process.h +++ b/src/bhyve/bhyve_process.h @@ -44,7 +44,8 @@ int virBhyveProcessStart(bhyveConn *driver, int virBhyveProcessStop(struct _bhyveConn *driver, virDomainObj *vm, - virDomainShutoffReason reason); + virDomainShutoffReason reason, + bool forceCleanup); int virBhyveProcessRestart(struct _bhyveConn *driver, virDomainObj *vm); -- 2.52.0
On Sun, Jul 12, 2026 at 09:32:30 +0200, Roman Bogorodskiy wrote:
Current cleanup routine for the domain start errors misses quite a few important steps. It does not set the proper domain state, it does not clean up sockets and so forth. The only thing it does is destroying of the actual domain and performing network cleanup.
To properly clean all resources, replace running virBhyveProcessBuildDestroyCmd() command with executing virBhyveProcessStop() which already does the necessary steps.
Extend virBhyveProcessStop() with the forceCleanup boolean argument which instructs it not to error out on some checks to account for inconsistent state after the partial startup.
Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com> --- src/bhyve/bhyve_driver.c | 2 +- src/bhyve/bhyve_monitor.c | 6 ++-- src/bhyve/bhyve_process.c | 68 ++++++++++++++++++++++++--------------- src/bhyve/bhyve_process.h | 3 +- 4 files changed, 48 insertions(+), 31 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Update virBhyveProcessStop() to set monitor to NULL after calling bhyveMonitorClose(). Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com> --- src/bhyve/bhyve_process.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/bhyve/bhyve_process.c b/src/bhyve/bhyve_process.c index d501aa8bcc..636d74afcc 100644 --- a/src/bhyve/bhyve_process.c +++ b/src/bhyve/bhyve_process.c @@ -673,7 +673,7 @@ virBhyveProcessStop(struct _bhyveConn *driver, /* 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); + g_clear_pointer(&priv->mon, bhyveMonitorClose); cmd = virBhyveProcessBuildDestroyCmd(driver, vm->def); if (virCommandRun(cmd, NULL) < 0) { -- 2.52.0
On Sun, Jul 12, 2026 at 09:32:31 +0200, Roman Bogorodskiy wrote:
Update virBhyveProcessStop() to set monitor to NULL after calling bhyveMonitorClose().
Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com> --- src/bhyve/bhyve_process.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
participants (2)
-
Peter Krempa -
Roman Bogorodskiy