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@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);
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);
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
--
2.13.6
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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/
> 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.
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.
> 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@virtuozzo.com>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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
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@virtuozzo.com>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.