[lldb] [Host] Make Terminal methods return llvm::Error

Differential Revision: https://reviews.llvm.org/D111890

lldb/include/lldb/Host/Terminal.h

@@ -12,9 +12,12 @@
 
 #include "lldb/Host/Config.h"
 #include "lldb/lldb-private.h"
+#include "llvm/Support/Error.h"
 
 namespace lldb_private {
 
+class TerminalState;
+
 class Terminal {
 public:
   Terminal(int fd = -1) : m_fd(fd) {}
@@ -31,12 +34,19 @@ public:
 
   void Clear() { m_fd = -1; }
 
-  bool SetEcho(bool enabled);
+  llvm::Error SetEcho(bool enabled);
 
-  bool SetCanonical(bool enabled);
+  llvm::Error SetCanonical(bool enabled);
 
 protected:
+  struct Data;
+
   int m_fd; // This may or may not be a terminal file descriptor
+
+  llvm::Expected<Data> GetData();
+  llvm::Error SetData(const Data &data);
+
+  friend class TerminalState;
 };
 
 /// \class TerminalState Terminal.h "lldb/Host/Terminal.h"
@@ -45,8 +55,6 @@ protected:
 /// This class can be used to remember the terminal state for a file
 /// descriptor and later restore that state as it originally was.
 class TerminalState {
-  struct Data;
-
 public:
   /// Construct a new instance and optionally save terminal state.
   ///
@@ -125,10 +133,10 @@ protected:
   bool ProcessGroupIsValid() const;
 
   // Member variables
-  Terminal m_tty;               ///< A terminal
-  int m_tflags = -1;            ///< Cached tflags information.
-  std::unique_ptr<Data> m_data; ///< Platform-specific implementation.
-  lldb::pid_t m_process_group = -1; ///< Cached process group information.
+  Terminal m_tty;                         ///< A terminal
+  int m_tflags = -1;                      ///< Cached tflags information.
+  std::unique_ptr<Terminal::Data> m_data; ///< Platform-specific implementation.
+  lldb::pid_t m_process_group = -1;       ///< Cached process group information.
 };
 
 } // namespace lldb_private

lldb/source/Host/common/Terminal.cpp

@@ -21,72 +21,79 @@
 
 using namespace lldb_private;
 
