7:08 a.m.
So I was playing with one program and I was getting weird behaviour.
It was also mentioned on the list by someone for some other client
they were writing. I found out what we were both missing was callback
removal before/after finish/abort was called. The problem is that was
not intuitive at all. In the docs we only have an example for
blocking streams, there is no mention of the callbacks and the
behaviour that libvirt has. I first wanted to fix the documentation,
but then I realized it should never make sense to have a callback
after stream has ended, so why not behave nicely for callers. Since
they should be removing the callback right before the finish, there
shouldn't even be any error message after they upgrade. To be even
nicer we could make RemoveCallback() not fail when there is no
callback registered (like DomainSuspend() when the domain is already
suspended).
Martin Kletzander (3):
Make virNetClientStreamEventRemoveCallback's errors optional
remote: Consolidate remoteStream{Abort,Finish} functions
virStream: Remove callbacks on Abort/Finish
src/remote/remote_driver.c | 42 +++++++++++-------------------------------
src/rpc/virnetclientstream.c | 10 +++++++---
src/rpc/virnetclientstream.h | 3 ++-
src/util/virfdstream.c | 12 ++++++++++++
4 files changed, 32 insertions(+), 35 deletions(-)
--
2.13.0
7:08 a.m.
New subject: [libvirt] [PATCH 1/3] Make virNetClientStreamEventRemoveCallback's errors optional
There will be new place for this function to be called, but we need
not to report error from there. Since the virNetClientStream
structure is private, there is no way to check for st->cb from outside
this file, so we need to make the error reporting optional. The
function name is already long enough to have yet another Quiet suffix
like some other functions do, plus it is only called from two places,
so just add bool for that.
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
src/remote/remote_driver.c | 2 +-
src/rpc/virnetclientstream.c | 10 +++++++---
src/rpc/virnetclientstream.h | 3 ++-
3 files changed, 10 insertions(+), 5 deletions(-)
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index d27e96ffc2b7..968fdfba191c 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -5838,7 +5838,7 @@ remoteStreamEventRemoveCallback(virStreamPtr st)
remoteDriverLock(priv);
- ret = virNetClientStreamEventRemoveCallback(privst);
+ ret = virNetClientStreamEventRemoveCallback(privst, false);
remoteDriverUnlock(priv);
return ret;
diff --git a/src/rpc/virnetclientstream.c b/src/rpc/virnetclientstream.c
index a9bf271dc5ba..1f8456f59455 100644
--- a/src/rpc/virnetclientstream.c
+++ b/src/rpc/virnetclientstream.c
@@ -712,14 +712,18 @@ int virNetClientStreamEventUpdateCallback(virNetClientStreamPtr st,
return ret;
}
-int virNetClientStreamEventRemoveCallback(virNetClientStreamPtr st)
+int virNetClientStreamEventRemoveCallback(virNetClientStreamPtr st, bool quiet)
{
int ret = -1;
virObjectLock(st);
if (!st->cb) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- "%s", _("no stream callback registered"));
+ if (quiet) {
+ ret = 0;
+ } else {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ "%s", _("no stream callback
registered"));
+ }
goto cleanup;
}
diff --git a/src/rpc/virnetclientstream.h b/src/rpc/virnetclientstream.h
index c4e01bf1cab0..4f114849e4e9 100644
--- a/src/rpc/virnetclientstream.h
+++ b/src/rpc/virnetclientstream.h
@@ -79,7 +79,8 @@ int virNetClientStreamEventAddCallback(virNetClientStreamPtr st,
int virNetClientStreamEventUpdateCallback(virNetClientStreamPtr st,
int events);
-int virNetClientStreamEventRemoveCallback(virNetClientStreamPtr st);
+int virNetClientStreamEventRemoveCallback(virNetClientStreamPtr st,
+ bool quiet);
bool virNetClientStreamEOF(virNetClientStreamPtr st)
ATTRIBUTE_NONNULL(1);
--
2.13.0
7:08 a.m.
New subject: [libvirt] [PATCH 2/3] remote: Consolidate remoteStream{Abort, Finish} functions
They do the same thing with only one difference. Let's put them
together (like we already do with virFDStreamCloseInt) so that future
changes don't miss one of the implementations. Also to clean up the
code.
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
src/remote/remote_driver.c | 39 +++++++++------------------------------
1 file changed, 9 insertions(+), 30 deletions(-)
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index 968fdfba191c..49909bf69747 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -5846,7 +5846,7 @@ remoteStreamEventRemoveCallback(virStreamPtr st)
static int
-remoteStreamFinish(virStreamPtr st)
+remoteStreamCloseInt(virStreamPtr st, bool streamAbort)
{
struct private_data *priv = st->conn->privateData;
virNetClientStreamPtr privst = st->privateData;
@@ -5862,7 +5862,7 @@ remoteStreamFinish(virStreamPtr st)
ret = virNetClientStreamSendPacket(privst,
priv->client,
- VIR_NET_OK,
+ streamAbort ? VIR_NET_ERROR : VIR_NET_OK,
NULL,
0);
@@ -5881,37 +5881,16 @@ remoteStreamFinish(virStreamPtr st)
static int
-remoteStreamAbort(virStreamPtr st)
+remoteStreamFinish(virStreamPtr st)
{
- struct private_data *priv = st->conn->privateData;
- virNetClientStreamPtr privst = st->privateData;
- int ret = -1;
-
- remoteDriverLock(priv);
-
- if (virNetClientStreamRaiseError(privst))
- goto cleanup;
-
- priv->localUses++;
- remoteDriverUnlock(priv);
-
- ret = virNetClientStreamSendPacket(privst,
- priv->client,
- VIR_NET_ERROR,
- NULL,
- 0);
-
- remoteDriverLock(priv);
- priv->localUses--;
+ return remoteStreamCloseInt(st, false);
+}
- cleanup:
- virNetClientRemoveStream(priv->client, privst);
- virObjectUnref(privst);
- st->privateData = NULL;
- st->driver = NULL;
- remoteDriverUnlock(priv);
- return ret;
+static int
+remoteStreamAbort(virStreamPtr st)
+{
+ return remoteStreamCloseInt(st, true);
}
--
2.13.0
4:06 a.m.
New subject: [libvirt] [PATCH 2/3] remote: Consolidate remoteStream{Abort, Finish} functions
On 06/01/2017 02:08 PM, Martin Kletzander wrote:
They do the same thing with only one difference. Let's put them
together (like we already do with virFDStreamCloseInt) so that future
changes don't miss one of the implementations. Also to clean up the
code.
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
src/remote/remote_driver.c | 39 +++++++++------------------------------
1 file changed, 9 insertions(+), 30 deletions(-)
ACK
Michal
7:08 a.m.
New subject: [libvirt] [PATCH 3/3] virStream: Remove callbacks on Abort/Finish
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(a)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
7:19 a.m.
New subject: [libvirt] [PATCH 3/3] virStream: Remove callbacks on Abort/Finish
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 :|
7:24 a.m.
New subject: [libvirt] [PATCH 3/3] virStream: Remove callbacks on Abort/Finish
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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
7:29 a.m.
New subject: [libvirt] [PATCH 3/3] virStream: Remove callbacks on Abort/Finish
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 :|
8:20 a.m.
New subject: [libvirt] [PATCH 3/3] virStream: Remove callbacks on Abort/Finish
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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
8:29 a.m.
New subject: [libvirt] [PATCH 3/3] virStream: Remove callbacks on Abort/Finish
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 :|
8:46 a.m.
New subject: [libvirt] [PATCH 3/3] virStream: Remove callbacks on Abort/Finish
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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
2727
days inactive
2731
days old
10 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Martin Kletzander -
Michal Privoznik