Fix running tests under valgrind 19/31219/3
authorPiotr Bartosiewicz <p.bartosiewi@partner.samsung.com>
Tue, 2 Dec 2014 15:27:25 +0000 (16:27 +0100)
committerPiotr Bartosiewicz <p.bartosiewi@partner.samsung.com>
Wed, 3 Dec 2014 14:25:28 +0000 (15:25 +0100)
[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
common/lxc/domain.hpp
common/utils/c-array.hpp [new file with mode: 0644]
common/utils/execute.cpp [new file with mode: 0644]
common/utils/execute.hpp [new file with mode: 0644]
server/container-admin.cpp
tests/unit_tests/lxc/ut-domain.cpp

index 191f405..089799d 100644 (file)
  * @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 <lxc/lxccontainer.h>
 #include <sys/stat.h>
@@ -39,18 +51,36 @@ namespace security_containers {
 namespace lxc {
 
 namespace {
+#define ITEM(X) {#X, LxcDomain::State::X},
     const std::map<std::string, LxcDomain::State> 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<char* const*>(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<char* const*>(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
index 60ae5a4..3c45b85 100644 (file)
@@ -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 (file)
index 0000000..73b037b
--- /dev/null
@@ -0,0 +1,73 @@
+/*
+ *  Copyright (c) 2014 Samsung Electronics Co., Ltd All Rights Reserved
+ *
+ *  Contact: Piotr Bartosiewicz <p.bartosiewi@partner.samsung.com>
+ *
+ *  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 <vector>
+
+namespace security_containers {
+namespace utils {
+
+template<typename T>
+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<T> mArray;
+};
+
+typedef CArrayBuilder<const char*> 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 (file)
index 0000000..2f2e927
--- /dev/null
@@ -0,0 +1,92 @@
+/*
+ *  Copyright (c) 2014 Samsung Electronics Co., Ltd All Rights Reserved
+ *
+ *  Contact: Piotr Bartosiewicz <p.bartosiewi@partner.samsung.com>
+ *
+ *  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 <stdlib.h>
+#include <unistd.h>
+#include <sys/wait.h>
+
+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<char* const*>(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 (file)
index 0000000..1fc7b29
--- /dev/null
@@ -0,0 +1,40 @@
+/*
+ *  Copyright (c) 2014 Samsung Electronics Co., Ltd All Rights Reserved
+ *
+ *  Contact: Piotr Bartosiewicz <p.bartosiewi@partner.samsung.com>
+ *
+ *  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
index 65179c3..1d8cdee 100644 (file)
@@ -29,6 +29,7 @@
 
 #include "logger/logger.hpp"
 #include "utils/paths.hpp"
+#include "utils/c-array.hpp"
 
 #include <cassert>
 
@@ -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<std::string>& 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<const char*> 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<std::string> 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");
     }
 
index a529f9f..a9942b8 100644 (file)
@@ -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);