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

On Wed, Dec 11, 2019 at 08:11:38PM -0500, Cole Robinson wrote: 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. 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. 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 :|