On Thu, Apr 26, 2018 at 05:06 PM +0200, John Ferlan <jferlan@redhat.com> 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@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@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.
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