Refactor the dpkg terminal creation code.

This straightens out some of the back-and-forth calls and replaces them
with more straightforward creation/connection/execution semantics.

3 files changed, 221 insertions, 235 deletions

diff --git a/src/gtk/dpkg_terminal.cc b/src/gtk/dpkg_terminal.cc
index 7a6fb42e..ade4ad0e 100644
--- a/src/gtk/dpkg_terminal.cc
+++ b/src/gtk/dpkg_terminal.cc
@@ -193,6 +193,10 @@ namespace gui
 				  info->handler_id);
     }
 
+    // Connect to the child-exited signal so we know when dpkg exits.
+    // We can't use a sigc++ connection because there's no C++ wrapper
+    // for VTE, and just using an old-school GTK+ binding is easier
+    // than writing a C++ wrapper.
     void connect_dpkg_result(pid_t pid,
 			     safe_slot1<void, pkgPackageManager::OrderResult> k)
     {
@@ -208,169 +212,162 @@ namespace gui
 			      &destroy_child_exited_info,
 			      (GConnectFlags)0);
     }
+  }
 
-    // This should always run from the foreground thread.
-    void do_run_dpkg(safe_slot1<pkgPackageManager::OrderResult, int> f,
-		     safe_slot1<void, Gtk::Widget *> register_terminal,
-		     safe_slot1<void, pkgPackageManager::OrderResult> k,
-		     safe_slot1<void, aptitude::apt::dpkg_status_message> report_message)
-    {
-      GtkWidget *vte = vte_terminal_new();
+  DpkgTerminal::DpkgTerminal()
+    : sent_finished_signal(false)
+  {
+    GtkWidget *vte = vte_terminal_new();
+    terminal = (Glib::wrap(vte, false));
+  }
 
-      Gtk::Widget *w(Glib::wrap(vte, false));
+  DpkgTerminal::~DpkgTerminal()
+  {
+    if(!sent_finished_signal)
+      finished(pkgPackageManager::Incomplete);
+    delete terminal;
+  }
 
-      register_terminal.get_slot()(w);
+  void DpkgTerminal::run(const safe_slot1<pkgPackageManager::OrderResult, int> &f)
+  {
+    // Create a temporary UNIX-domain socket to pass status
+    // information to the parent.
+    temp::dir tempdir("aptitude");
+    temp::name socketname(tempdir, "commsocket");
+
+    // To avoid races, we bind the receive end of the socket first and
+    // start accepting connections.
+    int listen_sock = open_unix_socket();
+    if(listen_sock == -1)
+      {
+	finished(pkgPackageManager::Failed);
+	return;
+      }
 
-      // Create a temporary UNIX-domain socket to pass status
-      // information to the parent.
-      temp::dir tempdir("aptitude");
-      temp::name socketname(tempdir, "commsocket");
+    // Ensure that the socket is always closed when this routine
+    // exits.
+    FileFd sock_fd(listen_sock);
 
-      // To avoid races, we bind the receive end of the socket first and
-      // start accepting connections.
-      int listen_sock = open_unix_socket();
-      if(listen_sock == -1)
-	{
-	  k.get_slot()(pkgPackageManager::Failed);
-	  return;
-	}
+    struct sockaddr_un sa;
 
-      // Ensure that the socket is always closed when this routine
-      // exits.
-      FileFd sock_fd(listen_sock);
+    const size_t max_socket_name = sizeof(sa.sun_path);
 
-      struct sockaddr_un sa;
+    if(socketname.get_name().size() > max_socket_name)
+      {
+	_error->Error("Internal error: the temporary socket name is too long!");
+	finished(pkgPackageManager::Failed);
+	return;
+      }
 
-      const size_t max_socket_name = sizeof(sa.sun_path);
+    sa.sun_family = AF_UNIX;
+    strncpy(sa.sun_path, socketname.get_name().c_str(), max_socket_name);
+    if(bind(listen_sock, (struct sockaddr *)&sa, sizeof(sa)) != 0)
+      {
+	int errnum = errno;
+	std::string err = cw::util::sstrerror(errnum);
+	_error->Error("%s: Unable to bind to the temporary socket: %s",
+		      __PRETTY_FUNCTION__,
+		      err.c_str());
+	finished(pkgPackageManager::Failed);
+	return;
+      }
 
-      if(socketname.get_name().size() > max_socket_name)
-	{
-	  _error->Error("Internal error: the temporary socket name is too long!");
-	  k.get_slot()(pkgPackageManager::Failed);
-	  return;
-	}
+    if(listen(listen_sock, 1) != 0)
+      {
+	int errnum = errno;
+	std::string err = cw::util::sstrerror(errnum);
+	_error->Error("%s: Unable to listen on the temporary socket: %s",
+		      __PRETTY_FUNCTION__,
+		      err.c_str());
+	finished(pkgPackageManager::Failed);
+	return;
+      }
 
-      sa.sun_family = AF_UNIX;
-      strncpy(sa.sun_path, socketname.get_name().c_str(), max_socket_name);
-      if(bind(listen_sock, (struct sockaddr *)&sa, sizeof(sa)) != 0)
-	{
-	  int errnum = errno;
-	  std::string err = cw::util::sstrerror(errnum);
-	  _error->Error("%s: Unable to bind to the temporary socket: %s",
-			__PRETTY_FUNCTION__,
-			err.c_str());
-	  k.get_slot()(pkgPackageManager::Failed);
-	  return;
-	}
+    GtkWidget *vte = terminal->gobj();
+    pid_t pid = vte_terminal_forkpty(VTE_TERMINAL(vte),
+				     NULL, NULL,
+				     FALSE, FALSE, FALSE);
 
-      if(listen(listen_sock, 1) != 0)
-	{
-	  int errnum = errno;
-	  std::string err = cw::util::sstrerror(errnum);
-	  _error->Error("%s: Unable to listen on the temporary socket: %s",
-			__PRETTY_FUNCTION__,
-			err.c_str());
-	  k.get_slot()(pkgPackageManager::Failed);
-	  return;
-	}
+    if(pid == 0)
+      {
+	// The child process.  It passes status information to the
+	// parent process and uses its *return code* to indicate the
+	// success / failure state.
 
-      pid_t pid = vte_terminal_forkpty(VTE_TERMINAL(vte),
-				       NULL, NULL,
-				       FALSE, FALSE, FALSE);
+	// NB: we should close the listen side here, but I don't
+	// because of my magic knowledge that vte will.
 
-      if(pid == 0)
-	{
-	  // The child process.  It passes status information to the
-	  // parent process and uses its *return code* to indicate the
-	  // success / failure state.
-
-	  // NB: we should close the listen side here, but I don't
-	  // because of my magic knowledge that vte will.
-
-	  int write_sock = open_unix_socket();
-	  if(write_sock == -1)
-	    {
-	      _error->DumpErrors();
-	      _exit(pkgPackageManager::Failed);
-	    }
-
-	  if(connect(write_sock, (struct sockaddr *)&sa, sizeof(sa)) != 0)
-	    {
-	      int errnum = errno;
-	      std::string err = cw::util::sstrerror(errnum);
-	      _error->Error("%s: Unable to bind to the temporary socket: %s",
-			    __PRETTY_FUNCTION__,
-			    err.c_str());
-	      _error->DumpErrors();
-	      _exit(pkgPackageManager::Failed);
-	    }
-
-	  pkgPackageManager::OrderResult result = f.get_slot()(write_sock);
-
-	  // Make sure errors appear somewhere (we really ought to push
-	  // them down the FIFO).
-	  _error->DumpErrors();
-
-	  _exit(result);
-	}
-      else
-	{
-	  int read_sock = accept(listen_sock, NULL, NULL);
-	  if(read_sock == -1)
-	    {
-	      int errnum = errno;
-	      std::string errmsg = cw::util::sstrerror(errnum);
-	      _error->Error(_("%s: Unable to accept a connection from the subprocess: %s"),
-			    __PRETTY_FUNCTION__,
-			    errmsg.c_str());
-	    }
-
-	  // Catch status output from the install process.
-	  dpkg_socket_data_processor *data_processor = new dpkg_socket_data_processor(read_sock, report_message);
-	  Glib::signal_io().connect(sigc::mem_fun(*data_processor, &dpkg_socket_data_processor::process_data_from_dpkg_socket),
-				    read_sock,
-				    Glib::IO_IN | Glib::IO_ERR | Glib::IO_HUP | Glib::IO_NVAL);
-
-	  // The parent process.  Here we just wait for the reaper to
-	  // tell us that the child finished, then return the result.
-	  // We use implicit locking here to avoid a race condition that
-	  // could occur: we know that the reaper won't fire before we
-	  // can connect to it because its signal executions go through
-	  // the main loop, and this function call is blocking the main
-	  // loop.
-	  connect_dpkg_result(pid, k);
-
-	  vte_reaper_add_child(pid);
-	}
-    }
-  }
+	int write_sock = open_unix_socket();
+	if(write_sock == -1)
+	  {
+	    _error->DumpErrors();
+	    _exit(pkgPackageManager::Failed);
+	  }
 
-  void run_dpkg_in_terminal(const safe_slot1<pkgPackageManager::OrderResult, int> &f,
-			    const safe_slot1<void, Gtk::Widget *> &register_terminal,
-			    const safe_slot1<void, pkgPackageManager::OrderResult> &k,
-			    const safe_slot1<void, aptitude::apt::dpkg_status_message> &report_message)
-  {
-    // Ask the main thread to start the dpkg run and to invoke the
-    // continuation.
-    sigc::slot0<void> run_dpkg_event =
-      sigc::bind(sigc::ptr_fun(&do_run_dpkg),
-		 f,
-		 register_terminal,
-		 k,
-		 report_message);
-    post_event(make_safe_slot(run_dpkg_event));
+	if(connect(write_sock, (struct sockaddr *)&sa, sizeof(sa)) != 0)
+	  {
+	    int errnum = errno;
+	    std::string err = cw::util::sstrerror(errnum);
+	    _error->Error("%s: Unable to bind to the temporary socket: %s",
+			  __PRETTY_FUNCTION__,
+			  err.c_str());
+	    _error->DumpErrors();
+	    _exit(pkgPackageManager::Failed);
+	  }
+
+	pkgPackageManager::OrderResult result = f.get_slot()(write_sock);
+
+	// Make sure errors appear somewhere (we really ought to push
+	// them down the FIFO).
+	_error->DumpErrors();
+
+	_exit(result);
+      }
+    else
+      {
+	int read_sock = accept(listen_sock, NULL, NULL);
+	if(read_sock == -1)
+	  {
+	    int errnum = errno;
+	    std::string errmsg = cw::util::sstrerror(errnum);
+	    _error->Error(_("%s: Unable to accept a connection from the subprocess: %s"),
+			  __PRETTY_FUNCTION__,
+			  errmsg.c_str());
+	  }
+
+	// Catch status output from the install process.
+	sigc::slot1<void, aptitude::apt::dpkg_status_message>
+	  report_message_slot(status_message.make_slot());
+	dpkg_socket_data_processor *data_processor = new dpkg_socket_data_processor(read_sock, make_safe_slot(report_message_slot));
+	Glib::signal_io().connect(sigc::mem_fun(*data_processor, &dpkg_socket_data_processor::process_data_from_dpkg_socket),
+				  read_sock,
+				  Glib::IO_IN | Glib::IO_ERR | Glib::IO_HUP | Glib::IO_NVAL);
+
+	// The parent process.  Here we just wait for the reaper to
+	// tell us that the child finished, then return the result.
+	// We use implicit locking here to avoid a race condition that
+	// could occur: we know that the reaper won't fire before we
+	// can connect to it because its signal executions go through
+	// the main loop, and this function call is blocking the main
+	// loop.
+	sigc::slot1<void, pkgPackageManager::OrderResult> finished_slot =
+	  finished.make_slot();
+	connect_dpkg_result(pid, make_safe_slot(finished_slot));
+
+	vte_reaper_add_child(pid);
+      }
   }
 
