[Lldb-commits] [PATCH] D78712: [lldb/Host] Improve error messages on unowned read files

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I'm not very fond of this reverse engineering of the checks that os performs when opening a file (this is not the only place doing it). Could we just make sure that the operation for opening a file returns a reasonable error message, and then do something like

  Status /*or Error, or Expected...*/ error = File::Open(core);
  if (error)
    SetErrorStringWithFormatv("Cannot open {0}: {1}.", core, error);

?

If done right, that should produce messages like:

  Cannot open /my/core.file: File does not exist.
  Cannot open /my/core.file: Permission denied.
  Cannot open /my/core.file: Is a directory.
  ...

It may not as "user friendly" as "file is not readable by the current user", but it will at least be correct (it's not a problem to come up with situations where this file will be readable by the "current user", but we will not print this error message, and vice-versa), and it will also match what other tools are likely to print. Plus it will be composable, and involve less code at each call site.



================
Comment at: lldb/source/Host/common/FileSystem.cpp:503
+bool FileSystem::ReadableByCurrentUser(const llvm::Twine &file_path) const {
+  const FileSpec file_spec(file_path.getSingleStringRef());
+  return ReadableByCurrentUser(file_spec);
----------------
This is not the correct way to work with `Twine`s. If you want to try to be efficient, you should call `toStringRef` (there are examples of how to do that throughout llvm). If you don't care about that, you can call `str()`.


================
Comment at: lldb/test/API/commands/target/basic/TestTargetCommand.py:343
+    def test_target_create_unowned_core_file(self):
+        self.expect("target create -c ~root", error=True,
+                    substrs=["core file '", "' is not readable"])
----------------
mib wrote:
> Currently, I try to open the root user home directory, since I sure it's not readable by other users on UNIX systems.
> 
> I went this route, because doing a `os.chown` requires privileges, and the test suites doesn't run in a privileged mode.
> 
> I'm open to suggestions on how I could test this on different platforms.
if you set the mode of a file to `0000`, you won't be able to open it even if you are still its owner.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78712