Patchwork [libfortran] Use access(2) instead of stat(2) to test file existence

login
register
mail settings
Submitter Janne Blomqvist
Date July 31, 2010, 10:41 p.m.
Message ID <AANLkTikbqPA0r_0j2O1QRFp71n-a_ubquvX5dfjtKPnm@mail.gmail.com>
Download mbox | patch
Permalink /patch/60440/
State New
Headers show

Comments

Janne Blomqvist - July 31, 2010, 10:41 p.m.
Hello,

this simple patch uses access(2) instead of stat(2) in the
file_exists() function used to test whether a file pathname exists.
The reason why this is better is because stat() is potentially a
heavyweight operation (as those who have waited half an hour for 'ls
-l' to complete on a distributed fs such as Lustre can attest). This
does not change any the behavior for any corner cases such as dangling
symlinks (see PR 41387), as both access(2) and stat(2) will follow
symlinks.

As an aside, we seem to be quite happily using stat(2). One way to
reduce it would be to cache the device and inode numbers for open
units; as Fortran apparently forbids having multiple units pointing to
the same file (?), every time we open a file we walk through all
existing units and compare device and inode numbers.

Regtested on x86_64-unknown-linux-gnu, ok for trunk?
Jerry DeLisle - Aug. 1, 2010, 4:03 a.m.
On 07/31/2010 03:41 PM, Janne Blomqvist wrote:
> Hello,
>
> this simple patch uses access(2) instead of stat(2) in the
> file_exists() function used to test whether a file pathname exists.
> The reason why this is better is because stat() is potentially a
> heavyweight operation (as those who have waited half an hour for 'ls
> -l' to complete on a distributed fs such as Lustre can attest). This
> does not change any the behavior for any corner cases such as dangling
> symlinks (see PR 41387), as both access(2) and stat(2) will follow
> symlinks.
>
> As an aside, we seem to be quite happily using stat(2). One way to
> reduce it would be to cache the device and inode numbers for open
> units; as Fortran apparently forbids having multiple units pointing to
> the same file (?), every time we open a file we walk through all
> existing units and compare device and inode numbers.
>
> Regtested on x86_64-unknown-linux-gnu, ok for trunk?
>

OK, thanks.

Jerry
Janne Blomqvist - Aug. 1, 2010, 11:23 a.m.
On Sun, Aug 1, 2010 at 07:03, Jerry DeLisle <jvdelisle@verizon.net> wrote:
> On 07/31/2010 03:41 PM, Janne Blomqvist wrote:
>>
>> Hello,
>>
>> this simple patch uses access(2) instead of stat(2) in the
>> file_exists() function used to test whether a file pathname exists.
>> The reason why this is better is because stat() is potentially a
>> heavyweight operation (as those who have waited half an hour for 'ls
>> -l' to complete on a distributed fs such as Lustre can attest). This
>> does not change any the behavior for any corner cases such as dangling
>> symlinks (see PR 41387), as both access(2) and stat(2) will follow
>> symlinks.
>>
>> As an aside, we seem to be quite happily using stat(2). One way to
>> reduce it would be to cache the device and inode numbers for open
>> units; as Fortran apparently forbids having multiple units pointing to
>> the same file (?), every time we open a file we walk through all
>> existing units and compare device and inode numbers.
>>
>> Regtested on x86_64-unknown-linux-gnu, ok for trunk?
>>
>
> OK, thanks.
>
> Jerry
>

Thanks, committed as r162798.
Richard Henderson - Aug. 2, 2010, 4:29 p.m.
On 07/31/2010 03:41 PM, Janne Blomqvist wrote:
> +  if ((mode & R_OK) && open (path, O_RDONLY) < 0)
> +    return -1;
> +
> +  if ((mode & W_OK) && open (path, O_WRONLY) < 0)
> +    return -1;

You're leaking file descriptors here.

Since you're not using [RW]_OK, better to just remove those cases.


r~
Janne Blomqvist - Aug. 2, 2010, 5:03 p.m.
Thanks for taking the time to look at the patch.

On Mon, Aug 2, 2010 at 19:29, Richard Henderson <rth@redhat.com> wrote:
> On 07/31/2010 03:41 PM, Janne Blomqvist wrote:
>> +  if ((mode & R_OK) && open (path, O_RDONLY) < 0)
>> +    return -1;
>> +
>> +  if ((mode & W_OK) && open (path, O_WRONLY) < 0)
>> +    return -1;
>
> You're leaking file descriptors here.

Yes, I realized that, however, as this is not actually code that I
wrote as part of this patch I decided to leave it for later. I just
moved the entire function upwards in the file before all users, hence
it showed up in the diff. fallback_access() has been around since
2007, see PR 21185.

In any case, I filed PR 45165 to keep track of this issue.

> Since you're not using [RW]_OK, better to just remove those cases.

It is used by unix.c:inquire_access() and callers of that function.

Patch

diff --git a/libgfortran/io/unix.c b/libgfortran/io/unix.c
index eea03ba..a2903af 100644
--- a/libgfortran/io/unix.c
+++ b/libgfortran/io/unix.c
@@ -131,6 +131,46 @@  typedef struct stat gfstat_t;
 #endif
 
 
+#ifndef HAVE_ACCESS
+
+#ifndef W_OK
+#define W_OK 2
+#endif
+
+#ifndef R_OK
+#define R_OK 4
+#endif
+
+#ifndef F_OK
+#define F_OK 0
+#endif
+
+/* Fallback implementation of access() on systems that don't have it.
+   Only modes R_OK, W_OK and F_OK are used in this file.  */
+
+static int
+fallback_access (const char *path, int mode)
+{
+  if ((mode & R_OK) && open (path, O_RDONLY) < 0)
+    return -1;
+
+  if ((mode & W_OK) && open (path, O_WRONLY) < 0)
+    return -1;
+
+  if (mode == F_OK)
+    {
+      gfstat_t st;
+      return stat (path, &st);
+    }
+
+  return 0;
+}
+
+#undef access
+#define access fallback_access
+#endif
+
+
 /* Unix and internal stream I/O module */
 
 static const int BUFFER_SIZE = 8192;
@@ -1580,15 +1620,11 @@  int
 file_exists (const char *file, gfc_charlen_type file_len)
 {
   char path[PATH_MAX + 1];
-  gfstat_t statbuf;
 
   if (unpack_filename (path, file, file_len))
     return 0;
 
-  if (stat (path, &statbuf) < 0)
-    return 0;
-
-  return 1;
+  return !(access (path, F_OK));
 }
 
 
@@ -1695,36 +1731,6 @@  inquire_unformatted (const char *string, int len)
 }
 
 
-#ifndef HAVE_ACCESS
-
-#ifndef W_OK
-#define W_OK 2
-#endif
-
-#ifndef R_OK
-#define R_OK 4
-#endif
-
-/* Fallback implementation of access() on systems that don't have it.
-   Only modes R_OK and W_OK are used in this file.  */
-
-static int
-fallback_access (const char *path, int mode)
-{
-  if ((mode & R_OK) && open (path, O_RDONLY) < 0)
-    return -1;
-
-  if ((mode & W_OK) && open (path, O_WRONLY) < 0)
-    return -1;
-
-  return 0;
-}
-
-#undef access
-#define access fallback_access
-#endif
-
-
 /* inquire_access()-- Given a fortran string, determine if the file is
  * suitable for access. */