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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Yes, this is definitely better.



================
Comment at: lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp:187
       if (error.Fail() || !exe_module_sp) {
-        if (FileSystem::Instance().Readable(
-                resolved_module_spec.GetFileSpec())) {
+        if (FileSystem::Instance().Open(resolved_module_spec.GetFileSpec(),
+                                        lldb_private::File::eOpenOptionRead)) {
----------------
JDevlieghere wrote:
> Not your fault, but it seems weird that we're doing this check after using the `resolved_module_spec` and only if it failed.. Shouldn't we try to read it first and stop early if that fails? 
In an ideal world the actual operation which  tries to create the module would return an error and we would use that. That way we wouldn't be accessing the file twice, and creating opportunities for inconsistencies (or races). In that sense, this code is still trying to "reverse engineer" why the earlier operation failed, but I think it is a step forward, as it reduces the amount if reversing.


================
Comment at: lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp:195
           error.SetErrorStringWithFormat(
-              "'%s' is not readable",
+              "'%s' is not readable by the current user",
               resolved_module_spec.GetFileSpec().GetPath().c_str());
----------------
I guess this should be also updated to use the error message from the os(?)


================
Comment at: lldb/test/API/commands/target/basic/TestTargetCommand.py:329
         self.expect("target create -c doesntexist", error=True,
-                    substrs=["core file 'doesntexist' doesn't exist"])
+                    substrs=["Cannot open 'doesntexist': No such file or directory"])
 
----------------
Now that the error message comes from the OS, we might need to adjust the expectation based on that. Just an FYI, in case you get some failure message from some bot (most likely the windows one).


================
Comment at: lldb/test/API/commands/target/basic/TestTargetCommand.py:342
+    @no_debug_info_test
+    def test_target_create_unowned_core_file(self):
+      tf = tempfile.NamedTemporaryFile()
----------------
This is not testing an unowned file anymore, and is probably redudant given the test above it. Given that the new implementation does not do anything special wrt. ownership, I don't think an explicit test for those.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78712