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

On Wed, Dec 11, 2019 at 08:11 PM -0500, Cole Robinson <crobinso(a)redhat.com&gt; 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…