[PATCH, libgfortran] PR 65200 Handle EPERM as EACCES

[PATCH, libgfortran] PR 65200 Handle EPERM as EACCES

[Janne Blomqvist]

[-- Attachment #1: Type: text/plain, Size: 2431 bytes --]

Hi,

the attached patch makes GFortran handle errno=EPERM in the same way
as EACCES (this affects only when no ACTION= specifier is used in the
OPEN statement and opening the file in read-write mode fails).
Ostensibly the distinction is roughly

- EACCES: Insufficient privilege. E.g. do the same as root and this should work.
- EPERM: Operation not permitted. That is, no matter the privilege
level, this isn't allowed. E.g. truncating a file opened with O_APPEND
and such.

However, it seems at least the Linux NFSv4 client returns EPERM when
trying to open a read-only file in read-write mode, whereas local
filesystems and the NFSv3 client return EACCES. So it seems reasonable
to check for EPERM as well in the same situations one checks for
EACCES.

The patch also contains a doc snippet to explain how GFortran behaves
when no ACTION= is given when opening a file. This brings me to a
standards interpretation question: When no ACTION= is given, F2008
9.5.6.4 (ACCESS= specifier in OPEN statement) says: "If this specifier
is omitted, the default value is processor dependent."  So does this
mean that the current GFortran behavior is allowed, or must the
behavior be exactly as with one of the possible ACTION= specifiers
(READ, WRITE, READWRITE)?

The patch as is causes gfortran.dg/open_errors.f90 to fail, due to
changed error messages. I'm a bit unsure of to fix this, as now
strerror* is used to generate part of the message, and thus the
message can be different on different targets, and even dependent on
the locale settings. Just checking for the fixed part of the error
message doesn't seem that useful in this case; should the entire
testcase just be removed?

Regtested on x86_64-unknown-linux-gnu, Ok for trunk (with some
decision wrt open_errors.f90)?

gcc/fortran ChangeLog:

2015-03-07  Janne Blomqvist  <jb@gcc.gnu.org>

    PR libfortran/65200
    * gfortran.texi: Document behavior when opening files without
    explicit ACTION= specifier.

libgfortran ChangeLog:

2015-03-07  Janne Blomqvist  <jb@gcc.gnu.org>

    PR libfortran/65200
    * io/open.c (new_unit): Use gf_strerror rather than hardcoding
    error messages for different errno values.
    * io/unix.c (regular_file2): Handle EPERM in addition to EACCES.

gcc/testsuite ChangeLog:

2015-03-07  Janne Blomqvist  <jb@gcc.gnu.org>

    PR libfortran/65200
    * gfortran.dg/open_new_segv.f90: Fix error message pattern.


-- 
Janne Blomqvist

[-- Attachment #2: eperm.diff --]
[-- Type: text/plain, Size: 4134 bytes --]

diff --git a/gcc/fortran/gfortran.texi b/gcc/fortran/gfortran.texi
index 300b8b8..34999db 100644
--- a/gcc/fortran/gfortran.texi
+++ b/gcc/fortran/gfortran.texi
@@ -1139,6 +1139,7 @@ might in some way or another become visible to the programmer.
 * Internal representation of LOGICAL variables::
 * Thread-safety of the runtime library::
 * Data consistency and durability::
+* Files opened without an explicit ACTION= specifier::
 @end menu
 
 
@@ -1328,6 +1329,22 @@ releasing @code{fcntl} file locks, if the server supports them, will
 also force cache validation and flushing dirty data and metadata.
 
 
+@node Files opened without an explicit ACTION= specifier
+@section Files opened without an explicit ACTION= specifier
+@cindex open, action
+
+The Fortran standard says that if an @code{OPEN} statement is executed
+without an explicit @code{ACTION=} specifier, the default value is
+processor dependent.  GNU Fortran behaves as follows:
+
+@enumerate
+@item Attempt to open the file with @code{ACTION='READWRITE'}
+@item If that fails, try to open with @code{ACTION='READ'}
+@item If that fails, try to open with @code{ACTION='WRITE'}
+@item If that fails, generate an error
+@end enumerate
+
+
 @c ---------------------------------------------------------------------
 @c Extensions
 @c ---------------------------------------------------------------------
diff --git a/gcc/testsuite/gfortran.dg/open_new_segv.f90 b/gcc/testsuite/gfortran.dg/open_new_segv.f90
index fe548f1..d9f2871 100644
--- a/gcc/testsuite/gfortran.dg/open_new_segv.f90
+++ b/gcc/testsuite/gfortran.dg/open_new_segv.f90
@@ -1,5 +1,5 @@
 ! { dg-do run }
-! { dg-shouldfail "File already exists" }
+! { dg-shouldfail "Cannot open file" }
 ! PR 64770 SIGSEGV when trying to open an existing file with status="new"
 program pr64770
   implicit none
@@ -10,5 +10,5 @@ program pr64770
        status="new")
 end program pr64770
 ! { dg-output "At line 10 of file.*" }
-! { dg-output "Fortran runtime error: File .pr64770test.dat. already exists" }
+! { dg-output "Fortran runtime error: Cannot open file .pr64770test.dat.:" }
 ! { dg-final { remote_file build delete "pr64770test.dat" } }
diff --git a/libgfortran/io/open.c b/libgfortran/io/open.c
index 0a2fda9..4654de2 100644
--- a/libgfortran/io/open.c
+++ b/libgfortran/io/open.c
@@ -502,34 +502,12 @@ new_unit (st_parameter_open *opp, gfc_unit *u, unit_flags * flags)
   s = open_external (opp, flags);
   if (s == NULL)
     {
+      char errbuf[256];
       char *path = fc_strdup (opp->file, opp->file_len);
-      size_t msglen = opp->file_len + 51;
+      size_t msglen = opp->file_len + 22 + sizeof (errbuf);
       char *msg = xmalloc (msglen);
-
-      switch (errno)
-	{
-	case ENOENT: 
-	  snprintf (msg, msglen, "File '%s' does not exist", path);
-	  break;
-
-	case EEXIST:
-	  snprintf (msg, msglen, "File '%s' already exists", path);
-	  break;
-
-	case EACCES:
-	  snprintf (msg, msglen, 
-		    "Permission denied trying to open file '%s'", path);
-	  break;
-
-	case EISDIR:
-	  snprintf (msg, msglen, "'%s' is a directory", path);
-	  break;
-
-	default:
-	  free (msg);
-	  msg = NULL;
-	}
-
+      snprintf (msg, msglen, "Cannot open file '%s': %s", path,
+		gf_strerror (errno, errbuf, sizeof (errbuf)));
       generate_error (&opp->common, LIBERROR_OS, msg);
       free (msg);
       free (path);
diff --git a/libgfortran/io/unix.c b/libgfortran/io/unix.c
index 912364b..e5fc6e1 100644
--- a/libgfortran/io/unix.c
+++ b/libgfortran/io/unix.c
@@ -1353,7 +1353,7 @@ regular_file2 (const char *path, st_parameter_open *opp, unit_flags *flags)
       flags->action = ACTION_READWRITE;
       return fd;
     }
-  if (errno != EACCES && errno != EROFS)
+  if (errno != EACCES && errno != EPERM && errno != EROFS)
      return fd;
 
   /* retry for read-only access */
@@ -1369,7 +1369,7 @@ regular_file2 (const char *path, st_parameter_open *opp, unit_flags *flags)
       return fd;		/* success */
     }
   
