On 04/12/2018 08:40 AM, 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.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
---
src/libvirt-host.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/src/libvirt-host.c b/src/libvirt-host.c
index 7ff7407a0874..cb2ace7d9778 100644
--- a/src/libvirt-host.c
+++ b/src/libvirt-host.c
@@ -1221,9 +1221,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);
+ }
I see this follows what Daniel suggests from v1:
https://www.redhat.com/archives/libvir-list/2018-April/msg00116.html
but I guess it still baffles me about calling @freecb w/ @opaque. If
there was a @freecb routine supplied it'd be called and free @opaque
which may actually be used afterwards - after all this is a
RegisterCloseCallback which would conceptually be called after open, but
before perhaps using the @opaque. E.G., the virsh code uses this - with
virshReconnect being called from virshInit before entering the loop
processing commands. So if @ctl was free'd - things wouldn't be good!
Running in debug using '-c test:///default' dumps you into the else.
I would think a little loss of memory is better than using memory you
didn't expect to be free'd when virConnectRegisterCloseCallback was
successful.
John
return 0;