[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
proposed tweaks for openat in coreutils
From: |
Paul Eggert |
Subject: |
proposed tweaks for openat in coreutils |
Date: |
Fri, 16 Dec 2005 01:07:42 -0800 |
User-agent: |
Gnus/5.1007 (Gnus v5.10.7) Emacs/21.4 (gnu/linux) |
I stared at the openat code in coreutils for a while and came up with
the following proposed cleanups of several small issues. Perhaps the
largest one is that I guess mkdirat uses openat_save_fail even when it
is a no-op macro, which doesn't sound right.
On the plus side, the proposed change makes the code 80 lines shorter....
I have some minor qualms about this part of the patch in remove.c:
- {
- bool t_cwd_restore_failed = false;
- status = remove_dir (fd_cwd, ds, filename, x, &t_cwd_restore_failed);
- *cwd_restore_failed |= t_cwd_restore_failed;
- }
+ status = remove_dir (fd_cwd, ds, filename, x, cwd_errno);
I can't figure out why the old code had that local boolean variable,
rather than just passing cwd_restore_failed to remove_dir. Perhaps
this was to help with debugging with GDB?
2005-12-16 Paul Eggert <address@hidden>
* lib/openat.c: Don't include <stdlib.h>, <unistd.h>, <fcntl.h>,
"gettext.h"; either no longer needed or are guaranteed by openat.h.
(_): Remove; no longer needed.
(openat): Renamed from rpl_openat; no need for rpl_openat
since openat.h renames openat for us.
Replace most of the body with a call to openat_permissive,
to avoid duplicate code.
Port to (probably hypothetical) environments were mode_t is
wider than int.
(openat_permissive): Require mode arg, so that we can check
types better. Put it just after flags. Change cwd failure
indicator from pointer-to-bool to pointer-to-errno-value.
All callers changed.
Invoke openat_save_fail and/or openat_restore_fail if
cwd_errno is null, so that openat can call us.
(openat_permissive, fdopendir, fstatat, unlinkat):
Simplify errno handling to avoid some duplicate code,
as it's OK to set errno on success.
* lib/openat.h: Revamp code so that function macros depend on
__OPENAT_PREFIX only, not also on AT_FDCWD.
(openat_ro): Remove. Caller changed to use openat_permissive.
(openat_permissive): Now a macro, if not a function.
(openat_restore_fail, openat_save_fail): Now always functions,
since mkdirat needs them even if __OPENAT_PREFIX is defined.
* src/remove.c (OPENAT_CWD_RESTORE__REQUIRE): Remove.
(OPENAT_CWD_RESTORE__ALLOW_FAILURE): Likewise.
(fd_to_subdirp): Remove openat_cwd_restore_allow_failure arg; its
value is now signified by whether cwd_errno is null.
(fd_to_subdirp, remove_dir, rm_1); Change cwd failure indicator from
pointer-to-bool to pointer-to-errno-value. All callers changed.
(rm_1): Don't bother setting a local cwd failure flag and then
ORing it into the caller's. Just set the caller's.
(rm): Use cwd failure errno value to print a slightly-better
diagnostic.
Index: lib/openat.c
===================================================================
RCS file: /fetish/cu/lib/openat.c,v
retrieving revision 1.21
diff -p -u -r1.21 openat.c
--- lib/openat.c 30 Nov 2005 13:04:26 -0000 1.21
+++ lib/openat.c 16 Dec 2005 09:00:25 -0000
@@ -23,19 +23,13 @@
#include "openat.h"
-#include <stdlib.h>
-#include <stdarg.h>
-#include <unistd.h>
-#include <errno.h>
-#include <fcntl.h>
-
#include "dirname.h" /* solely for definition of IS_ABSOLUTE_FILE_NAME */
+#include "openat-priv.h"
#include "save-cwd.h"
-#include "gettext.h"
-#define _(msgid) gettext (msgid)
-
-#include "openat-priv.h"
+#include <stdarg.h>
+#include <stddef.h>
+#include <errno.h>
/* Replacement for Solaris' openat function.
<http://www.google.com/search?q=openat+site:docs.sun.com>
@@ -46,61 +40,31 @@
Otherwise, upon failure, set errno and return -1, as openat does.
Upon successful completion, return a file descriptor. */
int
-rpl_openat (int fd, char const *file, int flags, ...)
+openat (int fd, char const *file, int flags, ...)
{
- struct saved_cwd saved_cwd;
- int saved_errno;
- int err;
mode_t mode = 0;
if (flags & O_CREAT)
{
va_list arg;
va_start (arg, flags);
- /* Use the promoted type (int), not mode_t, as second argument. */
- mode = (mode_t) va_arg (arg, int);
- va_end (arg);
- }
-
- if (fd == AT_FDCWD || IS_ABSOLUTE_FILE_NAME (file))
- return open (file, flags, mode);
-
- {
- char *proc_file;
- BUILD_PROC_NAME (proc_file, fd, file);
- err = open (proc_file, flags, mode);
- /* If the syscall succeeds, or if it fails with an unexpected
- errno value, then return right away. Otherwise, fall through
- and resort to using save_cwd/restore_cwd. */
- if (0 <= err || ! EXPECTED_ERRNO (errno))
- return err;
- }
- if (save_cwd (&saved_cwd) != 0)
- openat_save_fail (errno);
+ /* If mode_t is narrower than int, use the promoted type (int),
+ not mode_t. Use sizeof to guess whether mode_t is nerrower;
+ we don't know of any practical counterexamples. */
+ if (sizeof (mode_t) < sizeof (int))
+ mode = va_arg (arg, int);
+ else
+ mode = va_arg (arg, mode_t);
- if (fchdir (fd) != 0)
- {
- saved_errno = errno;
- free_cwd (&saved_cwd);
- errno = saved_errno;
- return -1;
+ va_end (arg);
}
- err = open (file, flags, mode);
- saved_errno = (err < 0 ? errno : 0);
-
- if (restore_cwd (&saved_cwd) != 0)
- openat_restore_fail (errno);
-
- free_cwd (&saved_cwd);
-
- if (saved_errno)
- errno = saved_errno;
- return err;
+ return openat_permissive (fd, file, flags, mode, NULL);
}
-/* Like openat above, but set *RESTORE_FAILED if unable to save
+/* Like openat (FD, FILE, FLAGS, MODE), but if CWD_ERRNO is
+ nonnull, set *CWD_ERRNO to an errno value if unable to save
or restore the initial working directory. This is needed only
the first time remove.c's remove_dir opens a command-line
directory argument.
@@ -111,23 +75,13 @@ rpl_openat (int fd, char const *file, in
in that case. */
int
-openat_permissive (int fd, char const *file, int flags,
- bool *cwd_restore_failed, ...)
+openat_permissive (int fd, char const *file, int flags, mode_t mode,
+ int *cwd_errno)
{
struct saved_cwd saved_cwd;
int saved_errno;
- int save_restore_errno = 0;
int err;
- mode_t mode = 0;
-
- if (flags & O_CREAT)
- {
- va_list arg;
- va_start (arg, cwd_restore_failed);
- /* Use the promoted type (int), not mode_t, as second argument. */
- mode = (mode_t) va_arg (arg, int);
- va_end (arg);
- }
+ bool save_ok;
if (fd == AT_FDCWD || IS_ABSOLUTE_FILE_NAME (file))
return open (file, flags, mode);
@@ -143,37 +97,31 @@ openat_permissive (int fd, char const *f
return err;
}
- if (save_cwd (&saved_cwd) != 0)
+ save_ok = (save_cwd (&saved_cwd) == 0);
+ if (! save_ok)
{
- *cwd_restore_failed = true;
- save_restore_errno = errno;
+ if (! cwd_errno)
+ openat_save_fail (errno);
+ *cwd_errno = errno;
}
- if (fchdir (fd) != 0)
- {
- saved_errno = errno;
- free_cwd (&saved_cwd);
- errno = saved_errno;
- return -1;
- }
-
- err = open (file, flags, mode);
- saved_errno = (err < 0 ? errno : 0);
+ err = fchdir (fd);
+ saved_errno = errno;
- if ( ! *cwd_restore_failed && restore_cwd (&saved_cwd) != 0)
+ if (! err)
{
- *cwd_restore_failed = true;
- save_restore_errno = errno;
+ err = open (file, flags, mode);
+ saved_errno = errno;
+ if (save_ok && restore_cwd (&saved_cwd) != 0)
+ {
+ if (! cwd_errno)
+ openat_restore_fail (errno);
+ *cwd_errno = errno;
+ }
}
- if ( ! *cwd_restore_failed)
- free_cwd (&saved_cwd);
-
- if (saved_errno)
- errno = saved_errno;
- else if (save_restore_errno)
- errno = save_restore_errno;
-
+ free_cwd (&saved_cwd);
+ errno = saved_errno;
return err;
}
@@ -198,43 +146,38 @@ fdopendir (int fd)
int saved_errno;
DIR *dir;
- {
- char *proc_file;
- BUILD_PROC_NAME (proc_file, fd, ".");
- dir = opendir (proc_file);
- saved_errno = (dir == NULL ? errno : 0);
- /* If the syscall succeeds, or if it fails with an unexpected
- errno value, then return right away. Otherwise, fall through
- and resort to using save_cwd/restore_cwd. */
- if (dir != NULL || ! EXPECTED_ERRNO (errno))
- goto close_and_return;
- }
+ char *proc_file;
+ BUILD_PROC_NAME (proc_file, fd, ".");
+ dir = opendir (proc_file);
+ saved_errno = errno;
+
+ /* If the syscall fails with an expected errno value, resort to
+ save_cwd/restore_cwd. */
+ if (! dir && EXPECTED_ERRNO (saved_errno))
+ {
+ if (save_cwd (&saved_cwd) != 0)
+ openat_save_fail (errno);
- if (save_cwd (&saved_cwd) != 0)
- openat_save_fail (errno);
+ if (fchdir (fd) != 0)
+ {
+ dir = NULL;
+ saved_errno = errno;
+ }
+ else
+ {
+ dir = opendir (".");
+ saved_errno = errno;
+
+ if (restore_cwd (&saved_cwd) != 0)
+ openat_restore_fail (errno);
+ }
- if (fchdir (fd) != 0)
- {
- saved_errno = errno;
free_cwd (&saved_cwd);
- errno = saved_errno;
- return NULL;
}
- dir = opendir (".");
- saved_errno = (dir == NULL ? errno : 0);
-
- if (restore_cwd (&saved_cwd) != 0)
- openat_restore_fail (errno);
-
- free_cwd (&saved_cwd);
-
- close_and_return:;
if (dir)
close (fd);
-
- if (saved_errno)
- errno = saved_errno;
+ errno = saved_errno;
return dir;
}
@@ -275,26 +218,22 @@ fstatat (int fd, char const *file, struc
if (save_cwd (&saved_cwd) != 0)
openat_save_fail (errno);
- if (fchdir (fd) != 0)
+ err = fchdir (fd);
+ saved_errno = errno;
+
+ if (! err)
{
+ err = (flag == AT_SYMLINK_NOFOLLOW
+ ? lstat (file, st)
+ : stat (file, st));
saved_errno = errno;
- free_cwd (&saved_cwd);
- errno = saved_errno;
- return -1;
- }
-
- err = (flag == AT_SYMLINK_NOFOLLOW
- ? lstat (file, st)
- : stat (file, st));
- saved_errno = (err < 0 ? errno : 0);
- if (restore_cwd (&saved_cwd) != 0)
- openat_restore_fail (errno);
+ if (restore_cwd (&saved_cwd) != 0)
+ openat_restore_fail (errno);
+ }
free_cwd (&saved_cwd);
-
- if (saved_errno)
- errno = saved_errno;
+ errno = saved_errno;
return err;
}
@@ -329,23 +268,19 @@ unlinkat (int fd, char const *file, int
if (save_cwd (&saved_cwd) != 0)
openat_save_fail (errno);
- if (fchdir (fd) != 0)
+ err = fchdir (fd);
+ saved_errno = errno;
+
+ if (! err)
{
+ err = (flag == AT_REMOVEDIR ? rmdir (file) : unlink (file));
saved_errno = errno;
- free_cwd (&saved_cwd);
- errno = saved_errno;
- return -1;
- }
-
- err = (flag == AT_REMOVEDIR ? rmdir (file) : unlink (file));
- saved_errno = (err < 0 ? errno : 0);
- if (restore_cwd (&saved_cwd) != 0)
- openat_restore_fail (errno);
+ if (restore_cwd (&saved_cwd) != 0)
+ openat_restore_fail (errno);
+ }
free_cwd (&saved_cwd);
-
- if (saved_errno)
- errno = saved_errno;
+ errno = saved_errno;
return err;
}
Index: lib/openat.h
===================================================================
RCS file: /fetish/cu/lib/openat.h,v
retrieving revision 1.14
diff -p -u -r1.14 openat.h
--- lib/openat.h 30 Nov 2005 12:40:09 -0000 1.14
+++ lib/openat.h 16 Dec 2005 09:00:25 -0000
@@ -36,38 +36,39 @@
#endif
#ifndef AT_FDCWD
-# define AT_FDCWD (-3041965) /* same value as Solaris 9 */
-# define AT_SYMLINK_NOFOLLOW 4096 /* same value as Solaris 9 */
-# define AT_REMOVEDIR (0x1) /* same value as Solaris 9 */
-
-# ifdef __OPENAT_PREFIX
-# undef openat
-# define __OPENAT_CONCAT(x, y) x ## y
-# define __OPENAT_XCONCAT(x, y) __OPENAT_CONCAT (x, y)
-# define __OPENAT_ID(y) __OPENAT_XCONCAT (__OPENAT_PREFIX, y)
-# define openat __OPENAT_ID (openat)
+/* Use the same values as Solaris 9. This shouldn't matter, but
+ there's no real reason to differ. */
+# define AT_FDCWD (-3041965)
+# define AT_SYMLINK_NOFOLLOW 4096
+# define AT_REMOVEDIR 1
+#endif
+
+#ifdef __OPENAT_PREFIX
+
+# undef openat
+# define __OPENAT_CONCAT(x, y) x ## y
+# define __OPENAT_XCONCAT(x, y) __OPENAT_CONCAT (x, y)
+# define __OPENAT_ID(y) __OPENAT_XCONCAT (__OPENAT_PREFIX, y)
+# define openat __OPENAT_ID (openat)
int openat (int fd, char const *file, int flags, /* mode_t mode */ ...);
-int openat_permissive (int fd, char const *file, int flags, bool
*restore_failed, ...);
-# if ! HAVE_FDOPENDIR
-# define fdopendir __OPENAT_ID (fdopendir)
-# endif
+int openat_permissive (int fd, char const *file, int flags, mode_t mode,
+ int *cwd_errno);
+# if ! HAVE_FDOPENDIR
+# define fdopendir __OPENAT_ID (fdopendir)
+# endif
DIR *fdopendir (int fd);
-# define fstatat __OPENAT_ID (fstatat)
+# define fstatat __OPENAT_ID (fstatat)
int fstatat (int fd, char const *file, struct stat *st, int flag);
-# define unlinkat __OPENAT_ID (unlinkat)
+# define unlinkat __OPENAT_ID (unlinkat)
int unlinkat (int fd, char const *file, int flag);
-void openat_restore_fail (int) ATTRIBUTE_NORETURN;
-void openat_save_fail (int) ATTRIBUTE_NORETURN;
-# define openat_ro(Fd, File, Flags, RF) openat_permissive (Fd, File, Flags,
RF)
-# else
-# define openat_restore_fail(Errno) /* empty */
-# define openat_save_fail(Errno) /* empty */
-# endif
-#endif
+#else
+
+# define openat_permissive(Fd, File, Flags, Mode, Cwd_errno) \
+ openat (Fd, File, Flags, Mode)
-#ifndef openat_ro
-# define openat_ro(Fd, File, Flags, RF) openat (Fd, File, Flags)
#endif
int mkdirat (int fd, char const *file, mode_t mode);
+void openat_restore_fail (int) ATTRIBUTE_NORETURN;
+void openat_save_fail (int) ATTRIBUTE_NORETURN;
Index: src/remove.c
===================================================================
RCS file: /fetish/cu/src/remove.c,v
retrieving revision 1.139
diff -p -u -r1.139 remove.c
--- src/remove.c 23 Nov 2005 04:52:48 -0000 1.139
+++ src/remove.c 16 Dec 2005 09:00:25 -0000
@@ -74,12 +74,6 @@ enum
CONSECUTIVE_READDIR_UNLINK_THRESHOLD = 200
};
-enum
- {
- OPENAT_CWD_RESTORE__REQUIRE = false,
- OPENAT_CWD_RESTORE__ALLOW_FAILURE = true
- };
-
enum Ternary
{
T_UNKNOWN = 2,
@@ -991,30 +985,25 @@ remove_entry (int fd_cwd, Dirstack_state
unlink- or rmdir-like system call -- use that value instead of ENOTDIR
if an opened file turns out not to be a directory. This is important
when the preceding non-dir-unlink failed due to e.g., EPERM or EACCES.
- The caller must set OPENAT_CWD_RESTORE_ALLOW_FAILURE to true the first
+ The caller must use a nonnnull CWD_ERRNO the first
time this function is called for each command-line-specified directory.
- Set *CWD_RESTORE_FAILED if OPENAT_CWD_RESTORE_ALLOW_FAILURE is true
- and openat_ro fails to restore the initial working directory.
- CWD_RESTORE_FAILED may be NULL. */
+ If CWD_ERRNO is not null, set *CWD_ERRNO to the appropriate error number
+ if this function fails to restore the initial working directory.
+ If it is null, report an error and exit if the working directory
+ isn't restored. */
static DIR *
fd_to_subdirp (int fd_cwd, char const *f,
struct rm_options const *x, int prev_errno,
struct stat *subdir_sb, Dirstack_state *ds,
- bool openat_cwd_restore_allow_failure,
- bool *cwd_restore_failed)
+ int *cwd_errno ATTRIBUTE_UNUSED)
{
- int fd_sub;
- bool dummy;
+ int fd_sub = openat_permissive (fd_cwd, f, O_RDONLY | OPEN_NO_FOLLOW_SYMLINK,
+ 0, cwd_errno);
/* Record dev/ino of F. We may compare them against saved values
to thwart any attempt to subvert the traversal. They are also used
to detect directory cycles. */
- if ((fd_sub = (openat_cwd_restore_allow_failure
- ? openat_ro (fd_cwd, f, O_RDONLY | OPEN_NO_FOLLOW_SYMLINK,
- (cwd_restore_failed
- ? cwd_restore_failed : &dummy))
- : openat (fd_cwd, f, O_RDONLY | OPEN_NO_FOLLOW_SYMLINK))) < 0
- || fstat (fd_sub, subdir_sb) != 0)
+ if (fd_sub < 0 || fstat (fd_sub, subdir_sb) != 0)
{
if (errno != ENOENT || !x->ignore_missing_files)
error (0, errno,
@@ -1126,9 +1115,7 @@ remove_cwd_entries (DIR **dirp,
case RM_NONEMPTY_DIR:
{
DIR *subdir_dirp = fd_to_subdirp (dirfd (*dirp), f,
- x, errno, subdir_sb, ds,
- OPENAT_CWD_RESTORE__REQUIRE,
- NULL);
+ x, errno, subdir_sb, ds, NULL);
if (subdir_dirp == NULL)
{
AD_mark_as_unremovable (ds, f);
@@ -1193,7 +1180,7 @@ The following directory is part of the c
static enum RM_status
remove_dir (int fd_cwd, Dirstack_state *ds, char const *dir,
- struct rm_options const *x, bool *cwd_restore_failed)
+ struct rm_options const *x, int *cwd_errno)
{
enum RM_status status;
struct stat dir_sb;
@@ -1206,9 +1193,7 @@ remove_dir (int fd_cwd, Dirstack_state *
fd_to_subdirp's fstat, along with the `fstat' and the dev/ino
comparison in AD_push ensure that we detect it and fail. */
- DIR *dirp = fd_to_subdirp (fd_cwd, dir, x, 0, &dir_sb, ds,
- OPENAT_CWD_RESTORE__ALLOW_FAILURE,
- cwd_restore_failed);
+ DIR *dirp = fd_to_subdirp (fd_cwd, dir, x, 0, &dir_sb, ds, cwd_errno);
if (dirp == NULL)
return RM_ERROR;
@@ -1320,7 +1305,7 @@ remove_dir (int fd_cwd, Dirstack_state *
static enum RM_status
rm_1 (Dirstack_state *ds, char const *filename,
- struct rm_options const *x, bool *cwd_restore_failed)
+ struct rm_options const *x, int *cwd_errno)
{
char const *base = base_name (filename);
if (DOT_OR_DOTDOT (base))
@@ -1345,11 +1330,7 @@ rm_1 (Dirstack_state *ds, char const *fi
if (setjmp (ds->current_arg_jumpbuf))
status = RM_ERROR;
else
- {
- bool t_cwd_restore_failed = false;
- status = remove_dir (fd_cwd, ds, filename, x, &t_cwd_restore_failed);
- *cwd_restore_failed |= t_cwd_restore_failed;
- }
+ status = remove_dir (fd_cwd, ds, filename, x, cwd_errno);
}
ds_clear (ds);
@@ -1364,12 +1345,12 @@ rm (size_t n_files, char const *const *f
{
enum RM_status status = RM_OK;
Dirstack_state *ds = ds_init ();
- bool cwd_restore_failed = false;
+ int cwd_errno = 0;
size_t i;
for (i = 0; i < n_files; i++)
{
- if (cwd_restore_failed && IS_RELATIVE_FILE_NAME (file[i]))
+ if (cwd_errno && IS_RELATIVE_FILE_NAME (file[i]))
{
error (0, 0, _("cannot remove relative-named %s"), quote (file[i]));
status = RM_ERROR;
@@ -1377,14 +1358,14 @@ rm (size_t n_files, char const *const *f
}
cycle_check_init (&ds->cycle_check_state);
- enum RM_status s = rm_1 (ds, file[i], x, &cwd_restore_failed);
+ enum RM_status s = rm_1 (ds, file[i], x, &cwd_errno);
assert (VALID_STATUS (s));
UPDATE_STATUS (status, s);
}
- if (x->require_restore_cwd && cwd_restore_failed)
+ if (x->require_restore_cwd && cwd_errno)
{
- error (0, 0,
+ error (0, cwd_errno,
_("cannot restore current working directory"));
status = RM_ERROR;
}
- proposed tweaks for openat in coreutils,
Paul Eggert <=