[libvirt] [PATCH 05/20] test: Implement virConnect(Un)RegisterCloseCallback

Marc Hartmayer posted 20 patches 7 years, 9 months ago
[libvirt] [PATCH 05/20] test: Implement virConnect(Un)RegisterCloseCallback
Posted by Marc Hartmayer 7 years, 9 months ago
Implement the hypervisor APIs virConnectRegisterCloseCallback and
virConnectUnregisterCloseCallbacks and mark this feature as supported
in testConnectSupportsFeature. This increases test test coverage of
the test suite as then the testConnectRegisterCloseCallback and
testConnectUnregisterCloseCallback (which are almost identical to the
implementations of the remote and vz driver) will be called by the
virsh tests.

Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com>
---
 src/test/test_driver.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 62 insertions(+), 1 deletion(-)

diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 203358c7017f..2773f5c758c8 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -121,6 +121,7 @@ struct _testDriver {
     virDomainObjListPtr domains;
     virNetworkObjListPtr networks;
     virObjectEventStatePtr eventState;
+    virConnectCloseCallbackDataPtr closeCallback;
 };
 typedef struct _testDriver testDriver;
 typedef testDriver *testDriverPtr;
@@ -157,6 +158,7 @@ testDriverFree(testDriverPtr driver)
     virObjectUnref(driver->ifaces);
     virObjectUnref(driver->pools);
     virObjectUnref(driver->eventState);
+    virObjectUnref(driver->closeCallback);
     virMutexUnlock(&driver->lock);
     virMutexDestroy(&driver->lock);
 
@@ -404,6 +406,7 @@ testDriverNew(void)
         .free = testDomainDefNamespaceFree,
     };
     testDriverPtr ret;
+    virConnectCloseCallbackDataPtr closeCallback;
 
     if (VIR_ALLOC(ret) < 0)
         return NULL;
@@ -423,6 +426,12 @@ testDriverNew(void)
         !(ret->pools = virStoragePoolObjListNew()))
         goto error;
 
+    closeCallback = virNewConnectCloseCallbackData();
+    if (!closeCallback)
+        goto error;
+
+    ret->closeCallback = closeCallback;
+
     virAtomicIntSet(&ret->nextDomID, 1);
 
     return ret;
