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

On 04/26/2018 12:09 PM, Marc Hartmayer wrote: > 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. > True virsh uses NULL so it's fine; however, I was thinking about more generically - why would a Register routine with a callback to free memory free the memory upon successful register. I'm still not sure I understand why the API cannot return a failure, but Daniel says it cannot. I'm really not sure what to do - maybe Daniel will pipe it - I'll try to ping him in the morning... John