diff mbox

[libfortran,1/3] Simplify handling of special files

Message ID CAO9iq9FxO=NvX=WC6PznUscgoZBnhsZy8sjLzeOzR_-o62E_dQ@mail.gmail.com
State New
Headers show

Commit Message

Janne Blomqvist Oct. 18, 2011, 2:42 p.m. UTC 
Hi,

in a few places in libgfortran we have some code for handling special
and/or non-seekable files differently. The problem is that special
files don't all have some nice consistent behavior. E.g. wrt. seeking,
some allow seeking just fine, others allow some seeks and not others,
others allow them but always return an offset of 0, and yet others
fail the seek completely.

The Fortran standard doesn't really help here except for noting that
some files may not be positionable, and thus statements requiring the
file position to be modified may fail on such files.

Obviously, libgfortran itself cannot enumerate all the possible
variations for how a special file may behave, and trying to impose
some kind of least common denominator may hide essential capability.
Having thought about this, my conclusion is that the only thing that
makes sense is that we do what the caller asks us to do, and if that
fails, we report the error back to the caller and let the caller
handle it. The attached patch implements this.

Regtested on x86_64-unknown-linux-gnu, Ok for trunk?

2011-10-18  Janne Blomqvist  <jb@gcc.gnu.org>

	* io/file_pos.c (st_rewind): Handle regular and special files
	identically.
	* io/intrinsics.c (fseek_sub): Don't check whether we think the
	file is seekable, just do what the caller says.
	* io/transfer.c (skip_record): First try to seek, then fallback to
	reading and throwing away what we read.
	* io/unit.c (update_position): Don't check whether file is
	seekable, just try to do what we're told.
	(unit_truncate): Likewise.
	* io/unix.c (struct unix_stream): Remove special_file flag.
	(buf_flush): Remove code for handling unseekable files.
	(buf_seek): Likewise.
	(fd_to_stream): Use buffered IO only for regular files.
	(file_length): Remove is_seekable() call.
	(is_seekable): Remove function.
	(is_special): Likewise.
	* io/unix.h: Remove prototypes for is_seekable and is_special.

Comments

Janne Blomqvist Oct. 25, 2011, 1:14 p.m. UTC | #1 
PING!

Also, PING for parts 2/3 and 3/3 of this (loosely related) patch series:

http://gcc.gnu.org/ml/gcc-patches/2011-10/msg01642.html

http://gcc.gnu.org/ml/gcc-patches/2011-10/msg01644.html

On Tue, Oct 18, 2011 at 17:42, Janne Blomqvist
<blomqvist.janne@gmail.com> wrote:
> Hi,
>
> in a few places in libgfortran we have some code for handling special
> and/or non-seekable files differently. The problem is that special
> files don't all have some nice consistent behavior. E.g. wrt. seeking,
> some allow seeking just fine, others allow some seeks and not others,
> others allow them but always return an offset of 0, and yet others
> fail the seek completely.
>
> The Fortran standard doesn't really help here except for noting that
> some files may not be positionable, and thus statements requiring the
> file position to be modified may fail on such files.
>
> Obviously, libgfortran itself cannot enumerate all the possible
> variations for how a special file may behave, and trying to impose
> some kind of least common denominator may hide essential capability.
> Having thought about this, my conclusion is that the only thing that
> makes sense is that we do what the caller asks us to do, and if that
> fails, we report the error back to the caller and let the caller
> handle it. The attached patch implements this.
>
> Regtested on x86_64-unknown-linux-gnu, Ok for trunk?
>
> 2011-10-18  Janne Blomqvist  <jb@gcc.gnu.org>
>
>        * io/file_pos.c (st_rewind): Handle regular and special files
>        identically.
>        * io/intrinsics.c (fseek_sub): Don't check whether we think the
>        file is seekable, just do what the caller says.
>        * io/transfer.c (skip_record): First try to seek, then fallback to
>        reading and throwing away what we read.
>        * io/unit.c (update_position): Don't check whether file is
>        seekable, just try to do what we're told.
>        (unit_truncate): Likewise.
>        * io/unix.c (struct unix_stream): Remove special_file flag.
>        (buf_flush): Remove code for handling unseekable files.
>        (buf_seek): Likewise.
>        (fd_to_stream): Use buffered IO only for regular files.
>        (file_length): Remove is_seekable() call.
>        (is_seekable): Remove function.
>        (is_special): Likewise.
>        * io/unix.h: Remove prototypes for is_seekable and is_special.
>
>
> --
> Janne Blomqvist
>
Mikael Morin Oct. 28, 2011, 9:48 p.m. UTC | #2 
On Tuesday 18 October 2011 16:42:45 Janne Blomqvist wrote:
> Hi,
> 
> in a few places in libgfortran we have some code for handling special
> and/or non-seekable files differently. The problem is that special
> files don't all have some nice consistent behavior. E.g. wrt. seeking,
> some allow seeking just fine, others allow some seeks and not others,
> others allow them but always return an offset of 0, and yet others
> fail the seek completely.
> 
> The Fortran standard doesn't really help here except for noting that
> some files may not be positionable, and thus statements requiring the
> file position to be modified may fail on such files.
> 
> Obviously, libgfortran itself cannot enumerate all the possible
> variations for how a special file may behave, and trying to impose
> some kind of least common denominator may hide essential capability.
> Having thought about this, my conclusion is that the only thing that
> makes sense is that we do what the caller asks us to do, and if that
> fails, we report the error back to the caller and let the caller
> handle it. The attached patch implements this.
> 
> Regtested on x86_64-unknown-linux-gnu, Ok for trunk?
> 
I know little about the library, but your approach looks good, and I have seen 
nothing obviously wrong in the patch.
Thus, as no one else has complained so far: OK.

