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