On Thu, Aug 06, 2026 at 11:19:23AM +0200, Roman Bogorodskiy wrote:
Martin Kletzander wrote:On Thu, Aug 06, 2026 at 10:03:33AM +0200, Roman Bogorodskiy wrote: > virBhyveProcessStop() calls virBhyveDomainObjStopWorker(), which expects > the domain object to be locked. It temporarily releases the lock while > stopping the event thread and acquires it again before returning. > > bhyveMonitorIO() called the process stop and restart paths without > holding the domain lock. As a result, the lock acquired by > virBhyveDomainObjStopWorker() was never released, causing subsequent > domain API calls to hang after the guest exited. > > Lock the domain object while processing the bhyve process exit event and > release it after the stop or restart operation completes. > > Fixes: 0041788857dafa46e047c09c90039209a642cb85 ("bhyve: clean up event thread") > Signed-off-by: Roman Bogorodskiy <[email protected]> > --- > src/bhyve/bhyve_monitor.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/src/bhyve/bhyve_monitor.c b/src/bhyve/bhyve_monitor.c > index a24696cad5..a7d7588ee5 100644 > --- a/src/bhyve/bhyve_monitor.c > +++ b/src/bhyve/bhyve_monitor.c > @@ -140,11 +140,13 @@ bhyveMonitorIO(int watch, int kq, int events G_GNUC_UNUSED, void *opaque) > } > > if (kev.filter == EVFILT_PROC && (kev.fflags & NOTE_EXIT) != 0) { If this condition is false you do not lock the vm, but ... > + virObjectLock(vm); > + > if ((pid_t)kev.ident != vm->pid) { > virReportError(VIR_ERR_INTERNAL_ERROR, > _("event from unexpected proc %1$ju!=%2$ju"), > (uintmax_t)vm->pid, (uintmax_t)kev.ident); > - return; > + goto cleanup; > } > > name = vm->def->name; > @@ -169,6 +171,9 @@ bhyveMonitorIO(int watch, int kq, int events G_GNUC_UNUSED, void *opaque) > virBhyveProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_UNKNOWN, false); > } > } > + > + cleanup: > + virObjectUnlock(vm); ... you try to unlock it anyway. The commit message sounds like it might even be wanted, which I doubt. But even if it was, such functions are source of a lot of problems.Hm, it might look like that in a diff, but both virObjectLock() and virObjectUnlock() are called in body of "if (kev.filter == EVFILT_PROC && (kev.fflags & NOTE_EXIT) != 0) {". So I believe there should be no unmatched lock/unlock calls, if I'm not missing something.
Oh yeah, it sure looks like it now. I misrepresented that since having
goto labels somewhere else than at the top level of the function is a
bit misleading. The patch applies the way you said and it is correct.
I think I would still rather prefer avoiding the possible error-prone
style and maybe switch it around a bit. Few ideas:
a) Reverse the condition:
if (kev.filter != EVFILT_PROC || (kev.fflags & NOTE_EXIT) == 0)
return;
virObjectLock(vm);
if ((pid_t)kev.ident != vm->pid) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("event from unexpected proc %1$ju!=%2$ju"),
(uintmax_t)vm->pid, (uintmax_t)kev.ident);
goto cleanup;
}
...
b) Handle the critical section in a separate function:
if (kev.filter == EVFILT_PROC && (kev.fflags & NOTE_EXIT) != 0) {
virObjectLock(vm);
virBhyveProcessHandleExitEvent(vm, kev);
virObjectUnlock(vm);
}
...
Or just keep it as is if you're fine with it. It just stuck out to me
when I noticed it. So either way
Reviewed-by: Martin Kletzander <[email protected]>
signature.asc
Description: PGP signature
