On 04/26/2018 12:09 PM, Marc Hartmayer wrote:
> On Thu, Apr 26, 2018 at 05:06 PM +0200, John Ferlan <jferlan@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx>
>>> Reviewed-by: Boris Fiuczynski <fiuczy@xxxxxxxxxxxxx>
>>> ---
>>> 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
> 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
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
