On 06/22/2017 08:30 AM, Michal Privoznik wrote: 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)... 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(a)redhat.com&gt; 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? [...]

On 06/22/2017 08:30 AM, Michal Privoznik wrote: 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

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(a)redhat.com&gt; --- 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, &sectionLen, opaque) < 0) + if (holeHandler(stream, &inData, &sectionLen, 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