Mikael
diff mbox

Patch

diff --git a/libgcc/configure b/libgcc/configure
old mode 100644
new mode 100755
diff --git a/libgfortran/io/file_pos.c b/libgfortran/io/file_pos.c
index b034f38..2caf601 100644
--- a/libgfortran/io/file_pos.c
+++ b/libgfortran/io/file_pos.c
@@ -407,20 +407,15 @@  st_rewind (st_parameter_filepos *fpp)
 	  if (sseek (u->s, 0, SEEK_SET) < 0)
 	    generate_error (&fpp->common, LIBERROR_OS, NULL);
 
-	  /* Handle special files like /dev/null differently.  */
-	  if (!is_special (u->s))
+	  /* Set this for compatibilty with g77 for /dev/null.  */
+	  if (file_length (u->s) == 0)
+	    u->endfile = AT_ENDFILE;
+	  else
 	    {
 	      /* We are rewinding so we are not at the end.  */
 	      u->endfile = NO_ENDFILE;
 	    }
-	  else
-	    {
-	      /* Set this for compatibilty with g77 for /dev/null.  */
-	      if (file_length (u->s) == 0  && stell (u->s) == 0)
-		u->endfile = AT_ENDFILE;
-	      /* Future refinements on special files can go here.  */
-	    }
-
+	  
 	  u->current_record = 0;
 	  u->strm_pos = 1;
 	  u->read_bad = 0;
diff --git a/libgfortran/io/intrinsics.c b/libgfortran/io/intrinsics.c
index f48bd77..c1287d4 100644
--- a/libgfortran/io/intrinsics.c
+++ b/libgfortran/io/intrinsics.c
@@ -246,7 +246,7 @@  fseek_sub (int * unit, GFC_IO_INT * offset, int * whence, int * status)
   gfc_unit * u = find_unit (*unit);
   ssize_t result = -1;
 
-  if (u != NULL && is_seekable(u->s))
+  if (u != NULL)
     {
       result = sseek(u->s, *offset, *whence);
 
diff --git a/libgfortran/io/transfer.c b/libgfortran/io/transfer.c
index 12aca97..1e054f8 100644
--- a/libgfortran/io/transfer.c
+++ b/libgfortran/io/transfer.c
@@ -2823,18 +2823,12 @@  skip_record (st_parameter_dt *dtp, ssize_t bytes)
   if (dtp->u.p.current_unit->bytes_left_subrecord == 0)
     return;
 
-  if (is_seekable (dtp->u.p.current_unit->s))
+  /* Direct access files do not generate END conditions,
+     only I/O errors.  */
+  if (sseek (dtp->u.p.current_unit->s, 
+	     dtp->u.p.current_unit->bytes_left_subrecord, SEEK_CUR) < 0)
     {
-      /* Direct access files do not generate END conditions,
-	 only I/O errors.  */
-      if (sseek (dtp->u.p.current_unit->s, 
-		 dtp->u.p.current_unit->bytes_left_subrecord, SEEK_CUR) < 0)
-	generate_error (&dtp->common, LIBERROR_OS, NULL);
-
-      dtp->u.p.current_unit->bytes_left_subrecord = 0;
-    }
-  else
-    {			/* Seek by reading data.  */
+      /* Seeking failed, fall back to seeking by reading data.  */
       while (dtp->u.p.current_unit->bytes_left_subrecord > 0)
 	{
 	  rlength = 
@@ -2850,8 +2844,9 @@  skip_record (st_parameter_dt *dtp, ssize_t bytes)
 
 	  dtp->u.p.current_unit->bytes_left_subrecord -= readb;
 	}
+      return;
     }
-
+  dtp->u.p.current_unit->bytes_left_subrecord = 0;
 }
 
 
