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

[Marc Hartmayer]

On Fri, Dec 13, 2019 at 03:32 PM -0500, Cole Robinson <crobinso at redhat.com> wrote:
> 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
>

Polite ping.

-- 
Kind regards / Beste Grüße
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Matthias Hartmann
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294