[Buildd-tools-devel] [PATCH 17/22] Don't rollback non-run scripts

Jan-Marek Glogowski glogow at fbihome.de
Thu Mar 26 21:13:55 UTC 2009


This just rolls back the scripts, which have run, excluding the
failed one.  On failure scripts should cleanup before exit.

This helps especially with the killprocs script.  If a second
loopback session was started, but mount failed (as expected),
killprocs killed the processes from the current running session.
---
 sbuild/sbuild-run-parts.cc |   58 ++++++++++++++++------
 sbuild/sbuild-run-parts.h  |   16 ++++--
 sbuild/sbuild-session.cc   |  115 +++++++++++++++++++++++++++++++++++++++-----
 sbuild/sbuild-session.h    |   20 ++++++--
 test/sbuild-run-parts.cc   |   18 +++++--
 5 files changed, 183 insertions(+), 44 deletions(-)

diff --git a/sbuild/sbuild-run-parts.cc b/sbuild/sbuild-run-parts.cc
index fb1696b..2ea20af 100644
--- a/sbuild/sbuild-run-parts.cc
+++ b/sbuild/sbuild-run-parts.cc
@@ -115,9 +115,17 @@ run_parts::set_reverse (bool reverse)
 
 int
 run_parts::run (string_list const& command,
-		environment const& env)
+		environment const& env,
+		std::string      & failed_program)
 {
-  int exit_status = 0;
+  int exit_status = EXIT_FAILURE;
+  bool found_offset = failed_program.empty();
+
+  if (found_offset && this->programs.empty())
+    return EXIT_SUCCESS;
+
+  string_list real_command = command;
+  real_command.insert( real_command.begin(), std::string() );
 
   if (!this->reverse)
     {
@@ -125,17 +133,26 @@ run_parts::run (string_list const& command,
 	   pos != this->programs.end();
 	   ++pos)
 	{
-	  string_list real_command;
-	  real_command.push_back(*pos);
-	  for (string_list::const_iterator spos = command.begin();
-	       spos != command.end();
-	       ++spos)
-	    real_command.push_back(*spos);
+	  if (!found_offset)
+	    {
+	      if (failed_program == *pos)
+		{
+		  found_offset = true;
+		  failed_program.clear();
+		  exit_status = EXIT_SUCCESS;
+		}
+	      continue;
+	    }
+
+	  *(real_command.begin()) = *pos;
 
 	  exit_status = run_child(*pos, real_command, env);
 
 	  if (exit_status && this->abort_on_error)
-	    return exit_status;
+            {
+              failed_program = *pos;
+	      break;
+            }
 	}
     }
   else
@@ -144,17 +161,26 @@ run_parts::run (string_list const& command,
 	   pos != this->programs.rend();
 	   ++pos)
 	{
-	  string_list real_command;
-	  real_command.push_back(*pos);
-	  for (string_list::const_iterator spos = command.begin();
-	       spos != command.end();
-	       ++spos)
-	    real_command.push_back(*spos);
+	  if (!found_offset)
+	    {
+	      if (failed_program == *pos)
+		{
+		  found_offset = true;
+		  failed_program.clear();
+		  exit_status = EXIT_SUCCESS;
+		}
+	      continue;
+	    }
+
+	  *(real_command.begin()) = *pos;
 
 	  exit_status = run_child(*pos, real_command, env);
 
 	  if (exit_status && this->abort_on_error)
-	    return exit_status;
+            {
+              failed_program = *pos;
+	      break;
+            }
 	}
     }
 
