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