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, &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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list