On Fri, Dec 13, 2019 at 03:32 PM -0500, Cole Robinson <crobinso@xxxxxxxxxx> wrote: > On 12/12/19 8:46 AM, Marc Hartmayer wrote: >> On Wed, Dec 11, 2019 at 08:11 PM -0500, Cole Robinson <crobinso@xxxxxxxxxx> 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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
