reopen 378469
found 378469 0.99.3-1
tags 378469 + confirmed patch fixed-upstream pending
thanks

Matthew Foulkes <[EMAIL PROTECTED]> writes:

> The trace.log file produced by running
>
>   $ strace -o trace.log schroot -c ia32 -p oowriter
>
> is attached. I did not know how to trace a version of schroot running in the
> background (adding an ampersand to the command above would presumably put
> strace into the background, not schroot) and did not try.

No worries.

> Your suggestion of redirecting stdin
>
>   $ schroot -c ia32 -p oowriter </dev/null &
>
> works! When oowriter's quit button is clicked, the schroot process quits
> along with the oowriter process. This is the behaviour we were hoping to
> see.

Excellent, thanks.

> Although I speak C++ and would not have found it difficult to comment
> out the lines you removed from schroot-base-main.cc, patching and
> rebuilding a debian package from source was a new adventure for me!
> Given your enthusiasm, however, I felt I had better try. To my surprise,
> it was all surprisingly simple (although the build took a while).

Thanks for trying that.  (For some reason, using static libraries
slows the link times down by a couple of orders of magnitude; this is
the main cause for the slow build.  I'll look at improving the build
times after I get version 1.0.0 released later in this week or next.)

> I think this provides enough information to pin down the problem.

Yes.  I've altered the code to only save and restore the termios when
running an interactive login shell.  It won't do it when running a
command, which will fix the problem you are seeing.

I have attached the patch I have applied to SVN.


Regards,
Roger

-- 
  .''`.  Roger Leigh
 : :' :  Debian GNU/Linux             http://people.debian.org/~rleigh/
 `. `'   Printing on GNU/Linux?       http://gutenprint.sourceforge.net/
   `-    GPG Public Key: 0x25BFB848   Please sign and encrypt your mail.
Index: schroot/schroot-base-main.cc
===================================================================
--- schroot/schroot-base-main.cc	(revision 894)
+++ schroot/schroot-base-main.cc	(revision 895)
@@ -30,9 +30,6 @@
 #include <iostream>
 #include <locale>
 
-#include <termios.h>
-#include <unistd.h>
-
 #include <syslog.h>
 
 #include <boost/format.hpp>
@@ -86,9 +83,6 @@
 main::run (int   argc,
 	   char *argv[])
 {
-  struct termios saved_termios;
-  bool termios_ok = false;
-
   try
     {
       // Set up locale.
@@ -96,20 +90,6 @@
       std::cout.imbue(std::locale());
       std::cerr.imbue(std::locale());
 
-      // Save terminal state.
-      if (isatty(STDIN_FILENO))
-	{
-	  if (tcgetattr(STDIN_FILENO, &saved_termios) < 0)
-	    {
-	      termios_ok = false;
-	      sbuild::log_warning()
-		<< _("Error saving terminal settings")
-		<< endl;
-	    }
-	  else
-	    termios_ok = true;
-	}
-
       bindtextdomain (GETTEXT_PACKAGE, LOCALEDIR);
       textdomain (GETTEXT_PACKAGE);
 
@@ -125,14 +105,6 @@
 
       closelog();
 
-      if (isatty(STDIN_FILENO) && termios_ok)
-	{
-	  if (tcsetattr(STDIN_FILENO, TCSANOW, &saved_termios) < 0)
-	    sbuild::log_warning()
-	      << _("Error restoring terminal settings")
-	      << endl;
-	}
-
       return status;
     }
   catch (std::exception const& e)
@@ -154,14 +126,6 @@
 
       closelog();
 
-      if (isatty(STDIN_FILENO) && termios_ok)
-	{
-	  if (tcsetattr(STDIN_FILENO, TCSANOW, &saved_termios) < 0)
-	    sbuild::log_warning()
-	      << _("Error restoring terminal settings")
-	      << endl;
-	}
-
       return EXIT_FAILURE;
     }
 }
Index: sbuild/sbuild-session.cc
===================================================================
--- sbuild/sbuild-session.cc	(revision 894)
+++ sbuild/sbuild-session.cc	(revision 895)
@@ -34,6 +34,8 @@
 
 #include <sys/types.h>
 #include <sys/stat.h>
+#include <fcntl.h>
+#include <termios.h>
 #include <unistd.h>
 
 #include <syslog.h>
@@ -218,6 +220,8 @@
   session_id(),
   force(false),
   saved_signals(),
