Handle reconnecting to the dbus, add glib helper
authorLukasz Pawelczyk <l.pawelczyk@partner.samsung.com>
Tue, 6 May 2014 10:42:28 +0000 (12:42 +0200)
committerJan Olszak <j.olszak@samsung.com>
Mon, 19 May 2014 11:47:16 +0000 (13:47 +0200)
[Bug/Feature]   Handle reconnecting to the dbus in case of the
                connection loss. Also add a Glib helper for
                scheduling a timer function to the glib loop.
                Style cosmetics.
[Cause]         In case the DBUS daemon gets restarted we loose
                connection, we need to handle that case.
[Solution]      Detect nameLost event and react appropriately.
[Verification]  Built, installed, run tests.
                The reconnect has been tested by hand as follows:
                1. Run the security-containers-server
                2. Make sure it started properly
                3. Enter the container and restart dbus
                4. See SCS logs, make sure it reconnected
                5. Enter the container stop dbus and dbus.socket
                6. See SCS logs, make sure the container stopped

Change-Id: I1185d8d46e0ace8e96b4d4136fbca20bd603bea9
Signed-off-by: Lukasz Pawelczyk <l.pawelczyk@partner.samsung.com>
20 files changed:
common/dbus/connection.cpp
common/log/backend-stderr.cpp
common/utils/file-wait.cpp
common/utils/fs.cpp
common/utils/glib-loop.cpp
common/utils/glib-loop.hpp
common/utils/latch.cpp
common/utils/latch.hpp
server/container-connection-transport.cpp
server/container-connection.cpp
server/container.cpp
server/container.hpp
server/containers-manager.cpp
unit_tests/server/configs/CMakeLists.txt
unit_tests/server/configs/ut-container/containers/test-dbus.conf [new file with mode: 0644]
unit_tests/server/configs/ut-container/libvirt-config/test-dbus.xml.in [new file with mode: 0644]
unit_tests/server/configs/ut-container/ut-dbus.conf [new file with mode: 0644]
unit_tests/server/ut-container-connection.cpp
unit_tests/server/ut-container.cpp
unit_tests/utils/ut-glib-loop.cpp [new file with mode: 0644]

