1. Patchew
  2. Libvirt
  3. [libvirt] [PATCH 0/3] Behave better for users of virStream
  4. View patch

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

Martin Kletzander posted 3 patches 7 years, 11 months ago
[libvirt] [PATCH 3/3] virStream: Remove callbacks on Abort/Finish
Posted by Martin Kletzander 7 years, 11 months ago 
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.

Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
---
 src/remote/remote_driver.c |  1 +
 src/util/virfdstream.c     | 12 ++++++++++++
 2 files changed, 13 insertions(+)

diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index 49909bf69747..0ab70a8761b5 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -5870,6 +5870,7 @@ remoteStreamCloseInt(virStreamPtr st, bool streamAbort)
     priv->localUses--;

  cleanup:
+    virNetClientStreamEventRemoveCallback(privst, true);
     virNetClientRemoveStream(priv->client, privst);
     virObjectUnref(privst);
     st->privateData = NULL;
diff --git a/src/util/virfdstream.c b/src/util/virfdstream.c
index 7ee58be136d1..ac1f4a24d60e 100644
--- a/src/util/virfdstream.c
+++ b/src/util/virfdstream.c
@@ -721,6 +721,15 @@ virFDStreamCloseInt(virStreamPtr st, bool streamAbort)

     st->privateData = NULL;

+    if (fdst->watch)
+        virEventRemoveHandle(fdst->watch);
+
+    fdst->watch = 0;
+    fdst->ff = NULL;
+    fdst->cb = NULL;
+    fdst->events = 0;
+    fdst->opaque = NULL;
+
     /* call the internal stream closing callback */
     if (fdst->icbCb) {
         /* the mutex is not accessible anymore, as private data is null */
@@ -731,8 +740,11 @@ virFDStreamCloseInt(virStreamPtr st, bool streamAbort)

     if (fdst->dispatching) {
         fdst->closed = true;
+        fdst->cbRemoved = true;
         virObjectUnlock(fdst);
     } else {
+        if (fdst->ff)
+            (fdst->ff)(fdst->opaque);
         virObjectUnlock(fdst);
         virObjectUnref(fdst);
     }
-- 
2.13.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] virStream: Remove callbacks on Abort/Finish
Posted by Daniel P. Berrange 7 years, 11 months ago 
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.


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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] virStream: Remove callbacks on Abort/Finish
Posted by Martin Kletzander 7 years, 11 months ago 
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?

>
>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@redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] virStream: Remove callbacks on Abort/Finish
Posted by Daniel P. Berrange 7 years, 11 months ago 
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.

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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] virStream: Remove callbacks on Abort/Finish
Posted by Martin Kletzander 7 years, 11 months ago 
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?

>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@redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] virStream: Remove callbacks on Abort/Finish
Posted by Daniel P. Berrange 7 years, 11 months ago 
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.


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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] virStream: Remove callbacks on Abort/Finish
Posted by Martin Kletzander 7 years, 11 months ago 
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@redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-8c64711

© 2016 - 2025 Red Hat, Inc.