| Message ID | AANLkTikbqPA0r_0j2O1QRFp71n-a_ubquvX5dfjtKPnm@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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
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.
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~
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.
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. */