[gnome-builder] foundry: simplify PTY usage in IdeRunner

[Christian Hergert <chergert src gnome org>]

commit 99dd4fb4334aead217d716c57a729b855360eec4
Author: Christian Hergert <chergert redhat com>
Date:   Sat Apr 27 13:16:11 2019 -0700

    foundry: simplify PTY usage in IdeRunner
    
    Rather than using FDs directly, we can just use a VtePty and allow
    consumers to create the child side of the PTY as necessary.

 src/libide/foundry/ide-runner.c                    | 171 ++++++++++-----------
 src/libide/foundry/ide-runner.h                    |  14 +-
 src/libide/terminal/ide-terminal-page.c            |  10 +-
 src/plugins/gdb/gbp-gdb-debugger.c                 |  22 ++-
 .../terminal/gbp-terminal-workspace-addin.c        |   7 +-
 5 files changed, 114 insertions(+), 110 deletions(-)
---
diff --git a/src/libide/foundry/ide-runner.c b/src/libide/foundry/ide-runner.c
index 58454a1ac..d79d586de 100644
--- a/src/libide/foundry/ide-runner.c
+++ b/src/libide/foundry/ide-runner.c
@@ -55,11 +55,12 @@ typedef struct
 
   GSubprocessFlags flags;
 
-  int tty_fd;
+  VtePty *pty;
 
   guint clear_env : 1;
   guint failed : 1;
   guint run_on_host : 1;
+  guint disable_pty : 1;
 } IdeRunnerPrivate;
 
 typedef struct
@@ -77,12 +78,13 @@ typedef struct
 enum {
   PROP_0,
   PROP_ARGV,
+  PROP_BUILD_TARGET,
   PROP_CLEAR_ENV,
   PROP_CWD,
+  PROP_DISABLE_PTY,
   PROP_ENV,
   PROP_FAILED,
   PROP_RUN_ON_HOST,
-  PROP_BUILD_TARGET,
   N_PROPS
 };
 