+  saved_termios(),
+  termios_ok(false),
   cwd(getcwd())
 {
 }
@@ -286,6 +290,48 @@
   this->force = force;
 }
 
+void
+session::save_termios ()
+{
+  int ctty = open("/dev/tty", O_RDONLY|O_NOCTTY);
+  string_list const& command(auth::get_command());
+
+  this->termios_ok = false;
+
+  // Save if running a login shell and have a controlling terminal.
+  if (ctty >= 0 &&
+      (command.empty() || command[0].empty()))
+    {
+      if (tcgetattr(ctty, &this->saved_termios) < 0)
+	{
+	  sbuild::log_warning()
+	    << _("Error saving terminal settings")
+	    << endl;
+	}
+      else
+	this->termios_ok = true;
+    }
+}
+
+void
+session::restore_termios ()
+{
+  int ctty = open("/dev/tty", O_WRONLY|O_NOCTTY);
+  string_list const& command(auth::get_command());
+
+  // Restore if running a login shell and have a controlling terminal,
+  // and have previously saved the terminal state.
+  if (ctty >= 0 &&
+      (command.empty() || command[0].empty()) &&
+      termios_ok)
+    {
+      if (tcsetattr(ctty, TCSANOW, &this->saved_termios) < 0)
+	sbuild::log_warning()
+	  << _("Error restoring terminal settings")
+	  << endl;
+    }
+}
+
 int
 session::get_child_status () const
 {
@@ -505,13 +551,16 @@
 		    try
 		      {
 			open_session();
+			save_termios();
 			run_chroot(chroot);
 		      }
 		    catch (std::runtime_error const& e)
 		      {
+			restore_termios();
 			close_session();
 			throw;
 		      }
+		    restore_termios();
 		    close_session();
 		  }
 
Index: sbuild/sbuild-session.h
===================================================================
--- sbuild/sbuild-session.h	(revision 894)
+++ sbuild/sbuild-session.h	(revision 895)
@@ -28,6 +28,8 @@
 
 #include <signal.h>
 #include <sys/types.h>
+#include <termios.h>
+#include <unistd.h>
 
 namespace sbuild
 {
@@ -197,6 +199,18 @@
     set_force (bool force);
 
     /**
+     * Save terminal state.
+     */
+    void
+    save_termios ();
+
+    /**
+     * Restore terminal state.
+     */
+    void
+    restore_termios ();
+
+    /**
      * Get the exit (wait) status of the last child process to run in this
      * session.
      *
@@ -384,6 +398,10 @@
     bool             force;
     /// Signals saved while sighup handler is set.
     struct sigaction saved_signals;
+    /// Saved terminal settings.
+    struct termios saved_termios;
+    /// Are the saved terminal settings valid?
+    bool termios_ok;
 
   protected:
     /// Current working directory.
Index: ChangeLog
===================================================================
--- ChangeLog	(revision 894)
+++ ChangeLog	(revision 895)
@@ -1,5 +1,21 @@
 2006-07-19  Roger Leigh  <[EMAIL PROTECTED]>
 
+	* NEWS: Document terminal state save and restore behaviour.
+
+	* sbuild/sbuild-session.cc
+	(save_termios): New method.  Save terminal state.
+	(restore_termios): New method.  Restore terminal state.
+	(run_impl): Save and restore terminal state between opening and
+	closing the session.
+
+	* sbuild/sbuild-session.h: Add saved_termios and termios_ok
+	members.
+
+	* schroot/schroot-base-main.cc (run): Remove termios save and
+	restore.
+
+2006-07-19  Roger Leigh  <[EMAIL PROTECTED]>
+
 	* po/LINGUAS: Remove vi.
 
 	* po/vi.po: Remove currently unmaintained translation.
Index: NEWS
===================================================================
--- NEWS	(revision 894)
+++ NEWS	(revision 895)
@@ -14,6 +14,12 @@
 
   2) Duplicate groups and keys with groups are now treated as errors.
 
+  3) The terminal state is only saved and restored when running a
+     login shell.  It is no longer saved and restored when running
+     commands.  This is to correct the problem of schroot being
+     stopped when running in the background while restoring the
+     terminal settings.
+
 * Major changes in 0.99.3:
 
   1) A new chroot type, "directory", has been added.  This is the same

Attachment: pgpEBYYAcIIRN.pgp
Description: PGP signature

Reply via email to