-  void inject_yes_into_dpkg_terminal(Gtk::Widget &w)
+  void DpkgTerminal::inject_yes()
   {
-    VteTerminal *vte = VTE_TERMINAL(w.gobj());
+    VteTerminal *vte = VTE_TERMINAL(terminal->gobj());
 
     vte_terminal_feed_child(vte, "y\n", 2);
   }
 
-  void inject_no_into_dpkg_terminal(Gtk::Widget &w)
+  void DpkgTerminal::inject_no()
   {
-    VteTerminal *vte = VTE_TERMINAL(w.gobj());
+    VteTerminal *vte = VTE_TERMINAL(terminal->gobj());
 
     vte_terminal_feed_child(vte, "n\n", 2);
   }
diff --git a/src/gtk/dpkg_terminal.h b/src/gtk/dpkg_terminal.h
index 0553278b..0d3acc39 100644
--- a/src/gtk/dpkg_terminal.h
+++ b/src/gtk/dpkg_terminal.h
@@ -37,41 +37,62 @@
 
 namespace gui
 {
-  /** \brief Run dpkg in a terminal, blocking this thread until it
-   *  completes.
-   *
-   *  This may be invoked in either a foreground thread or a
-   *  background thread.
-   *
-   *  \param f The function to invoke when we want to run dpkg.
-   *
-   *  \param register_terminal A function to invoke to register the
-   *  terminal (e.g., to store it in a variable or to add it to a new
-   *  tab).  This function assumes ownership of the terminal widget.
-   *
-   *  \param k   A function to invoke after the package manager is
-   *             finished running.
-   *
-   *  \param report_message   A function to invoke when a message is
-   *                          received on the dpkg status file
-   *                          descriptor.
+  /** \brief This object manages setting up and destroying the dpkg
+   *  terminal.
    */
-  void run_dpkg_in_terminal(const safe_slot1<pkgPackageManager::OrderResult, int> &f,
-			    const safe_slot1<void, Gtk::Widget *> &register_terminal,
-			    const safe_slot1<void, pkgPackageManager::OrderResult> &k,
-			    const safe_slot1<void, aptitude::apt::dpkg_status_message> &report_message);
-
-  /** \brief Send "yes" in reply to a "replace this conffile?" message.
-   *
-   *  \todo This API needs to be redesigned.
-   */
-  void inject_yes_into_dpkg_terminal(Gtk::Widget &w);
+  class DpkgTerminal : public sigc::trackable
+  {
+    Gtk::Widget *terminal;
+    bool sent_finished_signal;
 
-  /** \brief Send "no" in reply to a "replace this conffile?" message.
-   *
-   *  \todo This API needs to be redesigned.
-   */
-  void inject_no_into_dpkg_terminal(Gtk::Widget &w);
+    // Forbid copying.
+    DpkgTerminal(const DpkgTerminal &);
+
+  public:
+    /** \brief Initialize a new terminal. */
+    DpkgTerminal();
+
+    ~DpkgTerminal();
+
+    /** \brief Return a wrapper around the terminal object.
+     *
+     *  The wrapper is owned by this DpkgTerminal.
+     */
+    Gtk::Widget *get_widget() const { return terminal; }
+
+    /** \brief A signal that triggers in the main thread when dpkg
+     *	finishes running.
+     */
+    sigc::signal1<void, pkgPackageManager::OrderResult> finished; 
+
+    /** \brief A signal that triggers in the main thread when a
+     *  message is received on the dpkg status pipe.
+     */
+    sigc::signal1<void, aptitude::apt::dpkg_status_message> status_message;
+
+    /** \brief Start running dpkg in the encapsulated terminal.
+     *
+     *  This may be invoked in either a foreground thread or a
+     *  background thread.
+     *
+     *  \param f The function that actually invokes dpkg.
+     */
+    void run(const safe_slot1<pkgPackageManager::OrderResult, int> &f);
+
+    // It would be somewhat nicer to just pass around slots to say
+    // "yes" or "no" (so only something that read a
+    // conffile-replacement message would have a token that could
+    // invoke these).
+
+    /** \brief Send "yes" in reply to a "replace this conffile?"
+     *  message.
+     */
+    void inject_yes();
+
+    /** \brief Send "no" in reply to a "replace this conffile?" message.
+     */
+    void inject_no();
+  };
 }
 
 #endif
