[libvirt] [PATCH v2 2/5] uml: Fix umlInotifyEvent dom object handling

[Daniel P. Berrangé]

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 at redhat.com>
> ---
>  src/uml/uml_driver.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange at 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

> 
> 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 at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|