[buildd-tools-devel] [PATCH 6/8] Don't rollback non-run scripts
Jan-Marek Glogowski
glogow at fbihome.de
Wed May 20 17:55:56 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-chroot-loopback.cc | 167 ++++++++++++++++++++++++++++++++++++++++
test/sbuild-run-parts.cc | 18 +++-
6 files changed, 350 insertions(+), 44 deletions(-)
create mode 100644 test/sbuild-chroot-loopback.cc
diff --git a/sbuild/sbuild-run-parts.cc b/sbuild/sbuild-run-parts.cc
index 86f251a..68d8b42 100644
--- a/sbuild/sbuild-run-parts.cc
+++ b/sbuild/sbuild-run-parts.cc
@@ -120,9 +120,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)
{
@@ -130,17 +138,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
@@ -149,17 +166,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 7b5b6e6..b29b1a3 100644
--- a/sbuild/sbuild-run-parts.h
+++ b/sbuild/sbuild-run-parts.h
@@ -108,17 +108,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 3551e01..78b2ce8 100644
--- a/sbuild/sbuild-session.cc
+++ b/sbuild/sbuild-session.cc
@@ -76,6 +76,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")),
@@ -1090,6 +1091,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)
{
@@ -1098,12 +1106,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.*/
@@ -1111,9 +1126,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)
{
@@ -1124,11 +1154,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
@@ -1183,7 +1224,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()))
@@ -1216,7 +1257,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));
@@ -1282,14 +1323,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)
{
@@ -1313,27 +1383,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))
@@ -1347,6 +1434,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 f3f2ed7..c92c2b5 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.
@@ -383,14 +384,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.
@@ -453,7 +461,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.
@@ -467,9 +475,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-chroot-loopback.cc b/test/sbuild-chroot-loopback.cc
new file mode 100644
index 0000000..395bc4c
--- /dev/null
+++ b/test/sbuild-chroot-loopback.cc
@@ -0,0 +1,167 @@
+/* Copyright © 2008-2009 Jan-Marek Glogowski <glogow at fbihome.de>
+ *
+ * schroot is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * schroot is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see
+ * <http://www.gnu.org/licenses/>.
+ *
+ *********************************************************************/
+
+#include "config.h"
+
+#include <algorithm>
+#include <set>
+
+#include <sbuild/sbuild-chroot-loopback.h>
+
+#include "test-helpers.h"
+#include "test-sbuild-chroot.h"
+
+#include <cppunit/extensions/HelperMacros.h>
+
+#include <iostream>
+
+using std::cout;
+using std::endl;
+
+using namespace CppUnit;
+
+class chroot_loopback : public sbuild::chroot_loopback
+{
+public:
+ chroot_loopback():
+ sbuild::chroot_loopback()
+ {}
+
+ virtual ~chroot_loopback()
+ {}
+};
+
+class test_chroot_loopback : public test_chroot_base<chroot_loopback>
+{
+ CPPUNIT_TEST_SUITE(test_chroot_loopback);
+ CPPUNIT_TEST(test_file);
+ CPPUNIT_TEST(test_mount_options);
+ CPPUNIT_TEST(test_chroot_type);
+ CPPUNIT_TEST(test_setup_env);
+ CPPUNIT_TEST(test_setup_env2);
+ CPPUNIT_TEST(test_session_flags);
+ CPPUNIT_TEST(test_print_details);
+ CPPUNIT_TEST(test_print_config);
+ CPPUNIT_TEST_SUITE_END();
+
+protected:
+ std::string loopback_file;
+
+public:
+ test_chroot_loopback():
+ test_chroot_base<chroot_loopback>(),
+ loopback_file()
+ {
+ loopback_file = abs_testdata_dir;
+ loopback_file.append("/loopback-file");
+ }
+
+ void setUp()
+ {
+ test_chroot_base<chroot_loopback>::setUp();
+ sbuild::chroot_loopback *c = dynamic_cast<sbuild::chroot_loopback *>(chroot.get());
+ c->set_mount_options("-t jfs -o quota,rw");
+ }
+
+ void
+ test_file()
+ {
+ sbuild::chroot_loopback *c = dynamic_cast<sbuild::chroot_loopback *>(chroot.get());
+ CPPUNIT_ASSERT(c);
+ c->set_file("/dev/some/file");
+ CPPUNIT_ASSERT(c->get_file() == "/dev/some/file");
+ }
+
+ void
+ test_mount_options()
+ {
+ sbuild::chroot_loopback *c = dynamic_cast<sbuild::chroot_loopback *>(chroot.get());
+ CPPUNIT_ASSERT(c);
+ c->set_mount_options("-o opt1,opt2");
+ CPPUNIT_ASSERT(c->get_mount_options() == "-o opt1,opt2");
+ }
+
+ void test_chroot_type()
+ {
+ CPPUNIT_ASSERT(chroot->get_chroot_type() == "loopback");
+ }
+
+ void setup_env_common(sbuild::environment& expected)
+ {
+ expected.add("CHROOT_TYPE", "loopback");
+ expected.add("CHROOT_NAME", "test-name");
+ expected.add("CHROOT_DESCRIPTION", "test-description");
+ expected.add("CHROOT_MOUNT_LOCATION", "/mnt/mount-location");
+ expected.add("CHROOT_PATH", "/mnt/mount-location");
+ expected.add("CHROOT_MOUNT_OPTIONS", "-t jfs -o quota,rw");
+ expected.add("CHROOT_SCRIPT_CONFIG", sbuild::normalname(std::string(PACKAGE_SYSCONF_DIR) + "/script-defaults"));
+ expected.add("CHROOT_SESSION_CLONE", "false");
+ expected.add("CHROOT_SESSION_CREATE", "false");
+ expected.add("CHROOT_SESSION_PURGE", "false");
+ expected.add("CHROOT_FS_UNION_TYPE", "none");
+ }
+
+ void test_setup_env()
+ {
+ sbuild::environment expected;
+ setup_env_common(expected);
+
+ test_chroot_base<chroot_loopback>::test_setup_env(expected);
+ }
+
+ void test_setup_env2()
+ {
+ sbuild::chroot_loopback *c = dynamic_cast<sbuild::chroot_loopback *>(chroot.get());
+ CPPUNIT_ASSERT(c);
+ c->set_file(loopback_file);
+
+ sbuild::environment expected;
+ setup_env_common(expected);
+
+ expected.add("CHROOT_FILE", loopback_file);
+ expected.add("CHROOT_MOUNT_UUID", FS_UUID);
+
+ test_chroot_base<chroot_loopback>::test_setup_env(expected);
+ }
+
+ void test_session_flags()
+ {
+ CPPUNIT_ASSERT(chroot->get_session_flags() ==
+ sbuild::chroot::SESSION_NOFLAGS);
+ }
+
+ void test_print_details()
+ {
+ std::ostringstream os;
+ os << chroot;
+ // TODO: Compare output.
+ CPPUNIT_ASSERT(!os.str().empty());
+ }
+
+ void test_print_config()
+ {
+ std::ostringstream os;
+ sbuild::keyfile config;
+ config << chroot;
+ os << config;
+ // TODO: Compare output.
+ CPPUNIT_ASSERT(!os.str().empty());
+ }
+};
+
+CPPUNIT_TEST_SUITE_REGISTRATION(test_chroot_loopback);
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.3.1
More information about the Buildd-tools-devel
mailing list