Re: [libvirt] [PATCH 3/8] test: Refactor test driver domain object retrieval

[Michal Privoznik]

On 16.06.2015 19:43, Peter Krempa wrote:
> Add testDomObjFromDomainLocked and reuse it together with
> testDomObjFromDomain to retrieve domain objects in the qemu driver

s/qemu/test/

> instead of open-coding it in every API.
> ---
>  src/test/test_driver.c | 409 
> +++--
>  1 file changed, 92 insertions(+), 317 deletions(-)
> 
> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> index 6613ed7..dc6e49a 100644
> --- a/src/test/test_driver.c
> +++ b/src/test/test_driver.c
> @@ -477,13 +477,12 @@ static int 
> testStoragePoolObjSetDefaults(virStoragePoolObjPtr pool);
>  static int testNodeGetInfo(virConnectPtr conn, virNodeInfoPtr info);
> 
>  static virDomainObjPtr
> -testDomObjFromDomain(virDomainPtr domain)
> +testDomObjFromDomainLocked(testConnPtr driver,
> +   virDomainPtr domain)
>  {
>  virDomainObjPtr vm;
> -testConnPtr driver = domain->conn->privateData;
>  char uuidstr[VIR_UUID_STRING_BUFLEN];
> 
> -testDriverLock(driver);
>  vm = virDomainObjListFindByUUIDRef(driver->domains, domain->uuid);
>  if (!vm) {
>  virUUIDFormat(domain->uuid, uuidstr);
> @@ -492,10 +491,22 @@ testDomObjFromDomain(virDomainPtr domain)
> uuidstr, domain->name);
>  }
> 
> -testDriverUnlock(driver);
>  return vm;
>  }
> 
> +static virDomainObjPtr
> +testDomObjFromDomain(virDomainPtr domain)
> +{
> +testConnPtr driver = domain->conn->privateData;
> +virDomainObjPtr ret;
> +
> +testDriverLock(driver);
> +ret = testDomObjFromDomainLocked(driver, domain);
> +testDriverUnlock(driver);
> +
> +return ret;
> +}
> +

I don't think this is necessary. As I've said in the previous e-mail -
domainObjList has self-locking APIs. And the driver->domains is
immutable pointer. Or am I missing something?

ACK to the rest though.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 3/8] test: Refactor test driver domain object retrieval

[Peter Krempa]

Add testDomObjFromDomainLocked and reuse it together with
testDomObjFromDomain to retrieve domain objects in the qemu driver
instead of open-coding it in every API.
---
 src/test/test_driver.c | 409 +++--
 1 file changed, 92 insertions(+), 317 deletions(-)

diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 6613ed7..dc6e49a 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -477,13 +477,12 @@ static int 
testStoragePoolObjSetDefaults(virStoragePoolObjPtr pool);
 static int testNodeGetInfo(virConnectPtr conn, virNodeInfoPtr info);

 static virDomainObjPtr
-testDomObjFromDomain(virDomainPtr domain)
+testDomObjFromDomainLocked(testConnPtr driver,
+   virDomainPtr domain)
 {
 virDomainObjPtr vm;
-testConnPtr driver = domain->conn->privateData;
 char uuidstr[VIR_UUID_STRING_BUFLEN];

-testDriverLock(driver);
 vm = virDomainObjListFindByUUIDRef(driver->domains, domain->uuid);
 if (!vm) {
 virUUIDFormat(domain->uuid, uuidstr);
@@ -492,10 +491,22 @@ testDomObjFromDomain(virDomainPtr domain)
uuidstr, domain->name);
 }

-testDriverUnlock(driver);
 return vm;
 }

