[libvirt] [PATCH v3 2/6] fdstream: Report error from the I/O thread

[John Ferlan]

On 06/05/2017 04:22 AM, Michal Privoznik wrote:
> Problem with our error reporting is that the error object is a
> thread local variable. That means if there's an error reported
> within the I/O thread it gets logged and everything, but later
> when the event loop aborts the stream it doesn't see the original
> error. So we are left with some generic error. We can do better
> if we copy the error message between the threads.
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  daemon/stream.c        | 18 ++++++++++++------
>  src/util/virfdstream.c |  9 ++++++---
>  2 files changed, 18 insertions(+), 9 deletions(-)
> 

Perhaps I'm lost in weeds of this code, but I thought the magic of error
message details for the threads was handled through rerr (and
virNetMessageSaveError).

Still, based on later patches which seem to care about the value of
errno, it interesting this one removes an errno setting and replaces it
with an error message.

Maybe the answer is rather than replacing threadErr it should be
'augmented' with a 'threadErrMsg'.

> diff --git a/daemon/stream.c b/daemon/stream.c
> index 1d5b50ad7..5077ac8b0 100644
> --- a/daemon/stream.c
> +++ b/daemon/stream.c
> @@ -231,17 +231,23 @@ daemonStreamEvent(virStreamPtr st, int events, void *opaque)
>          int ret;
>          virNetMessagePtr msg;
>          virNetMessageError rerr;
> +        virErrorPtr origErr = virSaveLastError();
>  
>          memset(&rerr, 0, sizeof(rerr));
>          stream->closed = true;
>          virStreamEventRemoveCallback(stream->st);
>          virStreamAbort(stream->st);
> -        if (events & VIR_STREAM_EVENT_HANGUP)
> -            virReportError(VIR_ERR_RPC,
> -                           "%s", _("stream had unexpected termination"));
> -        else
> -            virReportError(VIR_ERR_RPC,
> -                           "%s", _("stream had I/O failure"));
> +        if (origErr && origErr->code != VIR_ERR_OK) {
> +            virSetError(origErr);
> +            virFreeError(origErr);
> +        } else {
> +            if (events & VIR_STREAM_EVENT_HANGUP)
> +                virReportError(VIR_ERR_RPC,
> +                               "%s", _("stream had unexpected termination"));
> +            else
> +                virReportError(VIR_ERR_RPC,
> +                               "%s", _("stream had I/O failure"));
> +        }

This seems fine - display the error that we got from some lower spot if
it exists; otherwise,

Might be important to note the saving of the error has a lot to do with
virStreamAbort's clearing.

At the very least this is an error in our processing vs. something
within the stream, correct? Or is that the thicket of weeds I'm lost in.

>  
>          msg = virNetMessageNew(false);
>          if (!msg) {
> diff --git a/src/util/virfdstream.c b/src/util/virfdstream.c
> index cd24757e6..5b59765fa 100644
> --- a/src/util/virfdstream.c
> +++ b/src/util/virfdstream.c
> @@ -106,7 +106,7 @@ struct virFDStreamData {
>      /* Thread data */
>      virThreadPtr thread;
>      virCond threadCond;
> -    int threadErr;
> +    virErrorPtr threadErr;
>      bool threadQuit;
>      bool threadAbort;
>      bool threadDoRead;
> @@ -123,6 +123,7 @@ virFDStreamDataDispose(void *obj)
>      virFDStreamDataPtr fdst = obj;
>  
>      VIR_DEBUG("obj=%p", fdst);
> +    virFreeError(fdst->threadErr);
>      virFDStreamMsgQueueFree(&fdst->msg);
>  }
>  
> @@ -312,8 +313,10 @@ static void virFDStreamEvent(int watch ATTRIBUTE_UNUSED,
>          return;
>      }
>  
> -    if (fdst->threadErr)
> +    if (fdst->threadErr) {
>          events |= VIR_STREAM_EVENT_ERROR;
> +        virSetError(fdst->threadErr);> +    }
>  
>      cb = fdst->cb;
>      cbopaque = fdst->opaque;
> @@ -637,7 +640,7 @@ virFDStreamThread(void *opaque)
>      return;
>  
>   error:
> -    fdst->threadErr = errno;
> +    fdst->threadErr = virSaveLastError();

"Typically" the processing is:

    orig_err = virSaveLastError();
    { do stuff };
    if (orig_err) {
        virSetError(orig_err);
        virFreeError(orig_err);
    }

So, how do we know threadErr got something in return and if it didn't
we've messed up "other" algorithms. In particular, the lines in the
previous hunk won't set 'events' any more.

I'm not worried about being called twice (we know that won't happen
;-)), but spreading out the error message processing through 3 functions
causes me wonder whether that virFreeError should be done right after
the virSetError rather than waiting for virFDStreamDataDispose.

John

>      goto cleanup;
>  }
>  
>