-  if (errno != EACCES && errno != ENOENT)
+  if (errno != EACCES && errno != EPERM && errno != ENOENT)
     return fd;			/* failure */
 
   /* retry for write-only access */

Re: [PATCH, libgfortran] PR 65200 Handle EPERM as EACCES

[FX]

Hi Janne,

> the attached patch makes GFortran handle errno=EPERM in the same way
> as EACCES (this affects only when no ACTION= specifier is used in the
> OPEN statement and opening the file in read-write mode fails).

Makes sense.

> The patch also contains a doc snippet to explain how GFortran behaves
> when no ACTION= is given when opening a file.

Great.


> This brings me to a
> standards interpretation question: When no ACTION= is given, F2008
> 9.5.6.4 (ACCESS= specifier in OPEN statement) says: "If this specifier
> is omitted, the default value is processor dependent."  So does this
> mean that the current GFortran behavior is allowed, or must the
> behavior be exactly as with one of the possible ACTION= specifiers
> (READ, WRITE, READWRITE)?

I think we are standard-confirming: we open the file with one of the specifiers, determined in a processor-dependent fashion. Except our process-dependent value is not a constant, it depends on the file chosen (and its permissions). I believe that’s still within “processor dependent”. And it certainly makes more sense that the other possibilities.


Thus, OK to commit.

FX

Re: [PATCH, libgfortran] PR 65200 Handle EPERM as EACCES

[Jerry DeLisle]

On 03/06/2015 04:23 PM, Janne Blomqvist wrote:

> The patch as is causes gfortran.dg/open_errors.f90 to fail, due to
> changed error messages. I'm a bit unsure of to fix this, as now
> strerror* is used to generate part of the message, and thus the
> message can be different on different targets, and even dependent on
> the locale settings. Just checking for the fixed part of the error
> message doesn't seem that useful in this case; should the entire
> testcase just be removed?
>

Generally the patch is OK as FX has stated. I am concerned about the strerror 
messages being less helpful to users than the ones we have.  However, you can 
either keep the same error messages for those few or update the test case to use 
the error codes (iostat) if that makes more sense.

Jerry