From a41c5f342fb73d6abd12298c103182bfabc979cc Mon Sep 17 00:00:00 2001
From: Piotr Bartosiewicz
Date: Tue, 2 Dec 2014 16:27:25 +0100
Subject: [PATCH] Fix running tests under valgrind
[Bug/Feature] Running LXC commands via exec, new building flag
USE_EXEC
[Cause] N/A
[Solution] N/A
[Verification] Build with and without USE_EXEC, run tests
Change-Id: I42a2745d81d309922704bd213a72727b4c613c40
---
common/lxc/domain.cpp | 160 ++++++++++++++++++++++++++++++++-----
common/lxc/domain.hpp | 63 +++++++++++++++
common/utils/c-array.hpp | 73 +++++++++++++++++
common/utils/execute.cpp | 92 +++++++++++++++++++++
common/utils/execute.hpp | 40 ++++++++++
server/container-admin.cpp | 60 ++++----------
tests/unit_tests/lxc/ut-domain.cpp | 1 -
7 files changed, 422 insertions(+), 67 deletions(-)
create mode 100644 common/utils/c-array.hpp
create mode 100644 common/utils/execute.cpp
create mode 100644 common/utils/execute.hpp
diff --git a/common/lxc/domain.cpp b/common/lxc/domain.cpp
index 191f405..089799d 100644
--- a/common/lxc/domain.cpp
+++ b/common/lxc/domain.cpp
@@ -22,10 +22,22 @@
* @brief Lxc domain
*/
+// Define macro USE_EXEC if you are goind to use valgrind
+// TODO Consider always using exec to control lxc. Advantages:
+// - 'ps' output is more descriptive with lxc-start
+// - lxc library is not tested well with multithread programs
+// (there are still some issues like using umask etc.)
+// - it could be possible not to be uid 0 in this process
+// - fork + unshare does not work with traced process (e.g. under valgrind)
+
#include "config.hpp"
#include "logger/logger.hpp"
#include "lxc/domain.hpp"
#include "lxc/exception.hpp"
+#ifdef USE_EXEC
+#include "utils/execute.hpp"
+#include "utils/c-array.hpp"
+#endif
#include
#include
@@ -39,18 +51,36 @@ namespace security_containers {
namespace lxc {
namespace {
+#define ITEM(X) {#X, LxcDomain::State::X},
const std::map STATE_MAP = {
- {"STOPPED", LxcDomain::State::STOPPED},
- {"STARTING", LxcDomain::State::STARTING},
- {"RUNNING", LxcDomain::State::RUNNING},
- {"STOPPING", LxcDomain::State::STOPPING},
- {"ABORTING", LxcDomain::State::ABORTING},
- {"FREEZING", LxcDomain::State::FREEZING},
- {"FROZEN", LxcDomain::State::FROZEN},
- {"THAWED", LxcDomain::State::THAWED}
+ ITEM(STOPPED)
+ ITEM(STARTING)
+ ITEM(RUNNING)
+ ITEM(STOPPING)
+ ITEM(ABORTING)
+ ITEM(FREEZING)
+ ITEM(FROZEN)
+ ITEM(THAWED)
};
+#undef ITEM
} // namespace
+std::string LxcDomain::toString(State state)
+{
+#define CASE(X) case LxcDomain::State::X: return #X;
+ switch (state) {
+ CASE(STOPPED)
+ CASE(STARTING)
+ CASE(RUNNING)
+ CASE(STOPPING)
+ CASE(ABORTING)
+ CASE(FREEZING)
+ CASE(FROZEN)
+ CASE(THAWED)
+ }
+#undef CASE
+}
+
LxcDomain::LxcDomain(const std::string& lxcPath, const std::string& domainName)
: mContainer(nullptr)
{
@@ -76,7 +106,7 @@ std::string LxcDomain::getConfigItem(const std::string& key)
char buffer[1024];
int len = mContainer->get_config_item(mContainer, key.c_str(), buffer, sizeof(buffer));
if (len < 0) {
- LOGE("Key '" + key + "' not found in domain " + getName());
+ LOGE("Key '" << key << "' not found in domain " << getName());
throw LxcException("Key not found");
}
return buffer;
@@ -95,20 +125,40 @@ LxcDomain::State LxcDomain::getState()
bool LxcDomain::create(const std::string& templatePath, const char* const* argv)
{
+#ifdef USE_EXEC
+ utils::CStringArrayBuilder args;
+ args.add("lxc-create")
+ .add("-n").add(mContainer->name)
+ .add("-t").add(templatePath.c_str())
+ .add("-P").add(mContainer->config_path);
+
+ while (*argv) {
+ args.add(*argv++);
+ }
+
+ if (!utils::executeAndWait("/usr/bin/lxc-create", args.c_array())) {
+ LOGE("Could not create domain " << getName());
+ return false;
+ }
+
+ refresh();
+ return true;
+#else
if (!mContainer->create(mContainer,
templatePath.c_str(),
NULL, NULL, 0,
const_cast(argv))) {
- LOGE("Could not create domain " + getName());
+ LOGE("Could not create domain " << getName());
return false;
}
return true;
+#endif
}
bool LxcDomain::destroy()
{
if (!mContainer->destroy(mContainer)) {
- LOGE("Could not destroy domain " + getName());
+ LOGE("Could not destroy domain " << getName());
return false;
}
return true;
@@ -116,25 +166,57 @@ bool LxcDomain::destroy()
bool LxcDomain::start(const char* const* argv)
{
+#ifdef USE_EXEC
+ if (mContainer->is_running(mContainer)) {
+ LOGE("Already started " << getName());
+ return false;
+ }
+
+ utils::CStringArrayBuilder args;
+ args.add("lxc-start")
+ .add("-d")
+ .add("-n").add(mContainer->name)
+ .add("-P").add(mContainer->config_path)
+ .add("--");
+
+ while (*argv) {
+ args.add(*argv++);
+ }
+
+ if (!utils::executeAndWait("/usr/bin/lxc-start", args.c_array())) {
+ LOGE("Could not start domain " << getName());
+ return false;
+ }
+
+ refresh();
+
+ // we have to check status because lxc-start runs in daemonized mode
+ if (!mContainer->is_running(mContainer)) {
+ LOGE("Could not start init in domain " << getName());
+ return false;
+ }
+ return true;
+#else
if (mContainer->is_running(mContainer)) {
- LOGE("Already started " + getName());
+ LOGE("Already started " << getName());
return false;
}
if (!mContainer->want_daemonize(mContainer, true)) {
- LOGE("Could not configure domain " + getName());
+ LOGE("Could not configure domain " << getName());
return false;
}
if (!mContainer->start(mContainer, false, const_cast(argv))) {
- LOGE("Could not start domain " + getName());
+ LOGE("Could not start domain " << getName());
return false;
}
return true;
+#endif
}
bool LxcDomain::stop()
{
if (!mContainer->stop(mContainer)) {
- LOGE("Could not stop domain " + getName());
+ LOGE("Could not stop domain " << getName());
return false;
}
return true;
@@ -143,7 +225,7 @@ bool LxcDomain::stop()
bool LxcDomain::reboot()
{
if (!mContainer->reboot(mContainer)) {
- LOGE("Could not reboot domain " + getName());
+ LOGE("Could not reboot domain " << getName());
return false;
}
return true;
@@ -160,10 +242,27 @@ bool LxcDomain::shutdown(int timeout)
return false;
}
+#ifdef USE_EXEC
+ utils::CStringArrayBuilder args;
+ std::string timeoutStr = std::to_string(timeout);
+ args.add("lxc-stop")
+ .add("-n").add(mContainer->name)
+ .add("-P").add(mContainer->config_path)
+ .add("-t").add(timeoutStr.c_str())
+ .add("--nokill");
+
+ if (!utils::executeAndWait("/usr/bin/lxc-stop", args.c_array())) {
+ LOGE("Could not gracefully shutdown domain " << getName() << " in " << timeout << "s");
+ return false;
+ }
+
+ refresh();
+ return true;
+#else
// try shutdown by sending poweroff to init
if (setRunLevel(utils::RUNLEVEL_POWEROFF)) {
- if (!mContainer->wait(mContainer, "STOPPED", timeout)) {
- LOGE("Could not gracefully shutdown domain " + getName() + " in " << timeout << "s");
+ if (!waitForState(State::STOPPED, timeout)) {
+ LOGE("Could not gracefully shutdown domain " << getName() << " in " << timeout << "s");
return false;
}
return true;
@@ -172,16 +271,17 @@ bool LxcDomain::shutdown(int timeout)
// fallback for other inits like bash: lxc sends 'lxc.haltsignal' signal to init
if (!mContainer->shutdown(mContainer, timeout)) {
- LOGE("Could not gracefully shutdown domain " + getName() + " in " << timeout << "s");
+ LOGE("Could not gracefully shutdown domain " << getName() << " in " << timeout << "s");
return false;
}
return true;
+#endif
}
bool LxcDomain::freeze()
{
if (!mContainer->freeze(mContainer)) {
- LOGE("Could not freeze domain " + getName());
+ LOGE("Could not freeze domain " << getName());
return false;
}
return true;
@@ -190,7 +290,16 @@ bool LxcDomain::freeze()
bool LxcDomain::unfreeze()
{
if (!mContainer->unfreeze(mContainer)) {
- LOGE("Could not unfreeze domain " + getName());
+ LOGE("Could not unfreeze domain " << getName());
+ return false;
+ }
+ return true;
+}
+
+bool LxcDomain::waitForState(State state, int timeout)
+{
+ if (!mContainer->wait(mContainer, toString(state).c_str(), timeout)) {
+ LOGD("Timeout while waiting for state " << toString(state) << " of domain " << getName());
return false;
}
return true;
@@ -216,6 +325,15 @@ bool LxcDomain::setRunLevel(int runLevel)
return status == 0;
}
+void LxcDomain::refresh()
+{
+ //TODO Consider make LxcDomain state-less
+ std::string domainName = mContainer->name;
+ std::string lxcPath = mContainer->config_path;
+ lxc_container_put(mContainer);
+ mContainer = lxc_container_new(domainName.c_str(), lxcPath.c_str());
+}
+
} // namespace lxc
} // namespace security_containers
diff --git a/common/lxc/domain.hpp b/common/lxc/domain.hpp
index 60ae5a4..3c45b85 100644
--- a/common/lxc/domain.hpp
+++ b/common/lxc/domain.hpp
@@ -50,34 +50,97 @@ public:
THAWED
};
+ /**
+ * LxcDomain constructor
+ * @param lxcPath path where containers lives
+ * @param domainName name of domain
+ */
LxcDomain(const std::string& lxcPath, const std::string& domainName);
~LxcDomain();
LxcDomain(const LxcDomain&) = delete;
LxcDomain& operator=(const LxcDomain&) = delete;
+ /**
+ * Get domain name
+ */
std::string getName() const;
+ /**
+ * Get item from lxc config file
+ * @throw LxcException if key not found
+ */
std::string getConfigItem(const std::string& key);
+ /**
+ * Is domain defined (created)?
+ */
bool isDefined();
+ /**
+ * String representation of state
+ */
+ static std::string toString(State state);
+
+ /**
+ * Get domain state
+ */
State getState();
+ /**
+ * Wait till domain is in specified state
+ * @return false on timeout
+ */
+ bool waitForState(State state, int timeout);
+
+ /**
+ * Create domain
+ * @param templatePath template from which domain will be created
+ * @param argv additional template arguments
+ */
bool create(const std::string& templatePath, const char* const* argv);
+
+ /**
+ * Destroy domain
+ */
bool destroy();
+ /**
+ * Start domain
+ * @param argv init process with arguments
+ */
bool start(const char* const* argv);
+
+ /**
+ * Immediate stop the domain
+ * It kills all processes within this domain
+ */
bool stop();
+
+ /**
+ * Reboot domain
+ */
bool reboot();
+
+ /**
+ * Gracefully shutdown domain.
+ */
bool shutdown(int timeout);
+ /**
+ * Freeze (pause/lock) domain
+ */
bool freeze();
+
+ /**
+ * Unfreeze domain
+ */
bool unfreeze();
private:
lxc_container* mContainer;
bool setRunLevel(int runLevel);
+ void refresh();
};
diff --git a/common/utils/c-array.hpp b/common/utils/c-array.hpp
new file mode 100644
index 0000000..73b037b
--- /dev/null
+++ b/common/utils/c-array.hpp
@@ -0,0 +1,73 @@
+/*
+ * Copyright (c) 2014 Samsung Electronics Co., Ltd All Rights Reserved
+ *
+ * Contact: Piotr Bartosiewicz
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License
+ */
+
+/**
+ * @file
+ * @author Piotr Bartosiewicz (p.bartosiewi@partner.samsung.com)
+ * @brief C array helper
+ */
+
+#ifndef COMMON_UTILS_C_ARRAY_HPP
+#define COMMON_UTILS_C_ARRAY_HPP
+
+#include
+
+namespace security_containers {
+namespace utils {
+
+template
+class CArrayBuilder {
+public:
+ CArrayBuilder(): mArray(1, nullptr) {}
+
+ CArrayBuilder& add(const T& v) {
+ mArray.back() = v;
+ mArray.push_back(nullptr);
+ return *this;
+ }
+
+ const T* c_array() const {
+ return mArray.data();
+ }
+
+ size_t size() const {
+ return mArray.size() - 1;
+ }
+
+ bool empty() const {
+ return size() == 0;
+ }
+
+ const T* begin() const {
+ return &*mArray.begin();
+ }
+
+ const T* end() const {
+ return &*(mArray.end() - 1);
+ }
+private:
+ std::vector mArray;
+};
+
+typedef CArrayBuilder CStringArrayBuilder;
+
+} // namespace utils
+} // namespace security_containers
+
+
+#endif // COMMON_UTILS_C_ARRAY_HPP
diff --git a/common/utils/execute.cpp b/common/utils/execute.cpp
new file mode 100644
index 0000000..2f2e927
--- /dev/null
+++ b/common/utils/execute.cpp
@@ -0,0 +1,92 @@
+/*
+ * Copyright (c) 2014 Samsung Electronics Co., Ltd All Rights Reserved
+ *
+ * Contact: Piotr Bartosiewicz
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License
+ */
+
+/**
+ * @file
+ * @author Piotr Bartosiewicz (p.bartosiewi@partner.samsung.com)
+ * @brief Execute utils
+ */
+
+#include "config.hpp"
+#include "utils/execute.hpp"
+#include "logger/logger.hpp"
+
+#include
+#include
+#include
+
+namespace security_containers {
+namespace utils {
+
+namespace {
+
+std::ostream& operator<< (std::ostream& out, const char* const* argv)
+{
+ if (*argv) {
+ argv++; //skip
+ }
+ while (*argv) {
+ out << " '" << *argv++ << "'";
+ }
+ return out;
+}
+
+} // namespace
+
+bool executeAndWait(const char* fname, const char* const* argv, int& status)
+{
+ LOGD("Execute " << fname << argv);
+
+ pid_t pid = fork();
+ if (pid == -1) {
+ LOGE("Fork failed");
+ return false;
+ }
+ if (pid == 0) {
+ execv(fname, const_cast(argv));
+ _exit(EXIT_FAILURE);
+ }
+ int ret = waitpid(pid, &status, 0);
+ if (ret != pid) {
+ LOGE("Waitpid failed");
+ return false;
+ }
+ return true;
+}
+
+bool executeAndWait(const char* fname, const char* const* argv)
+{
+ int status;
+ if (!executeAndWait(fname, argv, status)) {
+ return false;
+ }
+ if (status != EXIT_SUCCESS) {
+ if (WIFEXITED(status)) {
+ LOGW("Process " << fname << " has exited with status " << WEXITSTATUS(status));
+ } else if (WIFSIGNALED(status)) {
+ LOGW("Process " << fname << " was killed by signal " << WTERMSIG(status));
+ } else if (WIFSTOPPED(status)) {
+ LOGW("Process " << fname << " was stopped by signal " << WSTOPSIG(status));
+ }
+ return false;
+ }
+ return true;
+}
+
+} // namespace utils
+} // namespace security_containers
diff --git a/common/utils/execute.hpp b/common/utils/execute.hpp
new file mode 100644
index 0000000..1fc7b29
--- /dev/null
+++ b/common/utils/execute.hpp
@@ -0,0 +1,40 @@
+/*
+ * Copyright (c) 2014 Samsung Electronics Co., Ltd All Rights Reserved
+ *
+ * Contact: Piotr Bartosiewicz
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License
+ */
+
+/**
+ * @file
+ * @author Piotr Bartosiewicz (p.bartosiewi@partner.samsung.com)
+ * @brief Execute utils
+ */
+
+#ifndef COMMON_UTILS_EXECUTE_HPP
+#define COMMON_UTILS_EXECUTE_HPP
+
+
+namespace security_containers {
+namespace utils {
+
+bool executeAndWait(const char* fname, const char* const* argv);
+
+bool executeAndWait(const char* fname, const char* const* argv, int& status);
+
+} // namespace utils
+} // namespace security_containers
+
+
+#endif // COMMON_UTILS_EXECUTE_HPP
diff --git a/server/container-admin.cpp b/server/container-admin.cpp
index 65179c3..1d8cdee 100644
--- a/server/container-admin.cpp
+++ b/server/container-admin.cpp
@@ -29,6 +29,7 @@
#include "logger/logger.hpp"
#include "utils/paths.hpp"
+#include "utils/c-array.hpp"
#include
@@ -40,37 +41,6 @@ namespace {
// TODO: this should be in container's configuration file
const int SHUTDOWN_WAIT = 10;
-class Args {
-public:
- Args(const std::vector& args)
- {
- mArgs.reserve(args.size() + 1);
- for (const std::string& arg : args) {
- mArgs.push_back(arg.c_str());
- }
- mArgs.push_back(NULL);
- }
- bool empty() const
- {
- return mArgs.size() == 1;
- }
- const char* const* getAsCArray() const
- {
- return mArgs.data();
- }
- friend std::ostream& operator<<(std::ostream& os, const Args& a)
- {
- for (const char* arg : a.mArgs) {
- if (arg != NULL) {
- os << "'" << arg << "' ";
- }
- }
- return os;
- }
-private:
- std::vector mArgs;
-};
-
} // namespace
const std::uint64_t DEFAULT_CPU_SHARES = 1024;
@@ -82,7 +52,8 @@ ContainerAdmin::ContainerAdmin(const std::string& containersPath,
: mConfig(config),
mDom(containersPath, config.name),
mId(mDom.getName()),
- mDetachOnExit(false)
+ mDetachOnExit(false),
+ mDestroyOnExit(false)
{
LOGD(mId << ": Instantiating ContainerAdmin object");
@@ -91,16 +62,16 @@ ContainerAdmin::ContainerAdmin(const std::string& containersPath,
const std::string lxcTemplate = utils::getAbsolutePath(config.lxcTemplate,
lxcTemplatePrefix);
LOGI(mId << ": Creating domain from template: " << lxcTemplate);
- std::vector args;
+ utils::CStringArrayBuilder args;
if (!config.ipv4Gateway.empty()) {
- args.push_back("--ipv4-gateway");
- args.push_back(config.ipv4Gateway);
+ args.add("--ipv4-gateway");
+ args.add(config.ipv4Gateway.c_str());
}
if (!config.ipv4.empty()) {
- args.push_back("--ipv4");
- args.push_back(config.ipv4);
+ args.add("--ipv4");
+ args.add(config.ipv4.c_str());
}
- if (!mDom.create(lxcTemplate, Args(args).getAsCArray())) {
+ if (!mDom.create(lxcTemplate, args.c_array())) {
throw ContainerOperationException("Could not create domain");
}
}
@@ -145,16 +116,15 @@ void ContainerAdmin::start()
return;
}
- const Args args(mConfig.initWithArgs);
- bool result;
+ utils::CStringArrayBuilder args;
+ for (const std::string& arg : mConfig.initWithArgs) {
+ args.add(arg.c_str());
+ }
if (args.empty()) {
- result = mDom.start(NULL);
- } else {
- LOGD(mId << ": Init: " << args);
- result = mDom.start(args.getAsCArray());
+ args.add("/sbin/init");
}
- if (!result) {
+ if (!mDom.start(args.c_array())) {
throw ContainerOperationException("Could not start container");
}
diff --git a/tests/unit_tests/lxc/ut-domain.cpp b/tests/unit_tests/lxc/ut-domain.cpp
index a529f9f..a9942b8 100644
--- a/tests/unit_tests/lxc/ut-domain.cpp
+++ b/tests/unit_tests/lxc/ut-domain.cpp
@@ -161,7 +161,6 @@ BOOST_AUTO_TEST_CASE(StartHasStoppedTest)
NULL
};
BOOST_CHECK(lxc.start(argv));
- BOOST_CHECK(lxc.getState() == LxcDomain::State::RUNNING);
waitForInit();
BOOST_CHECK(lxc.getState() == LxcDomain::State::STOPPED);
--
2.7.4