On 31/01/2020 17:51, Pádraig Brady wrote:
On 31/01/2020 01:46, Matthew Pfeiffer wrote:
'rmdir --ignore-fail-on-non-empty' would not report errors on non-empty
directories that fail for a different reason.
---
src/rmdir.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/src/rmdir.c b/src/rmdir.c
index c9f417957..7b253ab0d 100644
--- a/src/rmdir.c
+++ b/src/rmdir.c
@@ -133,18 +133,19 @@ remove_parents (char *dir)
prog_fprintf (stdout, _("removing directory, %s"), quoteaf (dir));
ok = (rmdir (dir) == 0);
+ int rmdir_errno = errno;
if (!ok)
{
/* Stop quietly if --ignore-fail-on-non-empty. */
- if (ignorable_failure (errno, dir))
+ if (ignorable_failure (rmdir_errno, dir))
{
ok = true;
}
else
{
/* Barring race conditions, DIR is expected to be a directory.
*/
- error (0, errno, _("failed to remove directory %s"),
+ error (0, rmdir_errno, _("failed to remove directory %s"),
quoteaf (dir));
}
break;
@@ -233,12 +234,13 @@ main (int argc, char **argv)
if (rmdir (dir) != 0)
{
- if (ignorable_failure (errno, dir))
+ int rmdir_errno = errno;
+ if (ignorable_failure (rmdir_errno, dir))
continue;
/* Here, the diagnostic is less precise, since we have no idea
whether DIR is a directory. */
- error (0, errno, _("failed to remove %s"), quoteaf (dir));
+ error (0, rmdir_errno, _("failed to remove %s"), quoteaf (dir));
ok = false;
}
else if (remove_empty_parents)
This looks like a regression introduced in v6.10-21-ged5c4e7
For reference, this was originally discussed at:
https://lists.gnu.org/archive/html/bug-coreutils/2008-01/msg00283.html
I.E. is_empty_dir() is globbering errno, and thus
a non specific and non terminating warning is output,
rather than a specific error, and exit.
Actually I think the key issue is not errno handling,
but a logic error fixed with:
@@ -102,7 +102,7 @@ ignorable_failure (int error_number, char const *dir)
return (ignore_fail_on_non_empty
&& (errno_rmdir_non_empty (error_number)
|| (errno_may_be_empty (error_number)
- && is_empty_dir (AT_FDCWD, dir))));
+ && ! is_empty_dir (AT_FDCWD, dir))));
Attached is a full patch to address these issues.
cheers,
Pádraig
>From 3b379d77ec342b66a36d4f39435dccc121ca86eb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <p...@draigbrady.com>
Date: Thu, 30 Jan 2020 20:46:40 -0500
Subject: [PATCH] rmdir: fix --ignore-fail-on-non-empty with permissions errors
Since 6.11 `rmdir --ignore-fail-on-non-empty` had reversed
the failure status for directories that failed to be removed
for permissions reasons. I.E. it would have returned a
failure status for such non empty dirs, and vice versa.
* src/rmdir.c (errno_may_be_non_empty): Rename from the
more confusing errno_may_be_empty(), and remove the EEXIST
case (specific to Solaris), which is moot here since
handled in errno_rmdir_non_empty().
(ignorable_failure): Fix the logic error so that
_non_ empty dirs are deemed to have ignorable failures.
(main): Fix clobbering of errno by is_empty_dir().
(remove_parents): Likewise.
* tests/rmdir/ignore.sh: Add a test case.
* THANKS.in: Add reporter who fixed the errno handling.
* NEWS: Mention the bug fix.
Fixes https://bugs.gnu.org/39364
---
NEWS | 5 +++++
THANKS.in | 1 +
src/rmdir.c | 21 +++++++++++----------
tests/rmdir/ignore.sh | 12 ++++++++++++
4 files changed, 29 insertions(+), 10 deletions(-)
diff --git a/NEWS b/NEWS
index 5231b84ac..8a349634e 100644
--- a/NEWS
+++ b/NEWS
@@ -29,6 +29,11 @@ GNU coreutils NEWS -*- outline -*-
(like Solaris 10 and Solaris 11).
[bug introduced in coreutils-8.31]
+ rmdir --ignore-fail-on-non-empty now works correctly for directories
+ that fail to be removed due to permission issues. Previously the exit status
+ was reversed, failing for non empty and succeeding for empty directories.
+ [bug introduced in coreutils-6.11]
+
'shuf -r -n 0 file' no longer mistakenly reads from standard input.
[bug introduced with the --repeat feature in coreutils-8.22]
diff --git a/THANKS.in b/THANKS.in
index 23b089754..a667197ed 100644
--- a/THANKS.in
+++ b/THANKS.in
@@ -420,6 +420,7 @@ Matt Swift sw...@alum.mit.edu
Matthew Arnison maf...@cat.org.au
Matthew Braun matt...@ans.net
Matthew Clarke matthew_cla...@mindlink.bc.ca
+Matthew Pfeiffer spferi...@gmail.com
Matthew M. Boedicker matth...@boedicker.org
Matthew S. Levine mslev...@theory.lcs.mit.edu
Matthew Smith ma...@bluesguitar.org
diff --git a/src/rmdir.c b/src/rmdir.c
index c9f417957..a2f0f0813 100644
--- a/src/rmdir.c
+++ b/src/rmdir.c
@@ -77,16 +77,15 @@ errno_rmdir_non_empty (int error_number)
}
/* Return true if when rmdir fails with errno == ERROR_NUMBER
- the directory may be empty. */
+ the directory may be non empty. */
static bool
-errno_may_be_empty (int error_number)
+errno_may_be_non_empty (int error_number)
{
switch (error_number)
{
case EACCES:
case EPERM:
case EROFS:
- case EEXIST:
case EBUSY:
return true;
default:
@@ -101,8 +100,8 @@ ignorable_failure (int error_number, char const *dir)
{
return (ignore_fail_on_non_empty
&& (errno_rmdir_non_empty (error_number)
- || (errno_may_be_empty (error_number)
- && is_empty_dir (AT_FDCWD, dir))));
+ || (errno_may_be_non_empty (error_number)
+ && ! is_empty_dir (AT_FDCWD, dir))));
}
/* Remove any empty parent directories of DIR.
@@ -133,18 +132,19 @@ remove_parents (char *dir)
prog_fprintf (stdout, _("removing directory, %s"), quoteaf (dir));
ok = (rmdir (dir) == 0);
+ int rmdir_errno = errno;
- if (!ok)
+ if (! ok)
{
/* Stop quietly if --ignore-fail-on-non-empty. */
- if (ignorable_failure (errno, dir))
+ if (ignorable_failure (rmdir_errno, dir))
{
ok = true;
}
else
{
/* Barring race conditions, DIR is expected to be a directory. */
- error (0, errno, _("failed to remove directory %s"),
+ error (0, rmdir_errno, _("failed to remove directory %s"),
quoteaf (dir));
}
break;
@@ -233,12 +233,13 @@ main (int argc, char **argv)
if (rmdir (dir) != 0)
{
- if (ignorable_failure (errno, dir))
+ int rmdir_errno = errno;
+ if (ignorable_failure (rmdir_errno, dir))
continue;
/* Here, the diagnostic is less precise, since we have no idea
whether DIR is a directory. */
- error (0, errno, _("failed to remove %s"), quoteaf (dir));
+ error (0, rmdir_errno, _("failed to remove %s"), quoteaf (dir));
ok = false;
}
else if (remove_empty_parents)
diff --git a/tests/rmdir/ignore.sh b/tests/rmdir/ignore.sh
index d000c30cb..0d2be25ed 100755
--- a/tests/rmdir/ignore.sh
+++ b/tests/rmdir/ignore.sh
@@ -29,4 +29,16 @@ test -d "$cwd/a/x" || fail=1
test -d "$cwd/a/b" && fail=1
test -d "$cwd/a/b/c" && fail=1
+# Ensure that with --ignore-fail-on-non-empty, we still fail, e.g., for EPERM.
+# Between 6.11 and 8.31, the following rmdir would mistakenly succeed.
+mkdir -p x/y || framework_failure_
+chmod a-w x || framework_failure_
+returns_ 1 rmdir --ignore-fail-on-non-empty x/y || fail=1
+test -d x/y || fail=1
+# Between 6.11 and 8.31, the following rmdir would mistakenly fail,
+# and also give a non descript error
+touch x/y/z || framework_failure_
+rmdir --ignore-fail-on-non-empty x/y || fail=1
+test -d x/y || fail=1
+
Exit $fail
--
2.24.1