@@ -6834,9 +6843,9 @@ testConnectSupportsFeature(virConnectPtr conn ATTRIBUTE_UNUSED,
                            int feature)
 {
     switch ((virDrvFeature) feature) {
+    case VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK:
     case VIR_DRV_FEATURE_REMOTE_EVENT_CALLBACK:
         return 1;
-    case VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK:
     case VIR_DRV_FEATURE_FD_PASSING:
     case VIR_DRV_FEATURE_MIGRATE_CHANGE_PROTECTION:
     case VIR_DRV_FEATURE_MIGRATION_DIRECT:
@@ -6855,6 +6864,56 @@ testConnectSupportsFeature(virConnectPtr conn ATTRIBUTE_UNUSED,
     }
 }
 
+static int
+testConnectRegisterCloseCallback(virConnectPtr conn,
+                                 virConnectCloseFunc cb,
+                                 void *opaque,
+                                 virFreeCallback freecb)
+{
+    testDriverPtr priv = conn->privateData;
+    int ret = -1;
+
+    testDriverLock(priv);
+
+    if (virConnectCloseCallbackDataGetCallback(priv->closeCallback) != NULL) {
+        virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+                       _("A close callback is already registered"));
+        goto cleanup;
+    }
+
+    virConnectCloseCallbackDataRegister(priv->closeCallback, conn, cb,
+                                        opaque, freecb);
+    ret = 0;
+
+ cleanup:
+    testDriverUnlock(priv);
+    return ret;
+}
+
+
+static int
+testConnectUnregisterCloseCallback(virConnectPtr conn,
+                                   virConnectCloseFunc cb)
+{
+    testDriverPtr priv = conn->privateData;
+    int ret = -1;
+
+    testDriverLock(priv);
+
+    if (virConnectCloseCallbackDataGetCallback(priv->closeCallback) != cb) {
+        virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+                       _("A different callback was requested"));
+        goto cleanup;
+    }
+
+    virConnectCloseCallbackDataUnregister(priv->closeCallback, cb);
+    ret = 0;
+
+ cleanup:
+    testDriverUnlock(priv);
+    return ret;
+}
+
 
 static virHypervisorDriver testHypervisorDriver = {
     .name = "Test",
@@ -6863,7 +6922,9 @@ static virHypervisorDriver testHypervisorDriver = {
     .connectGetVersion = testConnectGetVersion, /* 0.1.1 */
     .connectGetHostname = testConnectGetHostname, /* 0.6.3 */
     .connectGetMaxVcpus = testConnectGetMaxVcpus, /* 0.3.2 */
+    .connectRegisterCloseCallback = testConnectRegisterCloseCallback, /* 4.2.0 */
     .connectSupportsFeature = testConnectSupportsFeature, /* 4.2.0 */
+    .connectUnregisterCloseCallback = testConnectUnregisterCloseCallback, /* 4.2.0 */
     .nodeGetInfo = testNodeGetInfo, /* 0.1.1 */
     .nodeGetCPUStats = testNodeGetCPUStats, /* 2.3.0 */
     .nodeGetFreeMemory = testNodeGetFreeMemory, /* 2.3.0 */
-- 
2.13.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 05/20] test: Implement virConnect(Un)RegisterCloseCallback
Posted by John Ferlan 7 years, 9 months ago

On 03/08/2018 07:20 AM, Marc Hartmayer wrote:
> Implement the hypervisor APIs virConnectRegisterCloseCallback and
> virConnectUnregisterCloseCallbacks and mark this feature as supported
> in testConnectSupportsFeature. This increases test test coverage of

s/test test//

> the test suite as then the testConnectRegisterCloseCallback and
> testConnectUnregisterCloseCallback (which are almost identical to the
> implementations of the remote and vz driver) will be called by the
> virsh tests.

Oh, um, hmmm... Now I'm confused again. It would seem that the
REMOTE_CLOSE_CALLBACK would need to be somehow marked as supported for
the remote driver (e.g. patch 2); otherwise, how would virsh really be
notified if say for example vz and test weren't in the target environment.

The rest seems fine, but I'm now mystified by the need for patch 2.

John

> 
> Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com>
> ---
>  src/test/test_driver.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 62 insertions(+), 1 deletion(-)
> 
> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> index 203358c7017f..2773f5c758c8 100644
> --- a/src/test/test_driver.c
> +++ b/src/test/test_driver.c
> @@ -121,6 +121,7 @@ struct _testDriver {
>      virDomainObjListPtr domains;
>      virNetworkObjListPtr networks;
>      virObjectEventStatePtr eventState;
> +    virConnectCloseCallbackDataPtr closeCallback;
>  };
>  typedef struct _testDriver testDriver;
>  typedef testDriver *testDriverPtr;
> @@ -157,6 +158,7 @@ testDriverFree(testDriverPtr driver)
>      virObjectUnref(driver->ifaces);
>      virObjectUnref(driver->pools);
>      virObjectUnref(driver->eventState);
> +    virObjectUnref(driver->closeCallback);
>      virMutexUnlock(&driver->lock);
>      virMutexDestroy(&driver->lock);
>  
> @@ -404,6 +406,7 @@ testDriverNew(void)
>          .free = testDomainDefNamespaceFree,
>      };
>      testDriverPtr ret;
> +    virConnectCloseCallbackDataPtr closeCallback;
>  
>      if (VIR_ALLOC(ret) < 0)
>          return NULL;
> @@ -423,6 +426,12 @@ testDriverNew(void)
>          !(ret->pools = virStoragePoolObjListNew()))
>          goto error;
>  
> +    closeCallback = virNewConnectCloseCallbackData();
> +    if (!closeCallback)
> +        goto error;
> +
> +    ret->closeCallback = closeCallback;
> +
>      virAtomicIntSet(&ret->nextDomID, 1);
>  
>      return ret;
> @@ -6834,9 +6843,9 @@ testConnectSupportsFeature(virConnectPtr conn ATTRIBUTE_UNUSED,
>                             int feature)
>  {
>      switch ((virDrvFeature) feature) {
> +    case VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK:
>      case VIR_DRV_FEATURE_REMOTE_EVENT_CALLBACK:
>          return 1;
> -    case VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK:
>      case VIR_DRV_FEATURE_FD_PASSING:
>      case VIR_DRV_FEATURE_MIGRATE_CHANGE_PROTECTION:
>      case VIR_DRV_FEATURE_MIGRATION_DIRECT:
> @@ -6855,6 +6864,56 @@ testConnectSupportsFeature(virConnectPtr conn ATTRIBUTE_UNUSED,
>      }
>  }
>  
> +static int
> +testConnectRegisterCloseCallback(virConnectPtr conn,
> +                                 virConnectCloseFunc cb,
> +                                 void *opaque,
> +                                 virFreeCallback freecb)
> +{
> +    testDriverPtr priv = conn->privateData;
> +    int ret = -1;
> +
> +    testDriverLock(priv);
> +
> +    if (virConnectCloseCallbackDataGetCallback(priv->closeCallback) != NULL) {
> +        virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> +                       _("A close callback is already registered"));
> +        goto cleanup;
> +    }
> +
> +    virConnectCloseCallbackDataRegister(priv->closeCallback, conn, cb,
> +                                        opaque, freecb);
> +    ret = 0;
> +
> + cleanup:
> +    testDriverUnlock(priv);
> +    return ret;
> +}
> +
> +
> +static int
> +testConnectUnregisterCloseCallback(virConnectPtr conn,
> +                                   virConnectCloseFunc cb)
> +{
> +    testDriverPtr priv = conn->privateData;
> +    int ret = -1;
> +
> +    testDriverLock(priv);
> +
> +    if (virConnectCloseCallbackDataGetCallback(priv->closeCallback) != cb) {
> +        virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> +                       _("A different callback was requested"));
> +        goto cleanup;
> +    }
> +
> +    virConnectCloseCallbackDataUnregister(priv->closeCallback, cb);
> +    ret = 0;
> +
> + cleanup:
> +    testDriverUnlock(priv);
> +    return ret;
> +}
> +
>  
>  static virHypervisorDriver testHypervisorDriver = {
>      .name = "Test",
> @@ -6863,7 +6922,9 @@ static virHypervisorDriver testHypervisorDriver = {
>      .connectGetVersion = testConnectGetVersion, /* 0.1.1 */
>      .connectGetHostname = testConnectGetHostname, /* 0.6.3 */
>      .connectGetMaxVcpus = testConnectGetMaxVcpus, /* 0.3.2 */
> +    .connectRegisterCloseCallback = testConnectRegisterCloseCallback, /* 4.2.0 */
>      .connectSupportsFeature = testConnectSupportsFeature, /* 4.2.0 */
> +    .connectUnregisterCloseCallback = testConnectUnregisterCloseCallback, /* 4.2.0 */
>      .nodeGetInfo = testNodeGetInfo, /* 0.1.1 */
>      .nodeGetCPUStats = testNodeGetCPUStats, /* 2.3.0 */
>      .nodeGetFreeMemory = testNodeGetFreeMemory, /* 2.3.0 */
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 05/20] test: Implement virConnect(Un)RegisterCloseCallback
Posted by Daniel P. Berrangé 7 years, 9 months ago
On Thu, Mar 08, 2018 at 01:20:28PM +0100, Marc Hartmayer wrote:
> Implement the hypervisor APIs virConnectRegisterCloseCallback and
> virConnectUnregisterCloseCallbacks and mark this feature as supported
> in testConnectSupportsFeature. This increases test test coverage of
> the test suite as then the testConnectRegisterCloseCallback and
> testConnectUnregisterCloseCallback (which are almost identical to the
> implementations of the remote and vz driver) will be called by the
> virsh tests.

I'm not really understanding why this is needed at all.

For most drivers, the close callback stuff does not need any interaction
with the driver code. It is handled entirely at the RPC protocol level
by the remote driver & libvirtd.

We only needed to wire it up in the VZ driver, because VZ had a further
internal connection that could be lost, and we want to propagate that
back out from the driver as a close event, despite the main reomte
driver <-> libvirtd connection being intact.

So I don't see any test for the test driver to require special handling
for close callbacks - if you run the test driver inside libvirtd, it
would work normally. If the test driver is run outside libvirtd, there
is no network connection  that could fail and thus no close callbacks
ever need to be run.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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