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@xxxxxxxxxx>
>> ---
>> src/uml/uml_driver.c | 12 +++++++++---
>> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> Reviewed-by: Daniel P. Berrangé <berrange@xxxxxxxxxx>
>
>
> 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@xxxxxxxxxx
>> https://www.redhat.com/mailman/listinfo/libvir-list
>
> Regards,
> Daniel
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
