From 7ab8a9b2efc85997d0a6f576c20a69d272bd98ff Mon Sep 17 00:00:00 2001 From: dgp Date: Tue, 28 Oct 2014 20:10:32 +0000 Subject: Work in progress restoring ability to [read] after [eof] and get non-empty strings back in those cases where the channel has them to offer. Also working through all the implications of this possibility on Tcl's more exotic channel features, like stacking. --- generic/tclIO.c | 88 ++++++++++++++++++++++++++++++++++++++++++------------- generic/tclIOGT.c | 20 ++++++++----- 2 files changed, 79 insertions(+), 29 deletions(-) diff --git a/generic/tclIO.c b/generic/tclIO.c index 0122ec9..c55c118 100644 --- a/generic/tclIO.c +++ b/generic/tclIO.c @@ -4414,6 +4414,17 @@ TclGetsObjBinary( if (bufPtr == NULL) { goto restore; } + } else { + /* + * There's something already in the buffer. If + * CHANNEL_STICKY_EOF is set we know that something begins + * with the eofchar. Otherwise, if CHANNEL_EOF is set, we + * know some earlier inputproc call returned zero bytes when + * we were trying to get more bytes to put in the buffer. + * which means..... ???? Place to probe with tests. + */ + assert (GotFlag(statePtr, CHANNEL_STICKY_EOF) + || !GotFlag(statePtr, CHANNEL_EOF) ); } dst = (unsigned char *) RemovePoint(bufPtr); @@ -4694,6 +4705,9 @@ FilterInputBytes( gsPtr->rawRead = 0; return -1; } + } else { + assert( GotFlag(statePtr, CHANNEL_STICKY_EOF) + || !GotFlag(statePtr, CHANNEL_EOF) ); } /* @@ -5018,6 +5032,7 @@ Tcl_ReadRaw( /* State info for channel */ int copied = 0; + assert(bytesToRead > 0); if (CheckChannelErrors(statePtr, TCL_READABLE | CHANNEL_RAW_MODE) != 0) { return -1; } @@ -5049,8 +5064,19 @@ Tcl_ReadRaw( } } - /* Go to the driver if more data needed. */ + /* + * Go to the driver only if we got nothing from pushback. + * Have to do it this way to avoid EOF mis-timings when we + * consider the ability that EOF may not be a permanent + * condition in the driver, and in that case we have to + * synchronize. + */ + + if (copied) { + return copied; + } + /* This test not needed. */ if (bytesToRead > 0) { int nread = ChanRead(chanPtr, readBuf, bytesToRead); @@ -5073,12 +5099,10 @@ Tcl_ReadRaw( if (!GotFlag(statePtr, CHANNEL_BLOCKED) || copied == 0) { copied = -1; } - } else if (copied > 0) { + } else { /* - * nread == 0. Driver is at EOF, but if copied>0 bytes - * from pushback, then we should not signal it yet. + * nread == 0. Driver is at EOF. Let that state filter up. */ - ResetFlag(statePtr, CHANNEL_EOF); } } return copied; @@ -6120,18 +6144,39 @@ GetInput( } /* - * 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. + * Strangely named "STICKY_EOF" really means we've seen the + * eofchar for this channel, and nothing since has reset it. + * (changed the eofchar, [seek]ed to a new offset, etc.) So, + * we know we're still poised to read that eofchar again, and + * there's no need to actually do it. */ - if (GotFlag(statePtr, CHANNEL_EOF)) { + if (GotFlag(statePtr, CHANNEL_STICKY_EOF)) { + assert(statePtr->inEofChar); + assert(statePtr->inQueueHead); + assert(RemovePoint(statePtr->inQueueHead)[0] == statePtr->inEofChar); return 0; } /* + * WARNING: There was once a comment here claiming that it was + * a bad idea to make another call to the inputproc of a channel + * driver when EOF has already been detected on the channel. Through + * much of Tcl's history, this warning was then completely negated + * by having all (most?) read paths clear the EOF setting before + * reaching here. So we had a guard that was never triggered. + * + * Don't be tempted to restore the guard. Even if EOF is set on + * the channel, continue through and call the inputproc again. This + * is the way to enable the ability to [read] again beyond the EOF, + * which seems a strange thing to do, but for which use cases exist + * [Tcl Bug 5adc350683] and which may even be essential for channels + * representing things like ttys or other devices where the stream + * might take the logical form of a series of 'files' separated by + * an EOF condition. + */ + + /* * 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 * transformation which went away without reading all the information @@ -8848,16 +8893,6 @@ DoRead( ChannelBuffer *bufPtr = statePtr->inQueueHead; /* - * When there's no buffered data to read, and we're at EOF, - * escape to the caller. - */ - - if (GotFlag(statePtr, CHANNEL_EOF) - && (bufPtr == NULL || IsBufferEmpty(bufPtr))) { - break; - } - - /* * Don't read more data if we have what we need. */ @@ -8969,12 +9004,23 @@ DoRead( statePtr->inQueueTail = NULL; } RecycleBuffer(statePtr, bufPtr, 0); + bufPtr = statePtr->inQueueHead; } if (GotFlag(statePtr, CHANNEL_NONBLOCKING|CHANNEL_BLOCKED) == (CHANNEL_NONBLOCKING|CHANNEL_BLOCKED)) { break; } + + /* + * When there's no buffered data to read, and we're at EOF, + * escape to the caller. + */ + + if (GotFlag(statePtr, CHANNEL_EOF) + && (bufPtr == NULL || IsBufferEmpty(bufPtr))) { + break; + } } if (bytesToRead == 0) { ResetFlag(statePtr, CHANNEL_BLOCKED); diff --git a/generic/tclIOGT.c b/generic/tclIOGT.c index fe0a880..a78a5b4 100644 --- a/generic/tclIOGT.c +++ b/generic/tclIOGT.c @@ -677,6 +677,18 @@ TransformInputProc( break; } + if (dataPtr->readIsFlushed) { + /* + * Already saw EOF from downChan; don't ask again. + * NOTE: Could move this up to avoid the last maxRead + * execution. Believe this would still be correct behavior, + * but the test suite tests the whole command callback + * sequence, so leave it unchanged for now. + */ + + break; + } + /* * Get bytes from the underlying channel. */ @@ -712,14 +724,6 @@ TransformInputProc( * on the down channel. */ - if (dataPtr->readIsFlushed) { - /* - * Already flushed, nothing to do anymore. - */ - - break; - } - dataPtr->readIsFlushed = 1; ExecuteCallback(dataPtr, NULL, A_FLUSH_READ, NULL, 0, TRANSMIT_IBUF, P_PRESERVE); -- cgit v0.12 From 75e9bc701fe237f31e8d1467070fadbcc5457a32 Mon Sep 17 00:00:00 2001 From: dgp Date: Wed, 29 Oct 2014 14:54:11 +0000 Subject: Base test for [5adc350683]. --- tests/io.test | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/tests/io.test b/tests/io.test index f6690ad..d533957 100644 --- a/tests/io.test +++ b/tests/io.test @@ -8399,6 +8399,26 @@ test io-73.2 {channel Tcl_Obj SetChannelFromAny, bug 2407783} {} { list $code [string map [list $f @@] $msg] } {1 {can not find channel named "@@"}} +test io-73.3 {[5adc350683] [gets] after EOF} -setup { + set fn [makeFile {} io-73.3] + set rfd [open $fn r] + set wfd [open $fn a] + chan configure $wfd -buffering line + read $rfd +} -body { + set result [eof $rfd] + puts $wfd "more data" + lappend result [eof $rfd] + lappend result [gets $rfd] + lappend result [eof $rfd] + lappend result [gets $rfd] + lappend result [eof $rfd] +} -cleanup { + close $wfd + close $rfd + removeFile io-73.3 +} -result {1 1 {more data} 0 {} 1} + # ### ### ### ######### ######### ######### # cleanup -- cgit v0.12 From f6161cafab22785e5a346b26bd0ef59cdb950c57 Mon Sep 17 00:00:00 2001 From: "jan.nijtmans" Date: Fri, 31 Oct 2014 09:59:36 +0000 Subject: When translating a reserved devicename to native pathname, strip ':' postfix. Possible fix for [dcc03414f5], but anyway a good idea. --- win/tclWinFile.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/win/tclWinFile.c b/win/tclWinFile.c index 9bf63b1..163050e 100644 --- a/win/tclWinFile.c +++ b/win/tclWinFile.c @@ -3203,7 +3203,10 @@ TclNativeCreateNativeRep( Tcl_WinUtfToTChar(str, len, &ds); if (tclWinProcs->useWide) { WCHAR *wp = (WCHAR *) Tcl_DStringValue(&ds); - len = Tcl_DStringLength(&ds)>>1; + /* For a reserved device, strip a possible postfix ':' */ + len = WinIsReserved(str); + /* For normal devices */ + if (len == 0) len = Tcl_DStringLength(&ds)>>1; /* ** If path starts with "//?/" or "\\?\" (extended path), translate ** any slashes to backslashes but accept the '?' as being valid. -- cgit v0.12 From 384950b6d12f0e3d0da23ed08707b41eb609c9cd Mon Sep 17 00:00:00 2001 From: "jan.nijtmans" Date: Fri, 31 Oct 2014 10:57:25 +0000 Subject: Extend WinIsReserved() to recognize COM[5-9]: as valid com ports as well. --- win/tclWinFile.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/win/tclWinFile.c b/win/tclWinFile.c index 163050e..7487022 100644 --- a/win/tclWinFile.c +++ b/win/tclWinFile.c @@ -1225,9 +1225,9 @@ WinIsReserved( if ((path[0] == 'c' || path[0] == 'C') && (path[1] == 'o' || path[1] == 'O')) { if ((path[2] == 'm' || path[2] == 'M') - && path[3] >= '1' && path[3] <= '4') { + && path[3] >= '1' && path[3] <= '9') { /* - * May have match for 'com[1-4]:?', which is a serial port. + * May have match for 'com[1-9]:?', which is a serial port. */ if (path[4] == '\0') { -- cgit v0.12 From 9d91e28e770e770f99f3c3115ab9e86e9d481a2d Mon Sep 17 00:00:00 2001 From: dgp Date: Sat, 1 Nov 2014 14:52:55 +0000 Subject: Disable assertion until tls bug it detects is fixed. --- generic/tclIO.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/generic/tclIO.c b/generic/tclIO.c index 207ce19..9cbc72c 100644 --- a/generic/tclIO.c +++ b/generic/tclIO.c @@ -2811,9 +2811,15 @@ FlushChannel( * write in this call, and we've completed the BG flush. * These are the two cases above. If we get here, that means * there is some kind failure in the writable event machinery. - */ + * + * The tls extension indeed suffers from flaws in its channel + * event mgmt. See http://core.tcl.tk/tcl/info/c31ca233ca. + * Until that patch is broadly distributed, disable the + * assertion checking here, so that programs using Tcl and + * tls can be debugged. assert(!calledFromAsyncFlush); + */ } } -- cgit v0.12 From 746cb2938e7c4bf900a55899aa1445ece67d52ea Mon Sep 17 00:00:00 2001 From: "jan.nijtmans" Date: Mon, 3 Nov 2014 10:14:48 +0000 Subject: Better errormessage when file path contains invalid characters. See: [03414f517b7a74]. --- win/tclWinChan.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/win/tclWinChan.c b/win/tclWinChan.c index 6d480a8..a271919 100644 --- a/win/tclWinChan.c +++ b/win/tclWinChan.c @@ -840,6 +840,11 @@ TclpOpenFileChannel( nativeName = (TCHAR*) Tcl_FSGetNativePath(pathPtr); if (nativeName == NULL) { + if (interp != (Tcl_Interp *) NULL) { + Tcl_AppendResult(interp, "couldn't open \"", + TclGetString(pathPtr), "\": filename is invalid on this platform", + NULL); + } return NULL; } -- cgit v0.12 From bce5edef8b197d622f6f22b25021afd987743698 Mon Sep 17 00:00:00 2001 From: "jan.nijtmans" Date: Mon, 3 Nov 2014 12:44:55 +0000 Subject: Add test-case for previous commit, which shows that when trying to open a filename with invalid characters gives the right error-message. (same bug existed on UNIX too, which is now fixed) --- tests/fileSystem.test | 3 +++ unix/tclUnixChan.c | 5 +++++ 2 files changed, 8 insertions(+) diff --git a/tests/fileSystem.test b/tests/fileSystem.test index 161ebc3..c255b1e 100644 --- a/tests/fileSystem.test +++ b/tests/fileSystem.test @@ -708,6 +708,9 @@ test filesystem-6.32 {empty file name} { test filesystem-6.33 {empty file name} { list [catch {file writable ""} msg] $msg } {0 0} +test filesystem-6.34 {file name with (invalid) nul character} { + list [catch "open foo\x00" msg] $msg +} [list 1 "couldn't open \"foo\x00\": filename is invalid on this platform"] # Make sure the testfilesystem hasn't been registered. if {[testConstraint testfilesystem]} { diff --git a/unix/tclUnixChan.c b/unix/tclUnixChan.c index 1d60340..89c9a27 100644 --- a/unix/tclUnixChan.c +++ b/unix/tclUnixChan.c @@ -1647,6 +1647,11 @@ TclpOpenFileChannel( native = Tcl_FSGetNativePath(pathPtr); if (native == NULL) { + if (interp != (Tcl_Interp *) NULL) { + Tcl_AppendResult(interp, "couldn't open \"", + TclGetString(pathPtr), "\": filename is invalid on this platform", + NULL); + } return NULL; } -- cgit v0.12 From c40bcd39bfbe1d61ef75fd3cc9a6604b6cd20193 Mon Sep 17 00:00:00 2001 From: dgp Date: Mon, 3 Nov 2014 20:39:25 +0000 Subject: [5adc350683] Reworked the management of the EOF states to re-enable the ability to read beyond EOF. Plenty of assert()s to keep thing from going off track again. --- generic/tclIO.c | 182 +++++++++++++++++++++++++++++++++++++++++++------------- 1 file changed, 139 insertions(+), 43 deletions(-) diff --git a/generic/tclIO.c b/generic/tclIO.c index c55c118..b3af1f5 100644 --- a/generic/tclIO.c +++ b/generic/tclIO.c @@ -3977,6 +3977,21 @@ Tcl_GetsObj( } /* + * If we're sitting ready to read the eofchar, there's no need to + * do it. + */ + + if (GotFlag(statePtr, CHANNEL_STICKY_EOF)) { + SetFlag(statePtr, CHANNEL_EOF); + assert( statePtr->inputEncodingFlags & TCL_ENCODING_END ); + assert( !GotFlag(statePtr, CHANNEL_BLOCKED|INPUT_SAW_CR) ); + + /* TODO: Do we need this? */ + UpdateInterest(chanPtr); + return -1; + } + + /* * A binary version of Tcl_GetsObj. This could also handle encodings that * are ascii-7 pure (iso8859, utf-8, ...) with a final encoding conversion * done on objPtr. @@ -4194,6 +4209,7 @@ Tcl_GetsObj( dstEnd = eof; SetFlag(statePtr, CHANNEL_EOF | CHANNEL_STICKY_EOF); statePtr->inputEncodingFlags |= TCL_ENCODING_END; + ResetFlag(statePtr, CHANNEL_BLOCKED|INPUT_SAW_CR); } if (GotFlag(statePtr, CHANNEL_EOF)) { skip = 0; @@ -4307,6 +4323,13 @@ Tcl_GetsObj( */ done: + assert(!GotFlag(statePtr, CHANNEL_EOF) + || GotFlag(statePtr, CHANNEL_STICKY_EOF) + || Tcl_InputBuffered((Tcl_Channel)chanPtr) == 0); + + assert( !(GotFlag(statePtr, CHANNEL_EOF|CHANNEL_BLOCKED) + == (CHANNEL_EOF|CHANNEL_BLOCKED)) ); + /* * Regenerate the top channel, in case it was changed due to * self-modifying reflected transforms. @@ -4330,6 +4353,11 @@ Tcl_GetsObj( * end-of-line or end-of-file has been seen. Bytes read from the input * channel return as a ByteArray obj. * + * WARNING! The notion of "binary" used here is different from + * notions of "binary" used in other places. In particular, this + * "binary" routine may be called when an -eofchar is set on the + * channel. + * * Results: * Number of characters accumulated in the object or -1 if error, * blocked, or EOF. If -1, use Tcl_GetErrno() to retrieve the POSIX error @@ -4416,15 +4444,15 @@ TclGetsObjBinary( } } else { /* - * There's something already in the buffer. If - * CHANNEL_STICKY_EOF is set we know that something begins - * with the eofchar. Otherwise, if CHANNEL_EOF is set, we - * know some earlier inputproc call returned zero bytes when - * we were trying to get more bytes to put in the buffer. - * which means..... ???? Place to probe with tests. - */ - assert (GotFlag(statePtr, CHANNEL_STICKY_EOF) - || !GotFlag(statePtr, CHANNEL_EOF) ); + * Incoming CHANNEL_STICKY_EOF is filtered out on entry. + * A new CHANNEL_STICKY_EOF set in this routine leads to + * return before coming back here. When we are not dealing + * with CHANNEL_STICKY_EOF, a CHANNEL_EOF implies an + * empty buffer. Here the buffer is non-empty so we know + * we're a non-EOF */ + + assert ( !GotFlag(statePtr, CHANNEL_STICKY_EOF) ); + assert ( !GotFlag(statePtr, CHANNEL_EOF) ); } dst = (unsigned char *) RemovePoint(bufPtr); @@ -4466,6 +4494,7 @@ TclGetsObjBinary( SetFlag(statePtr, CHANNEL_EOF | CHANNEL_STICKY_EOF); statePtr->inputEncodingFlags |= TCL_ENCODING_END; + ResetFlag(statePtr, CHANNEL_BLOCKED|INPUT_SAW_CR); } if (GotFlag(statePtr, CHANNEL_EOF)) { skip = 0; @@ -4575,6 +4604,11 @@ TclGetsObjBinary( */ done: + assert(!GotFlag(statePtr, CHANNEL_EOF) + || GotFlag(statePtr, CHANNEL_STICKY_EOF) + || Tcl_InputBuffered((Tcl_Channel)chanPtr) == 0); + assert( !(GotFlag(statePtr, CHANNEL_EOF|CHANNEL_BLOCKED) + == (CHANNEL_EOF|CHANNEL_BLOCKED)) ); UpdateInterest(chanPtr); TclChannelRelease((Tcl_Channel)chanPtr); return copiedTotal; @@ -4706,8 +4740,16 @@ FilterInputBytes( return -1; } } else { - assert( GotFlag(statePtr, CHANNEL_STICKY_EOF) - || !GotFlag(statePtr, CHANNEL_EOF) ); + /* + * Incoming CHANNEL_STICKY_EOF is filtered out on entry. + * A new CHANNEL_STICKY_EOF set in this routine leads to + * return before coming back here. When we are not dealing + * with CHANNEL_STICKY_EOF, a CHANNEL_EOF implies an + * empty buffer. Here the buffer is non-empty so we know + * we're a non-EOF */ + + assert ( !GotFlag(statePtr, CHANNEL_STICKY_EOF) ); + assert ( !GotFlag(statePtr, CHANNEL_EOF) ); } /* @@ -5201,19 +5243,11 @@ DoReadChars( ChannelState *statePtr = chanPtr->state; /* State info for channel */ ChannelBuffer *bufPtr; - int factor, copied, copiedNow, result; - Tcl_Encoding encoding; + int copied, copiedNow, result; + Tcl_Encoding encoding = statePtr->encoding; int binaryMode; #define UTF_EXPANSION_FACTOR 1024 - - /* - * This operation should occur at the top of a channel stack. - */ - - chanPtr = statePtr->topChanPtr; - encoding = statePtr->encoding; - factor = UTF_EXPANSION_FACTOR; - TclChannelPreserve((Tcl_Channel)chanPtr); + int factor = UTF_EXPANSION_FACTOR; binaryMode = (encoding == NULL) && (statePtr->inputTranslation == TCL_TRANSLATE_LF) @@ -5237,6 +5271,36 @@ DoReadChars( } } + /* + * Early out when next read will see eofchar. + * + * NOTE: See DoRead for argument that it's a bug (one we're keeping) + * to have this escape before the one for zero-char read request. + */ + + if (GotFlag(statePtr, CHANNEL_STICKY_EOF)) { + SetFlag(statePtr, CHANNEL_EOF); + assert( statePtr->inputEncodingFlags & TCL_ENCODING_END ); + assert( !GotFlag(statePtr, CHANNEL_BLOCKED|INPUT_SAW_CR) ); + + UpdateInterest(chanPtr); + return 0; + } + + /* Special handling for zero-char read request. */ + if (toRead == 0) { + ResetFlag(statePtr, CHANNEL_BLOCKED|CHANNEL_EOF); + UpdateInterest(chanPtr); + return 0; + } + + /* + * This operation should occur at the top of a channel stack. + */ + + chanPtr = statePtr->topChanPtr; + TclChannelPreserve((Tcl_Channel)chanPtr); + /* Must clear the BLOCKED flag here since we check before reading */ ResetFlag(statePtr, CHANNEL_BLOCKED|CHANNEL_EOF); for (copied = 0; (unsigned) toRead > 0; ) { @@ -5314,6 +5378,11 @@ DoReadChars( * Update the notifier state so we don't block while there is still data * in the buffers. */ + assert(!GotFlag(statePtr, CHANNEL_EOF) + || GotFlag(statePtr, CHANNEL_STICKY_EOF) + || Tcl_InputBuffered((Tcl_Channel)chanPtr) == 0); + assert( !(GotFlag(statePtr, CHANNEL_EOF|CHANNEL_BLOCKED) + == (CHANNEL_EOF|CHANNEL_BLOCKED)) ); UpdateInterest(chanPtr); TclChannelRelease((Tcl_Channel)chanPtr); return copied; @@ -5922,7 +5991,7 @@ TranslateInputEOL( SetFlag(statePtr, CHANNEL_EOF | CHANNEL_STICKY_EOF); statePtr->inputEncodingFlags |= TCL_ENCODING_END; - ResetFlag(statePtr, INPUT_SAW_CR); + ResetFlag(statePtr, CHANNEL_BLOCKED|INPUT_SAW_CR); } } @@ -6132,6 +6201,14 @@ GetInput( ChannelState *statePtr = chanPtr->state; /* State info for channel */ + /* + * Verify that all callers know better than to call us when + * it's recorded that the next char waiting to be read is the + * eofchar. + */ + + assert( !GotFlag(statePtr, CHANNEL_STICKY_EOF) ); + /* * Prevent reading from a dead channel -- a channel that has been closed * but not yet deallocated, which can happen if the exit handler for @@ -6143,21 +6220,6 @@ GetInput( return EINVAL; } - /* - * Strangely named "STICKY_EOF" really means we've seen the - * eofchar for this channel, and nothing since has reset it. - * (changed the eofchar, [seek]ed to a new offset, etc.) So, - * we know we're still poised to read that eofchar again, and - * there's no need to actually do it. - */ - - if (GotFlag(statePtr, CHANNEL_STICKY_EOF)) { - assert(statePtr->inEofChar); - assert(statePtr->inQueueHead); - assert(RemovePoint(statePtr->inQueueHead)[0] == statePtr->inEofChar); - return 0; - } - /* * WARNING: There was once a comment here claiming that it was * a bad idea to make another call to the inputproc of a channel @@ -6185,6 +6247,7 @@ GetInput( if (chanPtr->inQueueHead != NULL) { + /* TODO: Tests to cover this. */ assert(statePtr->inQueueHead == NULL); statePtr->inQueueHead = chanPtr->inQueueHead; @@ -6215,6 +6278,7 @@ GetInput( * Check the actual buffersize against the requested buffersize. * Saved buffers of the wrong size are squashed. This is done * to honor dynamic changes of the buffersize made by the user. + * TODO: Tests to cover this. */ if ((bufPtr != NULL) @@ -6758,9 +6822,7 @@ Tcl_Eof( ChannelState *statePtr = ((Channel *) chan)->state; /* State of real channel structure. */ - return (GotFlag(statePtr, CHANNEL_STICKY_EOF) || - (GotFlag(statePtr, CHANNEL_EOF) && - (Tcl_InputBuffered(chan) == 0))) ? 1 : 0; + return GotFlag(statePtr, CHANNEL_EOF) ? 1 : 0; } /* @@ -8882,6 +8944,36 @@ DoRead( ChannelState *statePtr = chanPtr->state; char *p = dst; + assert (bytesToRead >= 0); + + /* + * Early out when we know a read will get the eofchar. + * + * NOTE: This seems to be a bug. The special handling for + * a zero-char read request ought to come first. As coded + * the EOF due to eofchar has distinguishing behavior from + * the EOF due to reported EOF on the underlying device, and + * that seems undesirable. However recent history indicates + * that new inconsistent behavior in a patchlevel has problems + * too. Keep on keeping on for now. + */ + + if (GotFlag(statePtr, CHANNEL_STICKY_EOF)) { + SetFlag(statePtr, CHANNEL_EOF); + assert( statePtr->inputEncodingFlags & TCL_ENCODING_END ); + assert( !GotFlag(statePtr, CHANNEL_BLOCKED|INPUT_SAW_CR) ); + + UpdateInterest(chanPtr); + return 0; + } + + /* Special handling for zero-char read request. */ + if (bytesToRead == 0) { + ResetFlag(statePtr, CHANNEL_BLOCKED|CHANNEL_EOF); + UpdateInterest(chanPtr); + return 0; + } + TclChannelPreserve((Tcl_Channel)chanPtr); while (bytesToRead) { /* @@ -8952,8 +9044,7 @@ DoRead( * 1) We're @EOF because we saw eof char. */ - if (statePtr->inEofChar - && RemovePoint(bufPtr)[0] == statePtr->inEofChar) { + if (GotFlag(statePtr, CHANNEL_STICKY_EOF)) { UpdateInterest(chanPtr); break; } @@ -9026,6 +9117,11 @@ DoRead( ResetFlag(statePtr, CHANNEL_BLOCKED); } + assert(!GotFlag(statePtr, CHANNEL_EOF) + || GotFlag(statePtr, CHANNEL_STICKY_EOF) + || Tcl_InputBuffered((Tcl_Channel)chanPtr) == 0); + assert( !(GotFlag(statePtr, CHANNEL_EOF|CHANNEL_BLOCKED) + == (CHANNEL_EOF|CHANNEL_BLOCKED)) ); TclChannelRelease((Tcl_Channel)chanPtr); return (int)(p - dst); } -- cgit v0.12 From 77d8108158b1e45e8dc31209eccbe00787a73fb2 Mon Sep 17 00:00:00 2001 From: dgp Date: Mon, 3 Nov 2014 21:37:37 +0000 Subject: Make sure reflected channels do not make a double call to Tcl_ReadRaw(), with the unwarranted assumption that EOF is a permanent condition. --- generic/tclIORTrans.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/generic/tclIORTrans.c b/generic/tclIORTrans.c index 45ee08d..8f3ef3c 100644 --- a/generic/tclIORTrans.c +++ b/generic/tclIORTrans.c @@ -1097,6 +1097,11 @@ ReflectInput( goto stop; } + if (rtPtr->readIsDrained) { + goto stop; + } + + /* * The buffer is exhausted, but the caller wants even more. We now * have to go to the underlying channel, get more bytes and then @@ -1166,10 +1171,6 @@ ReflectInput( * on the down channel. */ - if (rtPtr->readIsDrained) { - goto stop; - } - /* * Now this is a bit different. The partial data waiting is * converted and returned. -- cgit v0.12 From 6329a786eedb7e86de0fb843dfa3c3a1faac5986 Mon Sep 17 00:00:00 2001 From: dgp Date: Wed, 5 Nov 2014 06:36:07 +0000 Subject: [214cc0eb22] Restore [lappend $var] return value to the 8.6.1- behavior. If this is going to change, lets not do it by accident. --- generic/tclCompCmdsGR.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/generic/tclCompCmdsGR.c b/generic/tclCompCmdsGR.c index 9d258fc..98407f7 100644 --- a/generic/tclCompCmdsGR.c +++ b/generic/tclCompCmdsGR.c @@ -871,7 +871,7 @@ TclCompileLappendCmd( /* TODO: Consider support for compiling expanded args. */ numWords = parsePtr->numWords; - if (numWords == 1) { + if (numWords < 3) { return TCL_ERROR; } -- cgit v0.12 From db0b450fcc8673487056f6292838cc14ffa54c5e Mon Sep 17 00:00:00 2001 From: dgp Date: Wed, 5 Nov 2014 16:47:00 +0000 Subject: New test iortrans-4.10 to demo failure of channel transformation to handle fleeting EOF in the base channel. Falls into infinite block. Regression compared with Tcl 8.6.1. --- tests/ioTrans.test | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+) diff --git a/tests/ioTrans.test b/tests/ioTrans.test index 53078f7..f1fa733 100644 --- a/tests/ioTrans.test +++ b/tests/ioTrans.test @@ -597,6 +597,62 @@ test iortrans-4.9 {chan read, gets, bug 2921116} -setup { rename foo {} } -result {{read rt* {test data }} {}} +test iortrans-4.10 {[5adbc350683] chan read, handle fleeting EOF} -setup { + proc driver {cmd args} { + variable buffer + variable index + set chan [lindex $args 0] + switch -- $cmd { + initialize { + set index($chan) 0 + set buffer($chan) ..... + return {initialize finalize watch read} + } + finalize { + if {![info exists index($chan)]} {return} + unset index($chan) buffer($chan) + return + } + watch {} + read { + set n [lindex $args 1] + if {![info exists index($chan)]} { + driver initialize $chan + } + set new [expr {$index($chan) + $n}] + set result [string range $buffer($chan) $index($chan) $new-1] + set index($chan) $new + if {[string length $result] == 0} { + driver finalize $chan + } + return $result + } + } + } + proc idxform {cmd handle args} { + switch -- $cmd { + initialize { + return {initialize finalize read} + } + finalize { + return + } + read { + lassign $args buffer + return $buffer + } + } + } +} -body { + set chan [chan push [chan create read driver] idxform] + list [eof $chan] [read $chan] [eof $chan] [read $chan 0] [eof $chan] \ + [read $chan] [eof $chan] +} -cleanup { + close $chan + rename idxform {} + rename driver {} +} -result {0 ..... 1 {} 0 ..... 1} + # --- === *** ########################### # method write (via puts) -- cgit v0.12 From 836a10622561a68136fe41a106892b55aafb9fc3 Mon Sep 17 00:00:00 2001 From: dgp Date: Wed, 5 Nov 2014 20:34:03 +0000 Subject: Reflected Transform channel fix. Be sure each EOF on the base channel gets passed up to become an eof of the transform before continuing on to additional ReadRaw() from the base channel. This way we don't miss fleeting EOFs. --- generic/tclIORTrans.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/generic/tclIORTrans.c b/generic/tclIORTrans.c index 8f3ef3c..8baa9ad 100644 --- a/generic/tclIORTrans.c +++ b/generic/tclIORTrans.c @@ -161,6 +161,7 @@ typedef struct { int mode; /* Mask of R/W mode */ int nonblocking; /* Flag: Channel is blocking or not. */ int readIsDrained; /* Flag: Read buffers are flushed. */ + int eofPending; /* Flag: EOF seen down, but not raised up */ int dead; /* Boolean signal that some operations * should no longer be attempted. */ ResultBuffer result; @@ -1082,6 +1083,10 @@ ReflectInput( bufObj = Tcl_NewByteArrayObj(NULL, toRead); Tcl_IncrRefCount(bufObj); gotBytes = 0; + if (rtPtr->eofPending) { + goto stop; + } + rtPtr->readIsDrained = 0; while (toRead > 0) { /* * Loop until the request is satisfied (or no data available from @@ -1097,9 +1102,9 @@ ReflectInput( goto stop; } - if (rtPtr->readIsDrained) { - goto stop; - } + if (rtPtr->eofPending) { + goto stop; + } /* @@ -1170,6 +1175,8 @@ ReflectInput( * Zero returned from Tcl_ReadRaw() always indicates EOF * on the down channel. */ + + rtPtr->eofPending = 1; /* * Now this is a bit different. The partial data waiting is @@ -1212,6 +1219,9 @@ ReflectInput( } /* while toRead > 0 */ stop: + if (gotBytes == 0) { + rtPtr->eofPending = 0; + } Tcl_DecrRefCount(bufObj); Tcl_Release(rtPtr); return gotBytes; @@ -1767,6 +1777,7 @@ NewReflectedTransform( rtPtr->timer = NULL; rtPtr->mode = 0; rtPtr->readIsDrained = 0; + rtPtr->eofPending = 0; rtPtr->nonblocking = (((Channel *) parentChan)->state->flags & CHANNEL_NONBLOCKING); rtPtr->dead = 0; @@ -3319,6 +3330,7 @@ TransformClear( (void) InvokeTclMethod(rtPtr, "clear", NULL, NULL, NULL); rtPtr->readIsDrained = 0; + rtPtr->eofPending = 0; ResultClear(&rtPtr->result); } -- cgit v0.12 From 7cc4c83aed245ab7ec48a2d037c43b8b59cfdddb Mon Sep 17 00:00:00 2001 From: dgp Date: Thu, 6 Nov 2014 13:38:01 +0000 Subject: New tests iortrans-4.11* demonstrate what was wrong with the "leaky EOF flag" approach in 8.6.1 and earlier. If each level of the channel stack is to have control over its EOF independently, we have to provide for that, even though the Filesystem read APIs make it a big pain. Also test robustness against varing buffer sizes. --- tests/ioTrans.test | 89 +++++++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 85 insertions(+), 4 deletions(-) diff --git a/tests/ioTrans.test b/tests/ioTrans.test index f1fa733..faae9d8 100644 --- a/tests/ioTrans.test +++ b/tests/ioTrans.test @@ -597,7 +597,9 @@ test iortrans-4.9 {chan read, gets, bug 2921116} -setup { rename foo {} } -result {{read rt* {test data }} {}} -test iortrans-4.10 {[5adbc350683] chan read, handle fleeting EOF} -setup { + +# Driver for a base channel that emits several short "files" +# with each terminated by a fleeting EOF proc driver {cmd args} { variable buffer variable index @@ -629,6 +631,8 @@ test iortrans-4.10 {[5adbc350683] chan read, handle fleeting EOF} -setup { } } } + +# Channel read transform that is just the identity - pass all through proc idxform {cmd handle args} { switch -- $cmd { initialize { @@ -643,15 +647,92 @@ test iortrans-4.10 {[5adbc350683] chan read, handle fleeting EOF} -setup { } } } -} -body { + +# Test that all EOFs pass through full xform stack. Proper data boundaries. +# Check robustness against buffer sizes. +test iortrans-4.10 {[5adbc350683] chan read, handle fleeting EOF} -body { set chan [chan push [chan create read driver] idxform] list [eof $chan] [read $chan] [eof $chan] [read $chan 0] [eof $chan] \ [read $chan] [eof $chan] } -cleanup { close $chan - rename idxform {} - rename driver {} } -result {0 ..... 1 {} 0 ..... 1} +test iortrans-4.10.1 {[5adbc350683] chan read, handle fleeting EOF} -body { + set chan [chan push [chan create read driver] idxform] + chan configure $chan -buffersize 3 + list [eof $chan] [read $chan] [eof $chan] [read $chan 0] [eof $chan] \ + [read $chan] [eof $chan] +} -cleanup { + close $chan +} -result {0 ..... 1 {} 0 ..... 1} +test iortrans-4.10.2 {[5adbc350683] chan read, handle fleeting EOF} -body { + set chan [chan push [chan create read driver] idxform] + chan configure $chan -buffersize 5 + list [eof $chan] [read $chan] [eof $chan] [read $chan 0] [eof $chan] \ + [read $chan] [eof $chan] +} -cleanup { + close $chan +} -result {0 ..... 1 {} 0 ..... 1} + +rename idxform {} + +# Channel read transform that delays the data + proc delayxform {cmd handle args} { + variable store + switch -- $cmd { + initialize { + set store($handle) {} + return {initialize finalize read drain} + } + finalize { + unset store($handle) + return + } + read { + lassign $args buffer + if {$store($handle) eq {}} { + set reply [string index $buffer 0] + set store($handle) [string range $buffer 1 end] + } else { + set reply $store($handle) + set store($handle) $buffer + } + return $reply + } + drain { + delayxform read $handle {} + } + } + } + +# Test that all EOFs pass through full xform stack. Proper data boundaries. +# Check robustness against buffer sizes. +test iortrans-4.11 {[5adbc350683] chan read, handle fleeting EOF} -body { + set chan [chan push [chan create read driver] delayxform] + list [eof $chan] [read $chan] [eof $chan] [read $chan 0] [eof $chan] \ + [read $chan] [eof $chan] +} -cleanup { + close $chan +} -result {0 ..... 1 {} 0 ..... 1} +test iortrans-4.11.1 {[5adbc350683] chan read, handle fleeting EOF} -body { + set chan [chan push [chan create read driver] delayxform] + chan configure $chan -buffersize 3 + list [eof $chan] [read $chan] [eof $chan] [read $chan 0] [eof $chan] \ + [read $chan] [eof $chan] +} -cleanup { + close $chan +} -result {0 ..... 1 {} 0 ..... 1} +test iortrans-4.11.2 {[5adbc350683] chan read, handle fleeting EOF} -body { + set chan [chan push [chan create read driver] delayxform] + chan configure $chan -buffersize 5 + list [eof $chan] [read $chan] [eof $chan] [read $chan 0] [eof $chan] \ + [read $chan] [eof $chan] +} -cleanup { + close $chan +} -result {0 ..... 1 {} 0 ..... 1} + + rename delayxform {} + rename driver {} # --- === *** ########################### -- cgit v0.12 From 06fa92e9a424d57ed4c9458474f5f8a7b42cc654 Mon Sep 17 00:00:00 2001 From: dgp Date: Thu, 6 Nov 2014 14:52:36 +0000 Subject: Another test checking that handling when transform returns nothing is right. --- tests/ioTrans.test | 36 +++++++++++++++++++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-) diff --git a/tests/ioTrans.test b/tests/ioTrans.test index faae9d8..aa2fbc7 100644 --- a/tests/ioTrans.test +++ b/tests/ioTrans.test @@ -676,7 +676,7 @@ test iortrans-4.10.2 {[5adbc350683] chan read, handle fleeting EOF} -body { rename idxform {} -# Channel read transform that delays the data +# Channel read transform that delays the data and always returns something proc delayxform {cmd handle args} { variable store switch -- $cmd { @@ -732,6 +732,40 @@ test iortrans-4.11.2 {[5adbc350683] chan read, handle fleeting EOF} -body { } -result {0 ..... 1 {} 0 ..... 1} rename delayxform {} + +# Channel read transform that delays the data and may return {} + proc delay2xform {cmd handle args} { + variable store + switch -- $cmd { + initialize { + set store($handle) {} + return {initialize finalize read drain} + } + finalize { + unset store($handle) + return + } + read { + lassign $args buffer + set reply $store($handle) + set store($handle) $buffer + return $reply + } + drain { + delay2xform read $handle {} + } + } + } + +test iortrans-4.12 {[5adbc350683] chan read, handle fleeting EOF} -body { + set chan [chan push [chan create read driver] delay2xform] + list [eof $chan] [read $chan] [eof $chan] [read $chan 0] [eof $chan] \ + [read $chan] [eof $chan] +} -cleanup { + close $chan +} -result {0 ..... 1 {} 0 ..... 1} + + rename delay2xform {} rename driver {} -- cgit v0.12 From a8c6a73058b823dbb1ea5e3832d016284820400e Mon Sep 17 00:00:00 2001 From: dgp Date: Thu, 6 Nov 2014 15:06:52 +0000 Subject: New test iogt-7.0 demos bug in [testchannel transform]. --- tests/iogt.test | 42 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/tests/iogt.test b/tests/iogt.test index 5fe3dc2..1a18afc 100644 --- a/tests/iogt.test +++ b/tests/iogt.test @@ -996,6 +996,48 @@ test iogt-6.1 {Push back and up} {testchannel knownBug} { set res } {xxxghi} +# Driver for a base channel that emits several short "files" +# with each terminated by a fleeting EOF + proc driver {cmd args} { + variable buffer + variable index + set chan [lindex $args 0] + switch -- $cmd { + initialize { + set index($chan) 0 + set buffer($chan) ..... + return {initialize finalize watch read} + } + finalize { + if {![info exists index($chan)]} {return} + unset index($chan) buffer($chan) + return + } + watch {} + read { + set n [lindex $args 1] + if {![info exists index($chan)]} { + driver initialize $chan + } + set new [expr {$index($chan) + $n}] + set result [string range $buffer($chan) $index($chan) $new-1] + set index($chan) $new + if {[string length $result] == 0} { + driver finalize $chan + } + return $result + } + } + } + +test iogt-7.0 {Handle fleeting EOF} -constraints {testchannel} -body { + set chan [chan create read [namespace which driver]] + identity -attach $chan + list [eof $chan] [read $chan] [eof $chan] [read $chan 0] [eof $chan] \ + [read $chan] [eof $chan] +} -cleanup { + close $chan +} -result {0 ..... 1 {} 0 ..... 1} # cleanup foreach file [list dummy dummyout __echo_srv__.tcl] { -- cgit v0.12 From 4ec8e3c11c2a7fdc1fd9efce21a8e6d92ce8a0e5 Mon Sep 17 00:00:00 2001 From: dgp Date: Thu, 6 Nov 2014 15:49:30 +0000 Subject: fix failing test --- generic/tclIOGT.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/generic/tclIOGT.c b/generic/tclIOGT.c index a78a5b4..7ba2f2a 100644 --- a/generic/tclIOGT.c +++ b/generic/tclIOGT.c @@ -187,6 +187,7 @@ struct TransformChannelData { Tcl_Channel self; /* Our own Channel handle. */ int readIsFlushed; /* Flag to note whether in.flushProc was * called or not. */ + int eofPending; /* Flag: EOF seen down, not raised up */ int flags; /* Currently CHANNEL_ASYNC or zero. */ int watchMask; /* Current watch/event/interest mask. */ int mode; /* Mode of parent channel, OR'ed combination @@ -292,6 +293,7 @@ TclChannelTransform( Tcl_DStringInit(&ds); Tcl_GetChannelOption(interp, chan, "-blocking", &ds); dataPtr->readIsFlushed = 0; + dataPtr->eofPending = 0; dataPtr->flags = 0; if (ds.string[0] == '0') { dataPtr->flags |= CHANNEL_ASYNC; @@ -624,7 +626,7 @@ TransformInputProc( if (toRead == 0 || dataPtr->self == NULL) { /* - * Catch a no-op. + * Catch a no-op. TODO: Is this a panic()? */ return 0; } @@ -676,8 +678,7 @@ TransformInputProc( if (toRead <= 0) { break; } - - if (dataPtr->readIsFlushed) { + if (dataPtr->eofPending) { /* * Already saw EOF from downChan; don't ask again. * NOTE: Could move this up to avoid the last maxRead @@ -724,6 +725,7 @@ TransformInputProc( * on the down channel. */ + dataPtr->eofPending = 1; dataPtr->readIsFlushed = 1; ExecuteCallback(dataPtr, NULL, A_FLUSH_READ, NULL, 0, TRANSMIT_IBUF, P_PRESERVE); @@ -751,8 +753,11 @@ TransformInputProc( break; } } /* while toRead > 0 */ - ReleaseData(dataPtr); + if (gotBytes == 0) { + dataPtr->eofPending = 0; + } + ReleaseData(dataPtr); return gotBytes; } @@ -863,6 +868,7 @@ TransformSeekProc( P_NO_PRESERVE); ResultClear(&dataPtr->result); dataPtr->readIsFlushed = 0; + dataPtr->eofPending = 0; } ReleaseData(dataPtr); @@ -936,6 +942,7 @@ TransformWideSeekProc( P_NO_PRESERVE); ResultClear(&dataPtr->result); dataPtr->readIsFlushed = 0; + dataPtr->eofPending = 0; } ReleaseData(dataPtr); -- cgit v0.12 From 4cb80cd4e64044f5891b073788734efd753e5100 Mon Sep 17 00:00:00 2001 From: dgp Date: Thu, 6 Nov 2014 16:12:26 +0000 Subject: Also test transfroms that delay. --- tests/iogt.test | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/tests/iogt.test b/tests/iogt.test index 1a18afc..89e62d4 100644 --- a/tests/iogt.test +++ b/tests/iogt.test @@ -1039,6 +1039,36 @@ test iogt-7.0 {Handle fleeting EOF} -constraints {testchannel} -body { close $chan } -result {0 ..... 1 {} 0 ..... 1} +proc delay {op data} { + variable store + switch -- $op { + create/write - create/read - + delete/write - delete/read - + flush/write - write - + clear_read {;#ignore} + flush/read - + read { + if {![info exists store]} {set store {}} + set reply $store + set store $data + return $reply + } + query/maxRead {return -1} + } +} + +test iogt-7.1 {Handle fleeting EOF} -constraints {testchannel} -body { + set chan [chan create read [namespace which driver]] + testchannel transform $chan -command [namespace code delay] + list [eof $chan] [read $chan] [eof $chan] [read $chan 0] [eof $chan] \ + [read $chan] [eof $chan] +} -cleanup { + close $chan +} -result {0 ..... 1 {} 0 ..... 1} + +rename delay {} +rename driver {} + # cleanup foreach file [list dummy dummyout __echo_srv__.tcl] { removeFile $file -- cgit v0.12 From f033a745db5e733d99ebc0a7895320435de1bd82 Mon Sep 17 00:00:00 2001 From: dgp Date: Thu, 6 Nov 2014 17:34:40 +0000 Subject: cleanup global namespace litter --- tests/ioTrans.test | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/ioTrans.test b/tests/ioTrans.test index aa2fbc7..f82c610 100644 --- a/tests/ioTrans.test +++ b/tests/ioTrans.test @@ -613,6 +613,8 @@ test iortrans-4.9 {chan read, gets, bug 2921116} -setup { finalize { if {![info exists index($chan)]} {return} unset index($chan) buffer($chan) + array unset index + array unset buffer return } watch {} -- cgit v0.12