diff --git a/src/gtk/gui.cc b/src/gtk/gui.cc
index 345d5713..b16f2715 100644
--- a/src/gtk/gui.cc
+++ b/src/gtk/gui.cc
@@ -466,7 +466,9 @@ namespace gui
       Gtk::ProgressBar *progress;
       // The active terminal information tab, or NULL if none.
       DpkgTerminalTab *tab;
-      Gtk::Widget *terminal;
+      DpkgTerminal *terminal;
+      // Invoked as an idle callback after dpkg finishes running.
+      safe_slot1<void, pkgPackageManager::OrderResult> k;
 
       void abort()
       {
@@ -479,18 +481,32 @@ namespace gui
 	tab = NULL;
       }
 
+      void finish_dpkg_run(pkgPackageManager::OrderResult res)
+      {
+	// Invoking this as an idle callback is a bit of a holdover
+	// from when I tried (disastrously) to run the download
+	// manager's finish() in a background thread.  Nonetheless, it
+	// avoids any nasty surprises because of single-thread
+	// reentrancy.
+	post_event(safe_bind(k, res));
+      }
+
     public:
-      DpkgTerminalNotification()
+	DpkgTerminalNotification(const safe_slot1<void, pkgPackageManager::OrderResult> &_k)
 	: Notification(true),
 	  progress(new Gtk::ProgressBar),
 	  tab(NULL),
