[libvirt] [PATCH v4 0/6] Fix error reporting in streams
v4 of: https://www.redhat.com/archives/libvir-list/2017-June/msg00190.html Frankly, nothing much changed since v3, but the discussion on those patches got very hairy and without any explicit ACKs. So I'm resending to get them :-) 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 virFileInData: preserve errno in cleanup path virStream*All: Report error if a callback fails daemon/stream.c | 18 ++++++--- include/libvirt/libvirt-stream.h | 17 +++++++- src/libvirt-stream.c | 83 +++++++++++++++++++++++++++++++--------- src/util/virfdstream.c | 26 ++++++++++--- src/util/virfile.c | 5 ++- 5 files changed, 117 insertions(+), 32 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 | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/src/util/virfdstream.c b/src/util/virfdstream.c index ae6f78e01..bd2355d17 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; @@ -791,10 +794,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 || @@ -870,7 +873,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")); @@ -971,6 +974,13 @@ virFDStreamSendHole(virStreamPtr st, * the thread to do the lseek() for us. Under no * circumstances we can do the lseek() ourselves here. We * might mess up file position for the thread. */ + + if (fdst->threadQuit || fdst->threadErr) { + virReportSystemError(EBADF, "%s", + _("stream is not open")); + goto cleanup; + } + if (fdst->threadDoRead) { msg = fdst->msg; if (msg->type != VIR_FDSTREAM_MSG_TYPE_HOLE) { @@ -1025,6 +1035,9 @@ virFDStreamInData(virStreamPtr st, if (fdst->thread) { virFDStreamMsgPtr msg; + if (fdst->threadErr) + goto cleanup; + while (!(msg = fdst->msg)) { if (fdst->threadQuit) { *inData = *length = 0; -- 2.13.0
On 06/22/2017 08:30 AM, Michal Privoznik wrote:
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 | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-)
Trying to keep a fresh an macroscopic view and not think about my previous myopic look at the code... I got too hung up on the "error" part (as in reporting an error in an || condition even though conceptually one would have been reported)...
diff --git a/src/util/virfdstream.c b/src/util/virfdstream.c index ae6f78e01..bd2355d17 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; +
I spent some cycles considering if there was any "odd possibility" that the @cb could be called twice w/ some virStreamAbort interaction, but I was able to convince myself that it shouldn't be possible. Still just want to "be sure" since virFDStreamCloseInt uses VIR_STREAM_EVENT_ERROR and also sets abortCallbackCalled to ensure it couldn't be called again. Is that something perhaps that could/should be done here as well - defensive type programming? In any case, I'm sufficiently convinced this is fine, but figured I'd ask as well! Reviewed-by: John Ferlan <jferlan@redhat.com> John Would it be a bad time to point out that virFDStreamJoinWorker returning -1 and setting ret = -1 in the caller doesn't really do anything since ret is immediately overwritten? [...]
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 bd2355d17..f54ba15ae 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; @@ -641,7 +644,7 @@ virFDStreamThread(void *opaque) return; error: - fdst->threadErr = errno; + fdst->threadErr = virSaveLastError(); goto cleanup; } -- 2.13.0
On 06/22/2017 08:30 AM, Michal Privoznik wrote:
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(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
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
On 06/22/2017 08:30 AM, Michal Privoznik wrote:
Our documentation to all four virStreamRecvAll, virStreamSendAll, virStreamSparseRecvAll, virStreamSparseSendAll says that if these functions fail, virStreamAbort() is called. But that is not
Our documentation to the virStreamRecvAll, virStreamSendAll, virStreamSparseRecvAll, and virStreamSparseSendAll functions indicates that if these functions fail, then virStreamAbort is called.... (essentially what I wrote in the previous review).
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(-)
Also of note from the previous review: My comment:
Another "irony" is that the comment code example for virStreamSend shows the need to call virStreamAbort after a virStreamSend failure, but the same is not true for virStreamRecv failure example.
Your response: Ah, I haven't realized that! Sure. this definitely needs to be fixed too. In fact, it belongs to this patch IMO - since it's fixing the very same issue. ... So, IOW: Please update the virStreamRecv example to add that virStreamAbort if (got < 0)... Reviewed-by: John Ferlan <jferlan@redhat.com> John
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
On 06/22/2017 08:30 AM, Michal Privoznik wrote:
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
This is a public API; therefore, the ...
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(+)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
Callers might be interested in the original value of errno. Let's not overwrite it with lseek() done in cleanup path. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virfile.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index d444b32f8..2be64f1db 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -3900,8 +3900,11 @@ virFileInData(int fd, ret = 0; cleanup: /* At any rate, reposition back to where we started. */ - if (cur != (off_t) -1) + if (cur != (off_t) -1) { + int save_errno = errno; ignore_value(lseek(fd, cur, SEEK_SET)); + errno = save_errno; + } return ret; } -- 2.13.0
On 06/22/2017 08:30 AM, Michal Privoznik wrote:
Callers might be interested in the original value of errno. Let's not overwrite it with lseek() done in cleanup path.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virfile.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
Looks like I did a lot of writing in my response to the previous version. Still not clear what value this is as it's been pointed out previously that lseek shouldn't fail and it had to have succeeded at least once (since "cur != -1") and maybe failed at least once (each of those getting a ReportSystemError). So what value is there in saving errno which may not even be used when ret = 0? I don't even see it used when ret = -1. The one thing that does happen is saving the errmsg from patch 2, but nothing w/ errno. IMO, I think failing that last lseek() should cause failure since the API hasn't truthfully represented what it does ("Upon its return, the position in the @fd is left unchanged,..."). If that last lseek() fails and we return 0, then the caller would be assuming we're back at the same position we started, but we're not. But I think I stated that a long time ago when the code was first being added and you convinced me otherwise! John
diff --git a/src/util/virfile.c b/src/util/virfile.c index d444b32f8..2be64f1db 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -3900,8 +3900,11 @@ virFileInData(int fd, ret = 0; cleanup: /* At any rate, reposition back to where we started. */ - if (cur != (off_t) -1) + if (cur != (off_t) -1) { + int save_errno = errno; ignore_value(lseek(fd, cur, SEEK_SET)); + errno = save_errno; + } return ret; }
On 07/08/2017 02:52 PM, John Ferlan wrote:
On 06/22/2017 08:30 AM, Michal Privoznik wrote:
Callers might be interested in the original value of errno. Let's not overwrite it with lseek() done in cleanup path.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virfile.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
Looks like I did a lot of writing in my response to the previous version. Still not clear what value this is as it's been pointed out previously that lseek shouldn't fail and it had to have succeeded at least once (since "cur != -1") and maybe failed at least once (each of those getting a ReportSystemError).
So what value is there in saving errno which may not even be used when ret = 0? I don't even see it used when ret = -1. The one thing that does happen is saving the errmsg from patch 2, but nothing w/ errno.
IMO, I think failing that last lseek() should cause failure since the API hasn't truthfully represented what it does ("Upon its return, the position in the @fd is left unchanged,..."). If that last lseek() fails and we return 0, then the caller would be assuming we're back at the same position we started, but we're not. But I think I stated that a long time ago when the code was first being added and you convinced me otherwise!
Okay, I'll post a patch that does that. Meanwhile I've pushed the rest modulo this patch. Thanks! Michal
All of these four functions (virStreamRecvAll, virStreamSendAll, virStreamSparseRecvAll, virStreamSparseSendAll) take one or more callback that handle various aspects of streams. However, if any of them fails no error is reported therefore caller does not know what went wrong. At the same time, we silently presumed callbacks to set errno on failure. With this change we should document it explicitly as the error is not properly reported. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- include/libvirt/libvirt-stream.h | 17 +++++++++++++- src/libvirt-stream.c | 50 +++++++++++++++++++++++++++++++++------- 2 files changed, 58 insertions(+), 9 deletions(-) diff --git a/include/libvirt/libvirt-stream.h b/include/libvirt/libvirt-stream.h index d18d43140..86f96b158 100644 --- a/include/libvirt/libvirt-stream.h +++ b/include/libvirt/libvirt-stream.h @@ -82,7 +82,10 @@ int virStreamRecvHole(virStreamPtr, * of bytes. The callback will continue to be * invoked until it indicates the end of the source * has been reached by returning 0. A return value - * of -1 at any time will abort the send operation + * of -1 at any time will abort the send operation. + * + * Please note that for more accurate error reporting the + * callback should set appropriate errno on failure. * * Returns the number of bytes filled, 0 upon end * of file, or -1 upon error @@ -119,6 +122,9 @@ int virStreamSendAll(virStreamPtr st, * This function should not adjust the current position within * the file. * + * Please note that for more accurate error reporting the + * callback should set appropriate errno on failure. + * * Returns 0 on success, * -1 upon error */ @@ -142,6 +148,9 @@ typedef int (*virStreamSourceHoleFunc)(virStreamPtr st, * processing the hole in the stream source and then return. * A return value of -1 at any time will abort the send operation. * + * Please note that for more accurate error reporting the + * callback should set appropriate errno on failure. + * * Returns 0 on success, * -1 upon error. */ @@ -176,6 +185,9 @@ int virStreamSparseSendAll(virStreamPtr st, * has been reached. A return value of -1 at any time * will abort the receive operation * + * Please note that for more accurate error reporting the + * callback should set appropriate errno on failure. + * * Returns the number of bytes consumed or -1 upon * error */ @@ -203,6 +215,9 @@ int virStreamRecvAll(virStreamPtr st, * hole in the stream target and then return. A return value of * -1 at any time will abort the receive operation. * + * Please note that for more accurate error reporting the + * callback should set appropriate errno on failure. + * * Returns 0 on success, * -1 upon error */ diff --git a/src/libvirt-stream.c b/src/libvirt-stream.c index 1594ed212..cf1b2293a 100644 --- a/src/libvirt-stream.c +++ b/src/libvirt-stream.c @@ -567,7 +567,7 @@ virStreamInData(virStreamPtr stream, * * Returns -1 upon any error, with virStreamAbort() already * having been called, so the caller need only call - * virStreamFree() + * virStreamFree(). */ int virStreamSendAll(virStreamPtr stream, @@ -595,9 +595,15 @@ virStreamSendAll(virStreamPtr stream, for (;;) { int got, offset = 0; + + errno = 0; got = (handler)(stream, bytes, want, opaque); - if (got < 0) + if (got < 0) { + if (errno == 0) + errno = EIO; + virReportSystemError(errno, "%s", _("send handler failed")); goto cleanup; + } if (got == 0) break; while (offset < got) { @@ -732,17 +738,24 @@ int virStreamSparseSendAll(virStreamPtr stream, size_t want = bufLen; const unsigned int skipFlags = 0; + errno = 0; if (!dataLen) { - if (holeHandler(stream, &inData, §ionLen, opaque) < 0) + if (holeHandler(stream, &inData, §ionLen, opaque) < 0) { + if (errno == 0) + errno = EIO; + virReportSystemError(errno, "%s", _("send holeHandler failed")); goto cleanup; + } if (!inData && sectionLen) { if (virStreamSendHole(stream, sectionLen, skipFlags) < 0) goto cleanup; if (skipHandler(stream, sectionLen, opaque) < 0) { + if (errno == 0) + errno = EIO; virReportSystemError(errno, "%s", - _("unable to skip hole")); + _("send skipHandler failed")); goto cleanup; } continue; @@ -755,8 +768,13 @@ int virStreamSparseSendAll(virStreamPtr stream, want = dataLen; got = (handler)(stream, bytes, want, opaque); - if (got < 0) + if (got < 0) { + if (errno == 0) + errno = EIO; + virReportSystemError(errno, "%s", + _("send handler failed")); goto cleanup; + } if (got == 0) break; while (offset < got) { @@ -854,6 +872,8 @@ virStreamRecvAll(virStreamPtr stream, for (;;) { int got, offset = 0; + + errno = 0; got = virStreamRecv(stream, bytes, want); if (got < 0) goto cleanup; @@ -862,8 +882,13 @@ virStreamRecvAll(virStreamPtr stream, while (offset < got) { int done; done = (handler)(stream, bytes + offset, got - offset, opaque); - if (done < 0) + if (done < 0) { + if (errno == 0) + errno = EIO; + virReportSystemError(errno, "%s", + _("recv handler failed")); goto cleanup; + } offset += done; } } @@ -971,13 +996,18 @@ virStreamSparseRecvAll(virStreamPtr stream, long long holeLen; const unsigned int holeFlags = 0; + errno = 0; got = virStreamRecvFlags(stream, bytes, want, flags); if (got == -3) { if (virStreamRecvHole(stream, &holeLen, holeFlags) < 0) goto cleanup; - if (holeHandler(stream, holeLen, opaque) < 0) + if (holeHandler(stream, holeLen, opaque) < 0) { + if (errno == 0) + errno = EIO; + virReportSystemError(errno, "%s", _("recv holeHandler failed")); goto cleanup; + } continue; } else if (got < 0) { goto cleanup; @@ -987,8 +1017,12 @@ virStreamSparseRecvAll(virStreamPtr stream, while (offset < got) { int done; done = (handler)(stream, bytes + offset, got - offset, opaque); - if (done < 0) + if (done < 0) { + if (errno == 0) + errno = EIO; + virReportSystemError(errno, "%s", _("recv handler failed")); goto cleanup; + } offset += done; } } -- 2.13.0
On 06/22/2017 08:30 AM, Michal Privoznik wrote:
All of these four functions (virStreamRecvAll, virStreamSendAll, virStreamSparseRecvAll, virStreamSparseSendAll) take one or more callback that handle various aspects of streams. However, if any
(same as previous review) s/callback/callback functions/
of them fails no error is reported therefore caller does not know what went wrong.
At the same time, we silently presumed callbacks to set errno on failure. With this change we should document it explicitly as the error is not properly reported.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- include/libvirt/libvirt-stream.h | 17 +++++++++++++- src/libvirt-stream.c | 50 +++++++++++++++++++++++++++++++++------- 2 files changed, 58 insertions(+), 9 deletions(-)
I think you could move all those "errno = 0;" settings outside the "for (;;) {" loop beginnings. Unless it's felt some called function would change errno to something and return 0. They're not wrong where they are, so it's your call. You could also alter each of the new comments requesting the setting of errno to something to indicate failure to set errno will cause libvirt to default to using EIO. Reviewed-by: John Ferlan <jferlan@redhat.com> John
On 07/08/2017 02:52 PM, John Ferlan wrote:
On 06/22/2017 08:30 AM, Michal Privoznik wrote:
All of these four functions (virStreamRecvAll, virStreamSendAll, virStreamSparseRecvAll, virStreamSparseSendAll) take one or more callback that handle various aspects of streams. However, if any
(same as previous review)
s/callback/callback functions/
of them fails no error is reported therefore caller does not know what went wrong.
At the same time, we silently presumed callbacks to set errno on failure. With this change we should document it explicitly as the error is not properly reported.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- include/libvirt/libvirt-stream.h | 17 +++++++++++++- src/libvirt-stream.c | 50 +++++++++++++++++++++++++++++++++------- 2 files changed, 58 insertions(+), 9 deletions(-)
I think you could move all those "errno = 0;" settings outside the "for (;;) {" loop beginnings. Unless it's felt some called function would change errno to something and return 0. They're not wrong where they are, so it's your call.
Yeah I can. I just wanted to be safe. What if a function touches errno (e.g. resets it) without returning error? Yes, such function is flawed, but it doesn't mean we can't work with it. On the other hand, the worst thing that can happen is we report EIO instead of real error. Also, setting errno in each loop is overkill, right? ;-) I'll move the lines in front of the loops.
You could also alter each of the new comments requesting the setting of errno to something to indicate failure to set errno will cause libvirt to default to using EIO.
I'm slightly hesitant to do this. What if we want to change the default at some point? I'd say leave the comments as they are.
Reviewed-by: John Ferlan <jferlan@redhat.com>
Thanks. Michal
participants (2)
-
John Ferlan -
Michal Privoznik