[Lldb-commits] [PATCH] D54020: [FileSystem] Open File instances through the FileSystem.

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.

In https://reviews.llvm.org/D54020#1285539, @JDevlieghere wrote:

> In https://reviews.llvm.org/D54020#1285201, @labath wrote:
>
> > I am not sure what do you mean by "translating paths" in the VFS. If you mean something like "return a `FILE *` for `/whatever/my_reproducer/vfs/bin/ls` when I ask for `/bin/ls`", then I think that's a good idea, as it's most likely to work with all of our existing code (e.g. mmapping). Although, in this case, i am not sure how much benefit will the llvm VFS bring to the game, as most of the operations could then be implemented by plainly prepending some  prefix to a given path.
>
>
> Let me try to explain this better. This is mostly a question about what kind of API the VFS (which lives in LLVM) provides to deal with files in lldb (i.e. through `FILE*` and file descriptors).
>
> The first possibility is providing a method in the VFS that takes a "virtual" path and returns the "underlying" path. Something like  `Optional<path> vfs::getUnderlyingPath(path)`. This doesn't just have to be a prefix thing but you are right that it's mostly going to be just that. The problem is that this method wouldn't work for virtual file systems that don't have files on disk. Hence the lack of generality.


Ok, I think we're on the same page here. What I was wondering is that given this lack of generality (this operation would only be supported on `DiskBackedFilesystem`, and not all other kinds of file systems), whether it is not better to just do a custom solution instead of going through the VFS layer (thereby removing one layer, since now we have `lldb_private::FileSystem` sitting on top of `llvm::VirtualFileSystem`, sitting on top of the real thing). E.g., if we know we can restrict ourselves to the case where the on disk layout matches the real system, then all filesystem operations can be implemented very trivially via something like:

  auto virtual_op(const Twine &Path, ...) { return real_op(Prefix+Path, ...); }

I am not sure whether this is a good idea (there's definitely a case to be made for reusing existing infrastructure), but it is something to think about.

>> Also, be aware that there are some places where we open `raw_ostream`s directly (`Debugger::EnableLog` comes to mind). I guess those will need to go through the `FileSystem` too...
> 
> Yup, we need to audit all file access. This particular example actually goes through file so it should be covered by this change.

You're right, I should have looked at this more closely. It should be fine (assuming we can have real fds for the virtual files).



================
Comment at: source/Host/common/FileSystem.cpp:253
+static int GetOpenFlags(uint32_t options) {
+  const bool read = options & File::OpenOptions::eOpenOptionRead;
+  const bool write = options & File::OpenOptions::eOpenOptionWrite;
----------------
JDevlieghere wrote:
> labath wrote:
> > `File::eOpenOptionRead` should be sufficient, no?
> Sorry, I don't know what you mean?
I meant to remove the superfluous `OpenOptions` qualification, As this is a non-class enum, it is not necessary, and it doesn't help clarity, since the enum value already contains the type name. None of the existing code uses this kind of qualification.


https://reviews.llvm.org/D54020