index 7d5f0bf..af48230 100644 (file)
@@ -185,7 +185,7 @@ void DbusConnection::onNameAcquired(GDBusConnection*, const gchar* name, gpointe
 
 void DbusConnection::onNameLost(GDBusConnection*, const gchar* name, gpointer userData)
 {
-    LOGE("Name lost " << name);
+    LOGD("Name lost " << name);
     const NameCallbacks& callbacks = utils::getCallbackFromPointer<NameCallbacks>(userData);
     if (callbacks.nameLost) {
         callbacks.nameLost();
index 4d0aaa7..e91c12d 100644 (file)
@@ -46,7 +46,7 @@ void StderrBackend::log(LogLevel logLevel,
     const std::string defaultColor = LogFormatter::getDefaultConsoleColor();
     const std::string header = LogFormatter::getHeader(logLevel, file, line, func);
     tokenizer tokens(message, charSeparator("\n"));
-    for(const auto& line : tokens) {
+    for (const auto& line : tokens) {
         if (!line.empty()) {
             fprintf(stderr,
                     "%s%s%s%s\n",
index c903780..cc468e3 100644 (file)
@@ -22,6 +22,7 @@
  * @brief   Wait for file utility function
  */
 
+#include "utils/exception.hpp"
 #include "utils/file-wait.hpp"
 
 #include <sys/stat.h>
@@ -42,11 +43,11 @@ void waitForFile(const std::string& filename, const unsigned int timeoutMs)
     unsigned int loops = 0;
     while (stat(filename.c_str(), &s) == -1) {
         if (errno != ENOENT) {
-            throw std::runtime_error("file access error: " + filename);
+            throw UtilsException("file access error: " + filename);
         }
         ++ loops;
         if (loops * GRANULARITY > timeoutMs) {
-            throw std::runtime_error("timeout on waiting for: " + filename);
+            throw UtilsException("timeout on waiting for: " + filename);
         }
         usleep(GRANULARITY * 1000);
     }
index 2b0b681..0446630 100644 (file)
@@ -101,7 +101,7 @@ bool createDirectories(const std::string& path, mode_t mode)
 
 bool mountTmpfs(const std::string& path)
 {
-    if (::mount("tmpfs", path.c_str(), "tmpfs", MS_NOSUID|MS_NODEV, "mode=755") != 0) {
+    if (::mount("tmpfs", path.c_str(), "tmpfs", MS_NOSUID | MS_NODEV, "mode=755") != 0) {
         LOGD("Mount failed for '" << path << "': " << strerror(errno));
         return false;
     }
index 7946c05..193a25f 100644 (file)
@@ -23,8 +23,7 @@
  */
 
 #include "utils/glib-loop.hpp"
-
-#include <glib.h>
+#include "utils/callback-wrapper.hpp"
 
 
 namespace security_containers {
@@ -34,7 +33,9 @@ namespace utils {
 ScopedGlibLoop::ScopedGlibLoop()
     : mLoop(g_main_loop_new(NULL, FALSE), g_main_loop_unref)
 {
-    mLoopThread = std::thread([this] {g_main_loop_run(mLoop.get());});
+    mLoopThread = std::thread([this] {
+                                  g_main_loop_run(mLoop.get());
+                              });
 }
 
 ScopedGlibLoop::~ScopedGlibLoop()
@@ -48,6 +49,26 @@ ScopedGlibLoop::~ScopedGlibLoop()
     mLoopThread.join();
 }
 
+void Glib::addTimerEvent(const unsigned int intervalMs,
+                         const OnTimerEventCallback& callback,
+                         const CallbackGuard& guard)
+{
+    g_timeout_add_full(G_PRIORITY_DEFAULT,
+                       intervalMs,
+                       &Glib::onTimerEvent,
+                       utils::createCallbackWrapper(callback, guard.spawn()),
+                       &utils::deleteCallbackWrapper<OnTimerEventCallback>);
+}
+
+gboolean Glib::onTimerEvent(gpointer data)
+{
+    const OnTimerEventCallback& callback = getCallbackFromPointer<OnTimerEventCallback>(data);
+    if (callback) {
+        return (gboolean)callback();
+    }
+    return FALSE;
+}
+
 
 } // namespace utils
 } // namespace security_containers
index a31099a..38d7020 100644 (file)
 #ifndef COMMON_UTILS_GLIB_LOOP_HPP
 #define COMMON_UTILS_GLIB_LOOP_HPP
 
+#include "utils/callback-guard.hpp"
+
 #include <thread>
 #include <memory>
-
-struct _GMainLoop;
-typedef struct _GMainLoop GMainLoop;
+#include <glib.h>
 
 
 namespace security_containers {
@@ -56,6 +56,29 @@ private:
     std::thread mLoopThread;
 };
 
+/**
+ * Miscellaneous helpers for the Glib library
+ */
+class Glib {
+public:
+    /**
+     * A user provided function that will be called succesively after an interval has passed.
+     *
+     * @return true if the callback is supposed to be called further,
+     *         false if the callback is not to be called anymore and be destroyed.
+     */
+    typedef std::function<bool()> OnTimerEventCallback;
+
+    /**
+     * Adds a timer event to the glib main loop.
+     */
+    static void addTimerEvent(const unsigned int intervalMs,
+                              const OnTimerEventCallback& callback,
+                              const CallbackGuard& guard);
+
+private:
+    static gboolean onTimerEvent(gpointer data);
+};
 
 } // namespace utils
 } // namespace security_containers
index 5dc7da1..a35aee5 100644 (file)
@@ -24,6 +24,8 @@
 
 #include "utils/latch.hpp"
 
+#include <cassert>
+
 
 namespace security_containers {
 namespace utils {
@@ -43,22 +45,33 @@ void Latch::set()
 
 void Latch::wait()
 {
-    std::unique_lock<std::mutex> lock(mMutex);
-    mCondition.wait(lock, [this] {return mCount > 0;});
-    --mCount;
+    waitForN(1);
 }
 
 bool Latch::wait(const unsigned int timeoutMs)
 {
+    return waitForN(1, timeoutMs);
+}
+
+void Latch::waitForN(const unsigned int n)
+{
+    std::unique_lock<std::mutex> lock(mMutex);
+    mCondition.wait(lock, [this, &n] {return mCount >= n;});
+    mCount -= n;
+}
+
+bool Latch::waitForN(const unsigned int n, const unsigned int timeoutMs)
+{
     std::unique_lock<std::mutex> lock(mMutex);
     if (!mCondition.wait_for(lock, std::chrono::milliseconds(timeoutMs),
-                             [this] {return mCount > 0;})) {
+                             [this, &n] {return mCount >= n;})) {
         return false;
     }
-    --mCount;
+    mCount -= n;
     return true;
 }
 
+
 bool Latch::empty()
 {
     std::unique_lock<std::mutex> lock(mMutex);
index 54239fc..2621d49 100644 (file)
@@ -54,19 +54,37 @@ public:
     void wait();
 
     /**
-     * Waits with timeout.
-     * @return false on timeout
+     * Waits for a single occurence of event with timeout.
+     *
+     * @param timeoutMs  timeout in ms to wait for
+     * @return           false on timeout
      */
     bool wait(const unsigned int timeoutMs);
 
     /**
+     * Waits for @ref n occurrences of event.
+     *
+     * @param n  number of occurences to wait for
+     */
+    void waitForN(const unsigned int n);
+
+    /**
+     * Waits for @ref n occurences of event with timeout.
+     *
+     * @param n          number of occurences to wait for
+     * @param timeoutMs  timeout in ms to wait for
+     * @return           false on timeout
+     */
+    bool waitForN(const unsigned int n, const unsigned int timeoutMs);
+
+    /**
      * Check if there are no pending events.
      */
     bool empty();
 private:
     std::mutex mMutex;
     std::condition_variable mCondition;
-    int mCount;
+    unsigned int mCount;
 };
 
 
index e1012eb..5842c81 100644 (file)
@@ -59,6 +59,14 @@ ContainerConnectionTransport::ContainerConnectionTransport(const std::string& ru
         LOGE("Initialization failed: could not mount " << runMountPoint);
         throw ContainerConnectionException("Could not mount: " + runMountPoint);
     }
+
+    // if there is no systemd in the container this dir won't be created automatically
+    // TODO: will require chown with USER namespace enabled
+    std::string dbusDirectory = runMountPoint + "/dbus";
+    if (!utils::createDirectories(dbusDirectory, 0755)) {
+        LOGE("Initialization failed: could not create " << dbusDirectory);
+        throw ContainerConnectionException("Could not create: " + dbusDirectory);
+    }
 }
 
 
index 80d4be5..8d06803 100644 (file)
@@ -45,6 +45,7 @@ ContainerConnection::ContainerConnection(const std::string& address, const OnNam
     , mNameLost(false)
 {
     if (address.empty()) {
+        // TODO: this should throw. Don't return cleanly unless the object is fully usable.
         LOGW("The connection to the container is disabled");
         return;
     }
index afa9ac8..36d0c34 100644 (file)
  */
 
 #include "container.hpp"
+
 #include "log/logger.hpp"
 #include "utils/paths.hpp"
 
 #include <string>
+#include <thread>
 
 
 namespace security_containers {
@@ -51,6 +53,14 @@ Container::Container(const std::string& containerConfigPath)
 
 Container::~Container()
 {
+    // Make sure all OnNameLostCallbacks get finished and no new will
+    // get called before proceeding further. This guarantees no race
+    // condition on the mReconnectThread.
+    mConnection.reset();
+
+    if (mReconnectThread.joinable()) {
+        mReconnectThread.join();
+    }
 }
 
 std::string Container::getId()
@@ -110,7 +120,36 @@ bool Container::isPaused()
 
 void Container::onNameLostCallback()
 {
-    // TODO: try to reconnect
+    LOGI(getId() << ": A connection to the DBUS server has been lost, reconnecting...");
+
+    if (mReconnectThread.joinable()) {
+        mReconnectThread.join();
+    }
+    mReconnectThread = std::thread(std::bind(&Container::reconnectHandler, this));
+}
+
+void Container::reconnectHandler()
+{
+    std::string address;
+
+    mConnection.reset();
+
+    try {
+        address = mConnectionTransport->acquireAddress();
+    } catch (SecurityContainersException& e) {
+        LOGE(getId() << "The socket does not exist anymore, something went terribly wrong, stopping the container");
+        stop(); // TODO: shutdownOrStop()
+        return;
+    }
+
+    try {
+        mConnection.reset(new ContainerConnection(address, std::bind(&Container::onNameLostCallback, this)));
+        LOGI(getId() << ": Reconnected");
+    } catch (SecurityContainersException& e) {
+        LOGE(getId() << ": Reconnecting to the DBUS has failed, stopping the container");
+        stop(); // TODO: shutdownOrStop()
+        return;
+    }
 }
 
 
index 89ef44e..871b802 100644 (file)
@@ -33,6 +33,7 @@
 
 #include <string>
 #include <memory>
+#include <thread>
 
 
 namespace security_containers {
@@ -98,11 +99,13 @@ public:
 
 private:
     ContainerConfig mConfig;
-    std::unique_ptr<ContainerAdmin> mAdmin;
     std::unique_ptr<ContainerConnectionTransport> mConnectionTransport;
+    std::unique_ptr<ContainerAdmin> mAdmin;
     std::unique_ptr<ContainerConnection> mConnection;
+    std::thread mReconnectThread;
 
     void onNameLostCallback();
+    void reconnectHandler();
 };
 
 
index 3af8276..2623ba5 100644 (file)
@@ -65,7 +65,7 @@ ContainersManager::~ContainersManager()
 {
     LOGD("Destroying ContainersManager object...");
     try {
-        stopAll();
+        stopAll(); // TODO: shutdownOrStop()
     } catch (ServerException& e) {
         LOGE("Failed to stop all of the containers");
     }
index 6847cac..e427b68 100644 (file)
@@ -27,6 +27,7 @@ FILE(GLOB manager_manager_CONF      ut-containers-manager/*.conf)
 FILE(GLOB manager_container_CONF    ut-containers-manager/containers/*.conf)
 FILE(GLOB manager_admin_CONF        ut-containers-manager/libvirt-config/*.xml)
 
+FILE(GLOB container_CONF            ut-container/*.conf)
 FILE(GLOB container_container_CONF  ut-container/containers/*.conf)
 FILE(GLOB container_admin_CONF      ut-container/libvirt-config/*.xml)
 
@@ -43,6 +44,10 @@ CONFIGURE_FILE(ut-container-admin/containers/test.conf.in
                ${CMAKE_BINARY_DIR}/ut-container-admin/containers/test.conf @ONLY)
 FILE(GLOB admin_container_CONF_GEN ${CMAKE_BINARY_DIR}/ut-container-admin/containers/*.conf)
 
+CONFIGURE_FILE(ut-container/libvirt-config/test-dbus.xml.in
+               ${CMAKE_BINARY_DIR}/ut-container/libvirt-config/test-dbus.xml @ONLY)
+FILE(GLOB container_admin_CONF_GEN ${CMAKE_BINARY_DIR}/ut-container/libvirt-config/*.xml)
+
 
 ## Install #####################################################################
 INSTALL(FILES        ${server_manager_CONF}
@@ -59,10 +64,14 @@ INSTALL(FILES        ${manager_container_CONF}
 INSTALL(FILES        ${manager_admin_CONF}
         DESTINATION  ${SC_TEST_CONFIG_INSTALL_DIR}/server/ut-containers-manager/libvirt-config)
 
+INSTALL(FILES        ${container_CONF}
+        DESTINATION  ${SC_TEST_CONFIG_INSTALL_DIR}/server/ut-container)
 INSTALL(FILES        ${container_container_CONF}
         DESTINATION  ${SC_TEST_CONFIG_INSTALL_DIR}/server/ut-container/containers)
 INSTALL(FILES        ${container_admin_CONF}
         DESTINATION  ${SC_TEST_CONFIG_INSTALL_DIR}/server/ut-container/libvirt-config)
+INSTALL(FILES        ${container_admin_CONF_GEN}
+        DESTINATION  ${SC_TEST_CONFIG_INSTALL_DIR}/server/ut-container/libvirt-config)
 
 INSTALL(FILES        ${admin_container_CONF}
         DESTINATION  ${SC_TEST_CONFIG_INSTALL_DIR}/server/ut-container-admin/containers)
diff --git a/unit_tests/server/configs/ut-container/containers/test-dbus.conf b/unit_tests/server/configs/ut-container/containers/test-dbus.conf
new file mode 100644 (file)
index 0000000..a39e267
--- /dev/null
@@ -0,0 +1,7 @@
+{
+    "privilege" : 10,
+    "config" : "../libvirt-config/test-dbus.xml",
+    "cpuQuotaForeground" : -1,
+    "cpuQuotaBackground" : 1000,
+    "runMountPoint" : "/tmp/ut-container"
+}
diff --git a/unit_tests/server/configs/ut-container/libvirt-config/test-dbus.xml.in b/unit_tests/server/configs/ut-container/libvirt-config/test-dbus.xml.in
new file mode 100644 (file)
index 0000000..a4b2d73
--- /dev/null
@@ -0,0 +1,19 @@
+<domain type="lxc">
+    <name>ut-container-test-dbus</name>
+    <uuid>35bb7989-f222-4b63-b0b1-facbdb05b495</uuid>
+    <memory>102400</memory>
+    <on_poweroff>destroy</on_poweroff>
+    <os>
+        <type>exe</type>
+        <init>/bin/dbus-daemon</init>
+        <initarg>--config-file=@SC_TEST_CONFIG_INSTALL_DIR@/server/ut-container/ut-dbus.conf</initarg>
+        <initarg>--nofork</initarg>
+    </os>
+    <devices>
+        <console type="pty"/>
+        <filesystem type='mount'>
+            <source dir='/tmp/ut-container'/>
+            <target dir='/var/run'/>
+        </filesystem>
+    </devices>
+</domain>
diff --git a/unit_tests/server/configs/ut-container/ut-dbus.conf b/unit_tests/server/configs/ut-container/ut-dbus.conf
new file mode 100644 (file)
index 0000000..e52a825
--- /dev/null
@@ -0,0 +1,18 @@
+<!-- This configuration file controls the containers message bus -->
+
+<!DOCTYPE busconfig PUBLIC "-//freedesktop//DTD D-Bus Bus Configuration 1.0//EN"
+ "http://www.freedesktop.org/standards/dbus/1.0/busconfig.dtd">
+
+<busconfig>
+    <type>custom</type>
+    <listen>unix:path=/tmp/ut-container/dbus/system_bus_socket</listen>
+
+    <policy context="default">
+        <!-- Allow everything to be sent -->
+        <allow send_destination="*" eavesdrop="true"/>
+        <!-- Allow everything to be received -->
+        <allow eavesdrop="true"/>
+        <!-- Allow anyone to own anything -->
+        <allow own="*"/>
+    </policy>
+</busconfig>
index 7721a3a..5f485f4 100644 (file)
@@ -60,7 +60,6 @@ public:
     ScopedDbusDaemon()
         : mTransport(TRANSPORT_MOUNT_POINT)
     {
-        utils::createDirectory(TRANSPORT_MOUNT_POINT + "/dbus", 0755);
         mDaemon.start(DBUS_DAEMON_PROC, DBUS_DAEMON_ARGS);
     }
 
index 43fa4f0..69d1a4a 100644 (file)
@@ -29,6 +29,7 @@
 #include "exception.hpp"
 
 #include "utils/exception.hpp"
+#include "utils/glib-loop.hpp"
 
 #include <memory>
 #include <string>
@@ -38,11 +39,16 @@ BOOST_AUTO_TEST_SUITE(ContainerSuite)
 
 using namespace security_containers;
 using namespace security_containers::config;
+using namespace security_containers::utils;
+
+namespace {
 
 const std::string TEST_CONFIG_PATH = SC_TEST_CONFIG_INSTALL_DIR "/server/ut-container/containers/test.conf";
+const std::string TEST_DBUS_CONFIG_PATH = SC_TEST_CONFIG_INSTALL_DIR "/server/ut-container/containers/test-dbus.conf";
 const std::string BUGGY_CONFIG_PATH = SC_TEST_CONFIG_INSTALL_DIR "/server/ut-container/containers/buggy.conf";
 const std::string MISSING_CONFIG_PATH = "/this/is/a/missing/file/path/config.conf";
 
+} // namespace
 
 BOOST_AUTO_TEST_CASE(ConstructorTest)
 {
@@ -65,5 +71,26 @@ BOOST_AUTO_TEST_CASE(MissingConfigTest)
     BOOST_REQUIRE_THROW(Container c(MISSING_CONFIG_PATH), ConfigException);
 }
 
+BOOST_AUTO_TEST_CASE(StartStopTest)
+{
+    ScopedGlibLoop loop;
+
+    std::unique_ptr<Container> c(new Container(TEST_CONFIG_PATH));
+    BOOST_REQUIRE_NO_THROW(c->start());
+    BOOST_REQUIRE_NO_THROW(c->stop());
+}
+
+BOOST_AUTO_TEST_CASE(DbusConnectionTest)
+{
+    ScopedGlibLoop loop;
+
+    std::unique_ptr<Container> c;
+    BOOST_REQUIRE_NO_THROW(c.reset(new Container(TEST_DBUS_CONFIG_PATH)));
+    BOOST_REQUIRE_NO_THROW(c->start());
+    BOOST_REQUIRE_NO_THROW(c->stop());
+}
+
+// TODO: DbusReconnectionTest
+
 
 BOOST_AUTO_TEST_SUITE_END()
diff --git a/unit_tests/utils/ut-glib-loop.cpp b/unit_tests/utils/ut-glib-loop.cpp
new file mode 100644 (file)
index 0000000..f34fdbc
--- /dev/null
@@ -0,0 +1,68 @@
+/*
+ *  Copyright (c) 2014 Samsung Electronics Co., Ltd All Rights Reserved
+ *
+ *  Contact: Lukasz Pawelczyk <l.pawelczyk@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  Lukasz Pawelczyk (l.pawelczyk@partner.samsung.com)
+ * @brief   Unit tests of glib-loop
+ */
+
+#include "ut.hpp"
+
+#include "utils/latch.hpp"
+#include "utils/glib-loop.hpp"
+
+#include <memory>
+
+BOOST_AUTO_TEST_SUITE(UtilsGlibLoopSuite)
+
+using namespace security_containers;
+using namespace security_containers::utils;
+
+
+namespace {
+
+const unsigned int TIMER_INTERVAL_MS = 10;
+const unsigned int TIMER_NUMBER      = 5;
+const unsigned int TIMER_WAIT_FOR    = 2 * TIMER_NUMBER * TIMER_INTERVAL_MS;
+
+} // namespace
+
+BOOST_AUTO_TEST_CASE(GlibTimerEventTest)
+{
+    ScopedGlibLoop loop;
+    CallbackGuard guard;
+    Latch latch;
+
+    Glib::OnTimerEventCallback callback = [&] {
+        static unsigned int counter = 0;
+        latch.set();
+        if (++counter >= TIMER_NUMBER) {
+            return false;
+        }
+        return true;
+    };
+
+    Glib::addTimerEvent(TIMER_INTERVAL_MS, callback, guard);
+
+    BOOST_REQUIRE(latch.waitForN(TIMER_NUMBER, TIMER_WAIT_FOR));
+    BOOST_REQUIRE(latch.wait(TIMER_WAIT_FOR) == false);
+}
+
+BOOST_AUTO_TEST_SUITE_END()