Rework the management of the CHANNEL_BLOCKED and CHANNEL_EOF flags, in

particular not allowing them to leak between multiple layers of a stacked
channel.  Much common code refactored into ChanRead().

4 files changed, 172 insertions, 161 deletions

diff --git a/generic/tclIO.c b/generic/tclIO.c
index 7a53373..9e1a723 100644
--- a/generic/tclIO.c
+++ b/generic/tclIO.c
@@ -167,6 +167,7 @@ static void		PreserveChannelBuffer(ChannelBuffer *bufPtr);
 static void		ReleaseChannelBuffer(ChannelBuffer *bufPtr);
 static int		IsShared(ChannelBuffer *bufPtr);
 static void		ChannelTimerProc(ClientData clientData);
+static int		ChanRead(Channel *chanPtr, char *dst, int dstSize);
 static int		CheckChannelErrors(ChannelState *statePtr,
 			    int direction);
 static int		CheckForDeadChannel(Tcl_Interp *interp,
@@ -345,22 +346,78 @@ static Tcl_ObjType chanObjType = {
 #define MAX_CHANNEL_BUFFER_SIZE (1024*1024)
 
 /*
- * ChanRead, dropped here by a time traveler, see 8.6
+ *---------------------------------------------------------------------------
+ *
+ * ChanRead --
+ *
+ *	Read up to dstSize bytes using the inputProc of chanPtr, store
+ *	them at dst, and return the number of bytes stored.
+ *
+ * Results:
+ *	The return value of the driver inputProc,
+ *	  - number of bytes stored at dst, ot
+ *	  - -1 on error, with a Posix error code available to the
+ *	    caller by calling Tcl_GetErrno().
+ *
+ * Side effects:
+ *	The CHANNEL_BLOCKED and CHANNEL_EOF flags of the channel state are
+ *	set as appropriate.
+ *	On EOF, the inputEncodingFlags are set to perform ending operations
+ *	on decoding.
+ *	TODO - Is this really the right place for that?
+ *
+ *---------------------------------------------------------------------------
  */
-static inline int
+static int
 ChanRead(
     Channel *chanPtr,
     char *dst,
-    int dstSize,
-    int *errnoPtr)
+    int dstSize)
 {
+    int bytesRead, result;
+
+    /*
+     * If the caller asked for zero bytes, we'd force the inputProc
+     * to return zero bytes, and then misinterpret that as EOF.
+     */
+    assert(dstSize > 0);
+
+    /*
+     * Each read op must set the blocked and eof states anew, not let
+     * the effect of prior reads leak through.
+     */
+    ResetFlag(chanPtr->state, CHANNEL_BLOCKED | CHANNEL_EOF);
     if (WillRead(chanPtr) < 0) {
-	*errnoPtr = Tcl_GetErrno();
         return -1;
     }
 
-    return chanPtr->typePtr->inputProc(chanPtr->instanceData, dst, dstSize,
-	    errnoPtr);
+    bytesRead = chanPtr->typePtr->inputProc(chanPtr->instanceData,
+	    dst, dstSize, &result);
+
+    /* Stop any flag leakage through stacked channel levels */
+    ResetFlag(chanPtr->state, CHANNEL_BLOCKED | CHANNEL_EOF);
+    if (bytesRead > 0) {
+	/*
+	 * If we get a short read, signal up that we may be BLOCKED.
+	 * We should avoid calling the driver because on some
+	 * platforms we will block in the low level reading code even
+	 * though the channel is set into nonblocking mode.
+	 */
+
+	if (bytesRead < dstSize) {
+	    SetFlag(chanPtr->state, CHANNEL_BLOCKED);
+	}
+    } else if (bytesRead == 0) {
+	SetFlag(chanPtr->state, CHANNEL_EOF);
+	chanPtr->state->inputEncodingFlags |= TCL_ENCODING_END;
+    } else if (bytesRead < 0) {
+	if ((result == EWOULDBLOCK) || (result == EAGAIN)) {
+	    SetFlag(chanPtr->state, CHANNEL_BLOCKED);
+	    result = EAGAIN;
+	}
+	Tcl_SetErrno(result);
+    }
+    return bytesRead;
 }
 
 static inline Tcl_WideInt
@@ -4121,6 +4178,7 @@ Tcl_GetsObj(
 		Tcl_SetObjLength(objPtr, oldLength);
 		CommonGetsCleanup(chanPtr);
 		copiedTotal = -1;
+		ResetFlag(statePtr, CHANNEL_BLOCKED);
 		goto done;
 	    }
 	    goto gotEOL;
@@ -4320,11 +4378,9 @@ TclGetsObjBinary(
 	     * device. Side effect is to allocate another channel buffer.
 	     */
 
-	    if (GotFlag(statePtr, CHANNEL_BLOCKED)) {
-		if (GotFlag(statePtr, CHANNEL_NONBLOCKING)) {
-		    goto restore;
-		}
-		ResetFlag(statePtr, CHANNEL_BLOCKED);
+	    if (GotFlag(statePtr, CHANNEL_BLOCKED|CHANNEL_NONBLOCKING)
+		    == (CHANNEL_BLOCKED|CHANNEL_NONBLOCKING)) {
+		goto restore;
 	    }
 	    if (GetInput(chanPtr) != 0) {
 		goto restore;
@@ -4387,6 +4443,7 @@ TclGetsObjBinary(
 		byteArray = Tcl_SetByteArrayLength(objPtr, oldLength);
 		CommonGetsCleanup(chanPtr);
 		copiedTotal = -1;
+		ResetFlag(statePtr, CHANNEL_BLOCKED);
 		goto done;
 	    }
 	    goto gotEOL;
@@ -4590,14 +4647,6 @@ FilterInputBytes(
 	 */
 
     read:
-	if (GotFlag(statePtr, CHANNEL_BLOCKED)) {
-	    if (GotFlag(statePtr, CHANNEL_NONBLOCKING)) {
-		gsPtr->charsWrote = 0;
-		gsPtr->rawRead = 0;
-		return -1;
-	    }
-	    ResetFlag(statePtr, CHANNEL_BLOCKED);
-	}
 	if (GetInput(chanPtr) != 0) {
 	    gsPtr->charsWrote = 0;
 	    gsPtr->rawRead = 0;
@@ -4685,9 +4734,15 @@ FilterInputBytes(
 	    } else {
 		/*
 		 * There are no more cached raw bytes left. See if we can get
-		 * some more.
+		 * some more, but avoid blocking on a non-blocking channel.
 		 */
 
+		if (GotFlag(statePtr, CHANNEL_NONBLOCKING|CHANNEL_BLOCKED)
+			== (CHANNEL_NONBLOCKING|CHANNEL_BLOCKED)) {
+		    gsPtr->charsWrote = 0;
+		    gsPtr->rawRead = 0;
+		    return -1;
+		}
 		goto read;
 	    }
 	} else {
@@ -4932,7 +4987,7 @@ Tcl_ReadRaw(
     Channel *chanPtr = (Channel *) chan;
     ChannelState *statePtr = chanPtr->state;
 				/* State info for channel */
-    int nread, result, copied, copiedNow;
+    int nread, copied, copiedNow = INT_MAX;
 
     /*
      * The check below does too much because it will reject a call to this
@@ -4957,74 +5012,31 @@ Tcl_ReadRaw(
      */
 
     Tcl_Preserve(chanPtr);
-    for (copied = 0; copied < bytesToRead; copied += copiedNow) {
-	copiedNow = CopyBuffer(chanPtr, bufPtr + copied,
-		bytesToRead - copied);
-	if (copiedNow == 0) {
-	    if (GotFlag(statePtr, CHANNEL_EOF)) {
-		goto done;
-	    }
-	    if (GotFlag(statePtr, CHANNEL_BLOCKED)) {
-		if (GotFlag(statePtr, CHANNEL_NONBLOCKING)) {
-		    goto done;
-		}
-		ResetFlag(statePtr, CHANNEL_BLOCKED);
-	    }
-
-	    /*
-	     * Now go to the driver to get as much as is possible to
-	     * fill the remaining request. Do all the error handling by
-	     * ourselves. The code was stolen from 'GetInput' and
-	     * slightly adapted (different return value here).
-	     *
-	     * The case of 'bytesToRead == 0' at this point cannot
-	     * happen.
-	     */
-
-	    nread = ChanRead(chanPtr, bufPtr + copied,
-		    bytesToRead - copied, &result);
-
-	    if (nread > 0) {
-		/*
-		 * If we get a short read, signal up that we may be BLOCKED.
-		 * We should avoid calling the driver because on some
-		 * platforms we will block in the low level reading code even
-		 * though the channel is set into nonblocking mode.
-		 */
-
-		if (nread < (bytesToRead - copied)) {
-		    SetFlag(statePtr, CHANNEL_BLOCKED);
-		}
-	    } else if (nread == 0) {
-		SetFlag(statePtr, CHANNEL_EOF);
-		statePtr->inputEncodingFlags |= TCL_ENCODING_END;
-
-	    } else if (nread < 0) {
-		if ((result == EWOULDBLOCK) || (result == EAGAIN)) {
-		    if (copied > 0) {
-			/*
-			 * Information that was copied earlier has precedence
-			 * over EAGAIN/WOULDBLOCK handling.
-			 */
-
-			goto done;
-		    }
+    for (copied = 0; bytesToRead > 0 && copiedNow > 0;
+	    bufPtr+=copiedNow, bytesToRead-=copiedNow, copied+=copiedNow) {
+	copiedNow = CopyBuffer(chanPtr, bufPtr, bytesToRead);
+    }
 
-		    SetFlag(statePtr, CHANNEL_BLOCKED);
-		    result = EAGAIN;
-		}
+    if (bytesToRead > 0) {
+	/*
+	 * Now go to the driver to get as much as is possible to
+	 * fill the remaining request.  Since we're directly filling
+	 * the caller's buffer, retain the blocked flag.
+	 */
 
-		Tcl_SetErrno(result);
+	nread = ChanRead(chanPtr, bufPtr, bytesToRead);
+	if (nread < 0) {
+	    if (!GotFlag(statePtr, CHANNEL_BLOCKED) || copied == 0) {
 		copied = -1;
-		goto done;
 	    }
-
+	} else {
 	    copied += nread;
-	    goto done;
+	}
+	if (copied != 0) {
+	    ResetFlag(statePtr, CHANNEL_EOF);
 	}
     }
 
-  done:
     Tcl_Release(chanPtr);
     return copied;
 }
@@ -5158,6 +5170,8 @@ DoReadChars(
 	}
     }
 
+    /* Must clear the BLOCKED flag here since we check before reading */
+    ResetFlag(statePtr, CHANNEL_BLOCKED);
     for (copied = 0; (unsigned) toRead > 0; ) {
 	copiedNow = -1;
 	if (statePtr->inQueueHead != NULL) {
@@ -5188,11 +5202,9 @@ DoReadChars(
 	    if (GotFlag(statePtr, CHANNEL_EOF)) {
 		break;
 	    }
-	    if (GotFlag(statePtr, CHANNEL_BLOCKED)) {
-		if (GotFlag(statePtr, CHANNEL_NONBLOCKING)) {
-		    break;
-		}
-		ResetFlag(statePtr, CHANNEL_BLOCKED);
+	    if (GotFlag(statePtr, CHANNEL_NONBLOCKING|CHANNEL_BLOCKED)
+		    == (CHANNEL_NONBLOCKING|CHANNEL_BLOCKED)) {
+		break;
 	    }
 	    result = GetInput(chanPtr);
 	    if (chanPtr != statePtr->topChanPtr) {
@@ -5201,11 +5213,10 @@ DoReadChars(
 		Tcl_Preserve(chanPtr);
 	    }
 	    if (result != 0) {
-		if (result == EAGAIN) {
-		    break;
+		if (!GotFlag(statePtr, CHANNEL_BLOCKED)) {
+		    copied = -1;
 		}
-		copied = -1;
-		goto done;
+		break;
 	    }
 	} else {
 	    copied += copiedNow;
@@ -5213,14 +5224,15 @@ DoReadChars(
 	}
     }
 
-    ResetFlag(statePtr, CHANNEL_BLOCKED);
-
     /*
-     * Update the notifier state so we don't block while there is still data
-     * in the buffers.
+     * Failure to fill a channel buffer may have left channel reporting
+     * a "blocked" state, but so long as we fulfilled the request here,
+     * the caller does not consider us blocked.
      */
+    if (toRead == 0) {
+	ResetFlag(statePtr, CHANNEL_BLOCKED);
+    }
 
-  done:
     /*
      * Regenerate the top channel, in case it was changed due to
      * self-modifying reflected transforms.
@@ -5230,6 +5242,11 @@ DoReadChars(
 	chanPtr = statePtr->topChanPtr;
 	Tcl_Preserve(chanPtr);
     }
+
+    /*
+     * Update the notifier state so we don't block while there is still data
+     * in the buffers.
+     */
     UpdateInterest(chanPtr);
     Tcl_Release(chanPtr);
     return copied;
@@ -6061,6 +6078,18 @@ GetInput(
 	return EINVAL;
     }
 
+    /* 
+     * For a channel at EOF do not bother allocating buffers; there's
+     * nothing more to read.  Avoid calling the driver inputproc in
+     * case some of them do not react well to additional calls after
+     * they've reported an eof state..
+     * TODO: Candidate for a can't happen panic.
+     */
+
+    if (GotFlag(statePtr, CHANNEL_EOF)) {
+	return 0;
+    }
+
     /*
      * First check for more buffers in the pushback area of the topmost
      * channel in the stack and use them. They can be the result of a
@@ -6125,43 +6154,16 @@ GetInput(
 	statePtr->inQueueTail = bufPtr;
     }
 
-    /*
-     * TODO - consider escape before buffer alloc
-     * If EOF is set, we should avoid calling the driver because on some
-     * platforms it is impossible to read from a device after EOF.
-     */
-
-    if (GotFlag(statePtr, CHANNEL_EOF)) {
-	return 0;
-    }
-
     PreserveChannelBuffer(bufPtr);
-    nread = ChanRead(chanPtr, InsertPoint(bufPtr), toRead, &result);
-    if (nread > 0) {
-	result = 0;
-	bufPtr->nextAdded += nread;
-
-	/*
-	 * If we get a short read, signal up that we may be BLOCKED. We should
-	 * avoid calling the driver because on some platforms we will block in
-	 * the low level reading code even though the channel is set into
-	 * nonblocking mode.
-	 */
+    nread = ChanRead(chanPtr, InsertPoint(bufPtr), toRead);
 
-	if (nread < toRead) {
-	    SetFlag(statePtr, CHANNEL_BLOCKED);
-	}
-    } else if (nread == 0) {
+    if (nread < 0) {
+	result = Tcl_GetErrno();
+    } else {
 	result = 0;
-	SetFlag(statePtr, CHANNEL_EOF);
-	statePtr->inputEncodingFlags |= TCL_ENCODING_END;
-    } else if (nread < 0) {
-	if ((result == EWOULDBLOCK) || (result == EAGAIN)) {
-	    SetFlag(statePtr, CHANNEL_BLOCKED);
-	    result = EAGAIN;
-	}
-	Tcl_SetErrno(result);
+	bufPtr->nextAdded += nread;
     }
+
     ReleaseChannelBuffer(bufPtr);
     return result;
 }
@@ -6649,12 +6651,7 @@ CheckChannelErrors(
     }
 
     if (direction == TCL_READABLE) {
-	/*
-	 * Clear the BLOCKED bit. We want to discover this condition
-	 * anew in each operation.
-	 */
-
-	ResetFlag(statePtr, CHANNEL_BLOCKED | CHANNEL_NEED_MORE_DATA);
+	ResetFlag(statePtr, CHANNEL_NEED_MORE_DATA);
     }
 
     return 0;
@@ -8831,7 +8828,6 @@ DoRead(
 	while (bufPtr == NULL || !IsBufferFull(bufPtr)) {
 	    int code;
 
-	    ResetFlag(statePtr, CHANNEL_BLOCKED);
 	moreData:
 	    code = GetInput(chanPtr);
 	    bufPtr = statePtr->inQueueHead;
@@ -8935,11 +8931,14 @@ DoRead(
 	    RecycleBuffer(statePtr, bufPtr, 0);
 	}
 
-	if (GotFlag(statePtr, CHANNEL_NONBLOCKING)
-		&& GotFlag(statePtr, CHANNEL_BLOCKED)) {
+	if (GotFlag(statePtr, CHANNEL_NONBLOCKING|CHANNEL_BLOCKED)
+		== (CHANNEL_NONBLOCKING|CHANNEL_BLOCKED)) {
 	    break;
 	}
     }
+    if (bytesToRead == 0) {
+	ResetFlag(statePtr, CHANNEL_BLOCKED);
+    }
 
     Tcl_Release(chanPtr);
     return (int)(p - dst);
diff --git a/generic/tclIOGT.c b/generic/tclIOGT.c
index c5372b1..fe0a880 100644
--- a/generic/tclIOGT.c
+++ b/generic/tclIOGT.c
@@ -683,38 +683,35 @@ TransformInputProc(
 
 	read = Tcl_ReadRaw(downChan, buf, toRead);
 	if (read < 0) {
-	    /*
-	     * Report errors to caller. EAGAIN is a special situation. If we
-	     * had some data before we report that instead of the request to
-	     * re-try.
-	     */
 
-	    if ((Tcl_GetErrno() == EAGAIN) && (gotBytes > 0)) {
+	    if (Tcl_InputBlocked(downChan) && (gotBytes > 0)) {
+		/*
+		 * Zero bytes available from downChan because blocked.
+		 * But nonzero bytes already copied, so total is a
+		 * valid blocked short read. Return to caller.
+		 */
+
 		break;
 	    }
 
+	    /*
+	     * Either downChan is not blocked (there's a real error).
+	     * or it is and there are no bytes copied yet.  In either
+	     * case we want to pass the "error" along to the caller,
+	     * either to report an error, or to signal to the caller
+	     * that zero bytes are available because blocked.
+	     */
+
 	    *errorCodePtr = Tcl_GetErrno();
 	    gotBytes = -1;
 	    break;
 	} else if (read == 0) {
+
 	    /*
-	     * Check wether we hit on EOF in the underlying channel or not. If
-	     * not differentiate between blocking and non-blocking modes. In
-	     * non-blocking mode we ran temporarily out of data. Signal this
-	     * to the caller via EWOULDBLOCK and error return (-1). In the
-	     * other cases we simply return what we got and let the caller
-	     * wait for more. On the other hand, if we got an EOF we have to
-	     * convert and flush all waiting partial data.
+	     * Zero returned from Tcl_ReadRaw() always indicates EOF
+	     * on the down channel.
 	     */
 
-	    if (!Tcl_Eof(downChan)) {
-		if ((gotBytes == 0) && (dataPtr->flags & CHANNEL_ASYNC)) {
-		    *errorCodePtr = EWOULDBLOCK;
-		    gotBytes = -1;
-		}
-		break;
-	    }
-
 	    if (dataPtr->readIsFlushed) {
 		/*
 		 * Already flushed, nothing to do anymore.
diff --git a/tests/io.test b/tests/io.test
index 5f31d8e..a2e2397 100644
--- a/tests/io.test
+++ b/tests/io.test
@@ -6705,11 +6705,23 @@ test io-52.4 {TclCopyChannel} {fcopy} {
     fconfigure $f1 -translation lf -blocking 0
     fconfigure $f2 -translation cr -blocking 0
     fcopy $f1 $f2 -size 40
-    set result [list [fconfigure $f1 -blocking] [fconfigure $f2 -blocking]]
+    set result [list [fblocked $f1] [fconfigure $f1 -blocking] [fconfigure $f2 -blocking]]
+    close $f1
+    close $f2
+    lappend result [file size $path(test1)]
+} {0 0 0 40}
+test io-52.4.1 {TclCopyChannel} {fcopy} {
+    file delete $path(test1)
+    set f1 [open $thisScript]
+    set f2 [open $path(test1) w]
+    fconfigure $f1 -translation lf -blocking 0 -buffersize 10000000
+    fconfigure $f2 -translation cr -blocking 0
+    fcopy $f1 $f2 -size 40
+    set result [list [fblocked $f1] [fconfigure $f1 -blocking] [fconfigure $f2 -blocking]]
     close $f1
     close $f2
     lappend result [file size $path(test1)]
-} {0 0 40}
+} {0 0 0 40}
 test io-52.5 {TclCopyChannel, all} {fcopy} {
     file delete $path(test1)
     set f1 [open $thisScript]
diff --git a/tests/iogt.test b/tests/iogt.test
index 0e2eb3c..5fe3dc2 100644
--- a/tests/iogt.test
+++ b/tests/iogt.test
@@ -552,6 +552,7 @@ query/maxRead
 read
 query/maxRead
 flush/read
+query/maxRead
 delete/read
 --------
 create/write
@@ -604,6 +605,7 @@ read {
 }
 query/maxRead {} -1
 flush/read {} {}
+query/maxRead {} -1
 delete/read {} *ignored*
 --------
 create/write {} *ignored*
@@ -658,6 +660,7 @@ read {%^&*()_+-=
 }
 query/maxRead {} -1
 flush/read {} {}
+query/maxRead {} -1
 write %^&*()_+-= %^&*()_+-=
 write {
 } {