Remove FileSystem::GetFilePermissions and FileSystem::SetFilePermissions

[zturner]

Summary

Use the LLVM functions instead

[labath]

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp

813 ↗

(On Diff #92167)

This doesn't seem right. At the very least I would expect to see a matching change in the client code (I assume llvm constants don't match whatever we have used here (?)).

We don't care much about protocol compatibility, but the apple guys might. If that is the case then we will need some encode/decode functions here.

[zturner]

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp

813 ↗

(On Diff #92167)

AFAICT both LLVM's enumeration and LLDB's enumeration map directly to the unix file permissions bits.

[zturner]

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp

813 ↗

(On Diff #92167)

Here is the implementation of setPermissions in LLVM:

std::error_code setPermissions(const Twine &Path, perms Permissions) {
  SmallString<128> PathStorage;
  StringRef P = Path.toNullTerminatedStringRef(PathStorage);

  if (::chmod(P.begin(), Permissions))
    return std::error_code(errno, std::generic_category());
  return std::error_code();
}

So it's literally the same as LLDB's implementation which I've deleted, which was:

Error FileSystem::SetFilePermissions(const FileSpec &file_spec,
                                     uint32_t file_permissions) {
  Error error;
  if (::chmod(file_spec.GetCString(), file_permissions) != 0)
    error.SetErrorToErrno();
  return error;
}

[labath]

Cool. Nevermind me then.

lldb/trunk/include/lldb/Host/FileSystem.h

 namespace lldb_private {
 class FileSystem {
 public:
   static const char *DEV_NULL;
   static const char *PATH_CONVERSION_ERROR;
 
   static FileSpec::PathSyntax GetNativePathSyntax();
 
-  static Error GetFilePermissions(const FileSpec &file_spec,
-                                  uint32_t &file_permissions);
-  static Error SetFilePermissions(const FileSpec &file_spec,
-                                  uint32_t file_permissions);
   static lldb::user_id_t GetFileSize(const FileSpec &file_spec);
   static bool GetFileExists(const FileSpec &file_spec);
 
   static Error Hardlink(const FileSpec &src, const FileSpec &dst);
   static int GetHardlinkCount(const FileSpec &file_spec);
   static Error Symlink(const FileSpec &src, const FileSpec &dst);
   static Error Readlink(const FileSpec &src, FileSpec &dst);
   static Error Unlink(const FileSpec &file_spec);

lldb/trunk/source/Host/posix/FileSystem.cpp

 using namespace lldb_private;
 
 const char *FileSystem::DEV_NULL = "/dev/null";
 
 FileSpec::PathSyntax FileSystem::GetNativePathSyntax() {
   return FileSpec::ePathSyntaxPosix;
 }
 
-Error FileSystem::GetFilePermissions(const FileSpec &file_spec,
-                                     uint32_t &file_permissions) {
-  Error error;
-  struct stat file_stats;
-  if (::stat(file_spec.GetCString(), &file_stats) == 0) {
-    // The bits in "st_mode" currently match the definitions
-    // for the file mode bits in unix.
-    file_permissions = file_stats.st_mode & (S_IRWXU | S_IRWXG | S_IRWXO);
-  } else {
-    error.SetErrorToErrno();
-  }
-  return error;
-}
-
-Error FileSystem::SetFilePermissions(const FileSpec &file_spec,
-                                     uint32_t file_permissions) {
-  Error error;
-  if (::chmod(file_spec.GetCString(), file_permissions) != 0)
-    error.SetErrorToErrno();
-  return error;
-}
-
 lldb::user_id_t FileSystem::GetFileSize(const FileSpec &file_spec) {
   return file_spec.GetByteSize();
 }
 
 bool FileSystem::GetFileExists(const FileSpec &file_spec) {
   return file_spec.Exists();
 }
 

lldb/trunk/source/Host/windows/FileSystem.cpp

 
 const char *FileSystem::PATH_CONVERSION_ERROR =
     "Error converting path between UTF-8 and native encoding";
 
 FileSpec::PathSyntax FileSystem::GetNativePathSyntax() {
   return FileSpec::ePathSyntaxWindows;
 }
 
-Error FileSystem::GetFilePermissions(const FileSpec &file_spec,
-                                     uint32_t &file_permissions) {
-  Error error;
-  // Beware that Windows's permission model is different from Unix's, and it's
-  // not clear if this API is supposed to check ACLs.  To match the caller's
-  // expectations as closely as possible, we'll use Microsoft's _stat, which
-  // attempts to emulate POSIX stat.  This should be good enough for basic
-  // checks like FileSpec::Readable.
-  struct _stat file_stats;
-  if (::_stat(file_spec.GetCString(), &file_stats) == 0) {
-    // The owner permission bits in "st_mode" currently match the definitions
-    // for the owner file mode bits.
-    file_permissions = file_stats.st_mode & (_S_IREAD | _S_IWRITE | _S_IEXEC);
-  } else {
-    error.SetErrorToErrno();
-  }
-
-  return error;
-}
-
-Error FileSystem::SetFilePermissions(const FileSpec &file_spec,
-                                     uint32_t file_permissions) {
-  Error error;
-  error.SetErrorStringWithFormat("%s is not supported on this host",
-                                 LLVM_PRETTY_FUNCTION);
-  return error;
-}
-
 lldb::user_id_t FileSystem::GetFileSize(const FileSpec &file_spec) {
   return file_spec.GetByteSize();
 }
 
 bool FileSystem::GetFileExists(const FileSpec &file_spec) {
   return file_spec.Exists();
 }
 

lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp

GDBRemoteCommunicationServerCommon::Handle_qPlatform_mkdir(
   return SendErrorResponse(20);
 }
 
 GDBRemoteCommunication::PacketResult
 GDBRemoteCommunicationServerCommon::Handle_qPlatform_chmod(
     StringExtractorGDBRemote &packet) {
   packet.SetFilePos(::strlen("qPlatform_chmod:"));
 
-  mode_t mode = packet.GetHexMaxU32(false, UINT32_MAX);
+  auto perms =
+      static_cast<llvm::sys::fs::perms>(packet.GetHexMaxU32(false, UINT32_MAX));
   if (packet.GetChar() == ',') {
     std::string path;
     packet.GetHexByteString(path);
-    Error error = FileSystem::SetFilePermissions(FileSpec{path, true}, mode);
+    Error error(llvm::sys::fs::setPermissions(path, perms));
 
     StreamGDBRemote response;
     response.Printf("F%u", error.GetError());
 
     return SendPacketNoLock(response.GetString());
   }
   return SendErrorResponse(19);
 }

lldb/trunk/source/Target/Platform.cpp

error.SetErrorStringWithFormat("remote platform %s doesn't support %s",
                                    GetPluginName().GetCString(),
                                    LLVM_PRETTY_FUNCTION);
     return error;
   }
 }
 
 Error Platform::GetFilePermissions(const FileSpec &file_spec,
                                    uint32_t &file_permissions) {
-  if (IsHost())
-    return FileSystem::GetFilePermissions(file_spec, file_permissions);
-  else {
+  if (IsHost()) {
+    auto Value = llvm::sys::fs::getPermissions(file_spec.GetPath());
+    if (Value)
+      file_permissions = Value.get();
+    return Error(Value.getError());
+  } else {
     Error error;
     error.SetErrorStringWithFormat("remote platform %s doesn't support %s",
                                    GetPluginName().GetCString(),
                                    LLVM_PRETTY_FUNCTION);
     return error;
   }
 }
 
 Error Platform::SetFilePermissions(const FileSpec &file_spec,
                                    uint32_t file_permissions) {
-  if (IsHost())
-    return FileSystem::SetFilePermissions(file_spec, file_permissions);
-  else {
+  if (IsHost()) {
+    auto Perms = static_cast<llvm::sys::fs::perms>(file_permissions);
+    return llvm::sys::fs::setPermissions(file_spec.GetPath(), Perms);
+  } else {
     Error error;
     error.SetErrorStringWithFormat("remote platform %s doesn't support %s",
                                    GetPluginName().GetCString(),
                                    LLVM_PRETTY_FUNCTION);
     return error;
   }
 }