Re: [libvirt] [PATCH libvirt v2 1/9] virConnectRegisterCloseCallback: Cleanup 'opaque' if there is no connectRegisterCloseCallback

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&gt; > Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com&gt; > --- > 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; > >