diff --git a/libgfortran/io/unit.c b/libgfortran/io/unit.c
index e8a9b84..d2fb6d0 100644
--- a/libgfortran/io/unit.c
+++ b/libgfortran/io/unit.c
@@ -714,23 +714,21 @@  update_position (gfc_unit *u)
   /* If unit is not seekable, this makes no sense (and the standard is
      silent on this matter), and thus we don't change the position for
      a non-seekable file.  */
-  if (is_seekable (u->s))
-    {
-      gfc_offset cur = stell (u->s);
-      if (cur == 0)
-	u->flags.position = POSITION_REWIND;
-      else if (cur != -1 && (file_length (u->s) == cur))
-	u->flags.position = POSITION_APPEND;
-      else
-	u->flags.position = POSITION_ASIS;
-    }
+  gfc_offset cur = stell (u->s);
+  if (cur == -1)
+    return;
+  else if (cur == 0)
+    u->flags.position = POSITION_REWIND;
+  else if (file_length (u->s) == cur)
+    u->flags.position = POSITION_APPEND;
+  else
+    u->flags.position = POSITION_ASIS;
 }
 
 
-/* High level interface to truncate a file safely, i.e. flush format
-   buffers, check that it's a regular file, and generate error if that
-   occurs.  Just like POSIX ftruncate, returns 0 on success, -1 on
-   failure.  */
+/* High level interface to truncate a file, i.e. flush format buffers,
+   and generate an error or set some flags.  Just like POSIX
+   ftruncate, returns 0 on success, -1 on failure.  */
 
 int
 unit_truncate (gfc_unit * u, gfc_offset pos, st_parameter_common * common)
@@ -746,24 +744,12 @@  unit_truncate (gfc_unit * u, gfc_offset pos, st_parameter_common * common)
 	fbuf_flush (u, u->mode);
     }
   
-  /* Don't try to truncate a special file, just pretend that it
-     succeeds.  */
-  if (is_special (u->s) || !is_seekable (u->s))
-    {
-      sflush (u->s);
-      return 0;
-    }
-
   /* struncate() should flush the stream buffer if necessary, so don't
      bother calling sflush() here.  */
   ret = struncate (u->s, pos);
 
   if (ret != 0)
