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