[libvirt] [PATCH v3 3/6] virStream*All: Call virStreamAbort() more frequently

[John Ferlan]

On 06/05/2017 04:22 AM, Michal Privoznik wrote:
> Our documentation to all four virStreamRecvAll, virStreamSendAll,

s/all four/the/

> virStreamSparseRecvAll, virStreamSparseSendAll says that if these

s/RecvAll, virStream/RecvAll, and virStream/

s/says that if these functions fail,/functions indicates that on failure/

> 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 at redhat.com>
> ---
>  src/libvirt-stream.c | 49 ++++++++++++++++++++-----------------------------
>  1 file changed, 20 insertions(+), 29 deletions(-)
> 

This patch particularly scares me because there are so many more paths
that could now call virStreamAbort. Yes, I know that's the point, but
nonetheless it's large leap of faith to only calling Abort as a result
of some Send/Recv callback failure to calling Abort for the ancillary
stuff surrounding or supporting those calls as well.

Would there be a chance of multiple callers to virStreamAbort?

It'd be nice if there were any other opinions on this. I'd lean towards
saying looks good, but I'm also interested if there's any other opposing
opinions or concerns.

John

> 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;
>  }
>