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(-)

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;
+
     cb = fdst->cb;
     cbopaque = fdst->opaque;
     ff = fdst->ff;
@@ -791,10 +794,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"));
-            return -1;
+            goto cleanup;
         }
 
         if (VIR_ALLOC(msg) < 0 ||
@@ -870,7 +873,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"));
@@ -971,6 +974,13 @@ virFDStreamSendHole(virStreamPtr st,
          * the thread to do the lseek() for us. Under no
          * circumstances we can do the lseek() ourselves here. We
          * might mess up file position for the thread. */
+
+        if (fdst->threadQuit || fdst->threadErr) {
+            virReportSystemError(EBADF, "%s",
+                                 _("stream is not open"));
+            goto cleanup;
+        }
+
         if (fdst->threadDoRead) {
             msg = fdst->msg;
             if (msg->type != VIR_FDSTREAM_MSG_TYPE_HOLE) {
@@ -1025,6 +1035,9 @@ virFDStreamInData(virStreamPtr st,
     if (fdst->thread) {
         virFDStreamMsgPtr msg;
 
+        if (fdst->threadErr)
+            goto cleanup;
+
         while (!(msg = fdst->msg)) {
             if (fdst->threadQuit) {
                 *inData = *length = 0;
-- 
2.13.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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?


[...]

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list