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, &sectionLen, opaque) < 0) {
-                virStreamAbort(stream);
+            if (holeHandler(stream, &inData, &sectionLen, 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

--
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:
> 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

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