-bool Terminal::IsATerminal() const { return m_fd >= 0 && ::isatty(m_fd); }
-
-bool Terminal::SetEcho(bool enabled) {
-  if (FileDescriptorIsValid()) {
-#if LLDB_ENABLE_TERMIOS
-    if (IsATerminal()) {
-      struct termios fd_termios;
-      if (::tcgetattr(m_fd, &fd_termios) == 0) {
-        bool set_corectly = false;
-        if (enabled) {
-          if (fd_termios.c_lflag & ECHO)
-            set_corectly = true;
-          else
-            fd_termios.c_lflag |= ECHO;
-        } else {
-          if (fd_termios.c_lflag & ECHO)
-            fd_termios.c_lflag &= ~ECHO;
-          else
-            set_corectly = true;
-        }
-
-        if (set_corectly)
-          return true;
-        return ::tcsetattr(m_fd, TCSANOW, &fd_termios) == 0;
-      }
-    }
-#endif // #if LLDB_ENABLE_TERMIOS
-  }
-  return false;
-}
-
-bool Terminal::SetCanonical(bool enabled) {
-  if (FileDescriptorIsValid()) {
-#if LLDB_ENABLE_TERMIOS
-    if (IsATerminal()) {
-      struct termios fd_termios;
-      if (::tcgetattr(m_fd, &fd_termios) == 0) {
-        bool set_corectly = false;
-        if (enabled) {
-          if (fd_termios.c_lflag & ICANON)
-            set_corectly = true;
-          else
-            fd_termios.c_lflag |= ICANON;
-        } else {
-          if (fd_termios.c_lflag & ICANON)
-            fd_termios.c_lflag &= ~ICANON;
-          else
-            set_corectly = true;
-        }
-
-        if (set_corectly)
-          return true;
-        return ::tcsetattr(m_fd, TCSANOW, &fd_termios) == 0;
-      }
-    }
-#endif // #if LLDB_ENABLE_TERMIOS
-  }
-  return false;
-}
-
-struct TerminalState::Data {
+struct Terminal::Data {
 #if LLDB_ENABLE_TERMIOS
   struct termios m_termios; ///< Cached terminal state information.
 #endif
 };
 
+bool Terminal::IsATerminal() const { return m_fd >= 0 && ::isatty(m_fd); }
+
+llvm::Expected<Terminal::Data> Terminal::GetData() {
+  if (!FileDescriptorIsValid())
+    return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                   "invalid fd");
+
+#if LLDB_ENABLE_TERMIOS
+  if (!IsATerminal())
+    return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                   "fd not a terminal");
+
+  Data data;
+  if (::tcgetattr(m_fd, &data.m_termios) != 0)
+    return llvm::createStringError(
+        std::error_code(errno, std::generic_category()),
+        "unable to get teletype attributes");
+  return data;
+#else // !LLDB_ENABLE_TERMIOS
+  return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                 "termios support missing in LLDB");
+#endif // LLDB_ENABLE_TERMIOS
+}
+
+llvm::Error Terminal::SetData(const Terminal::Data &data) {
+#if LLDB_ENABLE_TERMIOS
+  assert(FileDescriptorIsValid());
+  assert(IsATerminal());
+
+  if (::tcsetattr(m_fd, TCSANOW, &data.m_termios) != 0)
+    return llvm::createStringError(
+        std::error_code(errno, std::generic_category()),
+        "unable to set teletype attributes");
+  return llvm::Error::success();
+#else // !LLDB_ENABLE_TERMIOS
+  llvm_unreachable("SetData() should not be called if !LLDB_ENABLE_TERMIOS");
+#endif // LLDB_ENABLE_TERMIOS
+}
+
+llvm::Error Terminal::SetEcho(bool enabled) {
+  llvm::Expected<Data> data = GetData();
+  if (!data)
+    return data.takeError();
+
+#if LLDB_ENABLE_TERMIOS
+  struct termios &fd_termios = data->m_termios;
+  fd_termios.c_lflag &= ~ECHO;
+  if (enabled)
+    fd_termios.c_lflag |= ECHO;
+  return SetData(data.get());
+#endif // LLDB_ENABLE_TERMIOS
+}
+
+llvm::Error Terminal::SetCanonical(bool enabled) {
+  llvm::Expected<Data> data = GetData();
+  if (!data)
+    return data.takeError();
+
+#if LLDB_ENABLE_TERMIOS
+  struct termios &fd_termios = data->m_termios;
+  fd_termios.c_lflag &= ~ICANON;
+  if (enabled)
+    fd_termios.c_lflag |= ICANON;
+  return SetData(data.get());
+#endif // LLDB_ENABLE_TERMIOS
+}
+
 TerminalState::TerminalState(Terminal term, bool save_process_group)
     : m_tty(term) {
   Save(term, save_process_group);
@@ -109,7 +116,7 @@ bool TerminalState::Save(Terminal term, bool save_process_group) {
 #if LLDB_ENABLE_POSIX
     m_tflags = ::fcntl(fd, F_GETFL, 0);
 #if LLDB_ENABLE_TERMIOS
-    std::unique_ptr<Data> new_data{new Data()};
+    std::unique_ptr<Terminal::Data> new_data{new Terminal::Data()};
     if (::tcgetattr(fd, &new_data->m_termios) == 0)
       m_data = std::move(new_data);
 #endif // LLDB_ENABLE_TERMIOS

lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h

@@ -435,8 +435,9 @@ public:
         TerminalState terminal_state(terminal);
 
         if (terminal.IsATerminal()) {
-          terminal.SetCanonical(false);
-          terminal.SetEcho(true);
+          // FIXME: error handling?
+          llvm::consumeError(terminal.SetCanonical(false));
+          llvm::consumeError(terminal.SetEcho(true));
         }
 
         ScriptInterpreterPythonImpl::Locker locker(

lldb/source/Target/Process.cpp

@@ -4349,8 +4349,9 @@ public:
     const int read_fd = m_read_file.GetDescriptor();
     Terminal terminal(read_fd);
     TerminalState terminal_state(terminal, false);
-    terminal.SetCanonical(false);
-    terminal.SetEcho(false);
+    // FIXME: error handling?
+    llvm::consumeError(terminal.SetCanonical(false));
+    llvm::consumeError(terminal.SetEcho(false));
 // FD_ZERO, FD_SET are not supported on windows
 #ifndef _WIN32
     const int pipe_read_fd = m_pipe.GetReadFileDescriptor();

lldb/unittests/Host/posix/TerminalTest.cpp

@@ -51,11 +51,11 @@ TEST_F(TerminalTest, PipeIsNotATerminal) {
 TEST_F(TerminalTest, SetEcho) {
   struct termios terminfo;
 
-  ASSERT_EQ(m_term.SetEcho(true), true);
+  ASSERT_THAT_ERROR(m_term.SetEcho(true), llvm::Succeeded());
   ASSERT_EQ(tcgetattr(m_fd, &terminfo), 0);
   EXPECT_NE(terminfo.c_lflag & ECHO, 0U);
 
-  ASSERT_EQ(m_term.SetEcho(false), true);
+  ASSERT_THAT_ERROR(m_term.SetEcho(false), llvm::Succeeded());
   ASSERT_EQ(tcgetattr(m_fd, &terminfo), 0);
   EXPECT_EQ(terminfo.c_lflag & ECHO, 0U);
 }
@@ -63,11 +63,11 @@ TEST_F(TerminalTest, SetEcho) {
 TEST_F(TerminalTest, SetCanonical) {
   struct termios terminfo;
 
-  ASSERT_EQ(m_term.SetCanonical(true), true);
+  ASSERT_THAT_ERROR(m_term.SetCanonical(true), llvm::Succeeded());
   ASSERT_EQ(tcgetattr(m_fd, &terminfo), 0);
   EXPECT_NE(terminfo.c_lflag & ICANON, 0U);
 
-  ASSERT_EQ(m_term.SetCanonical(false), true);
+  ASSERT_THAT_ERROR(m_term.SetCanonical(false), llvm::Succeeded());
   ASSERT_EQ(tcgetattr(m_fd, &terminfo), 0);
   EXPECT_EQ(terminfo.c_lflag & ICANON, 0U);
 }