diff --git a/sbuild/sbuild-run-parts.h b/sbuild/sbuild-run-parts.h
index 125d986..15927f2 100644
--- a/sbuild/sbuild-run-parts.h
+++ b/sbuild/sbuild-run-parts.h
@@ -104,17 +104,23 @@ namespace sbuild
     set_reverse (bool reverse);
 
     /**
-     * Run all scripts in the specified directory.  If abort_on_error
-     * is true, execution will stop at the first script to fail.
+     * Run all scripts in the specified directory.
+     *
+     * If abort_on_error is true, execution will stop at the first script
+     * to fail.  If failed_program is set, all program execution is skipped
+     * until after the specified program.
      *
      * @param command the command to run.
      * @param env the environment to use.
-     * @returns the exit status of the scripts.  This will be 0 on
-     * success, or the exit status of the last failing script.
+     * @param failed_program will be set, if abort_on_error is true
+     * @returns the exit status of the scripts.  This will be 0 on success,
+     * or the exit status of the last failing script.  If failed_program is
+     * set but not found, exit status is failed.
      */
     int
     run(string_list const& command,
-	environment const& env);
+	environment const& env, 
+	std::string      & failed_program);
 
     /**
      * Output the environment to an ostream.
diff --git a/sbuild/sbuild-session.cc b/sbuild/sbuild-session.cc
index a6bbe8d..ce849e5 100644
--- a/sbuild/sbuild-session.cc
+++ b/sbuild/sbuild-session.cc
@@ -72,6 +72,7 @@ namespace
       emap(session::CHILD_CORE,     N_("Child dumped core")),
       emap(session::CHILD_FAIL,     N_("Child exited abnormally (reason unknown; not a signal or core dump)")),
       emap(session::CHILD_FORK,     N_("Failed to fork child")),
+      emap(session::CHILD_PIPE,     N_("Failed to create pipe for child communication")),
       // TRANSLATORS: %4% = signal name
       emap(session::CHILD_SIGNAL,   N_("Child terminated by signal '%4%'")),
       emap(session::CHILD_WAIT,     N_("Wait for child failed")),
@@ -1120,6 +1121,13 @@ session::setup_chroot (sbuild::chroot::ptr&       session_chroot,
 
   int exit_status = 0;
   pid_t pid;
+  int pipefd[2];
+  
+  if (pipe(pipefd) == -1)
+    {
+      this->chroot_status = false;
+      throw error(session_chroot->get_name(), CHILD_PIPE, strerror(errno));
+    }
 
   if ((pid = fork()) == -1)
     {
@@ -1128,12 +1136,19 @@ session::setup_chroot (sbuild::chroot::ptr&       session_chroot,
     }
   else if (pid == 0)
     {
+      int status = EXIT_FAILURE;
+
+      // Close read end
+      close(pipefd[0]);
+
       try
 	{
 	  // The setup scripts don't use our syslog fd.
 	  closelog();
 
-	  chdir("/");
+	  if (chdir ("/") != 0)
+	    throw error("/", CHDIR, strerror(errno));
+
 	  /* This is required to ensure the scripts run with uid=0 and gid=0,
 	     otherwise setuid programs such as mount(8) will fail.  This
 	     should always succeed, because our euid=0 and egid=0.*/
@@ -1141,9 +1156,24 @@ session::setup_chroot (sbuild::chroot::ptr&       session_chroot,
 	  setgid(0);
 	  initgroups("root", 0);
 
-	  int status = rp.run(arg_list, env);
-
-	  _exit (status);
+	  status = rp.run(arg_list, env, this->failed_program);
+          if (status != EXIT_SUCCESS)
+            {
+	      ssize_t offset = 0;
+              ssize_t left = this->failed_program.length();
+	      while (left > 0) 
+		{
+		  ssize_t count = write(pipefd[1], 
+				this->failed_program.data() + offset, left);
+		  if (count == -1)
+		    left = 0;
+		  else if (count > 0)
+		    {
+		      left -= count;
+		      offset += count;
+		    }
+		}
+	    }
 	}
       catch (std::exception const& e)
 	{
@@ -1154,11 +1184,22 @@ session::setup_chroot (sbuild::chroot::ptr&       session_chroot,
 	  sbuild::log_error()
 	    << _("An unknown exception occurred") << std::endl;
 	}
-      _exit(EXIT_FAILURE);
+
+      // Close write end
+      close(pipefd[1]);
+
+      _exit(status);
     }
   else
     {
-      wait_for_child(pid, exit_status);
+      // Close write end
+      close(pipefd[1]);
+
+      this->failed_program.clear();
+      wait_for_child(pid, exit_status, pipefd[0], &this->failed_program);
+
+      // Close read end
+      close(pipefd[0]);
     }
 
   try
