[libvirt] [PATCH v2 0/6] Fix error reporting in streams
Technically, just 1/6 and 2/6 are v2. The rest is brand new. Whatever. Michal Privoznik (6): virfdstream: Check for thread error more frequently fdstream: Report error from the I/O thread virStream*All: Call virStreamAbort() more frequently virStream*All: Preserve reported error virsh: Report errors from stream callbacks streams: Report errors if sendAll/recvAll callbacks fail daemon/stream.c | 18 ++++++++---- src/libvirt-stream.c | 74 +++++++++++++++++++++++++++++++++++++------------- src/util/virfdstream.c | 22 +++++++++++---- tools/virsh-util.c | 38 ++++++++++++++++++++------ tools/virsh-util.h | 2 +- tools/virsh-volume.c | 8 ++++-- 6 files changed, 120 insertions(+), 42 deletions(-) -- 2.13.0
When the I/O thread quits (e.g. due to an I/O error, lseek() error, whatever), any subsequent virFDStream API should return error too. Moreover, when invoking stream event callback, we must set the VIR_STREAM_EVENT_ERROR flag so that the callback knows something bad happened. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virfdstream.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/src/util/virfdstream.c b/src/util/virfdstream.c index 7ee58be13..cd24757e6 100644 --- a/src/util/virfdstream.c +++ b/src/util/virfdstream.c @@ -312,6 +312,9 @@ static void virFDStreamEvent(int watch ATTRIBUTE_UNUSED, return; } + if (fdst->threadErr) + events |= VIR_STREAM_EVENT_ERROR; + cb = fdst->cb; cbopaque = fdst->opaque; ff = fdst->ff; @@ -787,10 +790,10 @@ static int virFDStreamWrite(virStreamPtr st, const char *bytes, size_t nbytes) if (fdst->thread) { char *buf; - if (fdst->threadQuit) { + if (fdst->threadQuit || fdst->threadErr) { virReportSystemError(EBADF, "%s", _("cannot write to stream")); - return -1; + goto cleanup; } if (VIR_ALLOC(msg) < 0 || @@ -866,7 +869,7 @@ static int virFDStreamRead(virStreamPtr st, char *bytes, size_t nbytes) virFDStreamMsgPtr msg = NULL; while (!(msg = fdst->msg)) { - if (fdst->threadQuit) { + if (fdst->threadQuit || fdst->threadErr) { if (nbytes) { virReportSystemError(EBADF, "%s", _("stream is not open")); @@ -959,6 +962,9 @@ virFDStreamSendHole(virStreamPtr st, fdst->offset += length; } + if (fdst->threadErr) + goto cleanup; + if (fdst->thread) { /* Things are a bit complicated here. If FDStream is in a * read mode, then if the message at the queue head is @@ -1018,6 +1024,9 @@ virFDStreamInData(virStreamPtr st, virObjectLock(fdst); + if (fdst->threadErr) + goto cleanup; + if (fdst->thread) { virFDStreamMsgPtr msg; -- 2.13.0
Problem with our error reporting is that the error object is a thread local variable. That means if there's an error reported within the I/O thread it gets logged and everything, but later when the event loop aborts the stream it doesn't see the original error. So we are left with some generic error. We can do better if we copy the error message between the threads. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- daemon/stream.c | 18 ++++++++++++------ src/util/virfdstream.c | 9 ++++++--- 2 files changed, 18 insertions(+), 9 deletions(-) diff --git a/daemon/stream.c b/daemon/stream.c index 1d5b50ad7..5077ac8b0 100644 --- a/daemon/stream.c +++ b/daemon/stream.c @@ -231,17 +231,23 @@ daemonStreamEvent(virStreamPtr st, int events, void *opaque) int ret; virNetMessagePtr msg; virNetMessageError rerr; + virErrorPtr origErr = virSaveLastError(); memset(&rerr, 0, sizeof(rerr)); stream->closed = true; virStreamEventRemoveCallback(stream->st); virStreamAbort(stream->st); - if (events & VIR_STREAM_EVENT_HANGUP) - virReportError(VIR_ERR_RPC, - "%s", _("stream had unexpected termination")); - else - virReportError(VIR_ERR_RPC, - "%s", _("stream had I/O failure")); + if (origErr && origErr->code != VIR_ERR_OK) { + virSetError(origErr); + virFreeError(origErr); + } else { + if (events & VIR_STREAM_EVENT_HANGUP) + virReportError(VIR_ERR_RPC, + "%s", _("stream had unexpected termination")); + else + virReportError(VIR_ERR_RPC, + "%s", _("stream had I/O failure")); + } msg = virNetMessageNew(false); if (!msg) { diff --git a/src/util/virfdstream.c b/src/util/virfdstream.c index cd24757e6..5b59765fa 100644 --- a/src/util/virfdstream.c +++ b/src/util/virfdstream.c @@ -106,7 +106,7 @@ struct virFDStreamData { /* Thread data */ virThreadPtr thread; virCond threadCond; - int threadErr; + virErrorPtr threadErr; bool threadQuit; bool threadAbort; bool threadDoRead; @@ -123,6 +123,7 @@ virFDStreamDataDispose(void *obj) virFDStreamDataPtr fdst = obj; VIR_DEBUG("obj=%p", fdst); + virFreeError(fdst->threadErr); virFDStreamMsgQueueFree(&fdst->msg); } @@ -312,8 +313,10 @@ static void virFDStreamEvent(int watch ATTRIBUTE_UNUSED, return; } - if (fdst->threadErr) + if (fdst->threadErr) { events |= VIR_STREAM_EVENT_ERROR; + virSetError(fdst->threadErr); + } cb = fdst->cb; cbopaque = fdst->opaque; @@ -637,7 +640,7 @@ virFDStreamThread(void *opaque) return; error: - fdst->threadErr = errno; + fdst->threadErr = virSaveLastError(); goto cleanup; } -- 2.13.0
Our documentation to all four virStreamRecvAll, virStreamSendAll, virStreamSparseRecvAll, virStreamSparseSendAll says that if these functions fail, virStreamAbort() is called. But that is not necessarily true. For instance all of these functions allocate a buffer to work with. If the allocation fails, no virStreamAbort() is called despite -1 being returned. It's the same story with argument sanity checks and a lot of other checks. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt-stream.c | 49 ++++++++++++++++++++----------------------------- 1 file changed, 20 insertions(+), 29 deletions(-) diff --git a/src/libvirt-stream.c b/src/libvirt-stream.c index d7a8f5816..bff0a0571 100644 --- a/src/libvirt-stream.c +++ b/src/libvirt-stream.c @@ -596,10 +596,8 @@ virStreamSendAll(virStreamPtr stream, for (;;) { int got, offset = 0; got = (handler)(stream, bytes, want, opaque); - if (got < 0) { - virStreamAbort(stream); + if (got < 0) goto cleanup; - } if (got == 0) break; while (offset < got) { @@ -615,8 +613,10 @@ virStreamSendAll(virStreamPtr stream, cleanup: VIR_FREE(bytes); - if (ret != 0) + if (ret != 0) { + virStreamAbort(stream); virDispatchError(stream->conn); + } return ret; } @@ -728,21 +728,16 @@ int virStreamSparseSendAll(virStreamPtr stream, const unsigned int skipFlags = 0; if (!dataLen) { - if (holeHandler(stream, &inData, §ionLen, opaque) < 0) { - virStreamAbort(stream); + if (holeHandler(stream, &inData, §ionLen, opaque) < 0) goto cleanup; - } if (!inData && sectionLen) { - if (virStreamSendHole(stream, sectionLen, skipFlags) < 0) { - virStreamAbort(stream); + if (virStreamSendHole(stream, sectionLen, skipFlags) < 0) goto cleanup; - } if (skipHandler(stream, sectionLen, opaque) < 0) { virReportSystemError(errno, "%s", _("unable to skip hole")); - virStreamAbort(stream); goto cleanup; } continue; @@ -755,10 +750,8 @@ int virStreamSparseSendAll(virStreamPtr stream, want = dataLen; got = (handler)(stream, bytes, want, opaque); - if (got < 0) { - virStreamAbort(stream); + if (got < 0) goto cleanup; - } if (got == 0) break; while (offset < got) { @@ -775,8 +768,10 @@ int virStreamSparseSendAll(virStreamPtr stream, cleanup: VIR_FREE(bytes); - if (ret != 0) + if (ret != 0) { + virStreamAbort(stream); virDispatchError(stream->conn); + } return ret; } @@ -857,10 +852,8 @@ virStreamRecvAll(virStreamPtr stream, while (offset < got) { int done; done = (handler)(stream, bytes + offset, got - offset, opaque); - if (done < 0) { - virStreamAbort(stream); + if (done < 0) goto cleanup; - } offset += done; } } @@ -869,8 +862,10 @@ virStreamRecvAll(virStreamPtr stream, cleanup: VIR_FREE(bytes); - if (ret != 0) + if (ret != 0) { + virStreamAbort(stream); virDispatchError(stream->conn); + } return ret; } @@ -963,15 +958,11 @@ virStreamSparseRecvAll(virStreamPtr stream, got = virStreamRecvFlags(stream, bytes, want, flags); if (got == -3) { - if (virStreamRecvHole(stream, &holeLen, holeFlags) < 0) { - virStreamAbort(stream); + if (virStreamRecvHole(stream, &holeLen, holeFlags) < 0) goto cleanup; - } - if (holeHandler(stream, holeLen, opaque) < 0) { - virStreamAbort(stream); + if (holeHandler(stream, holeLen, opaque) < 0) goto cleanup; - } continue; } else if (got < 0) { goto cleanup; @@ -981,10 +972,8 @@ virStreamSparseRecvAll(virStreamPtr stream, while (offset < got) { int done; done = (handler)(stream, bytes + offset, got - offset, opaque); - if (done < 0) { - virStreamAbort(stream); + if (done < 0) goto cleanup; - } offset += done; } } @@ -993,8 +982,10 @@ virStreamSparseRecvAll(virStreamPtr stream, cleanup: VIR_FREE(bytes); - if (ret != 0) + if (ret != 0) { + virStreamAbort(stream); virDispatchError(stream->conn); + } return ret; } -- 2.13.0
If one these four functions fail (virStreamRecvAll, virStreamSendAll, virStreamSparseRecvAll, virStreamSparseSendAll) the stream is aborted by calling virStreamAbort(). This is, however, an public API - therefore the first thing it does is error reset. At that point any error that caused us to abort stream in the first place is gone. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt-stream.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/libvirt-stream.c b/src/libvirt-stream.c index bff0a0571..1594ed212 100644 --- a/src/libvirt-stream.c +++ b/src/libvirt-stream.c @@ -614,7 +614,12 @@ virStreamSendAll(virStreamPtr stream, VIR_FREE(bytes); if (ret != 0) { + virErrorPtr orig_err = virSaveLastError(); virStreamAbort(stream); + if (orig_err) { + virSetError(orig_err); + virFreeError(orig_err); + } virDispatchError(stream->conn); } @@ -769,7 +774,12 @@ int virStreamSparseSendAll(virStreamPtr stream, VIR_FREE(bytes); if (ret != 0) { + virErrorPtr orig_err = virSaveLastError(); virStreamAbort(stream); + if (orig_err) { + virSetError(orig_err); + virFreeError(orig_err); + } virDispatchError(stream->conn); } @@ -863,7 +873,12 @@ virStreamRecvAll(virStreamPtr stream, VIR_FREE(bytes); if (ret != 0) { + virErrorPtr orig_err = virSaveLastError(); virStreamAbort(stream); + if (orig_err) { + virSetError(orig_err); + virFreeError(orig_err); + } virDispatchError(stream->conn); } @@ -983,7 +998,12 @@ virStreamSparseRecvAll(virStreamPtr stream, VIR_FREE(bytes); if (ret != 0) { + virErrorPtr orig_err = virSaveLastError(); virStreamAbort(stream); + if (orig_err) { + virSetError(orig_err); + virFreeError(orig_err); + } virDispatchError(stream->conn); } -- 2.13.0
There are couple of callbacks we pass to virStreamSendAll(), virStreamRecvAll() or its sparse variants. However, none of these callbacks reports error if one occurs and neither do the virStream* functions leaving user with very unhelpful error message: error: cannot receive data from volume fedora.img error: An error occurred, but the cause is unknown Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/virsh-util.c | 38 +++++++++++++++++++++++++++++--------- tools/virsh-util.h | 2 +- tools/virsh-volume.c | 8 ++++++-- 3 files changed, 36 insertions(+), 12 deletions(-) diff --git a/tools/virsh-util.c b/tools/virsh-util.c index 44be3ad64..183f4c8bb 100644 --- a/tools/virsh-util.c +++ b/tools/virsh-util.c @@ -147,9 +147,15 @@ virshStreamSink(virStreamPtr st ATTRIBUTE_UNUSED, size_t nbytes, void *opaque) { - int *fd = opaque; + virshStreamCallbackDataPtr cbData = opaque; + int fd = cbData->fd; + const char *filename = cbData->filename; + int ret; - return safewrite(*fd, bytes, nbytes); + if ((ret = safewrite(fd, bytes, nbytes)) < 0) + virReportSystemError(errno, _("unable to write to %s"), filename); + + return ret; } @@ -161,8 +167,13 @@ virshStreamSource(virStreamPtr st ATTRIBUTE_UNUSED, { virshStreamCallbackDataPtr cbData = opaque; int fd = cbData->fd; + const char *filename = cbData->filename; + int ret; - return saferead(fd, bytes, nbytes); + if ((ret = saferead(fd, bytes, nbytes)) < 0) + virReportSystemError(errno, _("unable to read from %s"), filename); + + return ret; } @@ -173,10 +184,13 @@ virshStreamSourceSkip(virStreamPtr st ATTRIBUTE_UNUSED, { virshStreamCallbackDataPtr cbData = opaque; int fd = cbData->fd; + const char *filename = cbData->filename; off_t cur; - if ((cur = lseek(fd, offset, SEEK_CUR)) == (off_t) -1) + if ((cur = lseek(fd, offset, SEEK_CUR)) == (off_t) -1) { + virReportSystemError(errno, _("unable to seek in %s"), filename); return -1; + } return 0; } @@ -187,14 +201,20 @@ virshStreamSkip(virStreamPtr st ATTRIBUTE_UNUSED, long long offset, void *opaque) { - int *fd = opaque; + virshStreamCallbackDataPtr cbData = opaque; + int fd = cbData->fd; + const char *filename = cbData->filename; off_t cur; - if ((cur = lseek(*fd, offset, SEEK_CUR)) == (off_t) -1) + if ((cur = lseek(fd, offset, SEEK_CUR)) == (off_t) -1) { + virReportSystemError(errno, _("unable to seek in %s"), filename); return -1; + } - if (ftruncate(*fd, cur) < 0) + if (ftruncate(fd, cur) < 0) { + virReportSystemError(errno, _("unable to truncate %s"), filename); return -1; + } return 0; } @@ -207,12 +227,12 @@ virshStreamInData(virStreamPtr st ATTRIBUTE_UNUSED, void *opaque) { virshStreamCallbackDataPtr cbData = opaque; - vshControl *ctl = cbData->ctl; int fd = cbData->fd; + const char *filename = cbData->filename; int ret; if ((ret = virFileInData(fd, inData, offset)) < 0) - vshError(ctl, "%s", _("Unable to get current position in stream")); + virReportSystemError(errno, _("unable to seek in %s"), filename); return ret; } diff --git a/tools/virsh-util.h b/tools/virsh-util.h index 9a0af3513..0babb311b 100644 --- a/tools/virsh-util.h +++ b/tools/virsh-util.h @@ -60,8 +60,8 @@ virshStreamSink(virStreamPtr st, typedef struct _virshStreamCallbackData virshStreamCallbackData; typedef virshStreamCallbackData *virshStreamCallbackDataPtr; struct _virshStreamCallbackData { - vshControl *ctl; int fd; + const char *filename; }; int diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c index 0736bdcdb..ea4660fee 100644 --- a/tools/virsh-volume.c +++ b/tools/virsh-volume.c @@ -698,8 +698,8 @@ cmdVolUpload(vshControl *ctl, const vshCmd *cmd) goto cleanup; } - cbData.ctl = ctl; cbData.fd = fd; + cbData.filename = file; if (vshCommandOptBool(cmd, "sparse")) flags |= VIR_STORAGE_VOL_UPLOAD_SPARSE_STREAM; @@ -795,6 +795,7 @@ cmdVolDownload(vshControl *ctl, const vshCmd *cmd) bool created = false; virshControlPtr priv = ctl->privData; unsigned int flags = 0; + virshStreamCallbackData cbData; if (vshCommandOptULongLong(ctl, cmd, "offset", &offset) < 0) return false; @@ -821,6 +822,9 @@ cmdVolDownload(vshControl *ctl, const vshCmd *cmd) created = true; } + cbData.fd = fd; + cbData.filename = file; + if (!(st = virStreamNew(priv->conn, 0))) { vshError(ctl, _("cannot create a new stream")); goto cleanup; @@ -831,7 +835,7 @@ cmdVolDownload(vshControl *ctl, const vshCmd *cmd) goto cleanup; } - if (virStreamSparseRecvAll(st, virshStreamSink, virshStreamSkip, &fd) < 0) { + if (virStreamSparseRecvAll(st, virshStreamSink, virshStreamSkip, &cbData) < 0) { vshError(ctl, _("cannot receive data from volume %s"), name); goto cleanup; } -- 2.13.0
On Thu, Jun 01, 2017 at 02:02:13PM +0200, Michal Privoznik wrote:
There are couple of callbacks we pass to virStreamSendAll(), virStreamRecvAll() or its sparse variants. However, none of these callbacks reports error if one occurs and neither do the virStream* functions leaving user with very unhelpful error message:
error: cannot receive data from volume fedora.img error: An error occurred, but the cause is unknown
If we change the code to honour errno set in the callbacks, we only need to deal with ensuring virFileInData sets the errno correctly. 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 06/01/2017 03:39 PM, Daniel P. Berrange wrote:
On Thu, Jun 01, 2017 at 02:02:13PM +0200, Michal Privoznik wrote:
There are couple of callbacks we pass to virStreamSendAll(), virStreamRecvAll() or its sparse variants. However, none of these callbacks reports error if one occurs and neither do the virStream* functions leaving user with very unhelpful error message:
error: cannot receive data from volume fedora.img error: An error occurred, but the cause is unknown
If we change the code to honour errno set in the callbacks, we only need to deal with ensuring virFileInData sets the errno correctly.
Okay, makes sense. Will post v3. Meanwhile, do you have any clever idea on this? https://www.redhat.com/archives/libvir-list/2017-June/msg00015.html Michal
On Thu, Jun 01, 2017 at 03:43:06PM +0200, Michal Privoznik wrote:
On 06/01/2017 03:39 PM, Daniel P. Berrange wrote:
On Thu, Jun 01, 2017 at 02:02:13PM +0200, Michal Privoznik wrote:
There are couple of callbacks we pass to virStreamSendAll(), virStreamRecvAll() or its sparse variants. However, none of these callbacks reports error if one occurs and neither do the virStream* functions leaving user with very unhelpful error message:
error: cannot receive data from volume fedora.img error: An error occurred, but the cause is unknown
If we change the code to honour errno set in the callbacks, we only need to deal with ensuring virFileInData sets the errno correctly.
Okay, makes sense. Will post v3.
Meanwhile, do you have any clever idea on this?
https://www.redhat.com/archives/libvir-list/2017-June/msg00015.html
Hmm, no bright ideas, without spending some time peering into the code. 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 :|
We have couple of wrappers over our low level stream APIs: virSreamRecvAll(), virStreamSendAll() and their sparse stream variants. All of them take some callbacks and call them at appropriate times. If a callback fails it aborts the whole operation. However, if it so happens that the callback fails without setting any error users are left with very unhelpful error message: error: cannot receive data from volume fedora.img error: An error occurred, but the cause is unknown One way to avoid it is to report error if callback hasn't. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt-stream.c | 41 +++++++++++++++++++++++++++++++++-------- 1 file changed, 33 insertions(+), 8 deletions(-) diff --git a/src/libvirt-stream.c b/src/libvirt-stream.c index 1594ed212..efdbc9e44 100644 --- a/src/libvirt-stream.c +++ b/src/libvirt-stream.c @@ -596,8 +596,12 @@ virStreamSendAll(virStreamPtr stream, for (;;) { int got, offset = 0; got = (handler)(stream, bytes, want, opaque); - if (got < 0) + if (got < 0) { + if (!virGetLastError()) + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("send handler failed")); goto cleanup; + } if (got == 0) break; while (offset < got) { @@ -733,16 +737,21 @@ int virStreamSparseSendAll(virStreamPtr stream, const unsigned int skipFlags = 0; if (!dataLen) { - if (holeHandler(stream, &inData, §ionLen, opaque) < 0) + if (holeHandler(stream, &inData, §ionLen, opaque) < 0) { + if (!virGetLastError()) + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("send holeHandler failed")); goto cleanup; + } if (!inData && sectionLen) { if (virStreamSendHole(stream, sectionLen, skipFlags) < 0) goto cleanup; if (skipHandler(stream, sectionLen, opaque) < 0) { - virReportSystemError(errno, "%s", - _("unable to skip hole")); + if (!virGetLastError()) + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("send skipHandler failed")); goto cleanup; } continue; @@ -755,8 +764,12 @@ int virStreamSparseSendAll(virStreamPtr stream, want = dataLen; got = (handler)(stream, bytes, want, opaque); - if (got < 0) + if (got < 0) { + if (!virGetLastError()) + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("send handler failed")); goto cleanup; + } if (got == 0) break; while (offset < got) { @@ -862,8 +875,12 @@ virStreamRecvAll(virStreamPtr stream, while (offset < got) { int done; done = (handler)(stream, bytes + offset, got - offset, opaque); - if (done < 0) + if (done < 0) { + if (!virGetLastError()) + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("receive handler failed")); goto cleanup; + } offset += done; } } @@ -976,8 +993,12 @@ virStreamSparseRecvAll(virStreamPtr stream, if (virStreamRecvHole(stream, &holeLen, holeFlags) < 0) goto cleanup; - if (holeHandler(stream, holeLen, opaque) < 0) + if (holeHandler(stream, holeLen, opaque) < 0) { + if (!virGetLastError()) + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("receive holeHandler failed")); goto cleanup; + } continue; } else if (got < 0) { goto cleanup; @@ -987,8 +1008,12 @@ virStreamSparseRecvAll(virStreamPtr stream, while (offset < got) { int done; done = (handler)(stream, bytes + offset, got - offset, opaque); - if (done < 0) + if (done < 0) { + if (!virGetLastError()) + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("receive handler failed")); goto cleanup; + } offset += done; } } -- 2.13.0
On Thu, Jun 01, 2017 at 02:02:14PM +0200, Michal Privoznik wrote:
We have couple of wrappers over our low level stream APIs: virSreamRecvAll(), virStreamSendAll() and their sparse stream variants. All of them take some callbacks and call them at appropriate times. If a callback fails it aborts the whole operation. However, if it so happens that the callback fails without setting any error users are left with very unhelpful error message:
error: cannot receive data from volume fedora.img error: An error occurred, but the cause is unknown
One way to avoid it is to report error if callback hasn't.
The callbacks should never be expected to set a libvirt error, as they are implemented by code that is outside libvirt. The intention was really that the callbacks set an errno value on error, since that's all you have access to from application level impl....
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt-stream.c | 41 +++++++++++++++++++++++++++++++++-------- 1 file changed, 33 insertions(+), 8 deletions(-)
diff --git a/src/libvirt-stream.c b/src/libvirt-stream.c index 1594ed212..efdbc9e44 100644 --- a/src/libvirt-stream.c +++ b/src/libvirt-stream.c @@ -596,8 +596,12 @@ virStreamSendAll(virStreamPtr stream, for (;;) { int got, offset = 0;
For sanity we should set 'errno = 0' here.
got = (handler)(stream, bytes, want, opaque); - if (got < 0) + if (got < 0) { + if (!virGetLastError()) + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("send handler failed")); goto cleanup; + }
...then this should do if (errno == 0) { errno = EIO; } virReportSystemError(errno, ....) 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:10:45PM +0100, Daniel P. Berrange wrote:
On Thu, Jun 01, 2017 at 02:02:14PM +0200, Michal Privoznik wrote:
We have couple of wrappers over our low level stream APIs: virSreamRecvAll(), virStreamSendAll() and their sparse stream variants. All of them take some callbacks and call them at appropriate times. If a callback fails it aborts the whole operation. However, if it so happens that the callback fails without setting any error users are left with very unhelpful error message:
error: cannot receive data from volume fedora.img error: An error occurred, but the cause is unknown
One way to avoid it is to report error if callback hasn't.
The callbacks should never be expected to set a libvirt error, as they are implemented by code that is outside libvirt. The intention was really that the callbacks set an errno value on error, since that's all you have access to from application level impl....
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt-stream.c | 41 +++++++++++++++++++++++++++++++++-------- 1 file changed, 33 insertions(+), 8 deletions(-)
diff --git a/src/libvirt-stream.c b/src/libvirt-stream.c index 1594ed212..efdbc9e44 100644 --- a/src/libvirt-stream.c +++ b/src/libvirt-stream.c @@ -596,8 +596,12 @@ virStreamSendAll(virStreamPtr stream, for (;;) { int got, offset = 0;
For sanity we should set 'errno = 0' here.
got = (handler)(stream, bytes, want, opaque); - if (got < 0) + if (got < 0) { + if (!virGetLastError()) + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("send handler failed")); goto cleanup; + }
...then this should do
if (errno == 0) { errno = EIO; } virReportSystemError(errno, ....)
Oh, that's a good idea. But can we check for both? The callbacks can still call libvirt (to receive from the stream for example) and that error could be kept as well. Not all errors will set errno. On the other hand the application will already know about the problem and can deal with it how it wants, but we should also aim at intuitive usage. If we are to expect errno, we need to say that in the documentation, really.
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:23:30PM +0200, Martin Kletzander wrote:
On Thu, Jun 01, 2017 at 01:10:45PM +0100, Daniel P. Berrange wrote:
On Thu, Jun 01, 2017 at 02:02:14PM +0200, Michal Privoznik wrote:
We have couple of wrappers over our low level stream APIs: virSreamRecvAll(), virStreamSendAll() and their sparse stream variants. All of them take some callbacks and call them at appropriate times. If a callback fails it aborts the whole operation. However, if it so happens that the callback fails without setting any error users are left with very unhelpful error message:
error: cannot receive data from volume fedora.img error: An error occurred, but the cause is unknown
One way to avoid it is to report error if callback hasn't.
The callbacks should never be expected to set a libvirt error, as they are implemented by code that is outside libvirt. The intention was really that the callbacks set an errno value on error, since that's all you have access to from application level impl....
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt-stream.c | 41 +++++++++++++++++++++++++++++++++-------- 1 file changed, 33 insertions(+), 8 deletions(-)
diff --git a/src/libvirt-stream.c b/src/libvirt-stream.c index 1594ed212..efdbc9e44 100644 --- a/src/libvirt-stream.c +++ b/src/libvirt-stream.c @@ -596,8 +596,12 @@ virStreamSendAll(virStreamPtr stream, for (;;) { int got, offset = 0;
For sanity we should set 'errno = 0' here.
got = (handler)(stream, bytes, want, opaque); - if (got < 0) + if (got < 0) { + if (!virGetLastError()) + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("send handler failed")); goto cleanup; + }
...then this should do
if (errno == 0) { errno = EIO; } virReportSystemError(errno, ....)
Oh, that's a good idea. But can we check for both? The callbacks can still call libvirt (to receive from the stream for example) and that error could be kept as well. Not all errors will set errno. On the other hand the application will already know about the problem and can deal with it how it wants, but we should also aim at intuitive usage.
Libvirtd code never uses the SendAll/RecvAll functions AFAIK. The only user of the callbacks which might call other libvirt (internal) functions is really virsh, but even that is just calling plain posix functions which set errno (at least until patch 5 in this series). So I'm not seeing a compelling scenario where you need to preseve libvirt errors here.
If we are to expect errno, we need to say that in the documentation, really.
Agreed, in fact I thought we already did, but I guess not. 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 :|
participants (3)
-
Daniel P. Berrange -
Martin Kletzander -
Michal Privoznik