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

On Fri, Nov 01, 2019 at 06:35:44PM +0100, 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. > > Therefore call directly @freecb if the connectRegisterCloseCallback > API is not supported by the driver used by the connection. > > Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com&gt; > --- > src/libvirt-host.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/src/libvirt-host.c b/src/libvirt-host.c > index 221a1b7a4353..9c3a19f33b12 100644 > --- a/src/libvirt-host.c > +++ b/src/libvirt-host.c > @@ -1398,7 +1398,7 @@ virConnectIsAlive(virConnectPtr conn) > * @conn: pointer to connection object > * @cb: callback to invoke upon close > * @opaque: user data to pass to @cb > - * @freecb: callback to free @opaque > + * @freecb: callback to free @opaque when not used anymore > * > * Registers a callback to be invoked when the connection > * is closed. This callback is invoked when there is any > @@ -1428,9 +1428,13 @@ virConnectRegisterCloseCallback(virConnectPtr conn, > virCheckConnectReturn(conn, -1); > virCheckNonNullArgGoto(cb, error); > > - if (conn->driver->connectRegisterCloseCallback && > - conn->driver->connectRegisterCloseCallback(conn, cb, opaque, freecb) < 0) > - goto error; > + if (conn->driver->connectRegisterCloseCallback) { > + if (conn->driver->connectRegisterCloseCallback(conn, cb, opaque, freecb) < 0) > + goto error; > + } else { > + if (freecb) > + freecb(opaque); > + } Looks good but I think we should improve the documentation of this API to explicitly state that the @freecb is called only on success and if the API fails the caller is responsible to free the opaque data.

Pavel