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
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.
>> return;
>> }
>>
>> @@ -2246,7 +2247,7 @@ prlsdkHandlePerfEvent(vzDriverPtr driver,
>> virDomainObjPtr dom = NULL;
>> vzDomObjPtr privdom = NULL;
>>
>> - if (!(dom = virDomainObjListFindByUUID(driver->domains, uuid))) {
>> + if (!(dom = virDomainObjListFindByUUIDRef(driver->domains, uuid))) {
>> PrlHandle_Free(event);
>> return;
>> }
>> @@ -2255,7 +2256,7 @@ prlsdkHandlePerfEvent(vzDriverPtr driver,
>> PrlHandle_Free(privdom->stats);
>> privdom->stats = event;
>>
>> - virObjectUnlock(dom);
>> + virDomainObjEndAPI(&dom);
>> }
>>
>> static void
>> @@ -2269,7 +2270,7 @@ prlsdkHandleMigrationProgress(vzDriverPtr driver,
>> PRL_HANDLE param = PRL_INVALID_HANDLE;
>> PRL_RESULT pret;
>>
>> - if (!(dom = virDomainObjListFindByUUID(driver->domains, uuid)))
>> + if (!(dom = virDomainObjListFindByUUIDRef(driver->domains, uuid)))
>> return;
>>
>> pret = PrlEvent_GetParam(event, 0, ¶m);
>> @@ -2283,7 +2284,7 @@ prlsdkHandleMigrationProgress(vzDriverPtr driver,
>>
>> cleanup:
>> PrlHandle_Free(param);
>> - virObjectUnlock(dom);
>> + virDomainObjEndAPI(&dom);
>> }
>>
>> static PRL_RESULT
>>
>
> Reviewed-by: Nikolay Shirokovskiy <nshirokovskiy@xxxxxxxxxxxxx>
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
