On 9/26/26 19:28, Roman Bogorodskiy wrote:
In virBhyveProcessStartImpl() we call virBhyveProcessStop(), and even though we use ignore_value() it overrides the original error. So preserve last error in the cleanup routine and restore it before returning.
While here, relax error handling for devicemap removal, it is not critical enough to raise an error.
Additionally, apply a similar pattern to virBhyveProcessStart() so errors running hooks do not override domain startup errors.
Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com> --- src/bhyve/bhyve_process.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/src/bhyve/bhyve_process.c b/src/bhyve/bhyve_process.c index 65cf61c578..155037e99a 100644 --- a/src/bhyve/bhyve_process.c +++ b/src/bhyve/bhyve_process.c @@ -302,6 +302,7 @@ virBhyveProcessStartImpl(struct _bhyveConn *driver, VIR_AUTOCLOSE logfd = -1; g_autoptr(virCommand) cmd = NULL; g_autoptr(virCommand) load_cmd = NULL; + virErrorPtr save_err = NULL; bhyveDomainObjPrivate *priv = vm->privateData; g_autofree char *domain_vmm_path = NULL; virTimeBackOffVar timebackoff; @@ -430,16 +431,21 @@ virBhyveProcessStartImpl(struct _bhyveConn *driver, ret = 0;
cleanup: + if (ret < 0) + virErrorPreserveLast(&save_err); +
This check is not necessary and virErrorPreserveLast() can be called unconditionally. If there no error reported then the function is NOP (apart from setting save_err to NULL).
if (devicemap != NULL) { rc = unlink(devmap_file); if (rc < 0 && errno != ENOENT) - virReportSystemError(errno, _("cannot unlink file '%1$s'"), - devmap_file); + VIR_WARN("cannot unlink file '%s': %s", + devmap_file, g_strerror(errno)); }
- if (ret < 0) + if (ret < 0) { ignore_value(virBhyveProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, true)); + virErrorRestore(&save_err); + }
If save_err is NULL (i.e. no error was preserved when entering the cleanup label, then this is NOP. Thus, it too does not need the ret < 0 check. Additionally, virBhyveProcessStop(), well virBhyveProcessStopImpl() can be made so that it does not overwrite an error. I mean, the first thing it would call is virErrorPreserveLast() and the very last thing it would call is virErrorRestore(). This is because virBhyveProcessStop() is also called from other (cleanup) places.
return ret; } @@ -593,6 +599,8 @@ virBhyveProcessStart(bhyveConn *driver, virDomainRunningReason reason, unsigned int flags) { + virErrorPtr save_err = NULL; + if (virDomainObjSetDefTransient(driver->xmlopt, vm, NULL) < 0) return -1;
@@ -612,9 +620,11 @@ virBhyveProcessStart(bhyveConn *driver, return virBhyveProcessStartImpl(driver, vm, reason);
cleanup: + virErrorPreserveLast(&save_err); bhyveProcessStopHook(driver, vm, VIR_HOOK_BHYVE_OP_STOPPED); bhyveProcessStopHook(driver, vm, VIR_HOOK_BHYVE_OP_RELEASE); virDomainObjRemoveTransientDef(vm); + virErrorRestore(&save_err);
return -1; }
ACK to this hunk. Michal