+static virDomainObjPtr
+testDomObjFromDomain(virDomainPtr domain)
+{
+testConnPtr driver = domain->conn->privateData;
+virDomainObjPtr ret;
+
+testDriverLock(driver);
+ret = testDomObjFromDomainLocked(driver, domain);
+testDriverUnlock(driver);
+
+return ret;
+}
+
 static char *
 testDomainGenerateIfname(virDomainDefPtr domdef)
 {
@@ -1688,43 +1699,28 @@ static int testConnectNumOfDomains(virConnectPtr conn)

 static int testDomainIsActive(virDomainPtr dom)
 {
-testConnPtr privconn = dom->conn->privateData;
 virDomainObjPtr obj;
-int ret = -1;
+int ret;

-testDriverLock(privconn);
-obj = virDomainObjListFindByUUID(privconn->domains, dom->uuid);
-testDriverUnlock(privconn);
-if (!obj) {
-virReportError(VIR_ERR_NO_DOMAIN, NULL);
-goto cleanup;
-}
-ret = virDomainObjIsActive(obj);
+if (!(obj = testDomObjFromDomain(dom)))
+return -1;

- cleanup:
-if (obj)
-virObjectUnlock(obj);
+ret = virDomainObjIsActive(obj);
+virDomainObjEndAPI(&obj);
 return ret;
 }

 static int testDomainIsPersistent(virDomainPtr dom)
 {
-testConnPtr privconn = dom->conn->privateData;
 virDomainObjPtr obj;
-int ret = -1;
+int ret;
+
+if (!(obj = testDomObjFromDomain(dom)))
+return -1;

-testDriverLock(privconn);
-obj = virDomainObjListFindByUUID(privconn->domains, dom->uuid);
-testDriverUnlock(privconn);
-if (!obj) {
-virReportError(VIR_ERR_NO_DOMAIN, NULL);
-goto cleanup;
-}
 ret = obj->persistent;

- cleanup:
-if (obj)
-virObjectUnlock(obj);
+virDomainObjEndAPI(&obj);
 return ret;
 }

@@ -1885,13 +1881,9 @@ static int testDomainDestroy(virDomainPtr domain)
 int ret = -1;

 testDriverLock(privconn);
-privdom = virDomainObjListFindByName(privconn->domains,
- domain->name);

-if (privdom == NULL) {
-virReportError(VIR_ERR_INVALID_ARG, __FUNCTION__);
+if (!(privdom = testDomObjFromDomainLocked(privconn, domain)))
 goto cleanup;
-}

 testDomainShutdownState(domain, privdom, VIR_DOMAIN_SHUTOFF_DESTROYED);
 event = virDomainEventLifecycleNewFromObj(privdom,
@@ -1917,15 +1909,8 @@ static int testDomainResume(virDomainPtr domain)
 virObjectEventPtr event = NULL;
 int ret = -1;

-testDriverLock(privconn);
-privdom = virDomainObjListFindByName(privconn->domains,
- domain->name);
-testDriverUnlock(privconn);
-
-if (privdom == NULL) {
-virReportError(VIR_ERR_INVALID_ARG, __FUNCTION__);
-goto cleanup;
-}
+if (!(privdom = testDomObjFromDomain(domain)))
+return -1;

 if (virDomainObjGetState(privdom, NULL) != VIR_DOMAIN_PAUSED) {
 virReportError(VIR_ERR_INTERNAL_ERROR, _("domain '%s' not paused"),
@@ -1958,15 +1943,8 @@ static int testDomainSuspend(virDomainPtr domain)
 int ret = -1;
 int state;

-testDriverLock(privconn);
-privdom = virDomainObjListFindByName(privconn->domains,
- domain->name);
-testDriverUnlock(privconn);
-
-if (privdom == NULL) {
-virReportError(VIR_ERR_INVALID_ARG, __FUNCTION__);
-goto cleanup;
-}
+if (!(privdom = testDomObjFromDomain(domain)))
+return -1;

 state = virDomainObjGetState(privdom, NULL);
 if (state == VIR_DOMAIN_SHUTOFF || state == VIR_DOMAIN_PAUSED) {
@@ -2003,13 +1981,9 @@ static int testDomainShutdownFlags(virDomainPtr domain,
 virCheckFlags(0, -1);

 testDriverLock(privconn);
-privdom = virDomainObjListFindByName(privconn->domains,
- domain->name);

-