[PATCH] D52648: [Support] Listing a directory containing dangling symlinks is not an error.

[Sam McCall via Phabricator via llvm-commits]

sammccall added a comment.

Thanks for looking at this, good questions! I could have given more background.

Are there places you think better doc comments would help?



================
Comment at: include/llvm/Support/FileSystem.h:1259
     else {
-      ErrorOr<basic_file_status> status = State->Stack.top()->status();
-      if (status && is_directory(*status)) {
+      file_type type = State->Stack.top()->type();
+      if (type == file_type::symlink_file && Follow) {
----------------
vsk wrote:
> I'm struggling to understand how work is saved here. Do you mind explaining in some more detail? Specifically, I'm not sure why determining `file_type` is cheaper than getting a `basic_file_status`. Don't both stat()?
since r342232 `file_type` is often better:
 - on linux/bsd/mac with most FSs, you get `d_type` as part of the `struct dirent`, so `file_type` is free when traversing, while `basic_file_status` costs a stat()
 - on solaris (and others?), `d_type` is not present. On some filesystems (e.g. certain configurations of XFS), `d_type` is "unknown". In these cases `type()` falls back to `status()`, so this change is a no-op.
 - on windows, both are free (you get `basic_file_status` as part of the directory listing)


================
Comment at: tools/llvm-cov/CodeCoverage.cpp:218
 
-      if (EC) {
-        warning(EC.message(), F->path());
+      auto Status = F->status();
+      if (!Status) {
----------------
vsk wrote:
> Do all clients of the directory iterator need to check status(), or is llvm-cov the weird one?
I don't think `llvm-cov` particularly needs to check or warn here, I'm just trying to preserve the existing behavior as I'm not so familiar with it.

The original bug ("llvm-cov hangs") was a problem in `recursive_directory_iterator`, not `llvm-cov` itself. When it hit a dangling symlink (or anything that failed to stat), `increment()` would become a no-op and return no error code.

D44960 fixed this by having the iterator report the `stat()` failure as an `error_code`, and continue to advance. At this point the natural loop works fine. I believe the changes to `llvm-cov` just add a warning, and prevent the dangling symlink itself from being added as a collected path. (The latter shouldn't matter for most programs, but I preserved it to be safe).

Reporting the stat() error code had a kind of logic: `stat()` produces `ErrorOr<file_status>` and `directory_iterator` yielded (`basic_file_status`, `error_code`) so it seems to fit. But having `directory_iterator` return something closer to (`path`, `type`) and not error on dangling links is a better match for most OSes.

I don't think this changed API is error-prone: for a dangling symlink you get a `directory_entry` with just a path, and to do anything you must open/stat it etc which can fail. So it gets handled in the same way the application handles files you can see but can't read for any other reason, which seems appropriate.


Repository:
  rL LLVM

https://reviews.llvm.org/D52648