Re: [libvirt] [PATCH v4 1/6] virfdstream: Check for thread error more frequently
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(a)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(a)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?
[...]