[libvirt] [PATCH v3 1/6] virfdstream: Check for thread error more frequently

[John Ferlan]

On 06/05/2017 04:22 AM, Michal Privoznik wrote:
> When the I/O thread quits (e.g. due to an I/O error, lseek()
> error, whatever), any subsequent virFDStream API should return
> error too. Moreover, when invoking stream event callback, we must
> set the VIR_STREAM_EVENT_ERROR flag so that the callback knows
> something bad happened.
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/util/virfdstream.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 

> diff --git a/src/util/virfdstream.c b/src/util/virfdstream.c
> index 7ee58be13..cd24757e6 100644
> --- a/src/util/virfdstream.c
> +++ b/src/util/virfdstream.c
> @@ -312,6 +312,9 @@ static void virFDStreamEvent(int watch ATTRIBUTE_UNUSED,
>          return;
>      }
>  
> +    if (fdst->threadErr)
> +        events |= VIR_STREAM_EVENT_ERROR;
> +

This hunk almost feels separable... That is, it's updating the events
(VIR_STREAM_EVENT_ERROR) in the callback function feels different than
checking for threadErr more often, but I'll leave it up to you to decide
to split more or not.

>      cb = fdst->cb;
>      cbopaque = fdst->opaque;
>      ff = fdst->ff;
> @@ -787,10 +790,10 @@ static int virFDStreamWrite(virStreamPtr st, const char *bytes, size_t nbytes)
>      if (fdst->thread) {
>          char *buf;
>  
> -        if (fdst->threadQuit) {
> +        if (fdst->threadQuit || fdst->threadErr) {
>              virReportSystemError(EBADF, "%s",
>                                   _("cannot write to stream"));

Would this (and the subsequent similar check) possibly overwrite
something from a threadErr with this error message?

> -            return -1;
> +            goto cleanup;

ouch, <sigh> and this is separable too technically....

>          }
>  
>          if (VIR_ALLOC(msg) < 0 ||
> @@ -866,7 +869,7 @@ static int virFDStreamRead(virStreamPtr st, char *bytes, size_t nbytes)
>          virFDStreamMsgPtr msg = NULL;
>  
>          while (!(msg = fdst->msg)) {
> -            if (fdst->threadQuit) {
> +            if (fdst->threadQuit || fdst->threadErr) {
>                  if (nbytes) {
>                      virReportSystemError(EBADF, "%s",
>                                           _("stream is not open"));
> @@ -959,6 +962,9 @@ virFDStreamSendHole(virStreamPtr st,
>          fdst->offset += length;
>      }
>  
> +    if (fdst->threadErr)
> +        goto cleanup;
> +

So it's interesting in these paths the threadErr check is done outside
the fdst->thread check which is different than the virFDStreamRead and
Write API's.

>      if (fdst->thread) {
>          /* Things are a bit complicated here. If FDStream is in a
>           * read mode, then if the message at the queue head is
> @@ -1018,6 +1024,9 @@ virFDStreamInData(virStreamPtr st,
>  
>      virObjectLock(fdst);
>  
> +    if (fdst->threadErr)
> +        goto cleanup;
> +

This one in particular because threadQuit is checked in the subsequent
code similar to virFDStreamRead made me ponder a bit more, but I'm not
sure I could come to a conclusion...

I guess I'd want to see some consistency over when threadErr is checked
between the 4 functions or I'd wonder why 2 do things one way and 2 the
other. If they need to be different for a specific reason, then I
suppose more separation could be done or at least the commit message
indicate why they're different.

John

>      if (fdst->thread) {
>          virFDStreamMsgPtr msg;
>  
>