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

[Nikolay Shirokovskiy]

On 20.04.2018 14:44, John Ferlan wrote:
> 
> 
> 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 at redhat.com>
>>> ---
>>>  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

Ok

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

We use 'PTHREAD_MUTEX_NORMAL' mutexes and unlocking unlocked is undefined behaviour
for such mutexes...

> 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

Ok. Thanx for explanation!

> 
> 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.

It is just like unlock/lock in qemuDomainObjEnterMonitor/qemuDomainObjExitMonitor.

Waiting for a job result can take time and holding a domain lock all this
time is undesirable. Domain lock is not meant to be hold for a long time or
event simple domain list operation will hang.

To protect domain from unserialized changes while lock is dropped there is
a job condition just like in qemu driver.

Nikolay