-    {
-      generate_error (common, LIBERROR_OS, NULL);
-      u->endfile = NO_ENDFILE;
-      u->flags.position = POSITION_ASIS;
-    }
+    generate_error (common, LIBERROR_OS, NULL);
   else
     {
       u->endfile = AT_ENDFILE;
diff --git a/libgfortran/io/unix.c b/libgfortran/io/unix.c
index 25cb559..00f7c72 100644
--- a/libgfortran/io/unix.c
+++ b/libgfortran/io/unix.c
@@ -187,7 +187,7 @@  typedef struct
   gfc_offset buffer_offset;	/* File offset of the start of the buffer */
   gfc_offset physical_offset;	/* Current physical file offset */
   gfc_offset logical_offset;	/* Current logical file offset */
-  gfc_offset file_length;	/* Length of the file, -1 if not seekable. */
+  gfc_offset file_length;	/* Length of the file. */
 
   char *buffer;                 /* Pointer to the buffer.  */
   int fd;                       /* The POSIX file descriptor.  */
@@ -196,8 +196,6 @@  typedef struct
 
   int ndirty;			/* Dirty bytes starting at buffer_offset */
 
-  int special_file;             /* =1 if the fd refers to a special file */
-
   /* Cached stat(2) values.  */
   dev_t st_dev;
   ino_t st_ino;
@@ -413,7 +411,7 @@  raw_init (unix_stream * s)
 Buffered I/O functions. These functions have the same semantics as the
 raw I/O functions above, except that they are buffered in order to
 improve performance. The buffer must be flushed when switching from
-reading to writing and vice versa.
+reading to writing and vice versa. Only supported for regular files.
 *********************************************************************/
 
 static int
@@ -427,7 +425,7 @@  buf_flush (unix_stream * s)
   if (s->ndirty == 0)
     return 0;
   
-  if (s->file_length != -1 && s->physical_offset != s->buffer_offset
+  if (s->physical_offset != s->buffer_offset
       && lseek (s->fd, s->buffer_offset, SEEK_SET) < 0)
     return -1;
 
@@ -435,8 +433,7 @@  buf_flush (unix_stream * s)
 
   s->physical_offset = s->buffer_offset + writelen;
 
-  /* Don't increment file_length if the file is non-seekable.  */
-  if (s->file_length != -1 && s->physical_offset > s->file_length)
+  if (s->physical_offset > s->file_length)
       s->file_length = s->physical_offset;
 
   s->ndirty -= writelen;
@@ -481,7 +478,7 @@  buf_read (unix_stream * s, void * buf, ssize_t nbyte)
       /* At this point we consider all bytes in the buffer discarded.  */
       to_read = nbyte - nread;
       new_logical = s->logical_offset + nread;
-      if (s->file_length != -1 && s->physical_offset != new_logical
+      if (s->physical_offset != new_logical
           && lseek (s->fd, new_logical, SEEK_SET) < 0)
         return -1;
       s->buffer_offset = s->physical_offset = new_logical;
@@ -539,7 +536,7 @@  buf_write (unix_stream * s, const void * buf, ssize_t nbyte)
         }
       else
 	{
-	  if (s->file_length != -1 && s->physical_offset != s->logical_offset)
+	  if (s->physical_offset != s->logical_offset)
 	    {
 	      if (lseek (s->fd, s->logical_offset, SEEK_SET) < 0)
 		return -1;
@@ -551,8 +548,7 @@  buf_write (unix_stream * s, const void * buf, ssize_t nbyte)
 	}
     }
   s->logical_offset += nbyte;
-  /* Don't increment file_length if the file is non-seekable.  */
-  if (s->file_length != -1 && s->logical_offset > s->file_length)
+  if (s->logical_offset > s->file_length)
     s->file_length = s->logical_offset;
   return nbyte;
 }
@@ -560,11 +556,6 @@  buf_write (unix_stream * s, const void * buf, ssize_t nbyte)
 static gfc_offset
 buf_seek (unix_stream * s, gfc_offset offset, int whence)
 {
-  if (s->file_length == -1)
-    {
-      errno = ESPIPE;
-      return -1;
-    }
   switch (whence)
     {
     case SEEK_SET:
@@ -953,30 +944,18 @@  fd_to_stream (int fd)
 
   s->st_dev = statbuf.st_dev;
   s->st_ino = statbuf.st_ino;
-  s->special_file = !S_ISREG (statbuf.st_mode);
-
-  if (S_ISREG (statbuf.st_mode))
-    s->file_length = statbuf.st_size;
+  s->file_length = statbuf.st_size;
+
+  /* Only use buffered IO for regular files.  */
+  if (S_ISREG (statbuf.st_mode)
+      && !options.all_unbuffered
+      && !(options.unbuffered_preconnected && 
+	   (s->fd == STDIN_FILENO 
+	    || s->fd == STDOUT_FILENO 
+	    || s->fd == STDERR_FILENO)))
+    buf_init (s);
   else
-    {
-      /* Some character special files are seekable but most are not,
-	 so figure it out by trying to seek.  On Linux, /dev/null is
-	 an example of such a special file.  */
-      s->file_length = lseek (fd, 0, SEEK_END);
-      if (s->file_length > 0)
-	lseek (fd, 0, SEEK_SET);
-    }
-
-  if (!(S_ISREG (statbuf.st_mode) || S_ISBLK (statbuf.st_mode))
-      || options.all_unbuffered
-      ||(options.unbuffered_preconnected && 
-         (s->fd == STDIN_FILENO 
-          || s->fd == STDOUT_FILENO 
-          || s->fd == STDERR_FILENO))
-      || isatty (s->fd))
     raw_init (s);
-  else
-    buf_init (s);
 
   return (stream *) s;
 }
@@ -1767,8 +1746,6 @@  gfc_offset
 file_length (stream * s)
 {
   gfc_offset curr, end;
-  if (!is_seekable (s))
-    return -1;
   curr = stell (s);
   if (curr == -1)
     return curr;
@@ -1778,27 +1755,6 @@  file_length (stream * s)
 }
 
 
-/* is_seekable()-- Return nonzero if the stream is seekable, zero if
- * it is not */
-
-int
-is_seekable (stream *s)
-{
-  /* By convention, if file_length == -1, the file is not
-     seekable.  */
-  return ((unix_stream *) s)->file_length!=-1;
-}
-
-
-/* is_special()-- Return nonzero if the stream is not a regular file.  */
-
-int
-is_special (stream *s)
-{
-  return ((unix_stream *) s)->special_file;
-}
-
-
 int
 stream_isatty (stream *s)
 {
diff --git a/libgfortran/io/unix.h b/libgfortran/io/unix.h
index f7d6f08..08c83e4 100644
--- a/libgfortran/io/unix.h
+++ b/libgfortran/io/unix.h
@@ -158,12 +158,6 @@  internal_proto(inquire_readwrite);
 extern gfc_offset file_length (stream *);
 internal_proto(file_length);
 
-extern int is_seekable (stream *);
-internal_proto(is_seekable);
-
-extern int is_special (stream *);
-internal_proto(is_special);
-
 extern void flush_if_preconnected (stream *);
 internal_proto(flush_if_preconnected);