Patchwork [libfortran] PR 47694 Reads from named pipe fails

login
register
mail settings
Submitter Janne Blomqvist
Date Feb. 22, 2011, 7:40 p.m.
Message ID <AANLkTi=fpjK9u-dHj-u0NwiY6Rf=O3nj_SrqsaR90pzA@mail.gmail.com>
Download mbox | patch
Permalink /patch/83993/
State New
Headers show

Comments

Janne Blomqvist - Feb. 22, 2011, 7:40 p.m.
Hi,

The testcase in PR 47694 shows in issue where gfortran fails to read
from a named pipe. The reason is that libgfortran didn't make sure to
consume all the buffered data before trying to read more. The attached
patch, based on an idea by yours truly and refined by Jerry DeLisle
and me, fixes this by scanning through the buffer one character at a
time; the buffer still ensures that we're doing some amount of
buffering, reducing syscall overhead.

Regtested on x86_64-unknown-linux-gnu, and the pipe testcase in the PR
verified as working properly (AFAICS), Ok for trunk?

TODO: I believe that io/read.c(read_x) should be fixed in the same way
to use fbuf_getc rather than fbuf_read. But lets first see if this
approach works in read_sf and doesn't introduce new regressions..

2011-02-22  Janne Blomqvist  <jb@gcc.gnu.org>
	    Jerry DeLisle    <jvdelisle@gcc.gnu.org>

	PR libfortran/47694
	* io/fbuf.h (fbuf_getptr): New inline function.
	* io/transfer.c (read_sf): Use fbuf_getptr and fbuf_getc to scan
	through the string instead of fbuf_read.
Jerry DeLisle - Feb. 23, 2011, 4:32 a.m.
On 02/22/2011 11:40 AM, Janne Blomqvist wrote:
> Hi,
>
> The testcase in PR 47694 shows in issue where gfortran fails to read
> from a named pipe. The reason is that libgfortran didn't make sure to
> consume all the buffered data before trying to read more. The attached
> patch, based on an idea by yours truly and refined by Jerry DeLisle
> and me, fixes this by scanning through the buffer one character at a
> time; the buffer still ensures that we're doing some amount of
> buffering, reducing syscall overhead.
>
> Regtested on x86_64-unknown-linux-gnu, and the pipe testcase in the PR
> verified as working properly (AFAICS), Ok for trunk?
>

OK!  I see where I was stuck. I was not taking care of the EOF.  Thanks for 
finishing this!

Jerry

Patch

diff --git a/libgfortran/io/fbuf.h b/libgfortran/io/fbuf.h
index c82d01b..3a2883b 100644
--- a/libgfortran/io/fbuf.h
+++ b/libgfortran/io/fbuf.h
@@ -78,4 +78,10 @@  fbuf_getc (gfc_unit * u)
   return fbuf_getc_refill (u);
 }
 
+static inline char *
+fbuf_getptr (gfc_unit * u)
+{
+  return (char*) (u->fbuf->buf + u->fbuf->pos);
+}
+
 #endif
diff --git a/libgfortran/io/transfer.c b/libgfortran/io/transfer.c
index 8fffe0e..ad5d19d 100644
--- a/libgfortran/io/transfer.c
+++ b/libgfortran/io/transfer.c
@@ -284,7 +284,8 @@  static char *
 read_sf (st_parameter_dt *dtp, int * length)
 {
   static char *empty_string[0];
-  char *base, *p, q;
+  char *base;
+  int q, q2;
   int n, lorig, seen_comma;
 
   /* If we have seen an eor previously, return a length of 0.  The
@@ -301,18 +302,18 @@  read_sf (st_parameter_dt *dtp, int * length)
 
   /* Read data into format buffer and scan through it.  */
   lorig = *length;
-  base = p = fbuf_read (dtp->u.p.current_unit, length);
+  base = fbuf_getptr (dtp->u.p.current_unit);
   if (base == NULL)
     return NULL;
 
   while (n < *length)
     {
-      q = *p;
-
-      if (q == '\n' || q == '\r')
+      q = fbuf_getc (dtp->u.p.current_unit);
+      if (q == EOF)
+	break;
+      else if (q == '\n' || q == '\r')
 	{
 	  /* Unexpected end of line. Set the position.  */
-	  fbuf_seek (dtp->u.p.current_unit, n + 1 ,SEEK_CUR);
 	  dtp->u.p.sf_seen_eor = 1;
 
 	  /* If we see an EOR during non-advancing I/O, we need to skip
@@ -323,15 +324,12 @@  read_sf (st_parameter_dt *dtp, int * length)
 	  /* If we encounter a CR, it might be a CRLF.  */
 	  if (q == '\r') /* Probably a CRLF */
 	    {
-	      /* See if there is an LF. Use fbuf_read rather then fbuf_getc so
-		 the position is not advanced unless it really is an LF.  */
-	      int readlen = 1;
-	      p = fbuf_read (dtp->u.p.current_unit, &readlen);
-	      if (*p == '\n' && readlen == 1)
-	        {
-		  dtp->u.p.sf_seen_eor = 2;
-		  fbuf_seek (dtp->u.p.current_unit, 1 ,SEEK_CUR);
-		}
+	      /* See if there is an LF.  */
+	      q2 = fbuf_getc (dtp->u.p.current_unit);
+	      if (q2 == '\n')
+		dtp->u.p.sf_seen_eor = 2;
+	      else if (q2 != EOF) /* Oops, seek back.  */
+		fbuf_seek (dtp->u.p.current_unit, -1, SEEK_CUR);
 	    }
 
 	  /* Without padding, terminate the I/O statement without assigning
@@ -349,20 +347,18 @@  read_sf (st_parameter_dt *dtp, int * length)
       /*  Short circuit the read if a comma is found during numeric input.
 	  The flag is set to zero during character reads so that commas in
 	  strings are not ignored  */
-      if (q == ',')
+      else if (q == ',')
 	if (dtp->u.p.sf_read_comma == 1)
 	  {
             seen_comma = 1;
 	    notify_std (&dtp->common, GFC_STD_GNU,
 			"Comma in formatted numeric read.");
-	    *length = n;
 	    break;
 	  }
       n++;
-      p++;
-    } 
+    }
 
-  fbuf_seek (dtp->u.p.current_unit, n + seen_comma, SEEK_CUR);
+  *length = n;
 
   /* A short read implies we hit EOF, unless we hit EOR, a comma, or
      some other stuff. Set the relevant flags.  */