[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v3 6/6] virStream*All: Report error if a callback fails




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 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().

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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]