On 06/05/2017 04:23 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
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
s/should//
> error is not properly reported.
>
> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> ---
> 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().
Noted, spurious, IDC ;-)
> */
> 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"));
Since errno and vir*LastError are not necessarily linked - do we really
want to write an error message in the event that a different message was
already in the pipeline?
That is should all of the new virReportSystemError calls be prefixed by:
if (!virGetLastError())
or perhaps use virGetLastErrorMessage ?
John
> 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;
> }
> }
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
