Jim Meyering wrote: ... > I'll post this new pair of rm-related patches in a minute.
Here's what I'm ready to push. Finally, I'm not adding a test for what happens when one tries to run rm -r s/ (with s being a symlink), since that is so system-specific. I suppose I could add test solely to ensure that if it fails, it does not fail with the ELOOP message, but that seems way too weak. >From 57dd06703cb89ba53d05af95c11e89a2ca51af5c Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyer...@redhat.com> Date: Tue, 4 Sep 2012 14:40:11 +0200 Subject: [PATCH 1/2] rm: avoid bogus diagnostic for a slash-decorated symlink-to-dir These commands would evoke an invalid diagnostic: $ mkdir d && ln -s d s && env rm -r s/ rm: cannot remove 's': Too many levels of symbolic links remove.c was stripping trailing slashes from "s/" before passing the name to "rm". But a trailing slash may change the semantics, and thus should not be stripped. * src/remove.c (rm_fts): Do not strip trailing slashes. * tests/rm/v-slash.sh: Adapt to new expected output. * gnulib: Update to latest, for an improved fts.c that merely normalizes trailing slashes. Reported by Paul Eggert in discussion of http://bugs.gnu.org/12339 --- NEWS | 4 ++++ gnulib | 2 +- src/remove.c | 3 --- tests/rm/v-slash.sh | 2 +- 4 files changed, 6 insertions(+), 5 deletions(-) diff --git a/NEWS b/NEWS index f3874fd..a848ffe 100644 --- a/NEWS +++ b/NEWS @@ -19,6 +19,10 @@ GNU coreutils NEWS -*- outline -*- than ignoring the -d option and failing with an 'Is a directory' error. [bug introduced in coreutils-8.19, with the addition of --dir (-d)] + rm -r S/ (where S is a symlink-to-directory) no longer gives the invalid + "Too many levels of symbolic links" diagnostic. + [bug introduced in coreutils-8.6] + ** Improvements stat and tail work better with ZFS. stat -f --format=%T now reports the diff --git a/gnulib b/gnulib index 68f693f..3a9002d 160000 --- a/gnulib +++ b/gnulib @@ -1 +1 @@ -Subproject commit 68f693ff1db33bf24695f0f42c62e7801966fd06 +Subproject commit 3a9002d3cc63da7110f133b1040d2d2b0aad8305 diff --git a/src/remove.c b/src/remove.c index 69faae6..847a5cc 100644 --- a/src/remove.c +++ b/src/remove.c @@ -433,9 +433,6 @@ rm_fts (FTS *fts, FTSENT *ent, struct rm_options const *x) /* Perform checks that can apply only for command-line arguments. */ if (ent->fts_level == FTS_ROOTLEVEL) { - if (strip_trailing_slashes (ent->fts_path)) - ent->fts_pathlen = strlen (ent->fts_path); - /* If the basename of a command line argument is "." or "..", diagnose it and do nothing more with that argument. */ if (dot_or_dotdot (last_component (ent->fts_accpath))) diff --git a/tests/rm/v-slash.sh b/tests/rm/v-slash.sh index 504f4ff..ec77bd0 100755 --- a/tests/rm/v-slash.sh +++ b/tests/rm/v-slash.sh @@ -26,7 +26,7 @@ touch a/x || framework_failure_ rm --verbose -r a/// > out || fail=1 cat <<\EOF > exp || fail=1 removed 'a/x' -removed directory: 'a' +removed directory: 'a/' EOF compare exp out || fail=1 -- 1.7.12.176.g3fc0e4c >From eda8c84778bc72192592b79724466946ca7def97 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyer...@redhat.com> Date: Wed, 5 Sep 2012 16:48:50 +0200 Subject: [PATCH 2/2] rm: be more careful when using a replacement errno value * src/remove.c (excise): Tighten the test for when we defer to an old errno value: instead of relying solely on an FTS_DNR (unreadable directory) failure, also test current and replacement errno values. This change would also have solved the problem addressed by commit v8.19-106-g57dd067. For more info, see http://bugs.gnu.org/12339#113 --- gnulib | 2 +- src/remove.c | 12 +++++++----- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/gnulib b/gnulib index 3a9002d..68f693f 160000 --- a/gnulib +++ b/gnulib @@ -1 +1 @@ -Subproject commit 3a9002d3cc63da7110f133b1040d2d2b0aad8305 +Subproject commit 68f693ff1db33bf24695f0f42c62e7801966fd06 diff --git a/src/remove.c b/src/remove.c index 847a5cc..0c25462 100644 --- a/src/remove.c +++ b/src/remove.c @@ -392,11 +392,13 @@ excise (FTS *fts, FTSENT *ent, struct rm_options const *x, bool is_dir) if (ignorable_missing (x, errno)) return RM_OK; - /* When failing to rmdir an unreadable directory, the typical - errno value is EISDIR, but that is not as useful to the user - as the errno value from the failed open (probably EPERM). - Use the earlier, more descriptive errno value. */ - if (ent->fts_info == FTS_DNR) + /* When failing to rmdir an unreadable directory, the typical errno value + is EISDIR or ENOTDIR, but that would be meaningless in a diagnostic. + When that happens and the errno value from the failed open is EPERM + or EACCES, use the earlier, more descriptive errno value. */ + if (ent->fts_info == FTS_DNR + && (errno == ENOTEMPTY || errno == EISDIR || errno == ENOTDIR) + && (ent->fts_errno == EPERM || ent->fts_errno == EACCES)) errno = ent->fts_errno; error (0, errno, _("cannot remove %s"), quote (ent->fts_path)); mark_ancestor_dirs (ent); -- 1.7.12.176.g3fc0e4c