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@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;