-	  terminal(NULL)
+	  terminal(new DpkgTerminal),
+	  k(_k)
       {
 	progress->set_text(_("Applying changes..."));
 	progress->set_ellipsize(Pango::ELLIPSIZE_END);
 	progress->show();
 	prepend_widget(progress);
 
+	terminal->finished.connect(sigc::mem_fun(*this, &DpkgTerminalNotification::finish_dpkg_run));
+	terminal->status_message.connect(sigc::mem_fun(*this, &DpkgTerminalNotification::process_dpkg_message));
+
 	Gtk::Button *view_details_button = new Gtk::Button(_("View Details"));
 	view_details_button->signal_clicked().connect(sigc::mem_fun(*this, &DpkgTerminalNotification::view_details));
 	view_details_button->show();
@@ -505,38 +521,18 @@ namespace gui
 	show();
       }
 
-      // Needed because of signal connection ick.  We want to connect
-      // a slot from run_dpkg_in_terminal to the status message
-      // processing function in this class.  But the slot has to be
-      // created before we actually create the dpkg terminal (mostly
-      // because I want to hide the details of creating the terminal
-      // in dpkg_terminal.cc).  One alternative would be a two-step
-      // process where you first create the terminal, then call
-      // another routine to actually run dpkg (passing the message
-      // reporting slot only to the second routine).  This might be a
-      // cleaner design eventually, but for the time being I decided
-      // to just backpatch the terminal pointer.
-      //
-      // \todo Apply the redesign suggested above.
-      void set_terminal(Gtk::Widget *new_terminal)
+      void run_dpkg(const safe_slot1<pkgPackageManager::OrderResult, int> &f)
       {
-	if(tab != NULL)
-	  {
-	    delete tab;
-	    tab = NULL;
-	  }
-	if(terminal != NULL)
-	  delete terminal;
-	terminal = new_terminal;
+	terminal->run(f);
       }
 
       void view_details()
       {
 	if(tab != NULL)
 	  tab->get_widget()->show();
-	else if(terminal != NULL)
+	else
 	  {
-	    tab = new DpkgTerminalTab(terminal);
+	    tab = new DpkgTerminalTab(terminal->get_widget());
 	    tab->closed.connect(sigc::mem_fun(*this, &DpkgTerminalNotification::tab_destroyed));
 	    tab_add(tab);
 	  }
@@ -567,10 +563,10 @@ namespace gui
 
 	      if(terminal != NULL)
 		{
-		  tab->yes_clicked.connect(sigc::bind(sigc::ptr_fun(&inject_yes_into_dpkg_terminal),
-						      sigc::ref(*terminal)));
-		  tab->no_clicked.connect(sigc::bind(sigc::ptr_fun(&inject_no_into_dpkg_terminal),
-						     sigc::ref(*terminal)));
+		  tab->yes_clicked.connect(sigc::mem_fun(*terminal,
+							 &DpkgTerminal::inject_yes));
+		  tab->no_clicked.connect(sigc::mem_fun(*terminal,
+							&DpkgTerminal::inject_no));
 		}
 
 	      tab->yes_clicked.connect(tab->close_clicked.make_slot());
@@ -599,43 +595,15 @@ namespace gui
       }
     };
 
