Re: [libvirt] [PATCH v2 2/5] uml: Fix umlInotifyEvent dom object handling
On 04/19/2018 09:51 AM, Daniel P. Berrangé wrote:
On Mon, Apr 02, 2018 at 09:06:13AM -0400, John Ferlan wrote:
> The virDomainObjListFindByName will return a locked and reffed
> object. If we call virDomainObjListRemove that will unlock the
> object upon return, thus we need to relock the object before
> making the call to virDomainObjEndAPI.
>
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
> src/uml/uml_driver.c | 12 +++++++++---
> 1 file changed, 9 insertions(+), 3 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange(a)redhat.com>
Matches what we do everywhere else, but ewwww if the caller
owns the lock, and the 'dom' hasn't been freed entirely, it
is pretty evil semantics to have the lock be lost. Why
can't virDomainObjListRemove leave the lock state unchanged.
It already unlocks, then locks, then unlocks again. A final
lock would avoid such suprising semantics
Yep... 100% agree.
I'm trying to get to that type of model, but taking "baby steps" through
each of the domain object consumers. Still on list are the vmware and vz
to at least change the FindByUUID semantics.
Once that happens changing domain object Add and Remove are the next
step. I also have a series on the list undoing the lock fire dance
(Erik's term ;-)) for secret, interface, nodedev, and storage. I was
hoping to follow the same model for domain Remove.
Thanks for the review,
John
>
> diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c
> index ff2e7ac66b..b84585d728 100644
> --- a/src/uml/uml_driver.c
> +++ b/src/uml/uml_driver.c
> @@ -346,8 +346,10 @@ umlInotifyEvent(int watch,
> event = virDomainEventLifecycleNewFromObj(dom,
> VIR_DOMAIN_EVENT_STOPPED,
> VIR_DOMAIN_EVENT_STOPPED_SHUTDOWN);
> - if (!dom->persistent)
> + if (!dom->persistent) {
> virDomainObjListRemove(driver->domains, dom);
> + virObjectLock(dom);
> + }
> } else if (e.mask & (IN_CREATE | IN_MODIFY)) {
> VIR_DEBUG("Got inotify domain startup '%s'", name);
> if (virDomainObjIsActive(dom)) {
> @@ -377,8 +379,10 @@ umlInotifyEvent(int watch,
> event = virDomainEventLifecycleNewFromObj(dom,
> VIR_DOMAIN_EVENT_STOPPED,
> VIR_DOMAIN_EVENT_STOPPED_FAILED);
> - if (!dom->persistent)
> + if (!dom->persistent) {
> virDomainObjListRemove(driver->domains, dom);
> + virObjectLock(dom);
> + }
> } else if (umlIdentifyChrPTY(driver, dom) < 0) {
> VIR_WARN("Could not identify character devices for new
domain");
> umlShutdownVMDaemon(driver, dom,
> @@ -387,8 +391,10 @@ umlInotifyEvent(int watch,
> event = virDomainEventLifecycleNewFromObj(dom,
> VIR_DOMAIN_EVENT_STOPPED,
> VIR_DOMAIN_EVENT_STOPPED_FAILED);
> - if (!dom->persistent)
> + if (!dom->persistent) {
> virDomainObjListRemove(driver->domains, dom);
> + virObjectLock(dom);
> + }
> }
> }
> virDomainObjEndAPI(&dom);
> --
> 2.13.6
>
> --
> libvir-list mailing list
> libvir-list(a)redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
Regards,
Daniel