Re: [libvirt] [REPOST PATCH 2/2] vz: Use virDomainObjListFindBy{UUID|ID}Ref

On 04/20/2018 02:53 AM, Nikolay Shirokovskiy wrote: > > > On 02.04.2018 16:21, John Ferlan wrote: >> For vzDomainLookupByID and vzDomainLookupByUUID let's >> return a locked and referenced @vm object so that callers >> can then use the common and more consistent virDomainObjEndAPI >> in order to handle cleanup rather than needing to know that the >> returned object is locked and calling virObjectUnlock. >> >> The LookupByName already returns the ref counted and locked object, >> so this will make things more consistent. >> >> Also adjust the prlsdkHandle{VmState|VmRemoved|Perf}Event APIs >> in the same manner. >> >> Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; >> --- >> src/vz/vz_driver.c | 8 ++++---- >> src/vz/vz_sdk.c | 15 ++++++++------- >> 2 files changed, 12 insertions(+), 11 deletions(-) >> >> diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c >> index bcbccf6cc8..736424897a 100644 >> --- a/src/vz/vz_driver.c >> +++ b/src/vz/vz_driver.c >> @@ -578,7 +578,7 @@ vzDomainLookupByID(virConnectPtr conn, int id) >> virDomainPtr ret = NULL; >> virDomainObjPtr dom; >> >> - dom = virDomainObjListFindByID(privconn->driver->domains, id); >> + dom = virDomainObjListFindByIDRef(privconn->driver->domains, id); >> >> if (dom == NULL) { >> virReportError(VIR_ERR_NO_DOMAIN, NULL); >> @@ -591,7 +591,7 @@ vzDomainLookupByID(virConnectPtr conn, int id) >> ret = virGetDomain(conn, dom->def->name, dom->def->uuid, dom->def->id); >> >> cleanup: >> - virObjectUnlock(dom); >> + virDomainObjEndAPI(&dom); >> return ret; >> } >> >> @@ -602,7 +602,7 @@ vzDomainLookupByUUID(virConnectPtr conn, const unsigned char *uuid) >> virDomainPtr ret = NULL; >> virDomainObjPtr dom; >> >> - dom = virDomainObjListFindByUUID(privconn->driver->domains, uuid); >> + dom = virDomainObjListFindByUUIDRef(privconn->driver->domains, uuid); >> >> if (dom == NULL) { >> char uuidstr[VIR_UUID_STRING_BUFLEN]; >> @@ -618,7 +618,7 @@ vzDomainLookupByUUID(virConnectPtr conn, const unsigned char *uuid) >> ret = virGetDomain(conn, dom->def->name, dom->def->uuid, dom->def->id); >> >> cleanup: >> - virObjectUnlock(dom); >> + virDomainObjEndAPI(&dom); >> return ret; >> } >> >> diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c >> index a5b9f2da67..b8f13f88a7 100644 >> --- a/src/vz/vz_sdk.c >> +++ b/src/vz/vz_sdk.c >> @@ -2144,7 +2144,7 @@ prlsdkHandleVmStateEvent(vzDriverPtr driver, >> virDomainEventType lvEventType = 0; >> int lvEventTypeDetails = 0; >> >> - dom = virDomainObjListFindByUUID(driver->domains, uuid); >> + dom = virDomainObjListFindByUUIDRef(driver->domains, uuid); >> if (dom == NULL) >> return; >> >> @@ -2166,7 +2166,7 @@ prlsdkHandleVmStateEvent(vzDriverPtr driver, >> >> cleanup: >> PrlHandle_Free(eventParam); >> - virObjectUnlock(dom); >> + virObjectEndAPI(&dom); > > s/virObjectEndAPI/virDomainObjEndAPI/ > Oh right - it wasn't the first one of those too /-|... Just realized vz wasn't building on my host... >> return; >> } >> >> @@ -2225,7 +2225,7 @@ prlsdkHandleVmRemovedEvent(vzDriverPtr driver, >> { >> virDomainObjPtr dom = NULL; >> >> - dom = virDomainObjListFindByUUID(driver->domains, uuid); >> + dom = virDomainObjListFindByUUIDRef(driver->domains, uuid); >> /* domain was removed from the list from the libvirt >> * API function in current connection */ >> if (dom == NULL) >> @@ -2235,6 +2235,7 @@ prlsdkHandleVmRemovedEvent(vzDriverPtr driver, >> VIR_DOMAIN_EVENT_UNDEFINED_REMOVED); >> >> virDomainObjListRemove(driver->domains, dom); >> + virDomainObjEndAPI(&dom); > > virDomainObjListRemove unlocks dom object so we should only unref here. > Actually, I'd prefer to follow what I've done elsewhere which is add the virObjectLock(dom); prior to the EndAPI call. Since we don't fail on our

virObjectUnlock calls the fact that it wasn't locked before unlocking doesn't really register; however, upcoming changes to how *ListRemove

works will do the "right" and "expected" thing, so having all the current consumers be consistent is preferred. Oh and by right and expected I mean since it receives a locked and reffed object, it should return a locked and reffed object to let the caller decide how to dispose. > > Also I doubt of usefullness of using ref/end api in vz_sdk.c as these functions > are not API calls so it looks strange to see virDomainObjEndAPI name here. > The long winded explanation just in case you've missed it in other related series... In the long run virDomainObjEndAPI is just a name chosen long ago - it ends up being the complementary call for the *FindBy* and *ListAdd calls. In the long run the *EndAPI just Unlocks and Unrefs the object. The *FindBy* calls were "inconsistent" w/r/t *Name always returned locked/reffed objects, while FindBy{UUID|ID} returned unreffed, but locked while the FindBy{UUID|ID}Ref would return the reffed/locked object. Having to "require" callers to know whether to use *EndAPI or ObjectUnlock makes things 'inconsistent' and has led to bugs. The *FindByName has been misused in a few places where only the Unlock was done meaning that object was probably never reaped. The *Add function returns a locked and 2X ref object. Some drivers add the additional ref (like done in prlsdkLoadDomain for @dom) and other drivers don't - it's been a process to make things consistent. For those that didn't take the additional ref, only virObjectLock was called after the *Add call. For those that did the *EndAPI was called. Thus leaving the object w/ 2 references (as it should have) once the Add is successful and for each FindBy call not altering that. The *Add code should return w/ 3 refs - one for each hash table add and one for existence. When the caller is "done" with it (*EndAPI), then the 2 refs remain because of the hash tables. Callers shouldn't have to know all this, but they do because they've found that Unref's at the wrong time lead to disappearing objects. All drivers handle it now, but in different ways. I'm trying to add a bit of consistency. This is all important because the *ListRemove code will remove 2 references when calling the virHashRemoveEntry for each hash table the object is in. Upon entry, ListRemove actually expects 3X ref and locked object. So removing the 2 refs is fine and the rest "fire dance" can really be avoidable. The problem is that it takes making the *FindBy* and *Add* callers to be consistent before cleaning up the ListRemove

Thanks for the review - John BTW: It's not clear to me why getDomainJobResultHelper Unlocks and Locks @dom; however, I would think that could be dangerous if something else comes in and is able to lock/change @dom in the mean time. Seems to only matter for the prlsdkUpdateDomain though. Still if that code does what appears to be some sort of job wait and the code I'm unclear about is looking for job results, perhaps not an issue - just wasn't sure and figured I'd note/mention it.