-    // Scary callback functions.  This needs to be cleaned up.
-
-    void register_dpkg_terminal(Gtk::Widget *w,
-				DpkgTerminalNotification &n)
-    {
-      n.set_terminal(w);
-    }
-
-    // Asynchronously post the outcome of a dpkg run to the main
-    // thread.
-    void finish_gui_run_dpkg(pkgPackageManager::OrderResult res,
-			     safe_slot1<void, pkgPackageManager::OrderResult> k)
-    {
-      post_event(safe_bind(k, res));
-    }
-
     // Callback that kicks off a dpkg run.
     void gui_run_dpkg(sigc::slot1<pkgPackageManager::OrderResult, int> f,
 		      sigc::slot1<void, pkgPackageManager::OrderResult> k)
     {
-      DpkgTerminalNotification *n = new DpkgTerminalNotification;
-
-      sigc::slot1<void, Gtk::Widget *> register_terminal_slot =
-	sigc::bind(sigc::ptr_fun(&register_dpkg_terminal),
-		   sigc::ref(*n));
-      sigc::slot1<void, pkgPackageManager::OrderResult>
-	finish_slot(sigc::bind(sigc::ptr_fun(&finish_gui_run_dpkg),
-			       make_safe_slot(k)));
-      sigc::slot1<void, aptitude::apt::dpkg_status_message> process_dpkg_message_slot =
-	sigc::mem_fun(*n, &DpkgTerminalNotification::process_dpkg_message);
+      DpkgTerminalNotification *n = new DpkgTerminalNotification(make_safe_slot(k));
 
       pMainWindow->get_notifyview()->add_notification(n);
 
-      run_dpkg_in_terminal(make_safe_slot(f),
-			   make_safe_slot(register_terminal_slot),
-			   make_safe_slot(finish_slot),
-			   make_safe_slot(process_dpkg_message_slot));
+      n->run_dpkg(make_safe_slot(f));
     }
 
     void install_or_remove_packages()