[libvirt] [PATCH 0/3] Behave better for users of virStream
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
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@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
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@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
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@redhat.com> --- src/remote/remote_driver.c | 39 +++++++++------------------------------ 1 file changed, 9 insertions(+), 30 deletions(-)
ACK Michal
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
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 :|
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
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 :|
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
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 :|
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
participants (3)
-
Daniel P. Berrange -
Martin Kletzander -
Michal Privoznik