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 <[email protected]>
> ---
> 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
