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

On Thu, Apr 26, 2018 at 05:06 PM +0200, John Ferlan <jferlan(a)redhat.com&gt; wrote: > 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. I understand your concerns. > 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. virshReconnect doesn’t pass a @freecb != NULL argument, does it? So the registered callback has not the responsibility to free the memory.

But I can understand, there is surely someone who might think "hey, the loop is not running yet, the call was successful => I can still use the data". > > 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. Yes, definitely. But a memory leak is still a memory leak… So I'd like it fixed :) Especially, as 'remoteDispatchConnectSupportsFeature' has an hard coded “VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK” is supported. If we would change this (as I suggested in v1), at least our remote_driver would be fenced against a memory leak (even without this patch). But this would be an API change :( doRemoteOpen: … priv->serverCloseCallback = remoteConnectSupportsFeatureUnlocked(conn, priv, VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK); if (!priv->serverCloseCallback) { VIR_INFO("Close callback registering isn't supported " "by the remote side."); } … Thanks for taking a look! > > John > >> >> return 0; >> >> > -- Beste Grüße / Kind regards Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294