Bug 2838 – std.file.rmdirRecurse fails

The problem seems to be caused by incorrect behaviour of druntime's core.sys.posix.readdir, which fails to populate the dirent's d_type field - it is always set to 0. This causes directories to be identified as files and thus not recursed into.

Created attachment 323 Simple test case The unittest code in std.file will demonstrate the problem, but for what it is worth, here is my test code. I tracked the problem down as far as printing out the values passed to std.file.DirEntry.init, and fd.d_type was always zero when invoked by my test code. I haven't included my hacked version of file.d.

Unable to reproduce on Windows or Linux. Everything deletes fine. I should note that readdir() is a libc function, afaik. It's not a D-specific function. If it's not properly filling the structure it's a system problem. Also note, readdir() is not re-entrant. There's a readdir_r() for that, which phobos is not using. If you are using rmdirRecurse() in a threaded app with other directory reading happening, that may be the cause of your problems. -[Unknown]

I have looked into it some more, and it looks like setting of d_type is only supported on some filesystems. I am using reiserfs, which I guess is a bit unusual, and it could be that. Yes, I just tried it with a vfat filesystem on a flash-stick, and d_type was populated ok. Can I suggest that, since the posix standard makes population of d_type (and others) optional, we do something to handle the case where it is set to 0 (DT_UNKNOWN)? Maybe the easiest thing to do is to modify std.file.DirEntry.init to be something like: void init(string path, dirent *fd) { immutable len = std.c.string.strlen(fd.d_name.ptr); name = std.path.join(path, fd.d_name[0 .. len].idup); d_type = fd.d_type; didstat = false; if (d_type == DT_UNKNOWN) { ensureStatDone; } } or maybe defer it until isdir or isdile are called. ensureStatDone would need to be modified to be something like: void ensureStatDone() { if (didstat) return; enforce(core.sys.posix.sys.stat.stat(toStringz(name), &statbuf) == 0, "Failed to stat file `"~name~"'"); _size = cast(ulong)statbuf.st_size; _creationTime = cast(d_time)statbuf.st_ctime * std.date.TicksPerSecond; _lastAccessTime = cast(d_time)statbuf.st_atime * std.date.TicksPerSecond; _lastWriteTime = cast(d_time)statbuf.st_mtime * std.date.TicksPerSecond; if (d_type == DT_UNKNOWN) { if (S_ISDIR(statbuf.st_mode)) { d_type = DT_DIR; } else if (S_ISREG(statbuf.st_mode)) { d_type = DT_REG; } } didstat = true; } And of course std.file needs to be changed to use readdir_r too. If this approach is acceptable to you, I could have a go at submitting a patch if you like...

Ah, yes, that makes sense. Please feel free to attach a patch. I'm not the one who'd accept it, but I think your approach looks sound. But, beware of convention - curlies, call syntax, etc. - it's best to stay common within a file. -[Unknown]

Created attachment 325 Proposed patch for std.file to handle case of linux filesystems that don't set d_type Submitted a proposed patch to set DirEntry.d_type in the linux version when the filesystem doesn't provide d_type is direntry, such as reiserfs.

I expect that this fix fixes bug# 1635 (which you (Andrei) recently closed as worksforme) and bug #4929. In my patch for bug# 3848 (which also fixes this problem), I added this test to the unittest for dirEntries so that this situation would be caught: foreach (DirEntry e; dirEntries("/usr/share/zoneinfo", SpanMode.depth)) { assert(e.isfile || e.isdir, e.fullname); } I don't know whether or not you think that it would be worth adding such a test, but it it seems to be a test case which reliably catches the problem.