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

[Nikolay Shirokovskiy <nshirokovskiy@xxxxxxxxxxxxx>]

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/

>      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, &param);
> @@ -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