[PATCH] D121423: [VFS] OverlayFileSystem should consistently use the last-added FS first

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith added a comment.

I've thought of another concern: the "recursively call CWD" behaviour can result in CWD being called multiple times on a filesystem instance.

  overlay:
    base:
      /a/a/c
    overlay:
      base:
        /a/a/c
  operations:
    cd /
    cd a
    pwd   # /a
    cat c # cat /a/a/c

I think that has changed subtly with this patch:

- Before, `pwd` would give `/a/a`
- Now, `pwd` would give `/a`.

Another case:

  overlay:
    base:
      /a/b/c
    overlay:
      base:
        /a/b/c
  operations:
    cd /
    cd a
    pwd     # /a
    cat b/c # fail

- Before, `cd a` would have failed (but a subsequent `pwd` would give `/a`)
- Now it'll succeed

I think that's better? but only incidentally, I think, not by design.

> To alleviate the "getting stuck" maybe we could find the first setCWD that succeeds, getCWD, and then re-run setCWD on all the rest to that CWD. Thoughts?

That might work. Or, `getCWD()` might return `/bad/path/../..` because it doesn't know how to remove the `..`.

----

Stepping back, I'm not sure it's sound to be calling `cd` on the underlying FS. I think this is what led to `RedirectingFS` maintaining its own CWD and using absolute paths for access.



================
Comment at: llvm/lib/Support/VirtualFileSystem.cpp:468
+      Success = true;
+      CWD = *FS->getCurrentWorkingDirectory();
+    }
----------------
bnbarham wrote:
> dexonsmith wrote:
> > I think this will be easier to reason about if `CWD` is only updated on the first success inside a given call to `OverlayFileSystem::setCurrentWorkingDirectory`. (Note that different VFSes could be canonicalizing relative paths differently.)
> > 
> > I also wonder if, instead, we can just keep track of which FS currently has the canonical CWD:
> > ```
> > lang=c++
> > uint32_t IndexForCWD = 0; // Starts as BaseFS.
> > ```
> > Calls to `getCurrentWorkingDirectory()` would forward to `overlays_begin()[IndexForCWD].getCurrentWorkingDirectory()`. This way, the overlay inherits the CWD canonicalization of the first working FS (usually, `BaseFS`).
> Updating multiple times was definitely unintentional. I did consider keeping track of the first working FS. I decided against it since theoretically you could change the CWD of one of the underlying FS if you had a reference to it. That is:
> ```
> OverlayFS->setCurrentWorkingDirectory("/foo");
> ContainedFS->setCurrentWorkingDirectory("/bar");
> OverlayFS->getCurrentWorkingDirectory(); // Should this be /foo or /bar?
> ```
> 
> Also note that `overlays_begin()` is the reverse order, ie. `BaseFS` is the last FS used (not the first). I'd think it'd actually be fairly uncommon for it to be the first working FS.
Aha, right, `BaseFS` is last.

> I decided against it since theoretically you could change the CWD of one of the underlying FS if you had a reference to it.

Maybe a side-channel modification of the CWD of the underlying OS is not something that overlay-fs should have to reason about.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121423/new/

https://reviews.llvm.org/D121423