@@ -1213,7 +1254,7 @@ session::run_child (sbuild::chroot::ptr& session_chroot)
 			  << session_chroot->get_persona()<< std::endl;
 
   /* Enter the chroot */
-  if (chdir (location.c_str()))
+  if (chdir (location.c_str()) != 0)
     throw error(location, CHDIR, strerror(errno));
   log_debug(DEBUG_NOTICE) << "Changed directory to " << location << std::endl;
   if (::chroot (location.c_str()))
@@ -1246,7 +1287,7 @@ session::run_child (sbuild::chroot::ptr& session_chroot)
        dpos != dlist.end();
        ++dpos)
     {
-      if (chdir ((*dpos).c_str()) < 0)
+      if (chdir ((*dpos).c_str()) != 0)
 	{
 	  error e(*dpos, CHDIR, strerror(errno));
 
@@ -1312,14 +1353,43 @@ session::run_child (sbuild::chroot::ptr& session_chroot)
   _exit(EXIT_FAILURE);
 }
 
+#define BUFFER_SIZE 128
+
+static void
+flush_and_close (int          read_fd,
+		 std::string *data,
+		 char        *buffer)
+{
+  if (read_fd <= -1)
+    return;
+
+  while (1) 
+    {
+      ssize_t count = read(read_fd, buffer, BUFFER_SIZE);
+      if (count <= 0)
+	break;
+      if (data)
+        data->append(buffer, count);
+    }
+
+  close(read_fd);
+}
+
 void
-session::wait_for_child (pid_t pid,
-			 int&  child_status)
+session::wait_for_child (pid_t        pid,
+			 int&         child_status,
+			 int          read_fd,
+			 std::string *data)
 {
   child_status = EXIT_FAILURE; // Default exit status
 
-  int status;
+  int status, wait_status;
   bool child_killed = false;
+  int wait_option = (read_fd > -1) ? WNOHANG : 0;
+  char buffer[BUFFER_SIZE];
+
+  if (data != NULL)
+    data->clear();
 
   while (1)
     {
@@ -1343,27 +1413,44 @@ session::wait_for_child (pid_t pid,
 	  child_killed = true;
 	}
 
-      if (waitpid(pid, &status, 0) == -1)
+      wait_status = waitpid(pid, &status, wait_option);
+      if (wait_status == -1)
 	{
 	  if (errno == EINTR && (sighup_called || sigterm_called))
 	    continue; // Kill child and wait again.
 	  else
-	    throw error(CHILD_WAIT, strerror(errno));
+	    {
+	      flush_and_close(read_fd, data, buffer);
+	      throw error(CHILD_WAIT, strerror(errno));
+	    }
 	}
       else if (sighup_called)
 	{
 	  sighup_called = false;
+	  flush_and_close(read_fd, data, buffer);
 	  throw error(SIGNAL_CATCH, strsignal(SIGHUP));
 	}
       else if (sigterm_called)
 	{
 	  sigterm_called = false;
+	  flush_and_close(read_fd, data, buffer);
 	  throw error(SIGNAL_CATCH, strsignal(SIGTERM));
 	}
+      else if (wait_option)
+	{
+	  if (wait_status == pid)
+	    break;
+
+	  ssize_t count = read(read_fd, buffer, BUFFER_SIZE);
+	  if (data && (count > 0))
+	    data->append(buffer, count);
+	}
       else
 	break;
     }
 
+  flush_and_close(read_fd, data, buffer);
+
   if (!WIFEXITED(status))
     {
       if (WIFSIGNALED(status))
@@ -1377,6 +1464,8 @@ session::wait_for_child (pid_t pid,
   child_status = WEXITSTATUS(status);
 }
 
+#undef BUFFER_SIZE
+
 void
 session::run_chroot (sbuild::chroot::ptr& session_chroot)
 {
diff --git a/sbuild/sbuild-session.h b/sbuild/sbuild-session.h
index 966cec2..4e11932 100644
--- a/sbuild/sbuild-session.h
+++ b/sbuild/sbuild-session.h
@@ -64,6 +64,7 @@ namespace sbuild
 	CHILD_CORE,     ///< Child dumped core.
 	CHILD_FAIL,     ///< Child exited abnormally (reason unknown)
 	CHILD_FORK,     ///< Failed to fork child.
+	CHILD_PIPE,     ///< Failed to create pipe for child communication.
 	CHILD_SIGNAL,   ///< Child terminated by signal.
 	CHILD_WAIT,     ///< Wait for child failed.
 	CHROOT,         ///< Failed to change root to directory.
@@ -386,14 +387,21 @@ namespace sbuild
     /**
      * Wait for a child process to complete, and check its exit status.
      *
+     * Optionally read data from a child pipe while waiting. If the data 
+     * parameter is omitted, data will be read but tossed.
+     *
      * An error will be thrown on failure.
      *
      * @param pid the pid to wait for.
      * @param child_status the place to store the child exit status.
+     * @param read_fd readable pipe fd to get data from
+     * @param data pointer to a std:string to store the data.
      */
     void
-    wait_for_child (pid_t pid,
-		    int&  child_status);
+    wait_for_child (pid_t        pid,
+		    int&         child_status,
+		    int          read_fd = -1,
+		    std::string *data = 0);
 
     /**
      * Set the SIGHUP handler.
@@ -456,7 +464,7 @@ namespace sbuild
     /// The current chroot status.
     int              chroot_status;
     /// Lock status for locks acquired during chroot setup.
-    bool lock_status;
+    bool             lock_status;
     /// The child exit status.
     int              child_status;
     /// The session operation to perform.
@@ -470,9 +478,11 @@ namespace sbuild
     /// Signal saved while sigterm handler is set.
     struct sigaction saved_sigterm_signal;
     /// Saved terminal settings.
-    struct termios saved_termios;
+    struct termios   saved_termios;
     /// Are the saved terminal settings valid?
-    bool termios_ok;
+    bool             termios_ok;
+    /// Last failed program / script
+    std::string      failed_program;
 
   protected:
     /// Current working directory.
diff --git a/test/sbuild-run-parts.cc b/test/sbuild-run-parts.cc
index 208337c..8ba878d 100644
--- a/test/sbuild-run-parts.cc
+++ b/test/sbuild-run-parts.cc
@@ -80,22 +80,26 @@ public:
 
     int status;
 
+    std::string failed_program;
     sbuild::string_list command;
     sbuild::environment env(environ);
 
     command.push_back("ok");
-    status = rp.run(command, env);
+    status = rp.run(command, env, failed_program);
     CPPUNIT_ASSERT(status == EXIT_SUCCESS);
+    CPPUNIT_ASSERT(failed_program.empty());
 
     command.clear();
     command.push_back("fail");
-    status = rp.run(command, env);
+    status = rp.run(command, env, failed_program);
     CPPUNIT_ASSERT(status == EXIT_FAILURE);
+    CPPUNIT_ASSERT(failed_program == "20test2");
 
     command.clear();
     command.push_back("fail2");
-    status = rp.run(command, env);
+    status = rp.run(command, env, failed_program);
     CPPUNIT_ASSERT(status == EXIT_FAILURE);
+    CPPUNIT_ASSERT(failed_program == "30test3");
   }
 
   void test_run2()
@@ -104,12 +108,14 @@ public:
 
     int status;
 
+    std::string failed_program;
     sbuild::string_list command;
     sbuild::environment env(environ);
 
     command.push_back("ok");
-    status = rp.run(command, env);
+    status = rp.run(command, env, failed_program);
     CPPUNIT_ASSERT(status == EXIT_SUCCESS);
+    CPPUNIT_ASSERT(failed_program.empty());
   }
 
   void test_run3()
@@ -118,12 +124,14 @@ public:
 
     int status;
 
+    std::string failed_program;
     sbuild::string_list command;
     sbuild::environment env(environ);
 
     command.push_back("ok");
-    status = rp.run(command, env);
+    status = rp.run(command, env, failed_program);
     CPPUNIT_ASSERT(status == EXIT_FAILURE);
+    CPPUNIT_ASSERT(failed_program == "50invalid");
   }
 
 };
-- 
1.6.2.1




More information about the Buildd-tools-devel mailing list