@@ -251,12 +253,26 @@ ide_runner_real_run_async (IdeRunner           *self,
    * If we have a tty_fd set, then we want to override our stdin,
    * stdout, and stderr fds with our TTY.
    */
-  if (priv->tty_fd != -1)
+  if (priv->pty != NULL && !priv->disable_pty)
     {
-      IDE_TRACE_MSG ("Setting TTY fd to %d", priv->tty_fd);
-      ide_subprocess_launcher_take_stdin_fd (launcher, dup (priv->tty_fd));
-      ide_subprocess_launcher_take_stdout_fd (launcher, dup (priv->tty_fd));
-      ide_subprocess_launcher_take_stderr_fd (launcher, dup (priv->tty_fd));
+      gint master_fd;
+      gint tty_fd;
+
+      master_fd = vte_pty_get_fd (priv->pty);
+      tty_fd = ide_pty_intercept_create_slave (master_fd, TRUE);
+
+      IDE_TRACE_MSG ("Setting TTY fd to %d", tty_fd);
+
+      if (!(priv->flags & G_SUBPROCESS_FLAGS_STDIN_PIPE))
+        ide_subprocess_launcher_take_stdin_fd (launcher, dup (tty_fd));
+
+      if (!(priv->flags & (G_SUBPROCESS_FLAGS_STDOUT_PIPE | G_SUBPROCESS_FLAGS_STDOUT_SILENCE)))
+        ide_subprocess_launcher_take_stdout_fd (launcher, dup (tty_fd));
+
+      if (!(priv->flags & (G_SUBPROCESS_FLAGS_STDERR_PIPE | G_SUBPROCESS_FLAGS_STDERR_SILENCE)))
+        ide_subprocess_launcher_take_stderr_fd (launcher, dup (tty_fd));
+
+      close (tty_fd);
     }
 
   /*
@@ -368,46 +384,6 @@ ide_runner_real_get_stderr (IdeRunner *self)
   return NULL;
 }
 
-gint
-ide_runner_steal_tty (IdeRunner *self)
-{
-  IdeRunnerPrivate *priv = ide_runner_get_instance_private (self);
-  gint fd;
-
-  g_return_val_if_fail (IDE_IS_RUNNER (self), -1);
-
-  fd = priv->tty_fd;
-  priv->tty_fd = -1;
-
-  return fd;
-}
-
-static void
-ide_runner_real_set_tty (IdeRunner *self,
-                         int        tty_fd)
-{
-  IdeRunnerPrivate *priv = ide_runner_get_instance_private (self);
-
-  g_assert (IDE_IS_RUNNER (self));
-  g_assert (tty_fd >= -1);
-
-  if (tty_fd != priv->tty_fd)
-    {
-      if (priv->tty_fd != -1)
-        {
-          close (priv->tty_fd);
-          priv->tty_fd = -1;
-        }
-
-      if (tty_fd != -1)
-        {
-          priv->tty_fd = dup (tty_fd);
-          if (priv->tty_fd == -1)
-            g_warning ("Failed to dup() tty_fd: %s", g_strerror (errno));
-        }
-    }
-}
-
 static void
 ide_runner_real_force_quit (IdeRunner *self)
 {
@@ -511,12 +487,7 @@ ide_runner_finalize (GObject *object)
     }
 
   g_clear_pointer (&priv->fd_mapping, g_array_unref);
-
-  if (priv->tty_fd != -1)
-    {
-      close (priv->tty_fd);
-      priv->tty_fd = -1;
-    }
+  g_clear_object (&priv->pty);
 
   G_OBJECT_CLASS (ide_runner_parent_class)->finalize (object);
 }
@@ -543,6 +514,10 @@ ide_runner_get_property (GObject    *object,
       g_value_set_string (value, ide_runner_get_cwd (self));
       break;
 
+    case PROP_DISABLE_PTY:
+      g_value_set_boolean (value, ide_runner_get_disable_pty (self));
+      break;
+
     case PROP_ENV:
       g_value_set_object (value, ide_runner_get_environment (self));
       break;
@@ -586,6 +561,10 @@ ide_runner_set_property (GObject      *object,
       ide_runner_set_cwd (self, g_value_get_string (value));
       break;
 
+    case PROP_DISABLE_PTY:
+      ide_runner_set_disable_pty (self, g_value_get_boolean (value));
+      break;
+
     case PROP_FAILED:
       ide_runner_set_failed (self, g_value_get_boolean (value));
       break;
@@ -615,7 +594,6 @@ ide_runner_class_init (IdeRunnerClass *klass)
 
   klass->run_async = ide_runner_real_run_async;
   klass->run_finish = ide_runner_real_run_finish;
-  klass->set_tty = ide_runner_real_set_tty;
   klass->create_launcher = ide_runner_real_create_launcher;
   klass->get_stdin = ide_runner_real_get_stdin;
   klass->get_stdout = ide_runner_real_get_stdout;
@@ -643,6 +621,13 @@ ide_runner_class_init (IdeRunnerClass *klass)
                          NULL,
                          (G_PARAM_READWRITE | G_PARAM_EXPLICIT_NOTIFY | G_PARAM_STATIC_STRINGS));
 
+  properties [PROP_DISABLE_PTY] =
+    g_param_spec_boolean ("disable-pty",
+                          "Disable PTY",
+                          "If the pty should be disabled from use",
+                          FALSE,
+                          (G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));
+
   properties [PROP_ENV] =
     g_param_spec_object ("environment",
                          "Environment",
@@ -733,7 +718,6 @@ ide_runner_init (IdeRunner *self)
   priv->env = ide_environment_new ();
 
   priv->flags = 0;
-  priv->tty_fd = -1;
 }
 
 /**
@@ -1184,27 +1168,6 @@ ide_runner_set_clear_env (IdeRunner *self,
     }
 }
 
-void
-ide_runner_set_tty (IdeRunner *self,
-                    int        tty_fd)
-{
-  IDE_ENTRY;
-
-  g_return_if_fail (IDE_IS_RUNNER (self));
-  g_return_if_fail (tty_fd >= -1);
-
-  if (IDE_RUNNER_GET_CLASS (self)->set_tty)
-    {
-      IDE_RUNNER_GET_CLASS (self)->set_tty (self, tty_fd);
-      return;
-    }
-
-  g_warning ("%s does not support setting a TTY fd",
-             G_OBJECT_TYPE_NAME (self));
-
-  IDE_EXIT;
-}
-
 /**
  * ide_runner_set_pty:
  * @self: a #IdeRunner
@@ -1221,23 +1184,32 @@ void
 ide_runner_set_pty (IdeRunner *self,
                     VtePty    *pty)
 {
-  int child_fd = -1;
+  IdeRunnerPrivate *priv = ide_runner_get_instance_private (self);
 
   g_return_if_fail (IDE_IS_RUNNER (self));
   g_return_if_fail (!pty || VTE_IS_PTY (pty));
 
-  if (pty != NULL)
-    {
-      int parent_fd = vte_pty_get_fd (pty);
+  g_set_object (&priv->pty, pty);
+}
 
-      if (parent_fd != -1)
-        child_fd = ide_pty_intercept_create_slave (parent_fd, TRUE);
-    }
+/**
+ * ide_runner_get_pty:
+ * @self: a #IdeRunner
+ *
+ * Gets the #VtePty that was assigned.
+ *
+ * Returns: (nullable) (transfer none): a #VtePty or %NULL
+ *
+ * Since: 3.34
+ */
+VtePty *
+ide_runner_get_pty (IdeRunner *self)
+{
+  IdeRunnerPrivate *priv = ide_runner_get_instance_private (self);
 
-  ide_runner_set_tty (self, child_fd);
+  g_return_val_if_fail (IDE_IS_RUNNER (self), NULL);
 
-  if (child_fd != -1)
-    close (child_fd);
+  return priv->pty;
 }
 
 static gint
@@ -1476,3 +1448,30 @@ ide_runner_set_build_target (IdeRunner      *self,
   if (g_set_object (&priv->build_target, build_target))
     g_object_notify_by_pspec (G_OBJECT (self), properties [PROP_BUILD_TARGET]);
 }
+
+gboolean
+ide_runner_get_disable_pty (IdeRunner *self)
+{
+  IdeRunnerPrivate *priv = ide_runner_get_instance_private (self);
+
+  g_return_val_if_fail (IDE_IS_RUNNER (self), FALSE);
+
+  return priv->disable_pty;
+}
+
+void
+ide_runner_set_disable_pty (IdeRunner *self,
+                            gboolean   disable_pty)
+{
+  IdeRunnerPrivate *priv = ide_runner_get_instance_private (self);
+
+  g_return_if_fail (IDE_IS_RUNNER (self));
+
+  disable_pty = !!disable_pty;
+
+  if (disable_pty != priv->disable_pty)
+    {
+      priv->disable_pty = disable_pty;
+      g_object_notify_by_pspec (G_OBJECT (self), properties [PROP_DISABLE_PTY]);
+    }
+}
diff --git a/src/libide/foundry/ide-runner.h b/src/libide/foundry/ide-runner.h
index 99698eaa8..737a2da3a 100644
--- a/src/libide/foundry/ide-runner.h
+++ b/src/libide/foundry/ide-runner.h
@@ -52,8 +52,6 @@ struct _IdeRunnerClass
   gboolean               (*run_finish)      (IdeRunner             *self,
                                              GAsyncResult          *result,
                                              GError               **error);
-  void                   (*set_tty)         (IdeRunner             *self,
-                                             int                    tty_fd);
   IdeSubprocessLauncher *(*create_launcher) (IdeRunner             *self);
   void                   (*fixup_launcher)  (IdeRunner             *self,
                                              IdeSubprocessLauncher *launcher);
@@ -132,11 +130,13 @@ void               ide_runner_set_run_on_host  (IdeRunner            *self,
 IDE_AVAILABLE_IN_3_32
 void               ide_runner_set_pty          (IdeRunner            *self,
                                                 VtePty               *pty);
-IDE_AVAILABLE_IN_3_32
-void               ide_runner_set_tty          (IdeRunner            *self,
-                                                int                   tty_fd);
-IDE_AVAILABLE_IN_3_32
-gint               ide_runner_steal_tty        (IdeRunner            *self);
+IDE_AVAILABLE_IN_3_34
+VtePty            *ide_runner_get_pty          (IdeRunner            *self);
+IDE_AVAILABLE_IN_3_34
+gboolean           ide_runner_get_disable_pty  (IdeRunner            *self);
+IDE_AVAILABLE_IN_3_34
+void               ide_runner_set_disable_pty  (IdeRunner            *self,
+                                                gboolean              disable_pty);
 IDE_AVAILABLE_IN_3_32
 IdeBuildTarget    *ide_runner_get_build_target (IdeRunner            *self);
 IDE_AVAILABLE_IN_3_32
diff --git a/src/libide/terminal/ide-terminal-page.c b/src/libide/terminal/ide-terminal-page.c
index 504810647..973d7f0d9 100644
--- a/src/libide/terminal/ide-terminal-page.c
+++ b/src/libide/terminal/ide-terminal-page.c
@@ -230,9 +230,6 @@ gbp_terminal_respawn (IdeTerminalPage *self,
 
   vte_terminal_set_pty (terminal, pty);
 
-  if (-1 == (tty_fd = ide_vte_pty_create_slave (pty)))
-    IDE_GOTO (cleanup);
-
   if (self->runtime != NULL &&
       !ide_runtime_contains_program_in_path (self->runtime, shell, NULL))
     {
@@ -260,8 +257,7 @@ gbp_terminal_respawn (IdeTerminalPage *self,
         {
           IdeEnvironment *env = ide_runner_get_environment (runner);
 
-          /* set_tty() will dup() the fd */
-          ide_runner_set_tty (runner, tty_fd);
+          ide_runner_set_pty (runner, pty);
 
           ide_environment_setenv (env, "TERM", "xterm-256color");
           ide_environment_setenv (env, "INSIDE_GNOME_BUILDER", PACKAGE_VERSION);
@@ -281,6 +277,10 @@ gbp_terminal_respawn (IdeTerminalPage *self,
         }
     }
 
+  if (-1 == (tty_fd = ide_vte_pty_create_slave (pty)))
+    IDE_GOTO (cleanup);
+
+
   /* dup() is safe as it will inherit O_CLOEXEC */
   if (-1 == (stdout_fd = dup (tty_fd)) || -1 == (stderr_fd = dup (tty_fd)))
     IDE_GOTO (cleanup);
diff --git a/src/plugins/gdb/gbp-gdb-debugger.c b/src/plugins/gdb/gbp-gdb-debugger.c
index 5fc516dad..70c249b84 100644
--- a/src/plugins/gdb/gbp-gdb-debugger.c
+++ b/src/plugins/gdb/gbp-gdb-debugger.c
@@ -2345,7 +2345,7 @@ gbp_gdb_debugger_prepare (IdeDebugger *debugger,
 {
   static const gchar *prepend_argv[] = { "gdb", "--interpreter=mi2", "--args" };
   GbpGdbDebugger *self = (GbpGdbDebugger *)debugger;
-  int tty_fd;
+  VtePty *pty;
 
   IDE_ENTRY;
 
@@ -2360,12 +2360,21 @@ gbp_gdb_debugger_prepare (IdeDebugger *debugger,
   dzl_signal_group_set_target (self->runner_signals, runner);
 
   /*
-   * We steal and remap the PTY fd into the process so that gdb does not get
-   * the controlling terminal, but instead allow us to ask gdb to setup the
-   * inferior with that same PTY.
+   * If there is a PTY device to display the contents of the inferior, then
+   * we will create a new FD for that from the PTY and save it to map into
+   * the inferior.
    */
-  if (-1 != (tty_fd = ide_runner_steal_tty (runner)))
-    self->mapped_fd = ide_runner_take_fd (runner, tty_fd, -1);
+  if ((pty = ide_runner_get_pty (runner)))
+    {
+      int master_fd = vte_pty_get_fd (pty);
+      int tty_fd = ide_pty_intercept_create_slave (master_fd, TRUE);
+
+      if (tty_fd != -1)
+        {
+          self->mapped_fd = ide_runner_take_fd (runner, tty_fd, -1);
+          ide_runner_set_disable_pty (runner, TRUE);
+        }
+    }
 
   /* We need access to stdin/stdout for communicating with gdb */
   ide_runner_set_flags (runner, G_SUBPROCESS_FLAGS_STDIN_PIPE | G_SUBPROCESS_FLAGS_STDOUT_PIPE | 
G_SUBPROCESS_FLAGS_STDERR_SILENCE);
@@ -2512,6 +2521,7 @@ gbp_gdb_debugger_init (GbpGdbDebugger *self)
   self->parser = gdbwire_mi_parser_create (callbacks);
   self->read_cancellable = g_cancellable_new ();
   self->read_buffer = g_malloc (READ_BUFFER_LEN);
+  self->mapped_fd = -1;
 
   g_queue_init (&self->cmdqueue);
 
diff --git a/src/plugins/terminal/gbp-terminal-workspace-addin.c 
b/src/plugins/terminal/gbp-terminal-workspace-addin.c
index 1cb9a8332..14c170186 100644
--- a/src/plugins/terminal/gbp-terminal-workspace-addin.c
+++ b/src/plugins/terminal/gbp-terminal-workspace-addin.c
@@ -175,7 +175,6 @@ on_run_manager_run (GbpTerminalWorkspaceAddin *self,
 {
   IdeEnvironment *env;
   VtePty *pty = NULL;
-  int tty_fd;
   g_autoptr(GDateTime) now = NULL;
   g_autofree gchar *formatted = NULL;
   g_autofree gchar *tmp = NULL;
@@ -239,11 +238,7 @@ on_run_manager_run (GbpTerminalWorkspaceAddin *self,
       ide_terminal_page_set_pty (self->run_terminal, pty);
     }
 
-  if (-1 != (tty_fd = ide_vte_pty_create_slave (pty)))
-    {
-      ide_runner_set_tty (runner, tty_fd);
-      close (tty_fd);
-    }
+  ide_runner_set_pty (runner, pty);
 
   env = ide_runner_get_environment (runner);
   ide_environment_setenv (env, "TERM", "xterm-256color");