[libvirt] [PATCH v4 2/7] virConnectRegisterCloseCallback: Cleanup 'opaque' if there is no connectRegisterCloseCallback

[Cole Robinson]

On 12/12/19 8:46 AM, Marc Hartmayer wrote:
> On Wed, Dec 11, 2019 at 08:11 PM -0500, Cole Robinson <crobinso at redhat.com> wrote:
>> On 11/14/19 12:44 PM, 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.
>>>
>>
>>
>> Full context:
>> v1 subtrhead with jferlan and danpb:
>> https://www.redhat.com/archives/libvir-list/2018-March/msg00906.html
>> https://www.redhat.com/archives/libvir-list/2018-April/msg00116.html
>>
>> v2 subthread with john:
>> https://www.redhat.com/archives/libvir-list/2018-April/msg02534.html
>>
>> My first thought is, why not just make this API start raising an error
>> if it isn't supported?
>>
>> But you tried that here:
>> https://www.redhat.com/archives/libvir-list/2018-March/msg00405.html
> 
> First, thanks for doing all the “history research”.
> 
>>
>> I'm not really sure I buy the argument that we can't change the
>> semantics of the API here. This is the only callback API that seems to
>> not raise an explicit error. It's documented to raise an error. And
>> there's possible memory leak due the ambiguity.
> 
> If we’re doing this so let’s fix the behavior of
> 'virConnectUnregisterCloseCallback' as well.
> 
>>
>> Yeah I see that virsh needs to be updated to match. In practice virsh
>> shouldn't be a problem: this issue will only hit for local drivers, and
>> virsh and client library will be updated together for that case.
>>
>> In theory if a python app is using this API and not expecting an
>> exception, it could cause a regression. But it's also informing them
>> that, hey, that connection callback you requested wasn't working in the
>> first place. So it's arguably a correctness issue.
>>
>> So IMO we should be able to adjust this to return a proper error.
>>
>>
>> BUT, if we stick with this direction, then we need to extend the doc
>> text here to describe all of this:
>>
>> * Returns -1 if the driver can support close callback, but registering
>> one failed. User must free opaque?
>> * Returns 0 if the driver does not support close callback. We will free
>> data for you
>> * Returns 0 if the driver successfully registered a close callback. When
>> that callback is triggered, opaque will be free'd
>>
>> But that sounds pretty nutty IMO :/
> 
> I know…

I did a bit more digging. Even the virsh case isn't the biggest deal
because CloseCallback failing is non-fatal. But like I mentioned before
it shouldn't realistically be hit in practice because we can expect
virsh and libvirt-client to be updated in lockstep.

virt-manager, libguestfs, vdsm, kubevirt don't use this API
nova does (nova/virt/libvirt/host.py) but it has code to catch the error

So IMO this should be changed to have semantics like all the other event
functions. Yes it's a semantic change, but I see it as explicitly
erroring in a case that never actually worked. We've made changes like
that many times, happens with XML validation semi regularly, and the
UNDEFINE flag changes are other notable examples.

danpb has your thinking changed on this? Previous discussion context is
in this thread:
https://www.redhat.com/archives/libvir-list/2018-April/msg00116.html

Thanks,
Cole