Re: problem with readdir in Darwin-6.5 and hfs?

[Jim Meyering]

Paul Eggert <address@hidden> wrote:
> Jim Meyering <address@hidden> writes:
>
>> I just went to remove a directory on a local `hfs' file system from a
>> powerpc-apple-darwin6.5 system and was surprised to see the `rm -rf' fail.
>
> Wow, a local filesystem?  FreeBSD bug kern/26142 says only that the
> bug occurs with an NFS filesystem.

Yep.  `df -T -h' says it's of type `hfs' and with capacity of 38G.

> http://www.freebsd.org/cgi/query-pr.cgi?pr=kern/26142
>
>
>> Here's a demo:
>
> Perhaps you should submit your demo to the FreeBSD folks, as a
> followup to that bug.  They might raise the priority of fixing
> kern/26142.  That bug was filed over two years ago, with Severity
> "serious" and Priority "medium".  Your demo might talk them into
> raising the priority.

Thanks.  I've put it on my list.

>> I don't want yet another configure-time test for broken readdir
>> (SunOS had a problem very much like this long ago, but I obviated
>> the work-around code when I rewrote rm).
>
> I don't see any other alternative, other than something even worse
> like "#ifdef FreeBSD".
>
>> The work-around here is probably to call rewinddir from a readdir
>> wrapper to be used only on deficient systems.

Now, I am nearly convinced that it's not possible to do this with a wrapper.
Calling rewinddir unconditionally seems like it would lead to an infinite
loop when rm does not remove all entries in a directory (e.g., user
declined, unlink failed, etc.).

So I'm resigned to modifying remove.c itself.
Tentative patch below.

The configure-time test in m4/readdir.m4
(last included in fileutils-4.1.11) does almost
exactly what is needed here.

> 26142 gives the workaround "Call rewinddir after each unlink call".
> Is that what you mean?  Or were you just going to call rewinddir after
> readdir returns NULL?

That's what I meant, but I think it would have been insufficient.
The patch below does this:

        Work around nasty readdir bug with Darwin6.5 and hfs file system.
        * src/remove.c (IF_READDIR_NEEDS_REWINDDIR): Define.
        [! HAVE_WORKING_READDIR] (remove_cwd_entries): If readdir has just
        returned NULL and there has been at least one successful unlink or
        rmdir call since the opendir or previous rewinddir, then call
        rewinddir and reiterate the loop.

>> But then do we do the extra rewinddir unconditionally, or add
>> bookkeeping so we incur the cost only if we've removed more than
>> some minimum number of entries in a given directory.
>
> I would avoid the bookkeeping.  Just call rewinddir before each
> readdir.  Safe and simple.  (Only on buggy systems, of course.)

Index: remove.c
===================================================================
RCS file: /fetish/cu/src/remove.c,v
retrieving revision 1.86
diff -u -p -u -p -r1.86 remove.c
--- remove.c    2 May 2003 10:44:32 -0000       1.86
+++ remove.c    2 May 2003 12:35:11 -0000
@@ -55,6 +55,12 @@
 # define ROOT_CAN_UNLINK_DIRS 1
 #endif
 
+#if HAVE_WORKING_READDIR
+# define IF_READDIR_NEEDS_REWINDDIR(Code) /* empty */
+#else
+# define IF_READDIR_NEEDS_REWINDDIR(Code) Code
+#endif
+
 enum Ternary
   {
     T_UNKNOWN = 2,
@@ -804,6 +810,7 @@ remove_cwd_entries (Dirstack_state *ds, 
   DIR *dirp = opendir (".");
   struct AD_ent *top = AD_stack_top (ds);
   enum RM_status status = top->status;
+  IF_READDIR_NEEDS_REWINDDIR (int performed_unlink_or_rmdir = 0);
 
   assert (VALID_STATUS (status));
   *subdir = NULL;
@@ -839,6 +846,19 @@ remove_cwd_entries (Dirstack_state *ds, 
              /* Arrange to give a diagnostic after exiting this loop.  */
              dirp = NULL;
            }
+         else
+           {
+#if ! HAVE_WORKING_READDIR
+             /* On buggy systems, call rewinddir if we've called unlink
+                or rmdir since the opendir or a previous rewinddir.  */
+             if (performed_unlink_or_rmdir)
+               {
+                 rewinddir (dirp);
+                 performed_unlink_or_rmdir = 0;
+                 continue;
+               }
+#endif
+           }
          break;
        }
 
@@ -856,7 +876,9 @@ remove_cwd_entries (Dirstack_state *ds, 
       switch (tmp_status)
        {
        case RM_OK:
-         /* do nothing */
+         /* On buggy systems, record the fact that we've just
+            removed a directory entry.  */
+         IF_READDIR_NEEDS_REWINDDIR (performed_unlink_or_rmdir = 1);
          break;
 
        case RM_ERROR: