[libvirt] [PATCH 3/3] virStream: Remove callbacks on Abort/Finish

[Martin Kletzander]

On Thu, Jun 01, 2017 at 02:29:04PM +0100, Daniel P. Berrange wrote:
>On Thu, Jun 01, 2017 at 03:20:50PM +0200, Martin Kletzander wrote:
>> On Thu, Jun 01, 2017 at 01:29:45PM +0100, Daniel P. Berrange wrote:
>> > On Thu, Jun 01, 2017 at 02:24:19PM +0200, Martin Kletzander wrote:
>> > > On Thu, Jun 01, 2017 at 01:19:05PM +0100, Daniel P. Berrange wrote:
>> > > > On Thu, Jun 01, 2017 at 02:08:34PM +0200, Martin Kletzander wrote:
>> > > > > Users need to remove their callbacks before calling virStreamAbort()
>> > > > > or virStreamFinish() even though that's not documented anywhere.
>> > > > > Since it makes no sense to keep those callbacks, we can remove them
>> > > > > when the stream is being aborted or finished.  That way it is also
>> > > > > more intuitive for developers as that removes some confusing errors
>> > > > > being reported.
>> > > >
>> > > > This changes the semantics of a public API though, so even though
>> > > > the suggested behavious would be useful, we mustn't do this as it
>> > > > creates an API incompatability across versions.
>> > > >
>> > >
>> > > I couldn't find any case that could be broken by this change.  Do you
>> > > have any in mind?
>> >
>> > Someone writes an app that relies on this behaviour and it runs on a different
>> > libvirt version it'll never unregister the callbacks. This means any freefunc
>> > associated with the callback won't be triggered and thus opaque memory will
>> > leak. So apps will end up having todo "if libvirt version == x then .. else.."
>> > to deal with the semantic change of the API, or just never rely on this new
>> > behaviour at all.
>> >
>>
>> OK.  That would most likely happen on downgrade, but this would not be
>> very different to some other leak that we fix at some point.  We could
>> document this, but I have a feeling that would not help making my case,
>> would it?
>
>I don't really think those are the same scenario. Those are apps using APIs
>in the correct way, but it just happens that some libvirtd code paths have
>some leaks.
>
>In this case, the applications are failing to use the existing APIs in the
>correct way, and we're proposing changing semantics of the public API to
>cover up application bugs.
>

My point is that the definition of "correct way" is only to be guessed,
but I'm OK with just documenting our current behaviour.

>
>Regards,
>Daniel
>--
>|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
>|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
>|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
>
>--
>libvir-list mailing list
>libvir-list at redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170601/1e1afe91/attachment-0001.sig>