From 32fea5e38c09de7b9f9f19c93074ccdb8a6520d7 Mon Sep 17 00:00:00 2001 From: dgp Date: Thu, 20 Mar 2014 23:21:48 +0000 Subject: Both callers of ChanRead() have simlar epilogs. Shift that into ChanRead and refactor. --- generic/tclIO.c | 123 +++++++++++++++++++++++--------------------------------- 1 file changed, 50 insertions(+), 73 deletions(-) diff --git a/generic/tclIO.c b/generic/tclIO.c index e7653f6..18dab5a 100644 --- a/generic/tclIO.c +++ b/generic/tclIO.c @@ -348,15 +348,39 @@ static inline int ChanRead( Channel *chanPtr, char *dst, - int dstSize, - int *errnoPtr) + int dstSize) { + int bytesRead, result; + if (WillRead(chanPtr) < 0) { return -1; } - return chanPtr->typePtr->inputProc(chanPtr->instanceData, dst, dstSize, - errnoPtr); + bytesRead = chanPtr->typePtr->inputProc(chanPtr->instanceData, + dst, dstSize, &result); + + 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 { /* bytesRead < 0 */ + if ((result == EWOULDBLOCK) || (result == EAGAIN)) { + SetFlag(chanPtr->state, CHANNEL_BLOCKED); + result = EAGAIN; + } + Tcl_SetErrno(result); + } + return bytesRead; } static inline Tcl_WideInt @@ -4833,7 +4857,7 @@ Tcl_ReadRaw( Channel *chanPtr = (Channel *) chan; ChannelState *statePtr = chanPtr->state; /* State info for channel */ - int nread, result, copied, copiedNow; + int nread, copied, copiedNow; /* * The check below does too much because it will reject a call to this @@ -4862,11 +4886,11 @@ Tcl_ReadRaw( bytesToRead - copied); if (copiedNow == 0) { if (statePtr->flags & CHANNEL_EOF) { - goto done; + break; } if (statePtr->flags & CHANNEL_BLOCKED) { if (statePtr->flags & CHANNEL_NONBLOCKING) { - goto done; + break; } ResetFlag(statePtr, CHANNEL_BLOCKED); } @@ -4881,48 +4905,21 @@ Tcl_ReadRaw( * 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. - */ - - return copied; - } + nread = ChanRead(chanPtr, bufPtr+copied, bytesToRead-copied); - SetFlag(statePtr, CHANNEL_BLOCKED); - result = EAGAIN; + if (nread < 0) { + if (statePtr->flags & CHANNEL_BLOCKED && copied > 0) { + ResetFlag(statePtr, CHANNEL_BLOCKED); + break; } - - Tcl_SetErrno(result); return -1; } - return copied + nread; + copied += nread; + break; } } - done: return copied; } @@ -5019,7 +5016,7 @@ DoReadChars( ChannelState *statePtr = chanPtr->state; /* State info for channel */ ChannelBuffer *bufPtr; - int factor, copied, copiedNow, result; + int factor, copied, copiedNow; Tcl_Encoding encoding; int binaryMode; #define UTF_EXPANSION_FACTOR 1024 @@ -5090,9 +5087,8 @@ DoReadChars( } ResetFlag(statePtr, CHANNEL_BLOCKED); } - result = GetInput(chanPtr); - if (result != 0) { - if (result == EAGAIN) { + if (GetInput(chanPtr) != 0) { + if (statePtr->flags & CHANNEL_BLOCKED) { break; } copied = -1; @@ -5883,8 +5879,9 @@ DiscardInputQueued( * that is the top channel of a stack! * * Results: - * The return value is the Posix error code if an error occurred while - * reading from the file, or 0 otherwise. + * The return value is 0 to indicate success, or -1 if an error + * occurred while reading from the channel. Call Tcl_GetErrno() + * to get the Posix error code. * * Side effects: * Reads from the underlying device. @@ -5897,7 +5894,6 @@ GetInput( Channel *chanPtr) /* Channel to read input from. */ { int toRead; /* How much to read? */ - int result; /* Of calling driver. */ int nread; /* How much was read from channel? */ ChannelBuffer *bufPtr; /* New buffer to add to input queue. */ ChannelState *statePtr = chanPtr->state; @@ -5911,7 +5907,8 @@ GetInput( */ if (CheckForDeadChannel(NULL, statePtr)) { - return EINVAL; + Tcl_SetErrno(EINVAL); + return -1; } /* @@ -5988,31 +5985,11 @@ GetInput( return 0; } - nread = ChanRead(chanPtr, InsertPoint(bufPtr), toRead, &result); - if (nread > 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. - */ - - if (nread < toRead) { - 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)) { - SetFlag(statePtr, CHANNEL_BLOCKED); - result = EAGAIN; - } - Tcl_SetErrno(result); - return result; + nread = ChanRead(chanPtr, InsertPoint(bufPtr), toRead); + if (nread < 0) { + return -1; } + bufPtr->nextAdded += nread; return 0; } -- cgit v0.12 From f27a277c1e1c06a9ddd1c93606a9d884c8d844d7 Mon Sep 17 00:00:00 2001 From: dgp Date: Fri, 21 Mar 2014 02:36:47 +0000 Subject: Documentation header for ChanRead() --- generic/tclIO.c | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/generic/tclIO.c b/generic/tclIO.c index 18dab5a..6e5dc05 100644 --- a/generic/tclIO.c +++ b/generic/tclIO.c @@ -342,9 +342,30 @@ 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 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, or + * - -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, -- cgit v0.12 From e21cda73652e2cf4a060a0411e779935b427a154 Mon Sep 17 00:00:00 2001 From: dgp Date: Fri, 21 Mar 2014 12:56:46 +0000 Subject: io-34.21 - fix bugs in normally skipped test. io-35.18b - knownBug is not buggy on this branch. --- tests/io.test | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/io.test b/tests/io.test index cedb337..7f1a357 100644 --- a/tests/io.test +++ b/tests/io.test @@ -41,7 +41,7 @@ testConstraint testthread [llength [info commands testthread]] # You need a *very* special environment to do some tests. In # particular, many file systems do not support large-files... -testConstraint largefileSupport 0 +testConstraint largefileSupport 1 # some tests can only be run is umask is 2 # if "umask" cannot be run, the tests will be skipped. @@ -4433,10 +4433,10 @@ test io-34.21 {Tcl_Seek and Tcl_Tell on large files} {largefileSupport} { puts -nonewline $f abcdef lappend l [tell $f] close $f - lappend l [file size $f] + lappend l [file size $path(test3)] # truncate... close [open $path(test3) w] - lappend l [file size $f] + lappend l [file size $path(test3)] set l } {0 6 6 4294967296 4294967302 4294967302 0} @@ -4729,7 +4729,7 @@ test io-35.18a {Tcl_Eof, eof char, cr write, crlf read} -body { close $f list $s $l $e [scan [string index $in end] %c] } -result {9 8 1 13} -test io-35.18b {Tcl_Eof, eof char, cr write, crlf read} -constraints knownBug -body { +test io-35.18b {Tcl_Eof, eof char, cr write, crlf read} -body { file delete $path(test1) set f [open $path(test1) w] fconfigure $f -translation cr -eofchar \x1a -- cgit v0.12 From 657b902a0e5afa5596833584329b653f7c0f277d Mon Sep 17 00:00:00 2001 From: dgp Date: Fri, 21 Mar 2014 12:57:58 +0000 Subject: Fixup ChanRead() header. Note (dstSize > 0) precondition. --- generic/tclIO.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/generic/tclIO.c b/generic/tclIO.c index 6e5dc05..e7eaefa 100644 --- a/generic/tclIO.c +++ b/generic/tclIO.c @@ -346,8 +346,8 @@ static Tcl_ObjType chanObjType = { * * ChanRead -- * - * Read up to bytes using the inputProc of chanPtr, store them at dst, - * and return the number of bytes stored. + * 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, @@ -373,6 +373,12 @@ ChanRead( { 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); + if (WillRead(chanPtr) < 0) { return -1; } -- cgit v0.12 From 5bd5d7b876314f563f1ac021424b7f79f45196dd Mon Sep 17 00:00:00 2001 From: dgp Date: Fri, 21 Mar 2014 12:58:29 +0000 Subject: Restore default suppression of large file test. --- tests/io.test | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/io.test b/tests/io.test index 7f1a357..bfe6051 100644 --- a/tests/io.test +++ b/tests/io.test @@ -41,7 +41,7 @@ testConstraint testthread [llength [info commands testthread]] # You need a *very* special environment to do some tests. In # particular, many file systems do not support large-files... -testConstraint largefileSupport 1 +testConstraint largefileSupport 0 # some tests can only be run is umask is 2 # if "umask" cannot be run, the tests will be skipped. -- cgit v0.12 From 6b106c4d93bfc4a452a3cf9d78aa3ee25761e553 Mon Sep 17 00:00:00 2001 From: dgp Date: Fri, 21 Mar 2014 13:20:06 +0000 Subject: Convert "impossible" test to a "knownBug" test. Exposes a segfault! --- tests/ioCmd.test | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/tests/ioCmd.test b/tests/ioCmd.test index 768a748..8f0bfbf 100644 --- a/tests/ioCmd.test +++ b/tests/ioCmd.test @@ -1927,6 +1927,8 @@ test iocmd-32.0 {origin interpreter of moved channel gone} -match glob -body { test iocmd-32.1 {origin interpreter of moved channel destroyed during access} -match glob -body { + # This test segfaults; Ought to fix that. + set ida [interp create];#puts <<$ida>> set idb [interp create];#puts <<$idb>> @@ -1940,13 +1942,12 @@ test iocmd-32.1 {origin interpreter of moved channel destroyed during access} -m proc foo {args} { oninit; onfinal; track; # destroy interpreter during channel access - # Actually not possible for an interp to destroy itself. - interp delete {} - return} + suicide} set chan [chan create {r w} foo] fconfigure $chan -buffering none set chan }] + interp alias $ida suicide {} interp delete $ida # Move channel to 2nd thread. interp eval $ida [list testchannel cut $chan] @@ -1965,7 +1966,7 @@ test iocmd-32.1 {origin interpreter of moved channel destroyed during access} -m set res }] set res -} -constraints {testchannel impossible} \ +} -constraints {testchannel knownBug} \ -result {Owner lost} test iocmd-32.2 {delete interp of reflected chan} { -- cgit v0.12 From f715783b1bff0fe44a894ff26049debf8cb184f2 Mon Sep 17 00:00:00 2001 From: dgp Date: Fri, 21 Mar 2014 13:40:58 +0000 Subject: Added comment explaining the "knownBug" --- tests/iogt.test | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tests/iogt.test b/tests/iogt.test index 3882ecc..3b94747 100644 --- a/tests/iogt.test +++ b/tests/iogt.test @@ -916,6 +916,15 @@ test iogt-6.0 {Push back} testchannel { } {xxx} test iogt-6.1 {Push back and up} {testchannel knownBug} { + + # This test demonstrates the bug/misfeature in the stacked + # channel implementation that data can be discarded if it is + # read into the buffers of one channel in the stack, and then + # that channel is popped before anything above it reads. + # + # This bug can be worked around by always setting -buffersize + # to 1, but who wants to do that? + set f [open $path(dummy) r] # contents of dummy = "abcdefghi..." -- cgit v0.12 From 129e8e28a57eb14bc1a2c5e06dd60f90124ab2bf Mon Sep 17 00:00:00 2001 From: dgp Date: Fri, 21 Mar 2014 14:07:40 +0000 Subject: Correct namespace bugs in normally skipped tests. Constrain them as "knownBug" rather than "unknownFailure". --- tests/iogt.test | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/tests/iogt.test b/tests/iogt.test index 3b94747..0a25418 100644 --- a/tests/iogt.test +++ b/tests/iogt.test @@ -159,8 +159,8 @@ proc fevent {fdelay idelay blocks script data} { #puts stdout ">>>>>" ; flush stdout - uplevel #0 set sock $sk - set res [uplevel #0 $script] + uplevel 1 set sock $sk + set res [uplevel 1 $script] catch {close $sk} return $res @@ -634,7 +634,7 @@ delete/write {} *ignored*} test iogt-3.0 {Tcl_Channel valid after stack/unstack, fevent handling} \ - {testchannel unknownFailure} { + {testchannel knownBug} { # This test to check the validity of aquired Tcl_Channel references is # not possible because even a backgrounded fcopy will immediately start # to copy data, without waiting for the event loop. This is done only in @@ -651,6 +651,7 @@ test iogt-3.0 {Tcl_Channel valid after stack/unstack, fevent handling} \ set fin [open $path(dummy) r] fevent 1000 500 {20 20 20 10 1 1} { + variable copy close $fin set fout [open dummyout w] @@ -688,7 +689,7 @@ test iogt-3.0 {Tcl_Channel valid after stack/unstack, fevent handling} \ } {1 {create/write create/read write flush/write flush/read delete/write delete/read}} -test iogt-4.0 {fileevent readable, after transform} {testchannel unknownFailure} { +test iogt-4.0 {fileevent readable, after transform} {testchannel knownBug} { set fin [open $path(dummy) r] set data [read $fin] close $fin @@ -718,10 +719,11 @@ test iogt-4.0 {fileevent readable, after transform} {testchannel unknownFailure} } fevent 1000 500 {20 20 20 10 1} { + variable stop audit_flow trail -attach $sock rblocks_t rbuf trail 23 -attach $sock - fileevent $sock readable [list Get $sock] + fileevent $sock readable [namespace code [list Get $sock]] flush $sock ; # now, or fcopy will error us out # But the 1 second delay should be enough to @@ -819,7 +821,7 @@ delete/write {} *ignored* delete/read {} *ignored*} ; # catch unescaped quote " -test iogt-5.0 {EOF simulation} {testchannel unknownFailure} { +test iogt-5.0 {EOF simulation} {testchannel knownBug} { set fin [open $path(dummy) r] set fout [open $path(dummyout) w] -- cgit v0.12 From 2266861ddbdc18e12918eb94efeefc37edc894f8 Mon Sep 17 00:00:00 2001 From: dgp Date: Fri, 21 Mar 2014 14:45:00 +0000 Subject: missing declaration --- generic/tclIO.c | 1 + 1 file changed, 1 insertion(+) diff --git a/generic/tclIO.c b/generic/tclIO.c index e7eaefa..a62f80e 100644 --- a/generic/tclIO.c +++ b/generic/tclIO.c @@ -165,6 +165,7 @@ typedef struct CloseCallback { static ChannelBuffer * AllocChannelBuffer(int length); 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, -- cgit v0.12 From c3df58587ce6e9f21b652b23eb7f56f852a326f7 Mon Sep 17 00:00:00 2001 From: dgp Date: Tue, 13 May 2014 18:24:23 +0000 Subject: Rework Tcl_ReadRaw() mostly taking things out of the loop that never repeat. --- generic/tclIO.c | 56 ++++++++++++++++++++------------------------------------ 1 file changed, 20 insertions(+), 36 deletions(-) diff --git a/generic/tclIO.c b/generic/tclIO.c index 41f555b..a82c36b 100644 --- a/generic/tclIO.c +++ b/generic/tclIO.c @@ -4985,7 +4985,7 @@ Tcl_ReadRaw( Channel *chanPtr = (Channel *) chan; ChannelState *statePtr = chanPtr->state; /* State info for channel */ - int nread, copied, copiedNow; + int nread, copied, copiedNow = INT_MAX; /* * The check below does too much because it will reject a call to this @@ -5010,44 +5010,28 @@ 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)) { - break; - } - if (GotFlag(statePtr, CHANNEL_BLOCKED)) { - if (GotFlag(statePtr, CHANNEL_NONBLOCKING)) { - break; - } - 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. - */ + for (copied = 0; bytesToRead > 0 && copiedNow > 0; + bufPtr+=copiedNow, bytesToRead-=copiedNow, copied+=copiedNow) { + copiedNow = CopyBuffer(chanPtr, bufPtr, bytesToRead); + } - nread = ChanRead(chanPtr, bufPtr + copied, - bytesToRead - copied); + 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. + */ - if (nread < 0) { - if (GotFlag(statePtr, CHANNEL_BLOCKED) && copied > 0) { -/* TODO: comment out? */ -// ResetFlag(statePtr, CHANNEL_BLOCKED); - } else { - copied = -1; - } - } else { - copied += nread; + nread = ChanRead(chanPtr, bufPtr, bytesToRead); + if (nread < 0) { + if (!GotFlag(statePtr, CHANNEL_BLOCKED) || copied == 0) { + copied = -1; } - break; + } else { + copied += nread; + } + if (copied != 0) { + ResetFlag(statePtr, CHANNEL_EOF); } } -- cgit v0.12