[libvirt] [PATCH v4 2/7] virConnectRegisterCloseCallback: Cleanup 'opaque' if there is no connectRegisterCloseCallback

[Daniel P. Berrangé]

On Wed, Dec 11, 2019 at 08:11:38PM -0500, Cole Robinson wrote:
> On 11/14/19 12:44 PM, Marc Hartmayer wrote:
> > The commit 'close callback: move it to driver' (88f09b75eb99) moved
> > the responsibility for the close callback to the driver. But if the
> > driver doesn't support the connectRegisterCloseCallback API this
> > function does nothing, even no unsupported error report. This behavior
> > may lead to problems, for example memory leaks, as the caller cannot
> > differentiate whether the close callback was 'really' registered or
> > not.
> > 
> 
> 
> Full context:
> v1 subtrhead with jferlan and danpb:
> https://www.redhat.com/archives/libvir-list/2018-March/msg00906.html
> https://www.redhat.com/archives/libvir-list/2018-April/msg00116.html
> 
> v2 subthread with john:
> https://www.redhat.com/archives/libvir-list/2018-April/msg02534.html
> 
> My first thought is, why not just make this API start raising an error
> if it isn't supported?
> 
> But you tried that here:
> https://www.redhat.com/archives/libvir-list/2018-March/msg00405.html
> 
> I'm not really sure I buy the argument that we can't change the
> semantics of the API here. This is the only callback API that seems to
> not raise an explicit error. It's documented to raise an error. And
> there's possible memory leak due the ambiguity.

It can raise an error because you are only permitted to register the
close callback once - a duplicated call reports an error. Also any
other invalid parameters result in an error.  So this is not
inconsistent with the idea that registering a close callback is
supported for all drivers.

> 
> Yeah I see that virsh needs to be updated to match. In practice virsh
> shouldn't be a problem: this issue will only hit for local drivers, and
> virsh and client library will be updated together for that case.

The very fact that we need to update virsh shows that this would
be an API regression. That we only know of virsh having a problem
is not a valid reason. It is only by luck that virt-viewer doesn't
have the same problem, because for inexplicable reasons we didn't
handling the error as an error condition.

> BUT, if we stick with this direction, then we need to extend the doc
> text here to describe all of this:
> 
> * Returns -1 if the driver can support close callback, but registering
> one failed. User must free opaque?
> * Returns 0 if the driver does not support close callback. We will free
> data for you
> * Returns 0 if the driver successfully registered a close callback. When
> that callback is triggered, opaque will be free'd
> 
> But that sounds pretty nutty IMO :/

This is giving apps an uncessary level of impl detail for their
needs.

 * Returns -1: if a close callback was already registered, or
   if one of the parameters was invalid.
 * Returns 0: if the close callback was successfully registered


The driver specific caveat is described earlier in the docs, that
not all drivers will invoke the close callback, as some may not
ever be in a situation where there is a connection is closed. Not
ever invoking the callback is not a bug, as it simply means the
error condition the callback is designed to report has not
arisen.

To fix the leak of te opaque data, we need to partially revert
the change that caused this leak in the first place
88f09b75eb99415c7f2ce3d1b010600ba8e37580.

That commit introduces some per driver handling into the close
callbacks. This is conceptually fine. The mistake was that the

   virConnectCloseCallbackDataPtr closeCallback;

field was moved out of virConnectPtr, and into the per-driver
private structs. This meant that we no longer kept track of
the callback in other drivers, and thus no longer free'd the
opaque data when calling Unregister.

So from the public API entry point I think we need approx
this change:

diff --git a/src/datatypes.h b/src/datatypes.h
index 2d0407f7ec..924ef8d8e9 100644
--- a/src/datatypes.h
+++ b/src/datatypes.h
@@ -546,6 +546,9 @@ struct _virConnect {
     virError err;           /* the last error */
     virErrorFunc handler;   /* associated handler */
     void *userData;         /* the user data */
+
+    /* Per-connection close callback */
+    virConnectCloseCallbackDataPtr closeCallback;
 };
 
 G_DEFINE_AUTOPTR_CLEANUP_FUNC(virConnect, virObjectUnref);
diff --git a/src/libvirt-host.c b/src/libvirt-host.c
index bc3d1d2803..68517bae9c 100644
--- a/src/libvirt-host.c
+++ b/src/libvirt-host.c
@@ -1444,9 +1444,20 @@ virConnectRegisterCloseCallback(virConnectPtr conn,
     virCheckConnectReturn(conn, -1);
     virCheckNonNullArgGoto(cb, error);
 
+    if (virConnectCloseCallbackDataGetCallback(conn->closeCallback) != cb) {
+        virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+                       _("A different callback was requested"));
+        goto error;
+    }
+
+    virConnectCloseCallbackDataRegister(conn->closeCallback, conn, cb,
+                                        opaque, freecb);
+
     if (conn->driver->connectRegisterCloseCallback &&
-        conn->driver->connectRegisterCloseCallback(conn, cb, opaque, freecb) < 0)
+        conn->driver->connectRegisterCloseCallback(conn, cb, opaque, freecb) < 0) {
+        virConnectCloseCallbackDataUnregister(conn->closeCallback, cb);
         goto error;
+    }
 
     return 0;
 
@@ -1483,6 +1494,8 @@ virConnectUnregisterCloseCallback(virConnectPtr conn,
         conn->driver->connectUnregisterCloseCallback(conn, cb) < 0)
         goto error;
 
+    virConnectCloseCallbackDataUnregister(conn->closeCallback, cb);
+
     return 0;
 
  error:


The remote & vz drivers then need to be updated to use the
conn->closeCallback and remove their own direct calls to
virConnectCloseCallbackData{Unregister,Register}

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 :|