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@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@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? [...]