[lldb] Modernize PseudoTerminal::OpenSecondary

Author: labath

lldb/include/lldb/Host/PseudoTerminal.h

@@ -148,19 +148,10 @@ class PseudoTerminal {
   /// \param[in] oflag
   ///     Flags to use when calling \c open(\a oflag).
   ///
-  /// \param[out] error_str
-  ///     An pointer to an error that can describe any errors that
-  ///     occur. This can be NULL if no error status is desired.
-  ///
-  /// \return
-  ///     \b true when the primary files descriptor is
-  ///         successfully opened.
-  ///     \b false if anything goes wrong.
-  ///
   /// \see PseudoTerminal::OpenFirstAvailablePrimary() @see
   /// PseudoTerminal::GetSecondaryFileDescriptor() @see
   /// PseudoTerminal::ReleaseSecondaryFileDescriptor()
-  bool OpenSecondary(int oflag, char *error_str, size_t error_len);
+  llvm::Error OpenSecondary(int oflag);
 
   /// Release the primary file descriptor.
   ///

lldb/source/Host/common/PseudoTerminal.cpp

@@ -94,31 +94,16 @@ llvm::Error PseudoTerminal::OpenFirstAvailablePrimary(int oflag) {
 #endif
 }
 
-// Open the secondary pseudo terminal for the current primary pseudo terminal. A
-// primary pseudo terminal should already be valid prior to calling this
-// function (see OpenFirstAvailablePrimary()). The file descriptor is stored
-// this object's member variables and can be accessed via the
-// GetSecondaryFileDescriptor(), or released using the
-// ReleaseSecondaryFileDescriptor() member function.
-//
-// RETURNS:
-//  True when successful, false indicating an error occurred.
-bool PseudoTerminal::OpenSecondary(int oflag, char *error_str,
-                                   size_t error_len) {
-  if (error_str)
-    error_str[0] = '\0';
-
+llvm::Error PseudoTerminal::OpenSecondary(int oflag) {
   CloseSecondaryFileDescriptor();
 
   std::string name = GetSecondaryName();
   m_secondary_fd = llvm::sys::RetryAfterSignal(-1, ::open, name.c_str(), oflag);
-  if (m_secondary_fd < 0) {
-    if (error_str)
-      ErrnoToStr(error_str, error_len);
-    return false;
-  }
+  if (m_secondary_fd >= 0)
+    return llvm::Error::success();
 
-  return true;
+  return llvm::errorCodeToError(
+      std::error_code(errno, std::generic_category()));
 }
 
 std::string PseudoTerminal::GetSecondaryName() const {
@@ -174,35 +159,36 @@ lldb::pid_t PseudoTerminal::Fork(char *error_str, size_t error_len) {
     // Child Process
     ::setsid();
 
-    if (OpenSecondary(O_RDWR, error_str, error_len)) {
-      // Successfully opened secondary
+    if (llvm::Error Err = OpenSecondary(O_RDWR)) {
+      snprintf(error_str, error_len, "%s", toString(std::move(Err)).c_str());
+      return LLDB_INVALID_PROCESS_ID;
+    }
 
-      // Primary FD should have O_CLOEXEC set, but let's close it just in
-      // case...
-      ClosePrimaryFileDescriptor();
+    // Primary FD should have O_CLOEXEC set, but let's close it just in
+    // case...
+    ClosePrimaryFileDescriptor();
 
 #if defined(TIOCSCTTY)
-      // Acquire the controlling terminal
-      if (::ioctl(m_secondary_fd, TIOCSCTTY, (char *)0) < 0) {
-        if (error_str)
-          ErrnoToStr(error_str, error_len);
-      }
+    // Acquire the controlling terminal
+    if (::ioctl(m_secondary_fd, TIOCSCTTY, (char *)0) < 0) {
+      if (error_str)
+        ErrnoToStr(error_str, error_len);
+    }
 #endif
-      // Duplicate all stdio file descriptors to the secondary pseudo terminal
-      if (::dup2(m_secondary_fd, STDIN_FILENO) != STDIN_FILENO) {
-        if (error_str && !error_str[0])
-          ErrnoToStr(error_str, error_len);
-      }
-
-      if (::dup2(m_secondary_fd, STDOUT_FILENO) != STDOUT_FILENO) {
-        if (error_str && !error_str[0])
-          ErrnoToStr(error_str, error_len);
-      }
-
-      if (::dup2(m_secondary_fd, STDERR_FILENO) != STDERR_FILENO) {
-        if (error_str && !error_str[0])
-          ErrnoToStr(error_str, error_len);
-      }
+    // Duplicate all stdio file descriptors to the secondary pseudo terminal
+    if (::dup2(m_secondary_fd, STDIN_FILENO) != STDIN_FILENO) {
+      if (error_str && !error_str[0])
+        ErrnoToStr(error_str, error_len);
+    }
+
+    if (::dup2(m_secondary_fd, STDOUT_FILENO) != STDOUT_FILENO) {
+      if (error_str && !error_str[0])
+        ErrnoToStr(error_str, error_len);
+    }
+
+    if (::dup2(m_secondary_fd, STDERR_FILENO) != STDERR_FILENO) {
+      if (error_str && !error_str[0])
+        ErrnoToStr(error_str, error_len);
     }
   } else {
     // Parent Process

lldb/source/Plugins/Platform/MacOSX/objcxx/PlatformiOSSimulatorCoreSimulatorSupport.mm

@@ -411,15 +411,12 @@ static Status HandleFileAction(ProcessLaunchInfo &launch_info,
           if (file_spec == secondary_spec) {
             int secondary_fd =
                 launch_info.GetPTY().GetSecondaryFileDescriptor();
-            if (secondary_fd == PseudoTerminal::invalid_fd)
-              secondary_fd =
-                  launch_info.GetPTY().OpenSecondary(O_RDWR, nullptr, 0);
             if (secondary_fd == PseudoTerminal::invalid_fd) {
-              std::string secondary_path = secondary_spec.GetPath();
-              error.SetErrorStringWithFormat(
-                  "unable to open secondary pty '%s'", secondary_path.c_str());
-              return error; // Failure
+              if (llvm::Error Err = launch_info.GetPTY().OpenSecondary(O_RDWR))
+                return Status(std::move(Err));
             }
+            secondary_fd = launch_info.GetPTY().GetSecondaryFileDescriptor();
+            assert(secondary_fd != PseudoTerminal::invalid_fd);
             [options setValue:[NSNumber numberWithInteger:secondary_fd]
                        forKey:key];
             return error; // Success

lldb/unittests/Editline/EditlineTest.cpp

@@ -107,12 +107,7 @@ EditlineAdapter::EditlineAdapter()
   _pty_master_fd = _pty.GetPrimaryFileDescriptor();
 
   // Open the corresponding secondary pty.
-  char error_string[256];
-  error_string[0] = '\0';
-  if (!_pty.OpenSecondary(O_RDWR, error_string, sizeof(error_string))) {
-    fprintf(stderr, "failed to open secondary pty: '%s'\n", error_string);
-    return;
-  }
+  EXPECT_THAT_ERROR(_pty.OpenSecondary(O_RDWR), llvm::Succeeded());
   _pty_secondary_fd = _pty.GetSecondaryFileDescriptor();
 
   _el_secondary_file.reset(new FilePointer(fdopen(_pty_secondary_fd, "rw")));

lldb/unittests/Host/MainLoopTest.cpp

@@ -103,7 +103,7 @@ TEST_F(MainLoopTest, DetectsEOF) {
 
   PseudoTerminal term;
   ASSERT_THAT_ERROR(term.OpenFirstAvailablePrimary(O_RDWR), llvm::Succeeded());
-  ASSERT_TRUE(term.OpenSecondary(O_RDWR | O_NOCTTY, nullptr, 0));
+  ASSERT_THAT_ERROR(term.OpenSecondary(O_RDWR | O_NOCTTY), llvm::Succeeded());
   auto conn = std::make_unique<ConnectionFileDescriptor>(
       term.ReleasePrimaryFileDescriptor(), true);