Mailing-List: contact cygwin-developers-help AT cygwin DOT com; run by ezmlm
List-Subscribe: <mailto:cygwin-developers-subscribe AT cygwin DOT com>
List-Archive: <http://sources.redhat.com/ml/cygwin-developers/>
List-Post: <mailto:cygwin-developers AT cygwin DOT com>
List-Help: <mailto:cygwin-developers-help AT cygwin DOT com>, <http://sources.redhat.com/ml/#faqs>
Sender: cygwin-developers-owner AT cygwin DOT com
Delivered-To: mailing list cygwin-developers AT cygwin DOT com
Date: Sat, 14 Dec 2002 17:55:44 +0100
From: Corinna Vinschen <vinschen AT redhat DOT com>
To: cygwin-developers AT cygwin DOT com
Subject: Re: Changed fhandler_* read and raw_read methods throughout
Message-ID: <20021214175544.N19104@cygbert.vinschen.de>
Reply-To: cygwin-developers AT cygwin DOT com
Mail-Followup-To: cygwin-developers AT cygwin DOT com
References: <20021214040532 DOT GA3368 AT redhat DOT com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20021214040532.GA3368@redhat.com>
User-Agent: Mutt/1.3.22.1i

Hi Chris,

On Fri, Dec 13, 2002 at 11:05:32PM -0500, Chris Faylor wrote:
> To accommodate my recent pipe changes, I've changed all of the
> fhandler_* read and raw_read methods.  I've changed them to void
> functions whose second parameter is both the length and the return
> value.
> 
> I've done this so that if the ReadFile in raw_read succeeds but a signal
> happens shortly thereafter, the number of bytes read will be available
> to the caller even though the thread which did the read has been
> terminated.
> 
> It's possible that I got one of the many fhandler functions wrong when I
> went through making this change so if you see odd behavior this is the
> place to check.

your changes disable tcsh's ability to read script files correctly. 

AFAICS, you overoptimized the part looking for CRLF in
fhandler_base::read() by not writing the last character if it happens
not to be a \r.  This can only be observed when reading in O_TEXT mode.

Another point is the "else if (copied_chars <= 0)" stuff after the first
raw_read call.  As I understand that part, it actually should return
here if raw_read returned 0 or -1 and copied_chars is 0 as well.  Your
changes in the lines above don't copy the correct count of bytes read
back into copied_chars as the original code did so at this point, it
should now ask for the value of 'len' shouldn't it?  I attached a patch
for review.

Corinna

	* fhandler.cc (fhandler_base::read): Return immediately if
	if no bytes read.  Formally revert changes for CRLF handling.

Index: fhandler.cc
===================================================================
RCS file: /cvs/src/src/winsup/cygwin/fhandler.cc,v
retrieving revision 1.140
diff -u -p -r1.140 fhandler.cc
--- fhandler.cc	14 Dec 2002 04:01:32 -0000	1.140
+++ fhandler.cc	14 Dec 2002 16:53:33 -0000
@@ -497,7 +497,6 @@ done:
 void
 fhandler_base::read (void *in_ptr, size_t& len)
 {
-  size_t in_len = len;
   char *ptr = (char *) in_ptr;
   ssize_t copied_chars = 0;
   int c;
@@ -527,7 +526,7 @@ fhandler_base::read (void *in_ptr, size_
       else
 	len = copied_chars;
     }
-  else if (copied_chars <= 0)
+  if (len <= 0)
     {
       len = (size_t) copied_chars;
       return;
@@ -570,25 +569,26 @@ fhandler_base::read (void *in_ptr, size_
       *dst++ = *src++;
     }
 
-  len = dst - (char *) ptr;
-
+  c = *src;
   /* if last char is a '\r' then read one more to see if we should
      translate this one too */
-  if (len < in_len && *src == '\r')
+  if (c == '\r')
     {
       size_t clen = 1;
       raw_read (&c, clen);
       if (clen <= 0)
 	/* nothing */;
-      else if (c != '\n')
-	set_readahead_valid (1, c);
+      else if (c == '\n')
+	c = '\n';
       else
-	{
-	  *dst++ = '\n';
-	  len++;
+        {
+	  set_readahead_valid (1, c);
+	  c = '\r';
 	}
     }
 
+  *dst++ = c;
+  len = dst - (char *) ptr;
 
 #ifndef NOSTRACE
   if (strace.active)