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

[John Ferlan]

On 06/22/2017 08:30 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 | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 


Trying to keep a fresh an macroscopic view and not think about my
previous myopic look at the code... I got too hung up on the "error"
part (as in reporting an error in an || condition even though
conceptually one would have been reported)...

> diff --git a/src/util/virfdstream.c b/src/util/virfdstream.c
> index ae6f78e01..bd2355d17 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;
> +

I spent some cycles considering if there was any "odd possibility" that
the @cb could be called twice w/ some virStreamAbort interaction, but I
was able to convince myself that it shouldn't be possible. Still just
want to "be sure" since virFDStreamCloseInt uses VIR_STREAM_EVENT_ERROR
and also sets abortCallbackCalled to ensure it couldn't be called again.
Is that something perhaps that could/should be done here as well -
defensive type programming?

In any case, I'm sufficiently convinced this is fine, but figured I'd
ask as well!

Reviewed-by: John Ferlan <jferlan at redhat.com>

John

Would it be a bad time to point out that virFDStreamJoinWorker returning
-1 and setting ret = -1 in the caller doesn't really do anything since
ret is immediately overwritten?


[...]