From 3035b39d565506bc70dce035cd4178e883b7f976 Mon Sep 17 00:00:00 2001 From: Mateusz Malicki Date: Fri, 9 Jan 2015 12:34:12 +0100 Subject: [PATCH 01/16] Fixed generation of log_sink file [Bug]i log_sink file is invalid formated xml [Cause] Lxc invoke exit(0) after fork. Exit from child and from parent cause redundant log write [Solution] Add defintion to CMakeLists.txt [Verification] Build, install, build run test with '--log_sink=/tmp/log.xml', run 'xmllint --huge --format /tmp/log.xml' (there should be no parse errors). Note: StartHasStoppedTest can fail. Change-Id: I9a1d4bee9030eb8e3958ff914ef7cf2dc1a953ce --- CMakeLists.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/CMakeLists.txt b/CMakeLists.txt index a12367f..06a1064 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -73,6 +73,7 @@ ADD_DEFINITIONS("-pedantic") # Be pedantic ADD_DEFINITIONS("-pedantic-errors") # Make pedantic warnings into errors ADD_DEFINITIONS(-DPROGRAM_VERSION="${VERSION}") ADD_DEFINITIONS(-DPROJECT_SOURCE_DIR="${PROJECT_SOURCE_DIR}") +ADD_DEFINITIONS(-DUSE_EXEC) IF("${CMAKE_CXX_COMPILER_ID}" STREQUAL "Clang") # Warn about documentation problems -- 2.7.4 From a83342a7862458d2a6a35fe8889e80cd9a9dc1bb Mon Sep 17 00:00:00 2001 From: Lukasz Kostyra Date: Wed, 7 Jan 2015 15:39:02 +0100 Subject: [PATCH 02/16] IPC: Convert test synchronization method to Latch [Bug/Feature] Some tests were using sleep_for in order to synchronize threads [Cause] N/A [Solution] Use Latch instead of atomic_bool set and sleep_for to synchronize threads in tests [Verification] Build, install, run tests. Should pass as they did before. Change-Id: I3067ed1f13cdde047f720c0b6d05ce19ec156dbe --- tests/unit_tests/ipc/ut-ipc.cpp | 50 +++++++++++++++++++---------------------- 1 file changed, 23 insertions(+), 27 deletions(-) diff --git a/tests/unit_tests/ipc/ut-ipc.cpp b/tests/unit_tests/ipc/ut-ipc.cpp index 65ee27e..74c18f3 100644 --- a/tests/unit_tests/ipc/ut-ipc.cpp +++ b/tests/unit_tests/ipc/ut-ipc.cpp @@ -36,12 +36,12 @@ #include "ipc/ipc-gsource.hpp" #include "ipc/types.hpp" #include "utils/glib-loop.hpp" +#include "utils/latch.hpp" #include "config/fields.hpp" #include "logger/logger.hpp" #include -#include #include #include #include @@ -592,14 +592,14 @@ BOOST_AUTO_TEST_CASE(AddSignalInRuntime) Client c(socketPath); connect(s, c); - std::atomic_bool isHandlerACalled(false); - auto handlerA = [&isHandlerACalled](const FileDescriptor, std::shared_ptr&) { - isHandlerACalled = true; + utils::Latch latchA; + auto handlerA = [&latchA](const FileDescriptor, std::shared_ptr&) { + latchA.set(); }; - std::atomic_bool isHandlerBCalled(false); - auto handlerB = [&isHandlerBCalled](const FileDescriptor, std::shared_ptr&) { - isHandlerBCalled = true; + utils::Latch latchB; + auto handlerB = [&latchB](const FileDescriptor, std::shared_ptr&) { + latchB.set(); }; c.addSignalHandler(1, handlerA); @@ -610,8 +610,7 @@ BOOST_AUTO_TEST_CASE(AddSignalInRuntime) s.signal(1, data); // Wait for the signals to arrive - std::this_thread::sleep_for(std::chrono::milliseconds(TIMEOUT)); //TODO wait_for - BOOST_CHECK(isHandlerACalled && isHandlerBCalled); + BOOST_CHECK(latchA.wait(TIMEOUT) && latchB.wait(TIMEOUT)); } @@ -620,14 +619,14 @@ BOOST_AUTO_TEST_CASE(AddSignalOffline) Service s(socketPath); Client c(socketPath); - std::atomic_bool isHandlerACalled(false); - auto handlerA = [&isHandlerACalled](const FileDescriptor, std::shared_ptr&) { - isHandlerACalled = true; + utils::Latch latchA; + auto handlerA = [&latchA](const FileDescriptor, std::shared_ptr&) { + latchA.set(); }; - std::atomic_bool isHandlerBCalled(false); - auto handlerB = [&isHandlerBCalled](const FileDescriptor, std::shared_ptr&) { - isHandlerBCalled = true; + utils::Latch latchB; + auto handlerB = [&latchB](const FileDescriptor, std::shared_ptr&) { + latchB.set(); }; c.addSignalHandler(1, handlerA); @@ -642,8 +641,7 @@ BOOST_AUTO_TEST_CASE(AddSignalOffline) s.signal(1, data); // Wait for the signals to arrive - std::this_thread::sleep_for(std::chrono::milliseconds(2 * TIMEOUT)); - BOOST_CHECK(isHandlerACalled && isHandlerBCalled); + BOOST_CHECK(latchA.wait(TIMEOUT) && latchB.wait(TIMEOUT)); } @@ -652,9 +650,9 @@ BOOST_AUTO_TEST_CASE(AddSignalOffline) BOOST_AUTO_TEST_CASE(ServiceGSource) { ScopedGlibLoop loop; - std::atomic_bool isSignalCalled(false); - auto signalHandler = [&isSignalCalled](const FileDescriptor, std::shared_ptr&) { - isSignalCalled = true; + utils::Latch l; + auto signalHandler = [&l](const FileDescriptor, std::shared_ptr&) { + l.set(); }; IPCGSource::Pointer serviceGSource; @@ -672,17 +670,16 @@ BOOST_AUTO_TEST_CASE(ServiceGSource) auto data = std::make_shared(1); c.signal(2, data); - std::this_thread::sleep_for(std::chrono::milliseconds(TIMEOUT)); //TODO wait_for - BOOST_CHECK(isSignalCalled); + BOOST_CHECK(l.wait(TIMEOUT)); } BOOST_AUTO_TEST_CASE(ClientGSource) { ScopedGlibLoop loop; - std::atomic_bool isSignalCalled(false); - auto signalHandler = [&isSignalCalled](const FileDescriptor, std::shared_ptr&) { - isSignalCalled = true; + utils::Latch l; + auto signalHandler = [&l](const FileDescriptor, std::shared_ptr&) { + l.set(); }; Service s(socketPath); @@ -702,8 +699,7 @@ BOOST_AUTO_TEST_CASE(ClientGSource) auto data = std::make_shared(1); s.signal(2, data); - std::this_thread::sleep_for(std::chrono::milliseconds(TIMEOUT)); //TODO wait_for - BOOST_CHECK(isSignalCalled); + BOOST_CHECK(l.wait(TIMEOUT)); } #endif // GLIB_CHECK_VERSION -- 2.7.4 From e310694aca62b3359f7ca1e9027e4f8650a52ade Mon Sep 17 00:00:00 2001 From: Lukasz Kostyra Date: Wed, 17 Dec 2014 10:42:41 +0100 Subject: [PATCH 03/16] Add test for LoggerScope [Feature] Test for new LoggerScope interface. [Cause] New interface in libLogger. [Solution] Test for new LoggerScope interface. [Verification] Build, install, run Log tests. Change-Id: I43767c1f65d09ca8304046fd9d8586f1077a8b63 --- tests/unit_tests/log/ut-logger.cpp | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tests/unit_tests/log/ut-logger.cpp b/tests/unit_tests/log/ut-logger.cpp index e574505..74028db 100644 --- a/tests/unit_tests/log/ut-logger.cpp +++ b/tests/unit_tests/log/ut-logger.cpp @@ -26,6 +26,7 @@ #include "config.hpp" #include "ut.hpp" #include "logger/logger.hpp" +#include "logger/logger-scope.hpp" #include "logger/formatter.hpp" #include "logger/backend.hpp" #include "logger/backend-stderr.hpp" @@ -197,6 +198,19 @@ BOOST_AUTO_TEST_CASE(TestLogsTrace) BOOST_CHECK(tf.logContains("[TRACE]") == true); } +BOOST_AUTO_TEST_CASE(TestLoggerScope) +{ + LOGS("Main function scope"); + + { + LOGS("Scope inside function"); + LOGD("Some additional information in-between scoped logs"); + { + LOGS("Additional scope with " << "stringstream" << ' ' << "test" << 3 << ' ' << 3.42); + LOGD("More additional information in-between scoped logs"); + } + } +} BOOST_AUTO_TEST_SUITE_END() -- 2.7.4 From da1be5620b803b7d826376faffe7ecd0083b8222 Mon Sep 17 00:00:00 2001 From: Mateusz Malicki Date: Mon, 12 Jan 2015 16:16:47 +0100 Subject: [PATCH 04/16] Added testing wrong or unset union type [Bug/Feature] Added testing wrong or unset union type [Cause] N/A [Solution] Throw ConfigException [Verification] Build, install, run ConfigurationSuite tests Change-Id: I813b52870c4c12a58a9a614821056278a5445a68 --- tests/unit_tests/config/ut-configuration.cpp | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/tests/unit_tests/config/ut-configuration.cpp b/tests/unit_tests/config/ut-configuration.cpp index 8e82905..95fc52a 100644 --- a/tests/unit_tests/config/ut-configuration.cpp +++ b/tests/unit_tests/config/ut-configuration.cpp @@ -95,6 +95,9 @@ BOOST_AUTO_TEST_CASE(ToStringTest) std::string out = saveToString(testConfig); BOOST_CHECK_EQUAL(out, jsonTestString); + + TestConfig::SubConfigOption unionConfig; + BOOST_CHECK_THROW(saveToString(unionConfig), ConfigException); } namespace loadErrorsTest { @@ -111,6 +114,13 @@ DECLARE_CONFIG(BoolConfig, bool) DECLARE_CONFIG(ArrayConfig, std::vector) DECLARE_CONFIG(ObjectConfig, IntConfig) #undef DECLARE_CONFIG +struct UnionConfig { + CONFIG_DECLARE_UNION + ( + int, + bool + ) +}; } // namespace loadErrorsTest @@ -177,6 +187,12 @@ BOOST_AUTO_TEST_CASE(LoadErrorsTest) BOOST_CHECK_THROW(loadFromString("{\"field\": []}", objectConfig), ConfigException); BOOST_CHECK_THROW(loadFromString("{\"field\": {}}", objectConfig), ConfigException); BOOST_CHECK_NO_THROW(loadFromString("{\"field\": {\"field\": 1}}", objectConfig)); + + UnionConfig unionConfig; + BOOST_CHECK_THROW(loadFromString("{\"type\": \"long\", \"value\": 1}", unionConfig), ConfigException); + BOOST_CHECK_THROW(loadFromString("{\"type\": \"int\"}", unionConfig), ConfigException); + BOOST_CHECK_NO_THROW(loadFromString("{\"type\": \"int\", \"value\": 1}", unionConfig)); + BOOST_CHECK_NO_THROW(loadFromString("{\"type\": \"bool\", \"value\": true}", unionConfig)); } namespace hasVisitableTest { -- 2.7.4 From 04f7e267d40ba248d20251dcb13223134996095f Mon Sep 17 00:00:00 2001 From: Mateusz Malicki Date: Wed, 7 Jan 2015 14:29:30 +0100 Subject: [PATCH 05/16] Add get_zone_ids, get_active_zone_id and get_zones_status to cli [Feature] Add get_zone_ids, get_active_zone_id and get_zones_status to cli [Cause] N/A [Solution] N/A [Verification] Build, run appropriate functions (through cli) Change-Id: If6823243d66606d28bf6b45c5b2ece8ab7c06e49 --- cli/command-line-interface.cpp | 89 ++++++++++++++++++++++++++++++++++++++++++ cli/command-line-interface.hpp | 20 ++++++++++ cli/main.cpp | 24 ++++++++++++ 3 files changed, 133 insertions(+) diff --git a/cli/command-line-interface.cpp b/cli/command-line-interface.cpp index 1222a86..06be364 100644 --- a/cli/command-line-interface.cpp +++ b/cli/command-line-interface.cpp @@ -31,7 +31,12 @@ #include #include #include +#include +#include +#include +#include #include +#include using namespace std; @@ -82,6 +87,14 @@ finish: } } +template +string stringAsInStream(const T& value) +{ + std::ostringstream stream; + stream << value; + return stream.str(); +} + ostream& operator<<(ostream& out, const VsmZoneState& state) { const char* name; @@ -113,6 +126,30 @@ ostream& operator<<(ostream& out, const VsmZone& zone) return out; } +typedef vector> Table; + +ostream& operator<<(ostream& out, const Table& table) +{ + vector sizes; + for (const auto& row : table) { + if (sizes.size() < row.size()) { + sizes.resize(row.size()); + } + for (size_t i = 0; i < row.size(); ++i) { + sizes[i] = max(sizes[i], row[i].length()); + } + } + + for (const auto& row : table) { + for (size_t i = 0; i < row.size(); ++i) { + out << left << setw(sizes[i]+2) << row[i]; + } + out << "\n"; + } + + return out; +} + } // namespace void CommandLineInterface::printUsage(std::ostream& out) const @@ -210,6 +247,58 @@ void unlock_zone(int pos, int argc, const char** argv) one_shot(bind(vsm_unlock_zone, _1, argv[pos + 1])); } +void get_zones_status(int /* pos */, int /* argc */, const char** /* argv */) +{ + using namespace std::placeholders; + + VsmArrayString ids; + VsmString activeId; + Table table; + + one_shot(bind(vsm_get_zone_ids, _1, &ids)); + one_shot(bind(vsm_get_active_zone_id, _1, &activeId)); + table.push_back({"Active", "Id", "State", "Terminal", "Root"}); + for (VsmString* id = ids; *id; ++id) { + VsmZone zone; + one_shot(bind(vsm_lookup_zone_by_id, _1, *id, &zone)); + assert(string(zone->id) == string(*id)); + table.push_back({string(zone->id) == string(activeId) ? "*" : "", + zone->id, + stringAsInStream(zone->state), + to_string(zone->terminal), + zone->rootfs_path}); + vsm_zone_free(zone); + } + vsm_string_free(activeId); + vsm_array_string_free(ids); + cout << table << endl; +} + +void get_zone_ids(int /* pos */, int /* argc */, const char** /* argv */) +{ + using namespace std::placeholders; + + VsmArrayString ids; + one_shot(bind(vsm_get_zone_ids, _1, &ids)); + string delim; + for (VsmString* id = ids; *id; ++id) { + cout << delim << *id; + delim = ", "; + } + cout << endl; + vsm_array_string_free(ids); +} + +void get_active_zone_id(int /* pos */, int /* argc */, const char** /* argv */) +{ + using namespace std::placeholders; + + VsmString id; + one_shot(bind(vsm_get_active_zone_id, _1, &id)); + cout << id << endl; + vsm_string_free(id); +} + void lookup_zone_by_id(int pos, int argc, const char** argv) { using namespace std::placeholders; diff --git a/cli/command-line-interface.hpp b/cli/command-line-interface.hpp index 6a655d8..055bb1e 100644 --- a/cli/command-line-interface.hpp +++ b/cli/command-line-interface.hpp @@ -147,6 +147,26 @@ void lock_zone(int pos, int argc, const char** argv); void unlock_zone(int pos, int argc, const char** argv); /** + * Parses command line arguments and prints list of zone with + * some useful informations (id, state, terminal, root path) + */ +void get_zones_status(int pos, int argc, const char** argv); + +/** + * Parses command line arguments and call vsm_get_zone_ids + * + * @see vsm_get_zone_ids + */ +void get_zone_ids(int pos, int argc, const char** argv); + +/** + * Parses command line arguments and call vsm_get_active_zone_id + * + * @see vsm_get_active_zone_id + */ +void get_active_zone_id(int pos, int argc, const char** argv); + +/** * Parses command line arguments and call vsm_lookup_zone_by_id * * @see vsm_lookup_zone_by_id diff --git a/cli/main.cpp b/cli/main.cpp index 1afe134..d003d10 100644 --- a/cli/main.cpp +++ b/cli/main.cpp @@ -92,6 +92,30 @@ std::map commands = { } }, { + "get_zones_status", { + get_zones_status, + "get_zones_status", + "Get list of zone with some useful informations (id, state, terminal, root path)", + {} + } + }, + { + "get_zone_ids", { + get_zone_ids, + "get_zone_ids", + "Get all zone ids", + {} + } + }, + { + "get_active_zone_id", { + get_active_zone_id, + "get_active_zone_id", + "Get active (foreground) zone ids", + {} + } + }, + { "lookup_zone_by_id", { lookup_zone_by_id, "lookup_zone_by_id zone_id", -- 2.7.4 From b8e9e623277194fb1d620de64604dc821a97410a Mon Sep 17 00:00:00 2001 From: Mateusz Malicki Date: Tue, 13 Jan 2015 15:36:23 +0100 Subject: [PATCH 06/16] Fix: Crash when testing ut-zones-manager [Bug/Feature] Crash when testing ut-zones-manager [Cause] BOOST_CHECK is not thread safe [Solution] BOOSR_CHECK was removed from child thread [Verification] Build, install, run test on slp Change-Id: I6cb10f626d8e5ec908e359de15b3fbbed9abb77c --- tests/unit_tests/server/ut-zones-manager.cpp | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/tests/unit_tests/server/ut-zones-manager.cpp b/tests/unit_tests/server/ut-zones-manager.cpp index 06af8eb..a733e05 100644 --- a/tests/unit_tests/server/ut-zones-manager.cpp +++ b/tests/unit_tests/server/ut-zones-manager.cpp @@ -313,8 +313,9 @@ public: const VoidResultCallback& result) { auto asyncResult = [result](dbus::AsyncMethodCallResult& asyncMethodCallResult) { - BOOST_CHECK(g_variant_is_of_type(asyncMethodCallResult.get(), G_VARIANT_TYPE_UNIT)); - result(); + if (g_variant_is_of_type(asyncMethodCallResult.get(), G_VARIANT_TYPE_UNIT)) { + result(); + } }; assert(isHost()); @@ -332,8 +333,9 @@ public: const VoidResultCallback& result) { auto asyncResult = [result](dbus::AsyncMethodCallResult& asyncMethodCallResult) { - BOOST_CHECK(g_variant_is_of_type(asyncMethodCallResult.get(), G_VARIANT_TYPE_UNIT)); - result(); + if (g_variant_is_of_type(asyncMethodCallResult.get(), G_VARIANT_TYPE_UNIT)) { + result(); + } }; assert(isHost()); @@ -351,8 +353,9 @@ public: const VoidResultCallback& result) { auto asyncResult = [result](dbus::AsyncMethodCallResult& asyncMethodCallResult) { - BOOST_CHECK(g_variant_is_of_type(asyncMethodCallResult.get(), G_VARIANT_TYPE_UNIT)); - result(); + if (g_variant_is_of_type(asyncMethodCallResult.get(), G_VARIANT_TYPE_UNIT)) { + result(); + } }; assert(isHost()); @@ -370,8 +373,9 @@ public: const VoidResultCallback& result) { auto asyncResult = [result](dbus::AsyncMethodCallResult& asyncMethodCallResult) { - BOOST_CHECK(g_variant_is_of_type(asyncMethodCallResult.get(), G_VARIANT_TYPE_UNIT)); - result(); + if (g_variant_is_of_type(asyncMethodCallResult.get(), G_VARIANT_TYPE_UNIT)) { + result(); + } }; assert(isHost()); @@ -923,7 +927,7 @@ BOOST_AUTO_TEST_CASE(ZoneDbusesSignalsTest) cm.startAll(); - BOOST_CHECK(signalLatch.waitForN(TEST_DBUS_CONNECTION_ZONES_COUNT, EVENT_TIMEOUT)); + BOOST_REQUIRE(signalLatch.waitForN(TEST_DBUS_CONNECTION_ZONES_COUNT, EVENT_TIMEOUT)); BOOST_CHECK(signalLatch.empty()); BOOST_CHECK(EXPECTED_DBUSES_STARTED == collectedDbuses); collectedDbuses.clear(); -- 2.7.4 From 808219e7cc0fc49ae2f3edabf26b6d7a1a5ecf69 Mon Sep 17 00:00:00 2001 From: Jan Olszak Date: Fri, 9 Jan 2015 13:48:05 +0100 Subject: [PATCH 07/16] IPC: Single state mutex in Processor [Bug/Feature] Fixed a bug in callSync. Replaced all mutexes in Processor with only one. Added LOGS loggs [Cause] N/A [Solution] N/A [Verification] Build, install, run tests, run tests under valgrind Change-Id: I6b6ec26df5f5d7ba8b930e4321f766146fa556f0 --- common/ipc/client.cpp | 19 +- common/ipc/client.hpp | 14 +- common/ipc/internals/call-queue.cpp | 17 +- common/ipc/internals/call-queue.hpp | 21 +- common/ipc/internals/processor.cpp | 377 +++++++++++++++++------------------- common/ipc/internals/processor.hpp | 139 ++++++------- common/ipc/ipc-gsource.cpp | 16 +- common/ipc/service.cpp | 17 +- common/ipc/service.hpp | 16 +- tests/unit_tests/ipc/ut-ipc.cpp | 7 +- 10 files changed, 304 insertions(+), 339 deletions(-) diff --git a/common/ipc/client.cpp b/common/ipc/client.cpp index 3187f15..8455b19 100644 --- a/common/ipc/client.cpp +++ b/common/ipc/client.cpp @@ -34,18 +34,17 @@ namespace ipc { Client::Client(const std::string& socketPath) : mSocketPath(socketPath) { - LOGD("Creating client"); + LOGS("Client Constructor"); } Client::~Client() { - LOGD("Destroying client..."); + LOGS("Client Destructor"); try { stop(); } catch (IPCException& e) { LOGE("Error in Client's destructor: " << e.what()); } - LOGD("Destroyed client"); } void Client::connect() @@ -58,14 +57,9 @@ void Client::connect() void Client::start() { - LOGD("Starting client..."); - + LOGS("Client start"); connect(); - - // Start polling thread mProcessor.start(); - - LOGD("Started client"); } bool Client::isStarted() @@ -75,9 +69,8 @@ bool Client::isStarted() void Client::stop() { - LOGD("Stopping client..."); + LOGS("Client Destructor"); mProcessor.stop(); - LOGD("Stopped"); } std::vector Client::getFDs() @@ -107,17 +100,19 @@ void Client::handle(const FileDescriptor fd, const short pollEvent) void Client::setNewPeerCallback(const PeerCallback& newPeerCallback) { + LOGS("Client setNewPeerCallback"); mProcessor.setNewPeerCallback(newPeerCallback); } void Client::setRemovedPeerCallback(const PeerCallback& removedPeerCallback) { + LOGS("Client setRemovedPeerCallback"); mProcessor.setRemovedPeerCallback(removedPeerCallback); } void Client::removeMethod(const MethodID methodID) { - LOGD("Removing method id: " << methodID); + LOGS("Client removeMethod methodID: " << methodID); mProcessor.removeMethod(methodID); } diff --git a/common/ipc/client.hpp b/common/ipc/client.hpp index b5b00e5..de847a9 100644 --- a/common/ipc/client.hpp +++ b/common/ipc/client.hpp @@ -189,18 +189,16 @@ template void Client::addMethodHandler(const MethodID methodID, const typename MethodHandler::type& method) { - LOGD("Adding method with id " << methodID); + LOGS("Client addMethodHandler, methodID: " << methodID); mProcessor.addMethodHandler(methodID, method); - LOGD("Added method with id " << methodID); } template void Client::addSignalHandler(const MethodID methodID, const typename SignalHandler::type& handler) { - LOGD("Adding signal with id " << methodID); + LOGS("Client addSignalHandler, methodID: " << methodID); mProcessor.addSignalHandler(methodID, handler); - LOGD("Added signal with id " << methodID); } template @@ -208,7 +206,7 @@ std::shared_ptr Client::callSync(const MethodID methodID, const std::shared_ptr& data, unsigned int timeoutMS) { - LOGD("Sync calling method: " << methodID); + LOGS("Client callSync, methodID: " << methodID << ", timeoutMS: " << timeoutMS); return mProcessor.callSync(methodID, mServiceFD, data, timeoutMS); } @@ -217,22 +215,20 @@ void Client::callAsync(const MethodID methodID, const std::shared_ptr& data, const typename ResultHandler::type& resultCallback) { - LOGD("Async calling method: " << methodID); + LOGS("Client callAsync, methodID: " << methodID); mProcessor.callAsync(methodID, mServiceFD, data, resultCallback); - LOGD("Async called method: " << methodID); } template void Client::signal(const MethodID methodID, const std::shared_ptr& data) { - LOGD("Signaling: " << methodID); + LOGS("Client signal, methodID: " << methodID); mProcessor.signal(methodID, data); - LOGD("Signaled: " << methodID); } } // namespace ipc diff --git a/common/ipc/internals/call-queue.cpp b/common/ipc/internals/call-queue.cpp index df70f11..70871e5 100644 --- a/common/ipc/internals/call-queue.cpp +++ b/common/ipc/internals/call-queue.cpp @@ -27,6 +27,7 @@ #include "ipc/internals/call-queue.hpp" #include "ipc/exception.hpp" #include "logger/logger.hpp" +#include namespace vasum { namespace ipc { @@ -51,6 +52,20 @@ MessageID CallQueue::getNextMessageID() return ++mMessageIDCounter; } +bool CallQueue::erase(const MessageID messageID) +{ + LOGT("Erase messgeID: " << messageID); + auto it = std::find(mCalls.begin(), mCalls.end(), messageID); + if (it == mCalls.end()) { + LOGT("No such messgeID"); + return false; + } + + mCalls.erase(it); + LOGT("Erased"); + return true; +} + CallQueue::Call CallQueue::pop() { if (isEmpty()) { @@ -58,7 +73,7 @@ CallQueue::Call CallQueue::pop() throw IPCException("CallQueue is empty"); } Call call = std::move(mCalls.front()); - mCalls.pop(); + mCalls.pop_front(); return call; } diff --git a/common/ipc/internals/call-queue.hpp b/common/ipc/internals/call-queue.hpp index 03adfc8..a6e45ed 100644 --- a/common/ipc/internals/call-queue.hpp +++ b/common/ipc/internals/call-queue.hpp @@ -27,9 +27,10 @@ #include "ipc/types.hpp" #include "config/manager.hpp" +#include "logger/logger-scope.hpp" #include -#include +#include namespace vasum { namespace ipc { @@ -45,9 +46,15 @@ public: struct Call { Call(const Call& other) = delete; Call& operator=(const Call&) = delete; + Call& operator=(Call&&) = default; Call() = default; Call(Call&&) = default; + bool operator==(const MessageID m) + { + return m == messageID; + } + FileDescriptor peerFD; MethodID methodID; MessageID messageID; @@ -78,10 +85,12 @@ public: Call pop(); + bool erase(const MessageID messageID); + bool isEmpty() const; private: - std::queue mCalls; + std::list mCalls; std::atomic mMessageIDCounter; MessageID getNextMessageID(); @@ -103,21 +112,24 @@ MessageID CallQueue::push(const MethodID methodID, call.messageID = messageID; call.serialize = [](const int fd, std::shared_ptr& data)->void { + LOGS("Method serialize, peerFD: " << fd); config::saveToFD(fd, *std::static_pointer_cast(data)); }; call.parse = [](const int fd)->std::shared_ptr { + LOGS("Method parse, peerFD: " << fd); std::shared_ptr data(new ReceivedDataType()); config::loadFromFD(fd, *data); return data; }; call.process = [process](Status status, std::shared_ptr& data)->void { + LOGS("Method process, status: " << toString(status)); std::shared_ptr tmpData = std::static_pointer_cast(data); return process(status, tmpData); }; - mCalls.push(std::move(call)); + mCalls.push_back(std::move(call)); return messageID; } @@ -136,10 +148,11 @@ MessageID CallQueue::push(const MethodID methodID, call.messageID = messageID; call.serialize = [](const int fd, std::shared_ptr& data)->void { + LOGS("Signal serialize, peerFD: " << fd); config::saveToFD(fd, *std::static_pointer_cast(data)); }; - mCalls.push(std::move(call)); + mCalls.push_back(std::move(call)); return messageID; } diff --git a/common/ipc/internals/processor.cpp b/common/ipc/internals/processor.cpp index 6084718..d1e829e 100644 --- a/common/ipc/internals/processor.cpp +++ b/common/ipc/internals/processor.cpp @@ -59,25 +59,22 @@ Processor::Processor(const PeerCallback& newPeerCallback, mRemovedPeerCallback(removedPeerCallback), mMaxNumberOfPeers(maxNumberOfPeers) { - LOGT("Creating Processor"); + LOGS("Processor Constructor"); utils::signalBlock(SIGPIPE); using namespace std::placeholders; addMethodHandlerInternal(REGISTER_SIGNAL_METHOD_ID, std::bind(&Processor::onNewSignals, this, _1, _2)); - } Processor::~Processor() { - LOGT("Destroying Processor"); + LOGS("Processor Destructor"); try { stop(); } catch (IPCException& e) { LOGE("Error in Processor's destructor: " << e.what()); } - - LOGT("Destroyed Processor"); } bool Processor::isStarted() @@ -87,60 +84,60 @@ bool Processor::isStarted() void Processor::start() { - LOGT("Starting Processor"); + LOGS("Processor start"); + if (!isStarted()) { mThread = std::thread(&Processor::run, this); } - LOGT("Started Processor"); } void Processor::stop() { - LOGT("Stopping Processor"); + LOGS("Processor stop"); if (isStarted()) { - mEventQueue.send(Event::FINISH); + { + Lock lock(mStateMutex); + mEventQueue.send(Event::FINISH); + } + LOGT("Waiting for the Processor to stop"); mThread.join(); } - - LOGT("Stopped Processor"); } void Processor::setNewPeerCallback(const PeerCallback& newPeerCallback) { - Lock lock(mCallbacksMutex); + Lock lock(mStateMutex); mNewPeerCallback = newPeerCallback; } void Processor::setRemovedPeerCallback(const PeerCallback& removedPeerCallback) { - Lock lock(mCallbacksMutex); + Lock lock(mStateMutex); mRemovedPeerCallback = removedPeerCallback; } FileDescriptor Processor::getEventFD() { + Lock lock(mStateMutex); return mEventQueue.getFD(); } void Processor::removeMethod(const MethodID methodID) { - LOGT("Removing method " << methodID); - Lock lock(mCallsMutex); + Lock lock(mStateMutex); mMethodsCallbacks.erase(methodID); } FileDescriptor Processor::addPeer(const std::shared_ptr& socketPtr) { - LOGT("Adding socket"); - FileDescriptor peerFD; - { - Lock lock(mSocketsMutex); - peerFD = socketPtr->getFD(); - SocketInfo socketInfo(peerFD, std::move(socketPtr)); - mNewSockets.push(std::move(socketInfo)); - mEventQueue.send(Event::ADD_PEER); - } + LOGS("Processor addPeer"); + Lock lock(mStateMutex); + FileDescriptor peerFD = socketPtr->getFD(); + SocketInfo socketInfo(peerFD, std::move(socketPtr)); + mNewSockets.push(std::move(socketInfo)); + mEventQueue.send(Event::ADD_PEER); + LOGI("New peer added. Id: " << peerFD); return peerFD; @@ -148,18 +145,22 @@ FileDescriptor Processor::addPeer(const std::shared_ptr& socketPtr) void Processor::removePeer(const FileDescriptor peerFD) { - std::shared_ptr conditionPtr(new std::condition_variable()); + LOGS("Processor removePeer peerFD: " << peerFD); + + // TODO: Remove ADD_PEER event if it's not processed + + // Remove peer and wait till he's gone + std::shared_ptr conditionPtr(new std::condition_variable()); { - Lock lock(mSocketsMutex); + Lock lock(mStateMutex); RemovePeerRequest request(peerFD, conditionPtr); mPeersToDelete.push(std::move(request)); mEventQueue.send(Event::REMOVE_PEER); } - auto isPeerDeleted = [&peerFD, this]()->bool { - Lock lock(mSocketsMutex); + Lock lock(mStateMutex); return mSockets.count(peerFD) == 0; }; @@ -170,49 +171,41 @@ void Processor::removePeer(const FileDescriptor peerFD) void Processor::removePeerInternal(const FileDescriptor peerFD, Status status) { - LOGW("Removing peer. ID: " << peerFD); - { - Lock lock(mSocketsMutex); - if (!mSockets.erase(peerFD)) { - LOGW("No such peer. Another thread called removePeerInternal"); - return; - } + LOGS("Processor removePeerInternal peerFD: " << peerFD); + LOGI("Removing peer. peerFD: " << peerFD); - // Remove from signal addressees - for (auto it = mSignalsPeers.begin(); it != mSignalsPeers.end();) { - it->second.remove(peerFD); - if (it->second.empty()) { - it = mSignalsPeers.erase(it); - } else { - ++it; - } - } + if (!mSockets.erase(peerFD)) { + LOGW("No such peer. Another thread called removePeerInternal"); + return; } - { - // Erase associated return value callbacks - Lock lock(mReturnCallbacksMutex); - - std::shared_ptr data; - for (auto it = mReturnCallbacks.begin(); it != mReturnCallbacks.end();) { - if (it->second.peerFD == peerFD) { - IGNORE_EXCEPTIONS(it->second.process(status, data)); - it = mReturnCallbacks.erase(it); - } else { - ++it; - } + // Remove from signal addressees + for (auto it = mSignalsPeers.begin(); it != mSignalsPeers.end();) { + it->second.remove(peerFD); + if (it->second.empty()) { + it = mSignalsPeers.erase(it); + } else { + ++it; } } - - { - Lock lock(mCallbacksMutex); - if (mRemovedPeerCallback) { - // Notify about the deletion - mRemovedPeerCallback(peerFD); + // Erase associated return value callbacks + std::shared_ptr data; + for (auto it = mReturnCallbacks.begin(); it != mReturnCallbacks.end();) { + if (it->second.peerFD == peerFD) { + IGNORE_EXCEPTIONS(it->second.process(status, data)); + it = mReturnCallbacks.erase(it); + } else { + ++it; } } + if (mRemovedPeerCallback) { + // Notify about the deletion + mRemovedPeerCallback(peerFD); + } + + resetPolling(); } @@ -222,25 +215,29 @@ void Processor::resetPolling() return; } - LOGI("Resetting polling"); - // Setup polling on eventfd and sockets - Lock lock(mSocketsMutex); - mFDs.resize(mSockets.size() + 1); + { + Lock lock(mStateMutex); - mFDs[0].fd = mEventQueue.getFD(); - mFDs[0].events = POLLIN; + // Setup polling on eventfd and sockets + mFDs.resize(mSockets.size() + 1); - auto socketIt = mSockets.begin(); - for (unsigned int i = 1; i < mFDs.size(); ++i) { - mFDs[i].fd = socketIt->second->getFD(); - mFDs[i].events = POLLIN | POLLHUP; // Listen for input events - ++socketIt; - // TODO: It's possible to block on writing to fd. Maybe listen for POLLOUT too? + mFDs[0].fd = mEventQueue.getFD(); + mFDs[0].events = POLLIN; + + auto socketIt = mSockets.begin(); + for (unsigned int i = 1; i < mFDs.size(); ++i) { + mFDs[i].fd = socketIt->second->getFD(); + mFDs[i].events = POLLIN | POLLHUP; // Listen for input events + ++socketIt; + // TODO: It's possible to block on writing to fd. Maybe listen for POLLOUT too? + } } } void Processor::run() { + LOGS("Processor run"); + resetPolling(); mIsRunning = true; @@ -282,60 +279,53 @@ void Processor::run() bool Processor::handleLostConnections() { - std::vector peersToRemove; + Lock lock(mStateMutex); + + bool isPeerRemoved = false; { - Lock lock(mSocketsMutex); for (unsigned int i = 1; i < mFDs.size(); ++i) { if (mFDs[i].revents & POLLHUP) { LOGI("Lost connection to peer: " << mFDs[i].fd); mFDs[i].revents &= ~(POLLHUP); - peersToRemove.push_back(mFDs[i].fd); + removePeerInternal(mFDs[i].fd, Status::PEER_DISCONNECTED); + isPeerRemoved = true; } } } - for (const FileDescriptor peerFD : peersToRemove) { - removePeerInternal(peerFD, Status::PEER_DISCONNECTED); - } - - return !peersToRemove.empty(); + return isPeerRemoved; } bool Processor::handleLostConnection(const FileDescriptor peerFD) { + Lock lock(mStateMutex); removePeerInternal(peerFD, Status::PEER_DISCONNECTED); return true; } bool Processor::handleInputs() { - std::vector peersWithInput; - { - Lock lock(mSocketsMutex); - for (unsigned int i = 1; i < mFDs.size(); ++i) { - if (mFDs[i].revents & POLLIN) { - mFDs[i].revents &= ~(POLLIN); - peersWithInput.push_back(mFDs[i].fd); - } - } - } + Lock lock(mStateMutex); bool pollChanged = false; - // Handle input outside the critical section - for (const FileDescriptor peerFD : peersWithInput) { - pollChanged = pollChanged || handleInput(peerFD); + for (unsigned int i = 1; i < mFDs.size(); ++i) { + if (mFDs[i].revents & POLLIN) { + mFDs[i].revents &= ~(POLLIN); + pollChanged = pollChanged || handleInput(mFDs[i].fd); + } } + return pollChanged; } bool Processor::handleInput(const FileDescriptor peerFD) { - LOGT("Handle incoming data"); + LOGS("Processor handleInput peerFD: " << peerFD); + Lock lock(mStateMutex); std::shared_ptr socketPtr; try { // Get the peer's socket - Lock lock(mSocketsMutex); socketPtr = mSockets.at(peerFD); } catch (const std::out_of_range&) { LOGE("No such peer: " << peerFD); @@ -360,22 +350,18 @@ bool Processor::handleInput(const FileDescriptor peerFD) return onReturnValue(*socketPtr, messageID); } else { - Lock lock(mCallsMutex); if (mMethodsCallbacks.count(methodID)) { // Method std::shared_ptr methodCallbacks = mMethodsCallbacks.at(methodID); - lock.unlock(); return onRemoteCall(*socketPtr, methodID, messageID, methodCallbacks); } else if (mSignalsCallbacks.count(methodID)) { // Signal std::shared_ptr signalCallbacks = mSignalsCallbacks.at(methodID); - lock.unlock(); return onRemoteSignal(*socketPtr, methodID, messageID, signalCallbacks); } else { // Nothing - lock.unlock(); LOGW("No method or signal callback for methodID: " << methodID); removePeerInternal(socketPtr->getFD(), Status::NAUGHTY_PEER); return true; @@ -387,9 +373,9 @@ bool Processor::handleInput(const FileDescriptor peerFD) std::shared_ptr Processor::onNewSignals(const FileDescriptor peerFD, std::shared_ptr& data) { - LOGD("New signals for peer: " << peerFD); - Lock lock(mSocketsMutex); - for (MethodID methodID : data->ids) { + LOGS("Processor onNewSignals peerFD: " << peerFD); + + for (const MethodID methodID : data->ids) { mSignalsPeers[methodID].push_back(peerFD); } @@ -399,10 +385,11 @@ std::shared_ptr Processor::onNewSignals(const FileDescript bool Processor::onReturnValue(const Socket& socket, const MessageID messageID) { - LOGI("Return value for messageID: " << messageID); + LOGS("Processor onReturnValue messageID: " << messageID); + + // LOGI("Return value for messageID: " << messageID); ReturnCallbacks returnCallbacks; try { - Lock lock(mReturnCallbacksMutex); LOGT("Getting the return callback"); returnCallbacks = std::move(mReturnCallbacks.at(messageID)); mReturnCallbacks.erase(messageID); @@ -423,9 +410,10 @@ bool Processor::onReturnValue(const Socket& socket, return true; } - LOGT("Process return value callback for messageID: " << messageID); + // LOGT("Process return value callback for messageID: " << messageID); IGNORE_EXCEPTIONS(returnCallbacks.process(Status::OK, data)); + // LOGT("Return value for messageID: " << messageID << " processed"); return false; } @@ -434,7 +422,9 @@ bool Processor::onRemoteSignal(const Socket& socket, const MessageID messageID, std::shared_ptr signalCallbacks) { - LOGI("Remote signal; methodID: " << methodID << " messageID: " << messageID); + LOGS("Processor onRemoteSignal; methodID: " << methodID << " messageID: " << messageID); + + // LOGI("Processor onRemoteSignal; methodID: " << methodID << " messageID: " << messageID); std::shared_ptr data; try { @@ -446,7 +436,7 @@ bool Processor::onRemoteSignal(const Socket& socket, return true; } - LOGT("Signal callback for methodID: " << methodID << "; messageID: " << messageID); + // LOGT("Signal callback for methodID: " << methodID << "; messageID: " << messageID); try { signalCallbacks->signal(socket.getFD(), data); } catch (const std::exception& e) { @@ -463,7 +453,8 @@ bool Processor::onRemoteCall(const Socket& socket, const MessageID messageID, std::shared_ptr methodCallbacks) { - LOGI("Remote call; methodID: " << methodID << " messageID: " << messageID); + LOGS("Processor onRemoteCall; methodID: " << methodID << " messageID: " << messageID); + // LOGI("Remote call; methodID: " << methodID << " messageID: " << messageID); std::shared_ptr data; try { @@ -503,13 +494,16 @@ bool Processor::onRemoteCall(const Socket& socket, bool Processor::handleEvent() { + LOGS("Processor handleEvent"); + + Lock lock(mStateMutex); + switch (mEventQueue.receive()) { + case Event::FINISH: { LOGD("Event FINISH"); - mIsRunning = false; cleanCommunication(); - return false; } @@ -534,91 +528,69 @@ bool Processor::handleEvent() bool Processor::onNewPeer() { - SocketInfo socketInfo; - { - Lock lock(mSocketsMutex); + LOGS("Processor onNewPeer"); - socketInfo = std::move(mNewSockets.front()); - mNewSockets.pop(); - - if (mSockets.size() > mMaxNumberOfPeers) { - LOGE("There are too many peers. I don't accept the connection with " << socketInfo.peerFD); - return false; - } - if (mSockets.count(socketInfo.peerFD) != 0) { - LOGE("There already was a socket for peerFD: " << socketInfo.peerFD); - return false; - } + // TODO: What if there is no newSocket? (request removed in the mean time) + // Add new socket of the peer + SocketInfo socketInfo = std::move(mNewSockets.front()); + mNewSockets.pop(); - mSockets[socketInfo.peerFD] = std::move(socketInfo.socketPtr); + if (mSockets.size() > mMaxNumberOfPeers) { + LOGE("There are too many peers. I don't accept the connection with " << socketInfo.peerFD); + return false; + } + if (mSockets.count(socketInfo.peerFD) != 0) { + LOGE("There already was a socket for peerFD: " << socketInfo.peerFD); + return false; } + mSockets[socketInfo.peerFD] = std::move(socketInfo.socketPtr); - // Broadcast the new signal to peers - LOGW("Sending handled signals"); - std::list peersFDs; - { - Lock lock(mSocketsMutex); - for (const auto kv : mSockets) { - peersFDs.push_back(kv.first); - } - } + // LOGW("Sending handled signals"); std::vector ids; - { - Lock lock(mSocketsMutex); - for (const auto kv : mSignalsCallbacks) { - ids.push_back(kv.first); - } + for (const auto kv : mSignalsCallbacks) { + ids.push_back(kv.first); } auto data = std::make_shared(ids); - - for (const FileDescriptor peerFD : peersFDs) { - callInternal(REGISTER_SIGNAL_METHOD_ID, - peerFD, - data, - discardResultHandler); - } - LOGW("Sent handled signals"); - + callAsync(REGISTER_SIGNAL_METHOD_ID, + socketInfo.peerFD, + data, + discardResultHandler); + // LOGW("Sent handled signals"); resetPolling(); - { - Lock lock(mCallbacksMutex); - if (mNewPeerCallback) { - // Notify about the new user. - LOGT("Calling NewPeerCallback"); - mNewPeerCallback(socketInfo.peerFD); - } + if (mNewPeerCallback) { + // Notify about the new user. + LOGT("Calling NewPeerCallback"); + mNewPeerCallback(socketInfo.peerFD); } + return true; } bool Processor::onRemovePeer() { - RemovePeerRequest request; - { - Lock lock(mSocketsMutex); - request = std::move(mPeersToDelete.front()); - mPeersToDelete.pop(); - } + LOGS("Processor onRemovePeer"); - removePeerInternal(request.peerFD, Status::REMOVED_PEER); - request.conditionPtr->notify_all(); - return true; -} + removePeerInternal(mPeersToDelete.front().peerFD, Status::REMOVED_PEER); -CallQueue::Call Processor::getCall() -{ - Lock lock(mCallsMutex); - return mCalls.pop(); + mPeersToDelete.front().conditionPtr->notify_all(); + mPeersToDelete.pop(); + return true; } bool Processor::onCall() { - LOGT("Handle call (from another thread) to send a message."); - CallQueue::Call call = getCall(); + LOGS("Processor onCall"); + CallQueue::Call call; + try { + call = std::move(mCalls.pop()); + } catch (const IPCException&) { + LOGE("No calls to serve, but got an EVENT::CALL. Event got removed before serving"); + return false; + } if (call.parse && call.process) { return onMethodCall(call); @@ -629,10 +601,11 @@ bool Processor::onCall() bool Processor::onSignalCall(CallQueue::Call& call) { + LOGS("Processor onSignalCall"); + std::shared_ptr socketPtr; try { // Get the peer's socket - Lock lock(mSocketsMutex); socketPtr = mSockets.at(call.peerFD); } catch (const std::out_of_range&) { LOGE("Peer disconnected. No socket with a peerFD: " << call.peerFD); @@ -653,15 +626,16 @@ bool Processor::onSignalCall(CallQueue::Call& call) } return false; - } bool Processor::onMethodCall(CallQueue::Call& call) { + LOGS("Processor onMethodCall"); std::shared_ptr socketPtr; + + try { // Get the peer's socket - Lock lock(mSocketsMutex); socketPtr = mSockets.at(call.peerFD); } catch (const std::out_of_range&) { LOGE("Peer disconnected. No socket with a peerFD: " << call.peerFD); @@ -672,22 +646,19 @@ bool Processor::onMethodCall(CallQueue::Call& call) return false; } - { - // Set what to do with the return message, but only if needed - Lock lock(mReturnCallbacksMutex); - if (mReturnCallbacks.count(call.messageID) != 0) { - LOGE("There already was a return callback for messageID: " << call.messageID); - } - mReturnCallbacks[call.messageID] = std::move(ReturnCallbacks(call.peerFD, - std::move(call.parse), - std::move(call.process))); + if (mReturnCallbacks.count(call.messageID) != 0) { + LOGE("There already was a return callback for messageID: " << call.messageID); } + mReturnCallbacks[call.messageID] = std::move(ReturnCallbacks(call.peerFD, + std::move(call.parse), + std::move(call.process))); try { // Send the call with the socket Socket::Guard guard = socketPtr->getGuard(); socketPtr->write(&call.methodID, sizeof(call.methodID)); socketPtr->write(&call.messageID, sizeof(call.messageID)); + LOGT("Serializing the message"); call.serialize(socketPtr->getFD(), call.data); } catch (const std::exception& e) { LOGE("Error during sending a method: " << e.what()); @@ -695,13 +666,12 @@ bool Processor::onMethodCall(CallQueue::Call& call) // Inform about the error, IGNORE_EXCEPTIONS(mReturnCallbacks[call.messageID].process(Status::SERIALIZATION_ERROR, call.data)); - { - Lock lock(mReturnCallbacksMutex); - mReturnCallbacks.erase(call.messageID); - } + mReturnCallbacks.erase(call.messageID); removePeerInternal(call.peerFD, Status::SERIALIZATION_ERROR); + return true; + } return false; @@ -709,35 +679,36 @@ bool Processor::onMethodCall(CallQueue::Call& call) void Processor::cleanCommunication() { + LOGS("Processor cleanCommunication"); + while (!mEventQueue.isEmpty()) { switch (mEventQueue.receive()) { case Event::FINISH: { - LOGD("Event FINISH after FINISH"); + LOGE("Event FINISH after FINISH"); break; } case Event::CALL: { - LOGD("Event CALL after FINISH"); - CallQueue::Call call = getCall(); - if (call.process) { - IGNORE_EXCEPTIONS(call.process(Status::CLOSING, call.data)); + LOGW("Event CALL after FINISH"); + try { + CallQueue::Call call = mCalls.pop(); + if (call.process) { + IGNORE_EXCEPTIONS(call.process(Status::CLOSING, call.data)); + } + } catch (const IPCException&) { + // No more calls } break; } case Event::ADD_PEER: { - LOGD("Event ADD_PEER after FINISH"); + LOGW("Event ADD_PEER after FINISH"); break; } case Event::REMOVE_PEER: { - LOGD("Event REMOVE_PEER after FINISH"); - RemovePeerRequest request; - { - Lock lock(mSocketsMutex); - request = std::move(mPeersToDelete.front()); - mPeersToDelete.pop(); - } - request.conditionPtr->notify_all(); + LOGW("Event REMOVE_PEER after FINISH"); + mPeersToDelete.front().conditionPtr->notify_all(); + mPeersToDelete.pop(); break; } } diff --git a/common/ipc/internals/processor.hpp b/common/ipc/internals/processor.hpp index 0e8fe8f..b0f7ea0 100644 --- a/common/ipc/internals/processor.hpp +++ b/common/ipc/internals/processor.hpp @@ -33,6 +33,7 @@ #include "config/manager.hpp" #include "config/fields.hpp" #include "logger/logger.hpp" +#include "logger/logger-scope.hpp" #include #include @@ -75,13 +76,13 @@ const unsigned int DEFAULT_METHOD_TIMEOUT = 1000; * - new way to generate UIDs * - callbacks for serialization/parsing * - store Sockets in a vector, maybe SocketStore? -* - fix valgrind tests * - poll loop outside. * - waiting till the EventQueue is empty before leaving stop() * - no new events added after stop() called * - when using IPCGSource: addFD and removeFD can be called from addPeer removePeer callbacks, but * there is no mechanism to ensure the IPCSource exists.. therefore SIGSEGV :) -* +* - EventQueue should store std::shared_ptr and it should be the only queue to the Processor thread. +* It should have an API for removing events from the middle of the queue * */ class Processor { @@ -279,7 +280,7 @@ public: private: typedef std::function& data)> SerializeCallback; typedef std::function(int fd)> ParseCallback; - typedef std::unique_lock Lock; + typedef std::unique_lock Lock; struct EmptyData { CONFIG_REGISTER_EMPTY @@ -376,27 +377,22 @@ private: bool mIsRunning; - // Mutex for the Calls queue and the map of methods. - std::mutex mCallsMutex; + CallQueue mCalls; std::unordered_map> mMethodsCallbacks; std::unordered_map> mSignalsCallbacks; std::unordered_map> mSignalsPeers; - // Mutex for changing mSockets map. - // Shouldn't be locked on any read/write, that could block. Just copy the ptr. - std::mutex mSocketsMutex; std::unordered_map > mSockets; std::vector mFDs; std::queue mNewSockets; std::queue mPeersToDelete; - // Mutex for modifying the map with return callbacks - std::mutex mReturnCallbacksMutex; std::unordered_map mReturnCallbacks; - // Mutex for setting callbacks - std::mutex mCallbacksMutex; + // Mutex for modifying any internal data + std::recursive_mutex mStateMutex; + PeerCallback mNewPeerCallback; PeerCallback mRemovedPeerCallback; @@ -408,12 +404,6 @@ private: void addMethodHandlerInternal(const MethodID methodID, const typename MethodHandler::type& process); - template - MessageID callInternal(const MethodID methodID, - const FileDescriptor peerFD, - const std::shared_ptr& data, - const typename ResultHandler::type& process); - template static void discardResultHandler(Status, std::shared_ptr&) {} @@ -438,7 +428,6 @@ private: std::shared_ptr signalCallbacks); void resetPolling(); FileDescriptor getNextFileDescriptor(); - CallQueue::Call getCall(); void removePeerInternal(const FileDescriptor peerFD, Status status); std::shared_ptr onNewSignals(const FileDescriptor peerFD, @@ -470,7 +459,7 @@ void Processor::addMethodHandlerInternal(const MethodID methodID, }; { - Lock lock(mCallsMutex); + Lock lock(mStateMutex); mMethodsCallbacks[methodID] = std::make_shared(std::move(methodCall)); } } @@ -485,14 +474,16 @@ void Processor::addMethodHandler(const MethodID methodID, } { - Lock lock(mCallsMutex); + Lock lock(mStateMutex); + if (mSignalsCallbacks.count(methodID)) { LOGE("MethodID used by a signal: " << methodID); throw IPCException("MethodID used by a signal: " + std::to_string(methodID)); } + + addMethodHandlerInternal(methodID, method); } - addMethodHandlerInternal(methodID, method); } template @@ -504,64 +495,49 @@ void Processor::addSignalHandler(const MethodID methodID, throw IPCException("Forbidden methodID: " + std::to_string(methodID)); } + std::shared_ptr data; + std::vector peerFDs; { - Lock lock(mCallsMutex); + Lock lock(mStateMutex); + + // Andd the signal handler: if (mMethodsCallbacks.count(methodID)) { LOGE("MethodID used by a method: " << methodID); throw IPCException("MethodID used by a method: " + std::to_string(methodID)); } - } - SignalHandlers signalCall; + SignalHandlers signalCall; - signalCall.parse = [](const int fd)->std::shared_ptr { - std::shared_ptr data(new ReceivedDataType()); - config::loadFromFD(fd, *data); - return data; - }; + signalCall.parse = [](const int fd)->std::shared_ptr { + std::shared_ptr data(new ReceivedDataType()); + config::loadFromFD(fd, *data); + return data; + }; - signalCall.signal = [handler](const FileDescriptor peerFD, std::shared_ptr& data) { - std::shared_ptr tmpData = std::static_pointer_cast(data); - handler(peerFD, tmpData); - }; + signalCall.signal = [handler](const FileDescriptor peerFD, std::shared_ptr& data) { + std::shared_ptr tmpData = std::static_pointer_cast(data); + handler(peerFD, tmpData); + }; - { - Lock lock(mCallsMutex); mSignalsCallbacks[methodID] = std::make_shared(std::move(signalCall)); - } - std::vector ids {methodID}; - auto data = std::make_shared(ids); + // Broadcast the new signal: + std::vector ids {methodID}; + data = std::make_shared(ids); - std::list peersFDs; - { - Lock lock(mSocketsMutex); for (const auto kv : mSockets) { - peersFDs.push_back(kv.first); + peerFDs.push_back(kv.first); } } - for (const FileDescriptor peerFD : peersFDs) { + for (const auto peerFD : peerFDs) { callSync(REGISTER_SIGNAL_METHOD_ID, peerFD, data, DEFAULT_METHOD_TIMEOUT); } - } -template -MessageID Processor::callInternal(const MethodID methodID, - const FileDescriptor peerFD, - const std::shared_ptr& data, - const typename ResultHandler::type& process) -{ - Lock lock(mCallsMutex); - MessageID messageID = mCalls.push(methodID, peerFD, data, process); - mEventQueue.send(Event::CALL); - - return messageID; -} template MessageID Processor::callAsync(const MethodID methodID, @@ -569,7 +545,11 @@ MessageID Processor::callAsync(const MethodID methodID, const std::shared_ptr& data, const typename ResultHandler::type& process) { - return callInternal(methodID, peerFD, data, process); + Lock lock(mStateMutex); + MessageID messageID = mCalls.push(methodID, peerFD, data, process); + mEventQueue.send(Event::CALL); + + return messageID; } @@ -586,7 +566,6 @@ std::shared_ptr Processor::callSync(const MethodID methodID, Status returnStatus = ipc::Status::UNDEFINED; auto process = [&result, &mutex, &cv, &returnStatus](Status status, std::shared_ptr returnedData) { - std::unique_lock lock(mutex); returnStatus = status; result = returnedData; cv.notify_all(); @@ -603,30 +582,25 @@ std::shared_ptr Processor::callSync(const MethodID methodID, std::unique_lock lock(mutex); LOGT("Waiting for the response..."); - // TODO: There is a race here. mReturnCallbacks were used to indicate if the return call was served or not, - // but if the timeout occurs before the call is even sent, then this method is broken. if (!cv.wait_for(lock, std::chrono::milliseconds(timeoutMS), isResultInitialized)) { - // Timeout occurred: - // - call isn't sent => delete it - // - call is sent and no reply => throw IPCTimeoutError - // - call is being serviced => wait for it with the same timeout - LOGT("Probably a timeout in callSync. Checking..."); - - bool isTimeout = false; + LOGW("Probably a timeout in callSync. Checking..."); + bool isTimeout; { - Lock lock(mReturnCallbacksMutex); - if (1 == mReturnCallbacks.erase(messageID)) { - // Return callback was present, so there was a timeout - isTimeout = true; - } + Lock lock(mStateMutex); + // Call isn't sent or call is sent but there is no reply + isTimeout = mCalls.erase(messageID) || 1 == mReturnCallbacks.erase(messageID); } + if (isTimeout) { - removePeer(peerFD); LOGE("Function call timeout; methodID: " << methodID); + removePeer(peerFD); throw IPCTimeoutException("Function call timeout; methodID: " + std::to_string(methodID)); } else { - //Timeout started during the return value processing, so wait for it to finish - cv.wait(lock, isResultInitialized); + LOGW("Timeout started during the return value processing, so wait for it to finish"); + if (!cv.wait_for(lock, std::chrono::milliseconds(timeoutMS), isResultInitialized)) { + LOGE("Function call timeout; methodID: " << methodID); + throw IPCTimeoutException("Function call timeout; methodID: " + std::to_string(methodID)); + } } } @@ -639,14 +613,13 @@ template void Processor::signal(const MethodID methodID, const std::shared_ptr& data) { - std::list peersFDs; - { - Lock lock(mSocketsMutex); - peersFDs = mSignalsPeers[methodID]; + Lock lock(mStateMutex); + const auto it = mSignalsPeers.find(methodID); + if (it == mSignalsPeers.end()) { + LOGW("No peer is handling signal with methodID: " << methodID); + return; } - - for (const FileDescriptor peerFD : peersFDs) { - Lock lock(mCallsMutex); + for (const FileDescriptor peerFD : it->second) { mCalls.push(methodID, peerFD, data); mEventQueue.send(Event::CALL); } diff --git a/common/ipc/ipc-gsource.cpp b/common/ipc/ipc-gsource.cpp index 4c098d9..5a4e137 100644 --- a/common/ipc/ipc-gsource.cpp +++ b/common/ipc/ipc-gsource.cpp @@ -48,7 +48,8 @@ IPCGSource::IPCGSource(const std::vector fds, const HandlerCallback& handlerCallback) : mHandlerCallback(handlerCallback) { - LOGD("Constructing IPCGSource"); + LOGS("IPCGSource constructor"); + for (const FileDescriptor fd : fds) { addFD(fd); } @@ -56,13 +57,13 @@ IPCGSource::IPCGSource(const std::vector fds, IPCGSource::~IPCGSource() { - LOGD("Destroying IPCGSource"); + LOGS("~IPCGSource"); } IPCGSource::Pointer IPCGSource::create(const std::vector& fds, const HandlerCallback& handlerCallback) { - LOGD("Creating IPCGSource"); + LOGS("Creating IPCGSource"); static GSourceFuncs funcs = { &IPCGSource::prepare, &IPCGSource::check, @@ -94,12 +95,13 @@ IPCGSource::Pointer IPCGSource::create(const std::vector& fds, void IPCGSource::addFD(const FileDescriptor fd) { + if (!&mGSource) { // In case it's called as a callback but the IPCGSource is destroyed return; } + LOGS("Adding fd to glib"); - LOGD("Adding fd to glib"); gpointer tag = g_source_add_unix_fd(&mGSource, fd, conditions); @@ -114,7 +116,7 @@ void IPCGSource::removeFD(const FileDescriptor fd) return; } - LOGD("Removing fd from glib"); + LOGS("Removing fd from glib"); auto it = std::find(mFDInfos.begin(), mFDInfos.end(), fd); if (it == mFDInfos.end()) { LOGE("No such fd"); @@ -126,7 +128,7 @@ void IPCGSource::removeFD(const FileDescriptor fd) guint IPCGSource::attach(GMainContext* context) { - LOGD("Attaching to GMainContext"); + LOGS("Attaching to GMainContext"); guint ret = g_source_attach(&mGSource, context); g_source_unref(&mGSource); return ret; @@ -177,6 +179,8 @@ gboolean IPCGSource::dispatch(GSource* gSource, void IPCGSource::finalize(GSource* gSource) { + LOGS("IPCGSource Finalize"); + if (gSource) { IPCGSource* source = reinterpret_cast(gSource); source->~IPCGSource(); diff --git a/common/ipc/service.cpp b/common/ipc/service.cpp index 5e720d6..ef46346 100644 --- a/common/ipc/service.cpp +++ b/common/ipc/service.cpp @@ -40,30 +40,27 @@ Service::Service(const std::string& socketPath, mAcceptor(socketPath, std::bind(&Processor::addPeer, &mProcessor, _1)) { - LOGD("Creating server"); + LOGS("Service Constructor"); } Service::~Service() { - LOGD("Destroying server..."); + LOGS("Service Destructor"); try { stop(); } catch (IPCException& e) { LOGE("Error in Service's destructor: " << e.what()); } - LOGD("Destroyed"); } void Service::start() { - LOGD("Starting server"); + LOGS("Service start"); mProcessor.start(); // There can be an incoming connection from mAcceptor before mProcessor is listening, // but it's OK. It will handle the connection when ready. So no need to wait for mProcessor. mAcceptor.start(); - - LOGD("Started server"); } bool Service::isStarted() @@ -73,10 +70,9 @@ bool Service::isStarted() void Service::stop() { - LOGD("Stopping server.."); + LOGS("Service stop"); mAcceptor.stop(); mProcessor.stop(); - LOGD("Stopped"); } std::vector Service::getFDs() @@ -116,19 +112,20 @@ void Service::handle(const FileDescriptor fd, const short pollEvent) void Service::setNewPeerCallback(const PeerCallback& newPeerCallback) { + LOGS("Service setNewPeerCallback"); mProcessor.setNewPeerCallback(newPeerCallback); } void Service::setRemovedPeerCallback(const PeerCallback& removedPeerCallback) { + LOGS("Service setRemovedPeerCallback"); mProcessor.setRemovedPeerCallback(removedPeerCallback); } void Service::removeMethod(const MethodID methodID) { - LOGD("Removing method " << methodID); + LOGS("Service removeMethod methodID: " << methodID); mProcessor.removeMethod(methodID); - LOGD("Removed " << methodID); } diff --git a/common/ipc/service.hpp b/common/ipc/service.hpp index ed83606..fa12e30 100644 --- a/common/ipc/service.hpp +++ b/common/ipc/service.hpp @@ -188,18 +188,16 @@ template void Service::addMethodHandler(const MethodID methodID, const typename MethodHandler::type& method) { - LOGD("Adding method with id " << methodID); + LOGS("Service addMethodHandler, methodID " << methodID); mProcessor.addMethodHandler(methodID, method); - LOGD("Added method with id " << methodID); } template void Service::addSignalHandler(const MethodID methodID, const typename SignalHandler::type& handler) { - LOGD("Adding signal with id " << methodID); + LOGS("Service addSignalHandler, methodID " << methodID); mProcessor.addSignalHandler(methodID, handler); - LOGD("Added signal with id " << methodID); } template @@ -208,7 +206,9 @@ std::shared_ptr Service::callSync(const MethodID methodID, const std::shared_ptr& data, unsigned int timeoutMS) { - LOGD("Sync calling method: " << methodID << " for user: " << peerFD); + LOGS("Service callSync, methodID: " << methodID + << ", peerFD: " << peerFD + << ", timeoutMS: " << timeoutMS); return mProcessor.callSync(methodID, peerFD, data, timeoutMS); } @@ -218,22 +218,20 @@ void Service::callAsync(const MethodID methodID, const std::shared_ptr& data, const typename ResultHandler::type& resultCallback) { - LOGD("Async calling method: " << methodID << " for user: " << peerFD); + LOGS("Service callAsync, methodID: " << methodID << ", peerFD: " << peerFD); mProcessor.callAsync(methodID, peerFD, data, resultCallback); - LOGD("Async called method: " << methodID << "for user: " << peerFD); } template void Service::signal(const MethodID methodID, const std::shared_ptr& data) { - LOGD("Signaling: " << methodID); + LOGS("Service signal, methodID: " << methodID); mProcessor.signal(methodID, data); - LOGD("Signaled: " << methodID); } } // namespace ipc diff --git a/tests/unit_tests/ipc/ut-ipc.cpp b/tests/unit_tests/ipc/ut-ipc.cpp index 74c18f3..7c9df6c 100644 --- a/tests/unit_tests/ipc/ut-ipc.cpp +++ b/tests/unit_tests/ipc/ut-ipc.cpp @@ -63,7 +63,7 @@ const int TIMEOUT = 1000 /*ms*/; const int SHORT_OPERATION_TIME = TIMEOUT / 100; // Time that will cause "TIMEOUT" methods to throw -const int LONG_OPERATION_TIME = 3 * TIMEOUT; +const int LONG_OPERATION_TIME = 500 + TIMEOUT; struct Fixture { std::string socketPath; @@ -89,7 +89,7 @@ struct SendData { }; struct LongSendData { - LongSendData(int i = 0, int waitTime = 1000): mSendData(i), mWaitTime(waitTime), intVal(i) {} + LongSendData(int i, int waitTime): mSendData(i), mWaitTime(waitTime), intVal(i) {} template void accept(Visitor visitor) @@ -605,6 +605,9 @@ BOOST_AUTO_TEST_CASE(AddSignalInRuntime) c.addSignalHandler(1, handlerA); c.addSignalHandler(2, handlerB); + // Wait for the signals to propagate to the Service + std::this_thread::sleep_for(std::chrono::milliseconds(2 * TIMEOUT)); + auto data = std::make_shared(1); s.signal(2, data); s.signal(1, data); -- 2.7.4 From e649fc7da6a8db91320c1e9f2aed653fba8dce69 Mon Sep 17 00:00:00 2001 From: Mateusz Malicki Date: Wed, 14 Jan 2015 15:28:01 +0100 Subject: [PATCH 08/16] Fix: Drop exception thrown from async callback [Bug/Feature] Crash when exception are thrown from async callback [Cause] Exception from async callback [Solution] Drop exception [Verification] Build, install, run test on slp Change-Id: I00fad962beb488508d25a21a68ee7e487f317ba0 --- tests/unit_tests/server/ut-zones-manager.cpp | 33 +++++++++++++++++++++------- 1 file changed, 25 insertions(+), 8 deletions(-) diff --git a/tests/unit_tests/server/ut-zones-manager.cpp b/tests/unit_tests/server/ut-zones-manager.cpp index a733e05..5b29340 100644 --- a/tests/unit_tests/server/ut-zones-manager.cpp +++ b/tests/unit_tests/server/ut-zones-manager.cpp @@ -42,6 +42,7 @@ #include "utils/fs.hpp" #include "utils/img.hpp" #include "utils/scoped-dir.hpp" +#include "logger/logger.hpp" #include #include @@ -79,6 +80,22 @@ const std::string FILE_CONTENT = "File content\n" const std::string NON_EXISTANT_ZONE_ID = "NON_EXISTANT_ZONE_ID"; const std::string ZONES_PATH = "/tmp/ut-zones"; // the same as in daemon.conf +/** + * Currently there is no way to propagate an error from async call + * dropException are used to prevent system crash + **/ +DbusConnection::AsyncMethodCallCallback +dropException(const DbusConnection::AsyncMethodCallCallback& fun) +{ + return [fun](dbus::AsyncMethodCallResult& arg) -> void { + try { + fun(arg); + } catch (const std::exception& ex) { + LOGE("Got exception: " << ex.what()); + } + }; +} + class DbusAccessory { public: static const int HOST_ID = 0; @@ -326,7 +343,7 @@ public: api::host::METHOD_CREATE_ZONE, parameters, "()", - asyncResult); + dropException(asyncResult)); } void callAsyncMethodDestroyZone(const std::string& id, @@ -346,7 +363,7 @@ public: api::host::METHOD_DESTROY_ZONE, parameters, "()", - asyncResult); + dropException(asyncResult)); } void callAsyncMethodShutdownZone(const std::string& id, @@ -366,7 +383,7 @@ public: api::host::METHOD_SHUTDOWN_ZONE, parameters, "()", - asyncResult); + dropException(asyncResult)); } void callAsyncMethodStartZone(const std::string& id, @@ -386,7 +403,7 @@ public: api::host::METHOD_START_ZONE, parameters, "()", - asyncResult); + dropException(asyncResult)); } void callMethodLockZone(const std::string& id) @@ -576,8 +593,8 @@ BOOST_AUTO_TEST_CASE(NotifyActiveZoneTest) dbus.second->callMethodNotify(); } - BOOST_CHECK(signalReceivedLatch.waitForN(dbuses.size() - 1u, EVENT_TIMEOUT)); - BOOST_CHECK(signalReceivedLatch.empty()); + BOOST_REQUIRE(signalReceivedLatch.waitForN(dbuses.size() - 1u, EVENT_TIMEOUT)); + BOOST_REQUIRE(signalReceivedLatch.empty()); //check if there are no signals that was received more than once for (const auto& source : signalReceivedSourcesMap[1]) { @@ -718,8 +735,8 @@ BOOST_AUTO_TEST_CASE(MoveFileTest) BOOST_CHECK_EQUAL(dbuses.at(1)->callMethodMove(ZONE2, TMP + "/file"), api::zone::FILE_MOVE_SUCCEEDED); - BOOST_CHECK(notificationLatch.wait(EVENT_TIMEOUT)); - BOOST_CHECK(notificationLatch.empty()); + BOOST_REQUIRE(notificationLatch.wait(EVENT_TIMEOUT)); + BOOST_REQUIRE(notificationLatch.empty()); BOOST_CHECK_EQUAL(notificationSource, ZONE1); BOOST_CHECK_EQUAL(notificationPath, TMP + "/file"); BOOST_CHECK_EQUAL(notificationRetcode, api::zone::FILE_MOVE_SUCCEEDED); -- 2.7.4 From 3b289cf312278ae35bebd9f48fdf46d29123ff14 Mon Sep 17 00:00:00 2001 From: Mateusz Malicki Date: Wed, 14 Jan 2015 10:18:14 +0100 Subject: [PATCH 09/16] Unallocate provisioned resources in destructor [Bug/Feature] Provisioned resources aren't released when "zone->start" throws exception. [Cause] There is no destructor in ZoneProvsion class [Solution] Destructor was implemented [Verification] Build, install, run ZoneProvsionSuite tests Change-Id: I4d610cae7b1ffee4fd3b06f2906b9e4631a1dcf7 --- server/zone-provision.cpp | 31 ++++-- server/zone-provision.hpp | 3 + tests/unit_tests/server/ut-zone-provision.cpp | 143 ++++++++++++++++++++------ 3 files changed, 136 insertions(+), 41 deletions(-) diff --git a/server/zone-provision.cpp b/server/zone-provision.cpp index 3bff4b5..b9192a4 100644 --- a/server/zone-provision.cpp +++ b/server/zone-provision.cpp @@ -67,6 +67,11 @@ ZoneProvision::ZoneProvision(const std::string& zonePath, mValidLinkPrefixes = validLinkPrefixes; } +ZoneProvision::~ZoneProvision() +{ + stop(); +} + std::string ZoneProvision::getRootPath() const { return mRootPath; @@ -118,7 +123,9 @@ void ZoneProvision::start() noexcept } else if (unit.is()) { link(unit.as()); } - } catch (std::exception& ex) { + // mProvisioned must be FILO + mProvisioned.push_front(unit); + } catch (const std::exception& ex) { LOGE("Provsion error: " << ex.what()); } } @@ -127,13 +134,18 @@ void ZoneProvision::start() noexcept void ZoneProvision::stop() noexcept { - for (auto it = mProvisioningConfig.units.rbegin(); - it != mProvisioningConfig.units.rend(); - ++it) { - if (it->is()) { - umount(it->as()); + mProvisioned.remove_if([this](const ZoneProvisioning::Unit& unit) -> bool { + try { + if (unit.is()) { + umount(unit.as()); + } + // leaves files, links, fifo, untouched + return true; + } catch (const std::exception& ex) { + LOGE("Provsion error: " << ex.what()); + return false; } - } + }); } void ZoneProvision::file(const ZoneProvisioning::File& config) @@ -187,7 +199,10 @@ void ZoneProvision::mount(const ZoneProvisioning::Mount& config) void ZoneProvision::umount(const ZoneProvisioning::Mount& config) { const fs::path hostPath = fs::path(mRootPath) / fs::path(config.target); - utils::umount(hostPath.string()); + bool ret = utils::umount(hostPath.string()); + if (!ret) { + throw UtilsException("Umount operation failure - path : " + config.target); + } } void ZoneProvision::link(const ZoneProvisioning::Link& config) diff --git a/server/zone-provision.hpp b/server/zone-provision.hpp index c87fbe9..5585c1f 100644 --- a/server/zone-provision.hpp +++ b/server/zone-provision.hpp @@ -30,6 +30,7 @@ #include #include +#include namespace vasum { @@ -47,6 +48,7 @@ public: */ ZoneProvision(const std::string& zonePath, const std::vector& validLinkPrefixes); + ~ZoneProvision(); /** * Declare file, directory or pipe that will be created while zone startup @@ -82,6 +84,7 @@ private: std::string mRootPath; std::string mProvisionFile; std::vector mValidLinkPrefixes; + std::list mProvisioned; void mount(const ZoneProvisioning::Mount& config); void umount(const ZoneProvisioning::Mount& config); diff --git a/tests/unit_tests/server/ut-zone-provision.cpp b/tests/unit_tests/server/ut-zone-provision.cpp index 6a6e4ef..239d53e 100644 --- a/tests/unit_tests/server/ut-zone-provision.cpp +++ b/tests/unit_tests/server/ut-zone-provision.cpp @@ -20,22 +20,21 @@ /** * @file * @author Mateusz Malicki (m.malicki2@samsung.com) - * @brief Unit tests of the ZoneProvsion class + * @brief Unit tests of the ZoneProvision class */ #include "config.hpp" #include "ut.hpp" -#include "zone.hpp" - -#include "utils/glib-loop.hpp" #include "utils/scoped-dir.hpp" +#include "utils/exception.hpp" #include "config/manager.hpp" +#include "zone-provision.hpp" #include "zone-provision-config.hpp" #include "vasum-client.h" -#include #include +#include #include #include @@ -49,8 +48,7 @@ namespace fs = boost::filesystem; namespace { const std::string PROVISON_CONFIG_FILE = "provision.conf"; -const std::string ZONE = "ut-zone-test"; -const fs::path TEST_CONFIG_PATH = VSM_TEST_CONFIG_INSTALL_DIR "/server/ut-zone/zones/test.conf"; +const std::string ZONE = "ut-zone-provision-test"; const fs::path ZONES_PATH = "/tmp/ut-zones"; const fs::path LXC_TEMPLATES_PATH = VSM_TEST_LXC_TEMPLATES_INSTALL_DIR; const fs::path ZONE_PATH = ZONES_PATH / fs::path(ZONE); @@ -58,26 +56,14 @@ const fs::path PROVISION_FILE_PATH = ZONE_PATH / fs::path(PROVISON_CONFIG_FILE); const fs::path ROOTFS_PATH = ZONE_PATH / fs::path("rootfs"); struct Fixture { - utils::ScopedGlibLoop mLoop; utils::ScopedDir mZonesPathGuard; - utils::ScopedDir mRunGuard; utils::ScopedDir mRootfsPath; Fixture() : mZonesPathGuard(ZONES_PATH.string()) - , mRunGuard("/tmp/ut-run") , mRootfsPath(ROOTFS_PATH.string()) { } - - std::unique_ptr create(const std::string& configPath) - { - return std::unique_ptr(new Zone(utils::Worker::create(), - ZONES_PATH.string(), - configPath, - LXC_TEMPLATES_PATH.string(), - "")); - } }; } // namespace @@ -85,6 +71,34 @@ struct Fixture { BOOST_FIXTURE_TEST_SUITE(ZoneProvisionSuite, Fixture) +BOOST_AUTO_TEST_CASE(DestructorTest) +{ + const fs::path mountTarget = fs::path("/opt/usr/data/ut-from-host-provision"); + const fs::path mountSource = fs::path("/tmp/ut-provision"); + { + utils::ScopedDir provisionfs(mountSource.string()); + + ZoneProvisioning config; + ZoneProvisioning::Unit unit; + unit.set(ZoneProvisioning::File({VSMFILE_DIRECTORY, + mountTarget.string(), + 0, + 0777})); + config.units.push_back(unit); + unit.set(ZoneProvisioning::Mount({mountSource.string(), + mountTarget.string(), + "", + MS_BIND, + ""})); + config.units.push_back(unit); + + config::saveToFile(PROVISION_FILE_PATH.string(), config); + ZoneProvision zoneProvision(ZONE_PATH.string(), {}); + zoneProvision.start(); + } + BOOST_CHECK(!fs::exists(mountSource)); +} + BOOST_AUTO_TEST_CASE(FileTest) { //TODO: Test Fifo @@ -116,15 +130,16 @@ BOOST_AUTO_TEST_CASE(FileTest) 0777})); config.units.push_back(unit); config::saveToFile(PROVISION_FILE_PATH.string(), config); - auto c = create(TEST_CONFIG_PATH.string()); - c->start(); + + ZoneProvision zoneProvision(ZONE_PATH.string(), {}); + zoneProvision.start(); BOOST_CHECK(fs::exists(ROOTFS_PATH / regularFile.parent_path())); BOOST_CHECK(fs::exists(ROOTFS_PATH / regularFile)); BOOST_CHECK(fs::exists(ROOTFS_PATH / copyFile.parent_path())); BOOST_CHECK(fs::exists(ROOTFS_PATH / copyFile)); - c->stop(); + zoneProvision.stop(); } BOOST_AUTO_TEST_CASE(MountTest) @@ -155,16 +170,16 @@ BOOST_AUTO_TEST_CASE(MountTest) O_CREAT, 0777})); config.units.push_back(unit); - config::saveToFile(PROVISION_FILE_PATH.string(), config); - auto c = create(TEST_CONFIG_PATH.string()); - c->start(); + + ZoneProvision zoneProvision(ZONE_PATH.string(), {}); + zoneProvision.start(); BOOST_CHECK(fs::exists(ROOTFS_PATH / mountTarget)); BOOST_CHECK(fs::exists(ROOTFS_PATH / mountTarget / sharedFile)); BOOST_CHECK(fs::exists(mountSource / sharedFile)); - c->stop(); + zoneProvision.stop(); } BOOST_AUTO_TEST_CASE(LinkTest) @@ -178,15 +193,25 @@ BOOST_AUTO_TEST_CASE(LinkTest) linkFile.string()})); config.units.push_back(unit); config::saveToFile(PROVISION_FILE_PATH.string(), config); - auto c = create(TEST_CONFIG_PATH.string()); - c->start(); + { + ZoneProvision zoneProvision(ZONE_PATH.string(), {}); + zoneProvision.start(); - BOOST_CHECK(fs::exists(ROOTFS_PATH / linkFile)); + BOOST_CHECK(!fs::exists(ROOTFS_PATH / linkFile)); + + zoneProvision.stop(); + } + { + ZoneProvision zoneProvision(ZONE_PATH.string(), {"/tmp/"}); + zoneProvision.start(); + + BOOST_CHECK(fs::exists(ROOTFS_PATH / linkFile)); - c->stop(); + zoneProvision.stop(); + } } -BOOST_AUTO_TEST_CASE(DeclareFile) +BOOST_AUTO_TEST_CASE(DeclareFileTest) { ZoneProvision zoneProvision(ZONE_PATH.string(), {}); zoneProvision.declareFile(1, "path", 0747, 0777); @@ -204,7 +229,7 @@ BOOST_AUTO_TEST_CASE(DeclareFile) BOOST_CHECK_EQUAL(unit.mode, 0777); } -BOOST_AUTO_TEST_CASE(DeclareMount) +BOOST_AUTO_TEST_CASE(DeclareMountTest) { ZoneProvision zoneProvision(ZONE_PATH.string(), {}); zoneProvision.declareMount("/fake/path1", "/fake/path2", "tmpfs", 077, "fake"); @@ -223,7 +248,7 @@ BOOST_AUTO_TEST_CASE(DeclareMount) BOOST_CHECK_EQUAL(unit.data, "fake"); } -BOOST_AUTO_TEST_CASE(DeclareLink) +BOOST_AUTO_TEST_CASE(DeclareLinkTest) { ZoneProvision zoneProvision(ZONE_PATH.string(), {}); zoneProvision.declareLink("/fake/path1", "/fake/path2"); @@ -239,4 +264,56 @@ BOOST_AUTO_TEST_CASE(DeclareLink) BOOST_CHECK_EQUAL(unit.target, "/fake/path2"); } +BOOST_AUTO_TEST_CASE(ProvisionedAlreadyTest) +{ + const fs::path dir = fs::path("/opt/usr/data/ut-from-host-provision"); + const fs::path linkFile = fs::path("/ut-from-host-provision.conf"); + const fs::path regularFile = fs::path("/opt/usr/data/ut-regular-file"); + + ZoneProvisioning config; + ZoneProvisioning::Unit unit; + unit.set(ZoneProvisioning::File({VSMFILE_DIRECTORY, + dir.string(), + 0, + 0777})); + config.units.push_back(unit); + unit.set(ZoneProvisioning::Link({PROVISION_FILE_PATH.string(), + linkFile.string()})); + config.units.push_back(unit); + unit.set(ZoneProvisioning::File({VSMFILE_REGULAR, + regularFile.string(), + O_CREAT, + 0777})); + config.units.push_back(unit); + + config::saveToFile(PROVISION_FILE_PATH.string(), config); + + ZoneProvision zoneProvision(ZONE_PATH.string(), {"/tmp/"}); + zoneProvision.start(); + + BOOST_CHECK(fs::exists(ROOTFS_PATH / dir)); + BOOST_CHECK(fs::exists(ROOTFS_PATH / regularFile)); + BOOST_CHECK(fs::is_empty(ROOTFS_PATH / regularFile)); + BOOST_CHECK(fs::exists(ROOTFS_PATH / linkFile)); + + std::fstream file((ROOTFS_PATH / regularFile).string(), std::fstream::out); + BOOST_REQUIRE(file.is_open()); + file << "touch" << std::endl; + file.close(); + BOOST_REQUIRE(!fs::is_empty(ROOTFS_PATH / regularFile)); + + zoneProvision.stop(); + + BOOST_CHECK(fs::exists(ROOTFS_PATH / dir)); + BOOST_CHECK(fs::exists(ROOTFS_PATH / regularFile)); + BOOST_CHECK(!fs::is_empty(ROOTFS_PATH / regularFile)); + BOOST_CHECK(fs::exists(ROOTFS_PATH / linkFile)); + + zoneProvision.start(); + + BOOST_CHECK(!fs::is_empty(ROOTFS_PATH / regularFile)); + + zoneProvision.stop(); +} + BOOST_AUTO_TEST_SUITE_END() -- 2.7.4 From e1466434bea116c9ae0dbd6af5354f2dc492c444 Mon Sep 17 00:00:00 2001 From: Piotr Bartosiewicz Date: Tue, 13 Jan 2015 11:25:52 +0100 Subject: [PATCH 10/16] Fix persistence of dynamically created zones [Bug] No zones after daemon restart [Cause] N/A [Solution] Use database to store dynamic config [Verification] 1) run tests 2) - run daemon - create new zone - restart daemon, verify zone is working - destroy that zone - restart daemon, verify zone is gone Change-Id: I43f3e2f4f2d9c897d20b52db812dd837c90ef155 --- packaging/vasum.spec | 1 - server/configs/CMakeLists.txt | 4 +- server/configs/daemon.conf.in | 9 +-- server/configs/zones/business.conf | 20 ------ server/configs/zones/private.conf | 20 ------ server/main.cpp | 10 +-- server/server.cpp | 16 ++--- server/server.hpp | 17 ++--- server/zones-manager-config.hpp | 23 +++++-- server/zones-manager.cpp | 77 +++++++++++++--------- server/zones-manager.hpp | 7 +- .../configs/ut-client/test-dbus-daemon.conf.in | 5 +- .../server/configs/ut-server/buggy-daemon.conf.in | 3 +- .../server/configs/ut-server/test-daemon.conf.in | 3 +- .../configs/ut-zones-manager/buggy-daemon.conf.in | 3 +- .../ut-zones-manager/buggy-default-daemon.conf.in | 3 +- .../buggy-foreground-daemon.conf.in | 3 +- .../ut-zones-manager/empty-dbus-daemon.conf.in | 5 +- .../configs/ut-zones-manager/test-daemon.conf.in | 3 +- .../ut-zones-manager/test-dbus-daemon.conf.in | 3 +- tests/unit_tests/server/ut-server.cpp | 9 +-- tests/unit_tests/server/ut-zones-manager.cpp | 44 +++++++++++++ 22 files changed, 162 insertions(+), 126 deletions(-) delete mode 100644 server/configs/zones/business.conf delete mode 100644 server/configs/zones/private.conf diff --git a/packaging/vasum.spec b/packaging/vasum.spec index e743a75..113348b 100644 --- a/packaging/vasum.spec +++ b/packaging/vasum.spec @@ -45,7 +45,6 @@ between them. A process from inside a zone can request a switch of context %dir /etc/vasum/lxc-templates %dir /etc/vasum/templates %config /etc/vasum/daemon.conf -%config /etc/vasum/zones/*.conf %attr(755,root,root) /etc/vasum/lxc-templates/*.sh %config /etc/vasum/templates/*.conf %{_unitdir}/vasum.service diff --git a/server/configs/CMakeLists.txt b/server/configs/CMakeLists.txt index 9c0fa4b..f5f82c5 100644 --- a/server/configs/CMakeLists.txt +++ b/server/configs/CMakeLists.txt @@ -19,7 +19,6 @@ MESSAGE(STATUS "Installing configs to " ${VSM_CONFIG_INSTALL_DIR}) -FILE(GLOB zone_CONF zones/*.conf) FILE(GLOB admin_CONF lxc-templates/*.sh) FILE(GLOB template_CONF templates/*.conf) @@ -42,8 +41,7 @@ CONFIGURE_FILE(dbus-1/system.d/org.tizen.vasum.host.conf.in INSTALL(FILES ${CMAKE_BINARY_DIR}/dbus-1/system.d/org.tizen.vasum.host.conf DESTINATION ${SYSCONF_INSTALL_DIR}/dbus-1/system.d/) -INSTALL(FILES ${zone_CONF} - DESTINATION ${VSM_CONFIG_INSTALL_DIR}/zones) +INSTALL(DIRECTORY DESTINATION ${VSM_CONFIG_INSTALL_DIR}/zones) #TODO temporary solution INSTALL(PROGRAMS ${admin_CONF} DESTINATION ${VSM_CONFIG_INSTALL_DIR}/lxc-templates) diff --git a/server/configs/daemon.conf.in b/server/configs/daemon.conf.in index 4522bb5..b7d9c87 100644 --- a/server/configs/daemon.conf.in +++ b/server/configs/daemon.conf.in @@ -1,12 +1,13 @@ { - "zoneConfigs" : ["zones/private.conf", "zones/business.conf"], + "dbPath" : "/usr/dbspace/vasum.db", + "zoneConfigs" : [], "zonesPath" : "${DATA_DIR}/.zones", "zoneImagePath" : "", - "zoneTemplatePath" : "templates", + "zoneTemplatePath" : "/etc/vasum/templates/template.conf", "zoneNewConfigPrefix" : "/var/lib/vasum", "runMountPointPrefix" : "/var/run/zones", - "foregroundId" : "private", - "defaultId" : "private", + "foregroundId" : "", + "defaultId" : "", "lxcTemplatePrefix" : "/etc/vasum/lxc-templates", "inputConfig" : {"enabled" : true, "device" : "gpio_keys.6", diff --git a/server/configs/zones/business.conf b/server/configs/zones/business.conf deleted file mode 100644 index 74c3bad..0000000 --- a/server/configs/zones/business.conf +++ /dev/null @@ -1,20 +0,0 @@ -{ - "name" : "business", - "lxcTemplate" : "tizen-common-wayland.sh", - "initWithArgs" : [], - "ipv4Gateway" : "10.0.102.1", - "ipv4" : "10.0.102.2", - "cpuQuotaForeground" : -1, - "cpuQuotaBackground" : 10000, - "enableDbusIntegration" : true, - "privilege" : 1, - "vt" : 3, - "switchToDefaultAfterTimeout" : true, - "runMountPoint" : "business/run", - "permittedToSend" : [ "/tmp/.*" ], - "permittedToRecv" : [ "/tmp/.*" ], - "validLinkPrefixes" : [ "/tmp/", - "/run/", - "/opt/usr/data/", - "/opt/usr/dbsapce/" ] -} diff --git a/server/configs/zones/private.conf b/server/configs/zones/private.conf deleted file mode 100644 index ffb7e85..0000000 --- a/server/configs/zones/private.conf +++ /dev/null @@ -1,20 +0,0 @@ -{ - "name" : "private", - "lxcTemplate" : "tizen-common-wayland.sh", - "initWithArgs" : [], - "ipv4Gateway" : "10.0.101.1", - "ipv4" : "10.0.101.2", - "cpuQuotaForeground" : -1, - "cpuQuotaBackground" : 10000, - "enableDbusIntegration" : true, - "privilege" : 10, - "vt" : 2, - "switchToDefaultAfterTimeout" : true, - "runMountPoint" : "private/run", - "permittedToSend" : [ "/tmp/.*" ], - "permittedToRecv" : [ "/tmp/.*" ], - "validLinkPrefixes" : [ "/tmp/", - "/run/", - "/opt/usr/data/", - "/opt/usr/dbsapce/" ] -} diff --git a/server/main.cpp b/server/main.cpp index d641148..fb3eed4 100644 --- a/server/main.cpp +++ b/server/main.cpp @@ -52,11 +52,13 @@ namespace { const std::string PROGRAM_NAME_AND_VERSION = "Vasum Server " PROGRAM_VERSION; +const std::string CONFIG_PATH = "/etc/vasum/daemon.conf"; + + } // namespace int main(int argc, char* argv[]) { - std::string configPath; bool runAsRoot = false; try { @@ -67,7 +69,6 @@ int main(int argc, char* argv[]) ("root,r", "Don't drop root privileges at startup") ("version,v", "show application version") ("log-level,l", po::value()->default_value("DEBUG"), "set log level") - ("config,c", po::value()->default_value("/etc/vasum/daemon.conf"), "server configuration file") ; po::variables_map vm; @@ -108,7 +109,6 @@ int main(int argc, char* argv[]) Logger::setLogBackend(new SystemdJournalBackend()); #endif - configPath = vm["config"].as(); runAsRoot = vm.count("root") > 0; } catch (std::exception& e) { @@ -117,8 +117,8 @@ int main(int argc, char* argv[]) } try { - Server server(configPath, runAsRoot); - server.run(); + Server server(CONFIG_PATH); + server.run(runAsRoot); server.reloadIfRequired(argv); } catch (std::exception& e) { diff --git a/server/server.cpp b/server/server.cpp index d9a1628..2f30a55 100644 --- a/server/server.cpp +++ b/server/server.cpp @@ -67,17 +67,9 @@ extern char** environ; namespace vasum { -Server::Server(const std::string& configPath, bool runAsRoot) +Server::Server(const std::string& configPath) : mConfigPath(configPath) { - if (!prepareEnvironment(configPath, runAsRoot)) { - throw ServerException("Environment setup failed"); - } -} - - -Server::~Server() -{ } @@ -100,8 +92,12 @@ void signalHandler(const int sig) } // namespace -void Server::run() +void Server::run(bool asRoot) { + if (!prepareEnvironment(mConfigPath, asRoot)) { + throw ServerException("Environment setup failed"); + } + signal(SIGINT, signalHandler); signal(SIGTERM, signalHandler); signal(SIGUSR1, signalHandler); diff --git a/server/server.hpp b/server/server.hpp index 009a468..8bac0b0 100644 --- a/server/server.hpp +++ b/server/server.hpp @@ -36,18 +36,12 @@ namespace vasum { class Server { public: - Server(const std::string& configPath, bool runAsRoot = true); - virtual ~Server(); - - /** - * Set needed caps, groups and drop root privileges. - */ - static bool prepareEnvironment(const std::string& configPath, bool runAsRoot); + Server(const std::string& configPath); /** * Starts all the zones and blocks until SIGINT, SIGTERM or SIGUSR1 */ - void run(); + void run(bool asRoot); /** * Reload the server by launching execve on itself if SIGUSR1 was sent to server. @@ -59,8 +53,15 @@ public: * Equivalent of sending SIGINT or SIGTERM signal */ void terminate(); + private: std::string mConfigPath; + + /** + * Set needed caps, groups and drop root privileges. + */ + static bool prepareEnvironment(const std::string& configPath, bool runAsRoot); + }; diff --git a/server/zones-manager-config.hpp b/server/zones-manager-config.hpp index 1838d25..ab55762 100644 --- a/server/zones-manager-config.hpp +++ b/server/zones-manager-config.hpp @@ -36,16 +36,12 @@ namespace vasum { - -const std::string ZONES_MANAGER_CONFIG_PATH = "/etc/vasum/config/daemon.conf"; - struct ZonesManagerConfig { /** - * List of zones' configs that we manage. - * File paths can be relative to the ZoneManager config file. + * Path to config database. */ - std::vector zoneConfigs; + std::string dbPath; /** * An ID of a currently focused/foreground zone. @@ -100,7 +96,7 @@ struct ZonesManagerConfig { CONFIG_REGISTER ( - zoneConfigs, + dbPath, foregroundId, defaultId, zonesPath, @@ -114,6 +110,19 @@ struct ZonesManagerConfig { ) }; +struct ZonesManagerDynamicConfig { + + /** + * List of zones' configs that we manage. + * File paths can be relative to the ZoneManager config file. + */ + std::vector zoneConfigs; + + CONFIG_REGISTER + ( + zoneConfigs + ) +}; } // namespace vasum diff --git a/server/zones-manager.cpp b/server/zones-manager.cpp index ff6fb7e..840d8c4 100644 --- a/server/zones-manager.cpp +++ b/server/zones-manager.cpp @@ -75,15 +75,28 @@ const boost::regex ZONE_VT_REGEX("~VT~"); const unsigned int ZONE_IP_BASE_THIRD_OCTET = 100; const unsigned int ZONE_VT_BASE = 1; +std::string getConfigName(const std::string& zoneId) +{ + return "zones/" + zoneId + ".conf"; +} + +template +void remove(std::vector& v, const T& item) +{ + // erase-remove idiom, ask google for explanation + v.erase(std::remove(v.begin(), v.end(), item), v.end()); +} + } // namespace -ZonesManager::ZonesManager(const std::string& managerConfigPath) - : mWorker(utils::Worker::create()), mDetachOnExit(false) +ZonesManager::ZonesManager(const std::string& configPath) + : mWorker(utils::Worker::create()) + , mDetachOnExit(false) { LOGD("Instantiating ZonesManager object..."); - mConfigPath = managerConfigPath; - config::loadFromFile(mConfigPath, mConfig); + config::loadFromFile(configPath, mConfig); + config::loadFromKVStoreWithJsonFile(mConfig.dbPath, configPath, mDynamicConfig); mProxyCallPolicy.reset(new ProxyCallPolicy(mConfig.proxyCallRules)); @@ -139,8 +152,8 @@ ZonesManager::ZonesManager(const std::string& managerConfigPath) mHostConnection.setRevokeDeviceCallback(bind(&ZonesManager::handleRevokeDeviceCall, this, _1, _2, _3)); - for (auto& zoneConfig : mConfig.zoneConfigs) { - createZone(zoneConfig); + for (const auto& zoneConfig : mDynamicConfig.zoneConfigs) { + createZone(utils::createFilePath(mConfig.zoneNewConfigPrefix, zoneConfig)); } // check if default zone exists, throw ZoneOperationException if not found @@ -178,11 +191,13 @@ ZonesManager::~ZonesManager() LOGD("ZonesManager object destroyed"); } -void ZonesManager::createZone(const std::string& zoneConfig) +void ZonesManager::saveDynamicConfig() { - std::string baseConfigPath = utils::dirName(mConfigPath); - std::string zoneConfigPath = utils::getAbsolutePath(zoneConfig, baseConfigPath); + config::saveToKVStore(mConfig.dbPath, mDynamicConfig); +} +void ZonesManager::createZone(const std::string& zoneConfigPath) +{ LOGT("Creating Zone " << zoneConfigPath); std::unique_ptr zone(new Zone(mWorker->createSubWorker(), mConfig.zonesPath, @@ -217,7 +232,7 @@ void ZonesManager::createZone(const std::string& zoneConfig) // after zone is created successfully, put a file informing that zones are enabled if (mZones.size() == 1) { if (!utils::saveFileContent( - utils::createFilePath(mConfig.zonesPath, "/", ENABLED_FILE_NAME), "")) { + utils::createFilePath(mConfig.zonesPath, ENABLED_FILE_NAME), "")) { throw ZoneOperationException(ENABLED_FILE_NAME + ": cannot create."); } } @@ -238,10 +253,14 @@ void ZonesManager::destroyZone(const std::string& zoneId) mZones.erase(it); if (mZones.size() == 0) { - if (!utils::removeFile(utils::createFilePath(mConfig.zonesPath, "/", ENABLED_FILE_NAME))) { + if (!utils::removeFile(utils::createFilePath(mConfig.zonesPath, ENABLED_FILE_NAME))) { LOGE("Failed to remove enabled file."); } } + + // update dynamic config + remove(mDynamicConfig.zoneConfigs, getConfigName(zoneId)); + saveDynamicConfig(); } void ZonesManager::focus(const std::string& zoneId) @@ -748,20 +767,19 @@ void ZonesManager::generateNewConfig(const std::string& id, { namespace fs = boost::filesystem; - std::string resultFileDir = utils::dirName(resultPath); - if (!fs::exists(resultFileDir)) { - if (!utils::createEmptyDir(resultFileDir)) { + if (fs::exists(resultPath)) { + LOGT(resultPath << " already exists, removing"); + fs::remove(resultPath); + } else { + std::string resultFileDir = utils::dirName(resultPath); + if (!utils::createDirs(resultFileDir, fs::perms::owner_all | + fs::perms::group_read | fs::perms::group_exe | + fs::perms::others_read | fs::perms::others_exe)) { LOGE("Unable to create directory for new config."); throw ZoneOperationException("Unable to create directory for new config."); } } - fs::path resultFile(resultPath); - if (fs::exists(resultFile)) { - LOGT(resultPath << " already exists, removing"); - fs::remove(resultFile); - } - std::string config; if (!utils::readFileContent(templatePath, config)) { LOGE("Failed to read template config file."); @@ -788,7 +806,7 @@ void ZonesManager::generateNewConfig(const std::string& id, } // restrict new config file so that only owner (vasum) can write it - fs::permissions(resultPath, fs::perms::owner_all | + fs::permissions(resultPath, fs::perms::owner_read | fs::perms::owner_write | fs::perms::group_read | fs::perms::others_read); } @@ -817,7 +835,7 @@ void ZonesManager::handleCreateZoneCall(const std::string& id, return; } - const std::string zonePathStr = utils::createFilePath(mConfig.zonesPath, "/", id, "/"); + const std::string zonePathStr = utils::createFilePath(mConfig.zonesPath, id, "/"); // copy zone image if config contains path to image LOGT("Image path: " << mConfig.zoneImagePath); @@ -834,12 +852,8 @@ void ZonesManager::handleCreateZoneCall(const std::string& id, } // generate paths to new configuration files - std::string baseDir = utils::dirName(mConfigPath); - std::string configDir = utils::getAbsolutePath(mConfig.zoneNewConfigPrefix, baseDir); - std::string templateDir = utils::getAbsolutePath(mConfig.zoneTemplatePath, baseDir); - - std::string configPath = utils::createFilePath(templateDir, "/", ZONE_TEMPLATE_CONFIG_PATH); - std::string newConfigPath = utils::createFilePath(configDir, "/zones/", id + ".conf"); + std::string newConfigName = getConfigName(id); + std::string newConfigPath = utils::createFilePath(mConfig.zoneNewConfigPrefix, newConfigName); auto removeAllWrapper = [](const std::string& path) -> bool { try { @@ -852,8 +866,8 @@ void ZonesManager::handleCreateZoneCall(const std::string& id, }; try { - LOGI("Generating config from " << configPath << " to " << newConfigPath); - generateNewConfig(id, configPath, newConfigPath); + LOGI("Generating config from " << mConfig.zoneTemplatePath << " to " << newConfigPath); + generateNewConfig(id, mConfig.zoneTemplatePath, newConfigPath); } catch (VasumException& e) { LOGE("Generate config failed: " << e.what()); @@ -872,6 +886,9 @@ void ZonesManager::handleCreateZoneCall(const std::string& id, return; } + mDynamicConfig.zoneConfigs.push_back(newConfigName); + saveDynamicConfig(); + result->setVoid(); } diff --git a/server/zones-manager.hpp b/server/zones-manager.hpp index a0a2695..5bdb6b5 100644 --- a/server/zones-manager.hpp +++ b/server/zones-manager.hpp @@ -52,7 +52,7 @@ public: * * @param zoneConfig config of new zone */ - void createZone(const std::string& zoneConfig); + void createZone(const std::string& zoneConfigPath); /** * Destroy zone. @@ -111,8 +111,8 @@ private: utils::Worker::Pointer mWorker; mutable Mutex mMutex; // used to protect mZones - ZonesManagerConfig mConfig; - std::string mConfigPath; + ZonesManagerConfig mConfig; //TODO make it const + ZonesManagerDynamicConfig mDynamicConfig; HostConnection mHostConnection; // to hold InputMonitor pointer to monitor if zone switching sequence is recognized std::unique_ptr mSwitchingSequenceMonitor; @@ -121,6 +121,7 @@ private: ZoneMap mZones; // map of zones, id is the key bool mDetachOnExit; + void saveDynamicConfig(); void switchingSequenceMonitorNotify(); void generateNewConfig(const std::string& id, const std::string& templatePath, diff --git a/tests/unit_tests/client/configs/ut-client/test-dbus-daemon.conf.in b/tests/unit_tests/client/configs/ut-client/test-dbus-daemon.conf.in index 1b90e67..544accc 100644 --- a/tests/unit_tests/client/configs/ut-client/test-dbus-daemon.conf.in +++ b/tests/unit_tests/client/configs/ut-client/test-dbus-daemon.conf.in @@ -1,4 +1,5 @@ { + "dbPath" : "/tmp/ut-zones/vasum.db", "zoneConfigs" : ["zones/console1-dbus.conf", "zones/console2-dbus.conf", "zones/console3-dbus.conf"], @@ -6,8 +7,8 @@ "defaultId" : "ut-zones-manager-console1-dbus", "zonesPath" : "/tmp/ut-zones", "zoneImagePath" : "", - "zoneTemplatePath" : "", - "zoneNewConfigPrefix" : "", + "zoneTemplatePath" : "no_need_for_templates_in_this_test", + "zoneNewConfigPrefix" : "@VSM_TEST_CONFIG_INSTALL_DIR@/client/ut-client/", "runMountPointPrefix" : "", "lxcTemplatePrefix" : "@VSM_TEST_LXC_TEMPLATES_INSTALL_DIR@", "inputConfig" : {"enabled" : false, diff --git a/tests/unit_tests/server/configs/ut-server/buggy-daemon.conf.in b/tests/unit_tests/server/configs/ut-server/buggy-daemon.conf.in index 8ea6cb8..9df74dc 100644 --- a/tests/unit_tests/server/configs/ut-server/buggy-daemon.conf.in +++ b/tests/unit_tests/server/configs/ut-server/buggy-daemon.conf.in @@ -1,9 +1,10 @@ { + "dbPath" : "/tmp/ut-zones/vasum.db", "zoneConfigs" : ["zones/zone1.conf", "missing/file/path/missing.conf", "zones/zone3.conf"], "zonesPath" : "/tmp/ut-zones", "zoneImagePath" : "", "zoneTemplatePath" : "no_need_for_templates_in_this_test", - "zoneNewConfigPrefix" : "", + "zoneNewConfigPrefix" : "@VSM_TEST_CONFIG_INSTALL_DIR@/server/ut-server/", "runMountPointPrefix" : "", "foregroundId" : "ut-server-zone1", "defaultId" : "ut-server-zone1", diff --git a/tests/unit_tests/server/configs/ut-server/test-daemon.conf.in b/tests/unit_tests/server/configs/ut-server/test-daemon.conf.in index 24e1c4e..697a468 100644 --- a/tests/unit_tests/server/configs/ut-server/test-daemon.conf.in +++ b/tests/unit_tests/server/configs/ut-server/test-daemon.conf.in @@ -1,9 +1,10 @@ { + "dbPath" : "/tmp/ut-zones/vasum.db", "zoneConfigs" : ["zones/zone1.conf", "zones/zone2.conf", "zones/zone3.conf"], "zonesPath" : "/tmp/ut-zones", "zoneImagePath" : "", "zoneTemplatePath" : "no_need_for_templates_in_this_test", - "zoneNewConfigPrefix" : "", + "zoneNewConfigPrefix" : "@VSM_TEST_CONFIG_INSTALL_DIR@/server/ut-server/", "runMountPointPrefix" : "", "foregroundId" : "ut-server-zone1", "defaultId" : "ut-server-zone1", diff --git a/tests/unit_tests/server/configs/ut-zones-manager/buggy-daemon.conf.in b/tests/unit_tests/server/configs/ut-zones-manager/buggy-daemon.conf.in index f2bcbb9..754594f 100644 --- a/tests/unit_tests/server/configs/ut-zones-manager/buggy-daemon.conf.in +++ b/tests/unit_tests/server/configs/ut-zones-manager/buggy-daemon.conf.in @@ -1,11 +1,12 @@ { + "dbPath" : "/tmp/ut-zones/vasum.db", "zoneConfigs" : ["zones/console1.conf", "missing/file/path/missing.conf", "zones/console3.conf"], "runMountPointPrefix" : "", "foregroundId" : "ut-zones-manager-console1", "defaultId" : "ut-zones-manager-console1", "zonesPath" : "/tmp/ut-zones", "zoneImagePath" : "", - "zoneTemplatePath" : "templates", + "zoneTemplatePath" : "@VSM_TEST_CONFIG_INSTALL_DIR@/server/ut-zones-manager/templates/template.conf", "zoneNewConfigPrefix" : "@VSM_TEST_CONFIG_INSTALL_DIR@/server/ut-zones-manager/", "lxcTemplatePrefix" : "@VSM_TEST_LXC_TEMPLATES_INSTALL_DIR@", "inputConfig" : {"enabled" : false, diff --git a/tests/unit_tests/server/configs/ut-zones-manager/buggy-default-daemon.conf.in b/tests/unit_tests/server/configs/ut-zones-manager/buggy-default-daemon.conf.in index 90b9107..3eb9e02 100644 --- a/tests/unit_tests/server/configs/ut-zones-manager/buggy-default-daemon.conf.in +++ b/tests/unit_tests/server/configs/ut-zones-manager/buggy-default-daemon.conf.in @@ -1,11 +1,12 @@ { + "dbPath" : "/tmp/ut-zones/vasum.db", "zoneConfigs" : ["zones/console1.conf", "zones/console2.conf", "zones/console3.conf"], "runMountPointPrefix" : "", "foregroundId" : "ut-zones-manager-console1", "defaultId" : "in_no_way_there_is_a_valid_id_here", "zonesPath" : "/tmp/ut-zones", "zoneImagePath" : "", - "zoneTemplatePath" : "templates", + "zoneTemplatePath" : "@VSM_TEST_CONFIG_INSTALL_DIR@/server/ut-zones-manager/templates/template.conf", "zoneNewConfigPrefix" : "@VSM_TEST_CONFIG_INSTALL_DIR@/server/ut-zones-manager/", "lxcTemplatePrefix" : "@VSM_TEST_LXC_TEMPLATES_INSTALL_DIR@", "inputConfig" : {"enabled" : false, diff --git a/tests/unit_tests/server/configs/ut-zones-manager/buggy-foreground-daemon.conf.in b/tests/unit_tests/server/configs/ut-zones-manager/buggy-foreground-daemon.conf.in index 49c4c2d..6e02372 100644 --- a/tests/unit_tests/server/configs/ut-zones-manager/buggy-foreground-daemon.conf.in +++ b/tests/unit_tests/server/configs/ut-zones-manager/buggy-foreground-daemon.conf.in @@ -1,11 +1,12 @@ { + "dbPath" : "/tmp/ut-zones/vasum.db", "zoneConfigs" : ["zones/console1.conf", "zones/console2.conf", "zones/console3.conf"], "runMountPointPrefix" : "", "foregroundId" : "this_id_does_not_exist", "defaultId" : "ut-zones-manager-console1", "zonesPath" : "/tmp/ut-zones", "zoneImagePath" : "", - "zoneTemplatePath" : "templates", + "zoneTemplatePath" : "@VSM_TEST_CONFIG_INSTALL_DIR@/server/ut-zones-manager/templates/template.conf", "zoneNewConfigPrefix" : "@VSM_TEST_CONFIG_INSTALL_DIR@/server/ut-zones-manager/", "lxcTemplatePrefix" : "@VSM_TEST_LXC_TEMPLATES_INSTALL_DIR@", "inputConfig" : {"enabled" : false, diff --git a/tests/unit_tests/server/configs/ut-zones-manager/empty-dbus-daemon.conf.in b/tests/unit_tests/server/configs/ut-zones-manager/empty-dbus-daemon.conf.in index 4e20535..a80be26 100644 --- a/tests/unit_tests/server/configs/ut-zones-manager/empty-dbus-daemon.conf.in +++ b/tests/unit_tests/server/configs/ut-zones-manager/empty-dbus-daemon.conf.in @@ -1,11 +1,12 @@ { + "dbPath" : "/tmp/ut-zones/vasum.db", "zoneConfigs" : [], "foregroundId" : "", "defaultId" : "", "zonesPath" : "/tmp/ut-zones", "zoneImagePath" : "", - "zoneTemplatePath" : "templates", - "zoneNewConfigPrefix" : "@VSM_TEST_CONFIG_INSTALL_DIR@/server/ut-zones-manager/", + "zoneTemplatePath" : "@VSM_TEST_CONFIG_INSTALL_DIR@/server/ut-zones-manager/templates/template.conf", + "zoneNewConfigPrefix" : "/tmp/ut-zones/generated-configs/", "runMountPointPrefix" : "", "lxcTemplatePrefix" : "@VSM_TEST_LXC_TEMPLATES_INSTALL_DIR@", "inputConfig" : {"enabled" : false, diff --git a/tests/unit_tests/server/configs/ut-zones-manager/test-daemon.conf.in b/tests/unit_tests/server/configs/ut-zones-manager/test-daemon.conf.in index d72c440..14cea0b 100644 --- a/tests/unit_tests/server/configs/ut-zones-manager/test-daemon.conf.in +++ b/tests/unit_tests/server/configs/ut-zones-manager/test-daemon.conf.in @@ -1,11 +1,12 @@ { + "dbPath" : "/tmp/ut-zones/vasum.db", "zoneConfigs" : ["zones/console1.conf", "zones/console2.conf", "zones/console3.conf"], "runMountPointPrefix" : "", "foregroundId" : "ut-zones-manager-console1", "defaultId" : "ut-zones-manager-console1", "zonesPath" : "/tmp/ut-zones", "zoneImagePath" : "", - "zoneTemplatePath" : "templates", + "zoneTemplatePath" : "@VSM_TEST_CONFIG_INSTALL_DIR@/server/ut-zones-manager/templates/template.conf", "zoneNewConfigPrefix" : "@VSM_TEST_CONFIG_INSTALL_DIR@/server/ut-zones-manager/", "lxcTemplatePrefix" : "@VSM_TEST_LXC_TEMPLATES_INSTALL_DIR@", "inputConfig" : {"enabled" : false, diff --git a/tests/unit_tests/server/configs/ut-zones-manager/test-dbus-daemon.conf.in b/tests/unit_tests/server/configs/ut-zones-manager/test-dbus-daemon.conf.in index 72b3523..2a55718 100644 --- a/tests/unit_tests/server/configs/ut-zones-manager/test-dbus-daemon.conf.in +++ b/tests/unit_tests/server/configs/ut-zones-manager/test-dbus-daemon.conf.in @@ -1,4 +1,5 @@ { + "dbPath" : "/tmp/ut-zones/vasum.db", "zoneConfigs" : ["zones/console1-dbus.conf", "zones/console2-dbus.conf", "zones/console3-dbus.conf"], @@ -6,7 +7,7 @@ "defaultId" : "ut-zones-manager-console1-dbus", "zonesPath" : "/tmp/ut-zones", "zoneImagePath" : "", - "zoneTemplatePath" : "templates", + "zoneTemplatePath" : "@VSM_TEST_CONFIG_INSTALL_DIR@/server/ut-zones-manager/templates/template.conf", "zoneNewConfigPrefix" : "@VSM_TEST_CONFIG_INSTALL_DIR@/server/ut-zones-manager/", "runMountPointPrefix" : "", "lxcTemplatePrefix" : "@VSM_TEST_LXC_TEMPLATES_INSTALL_DIR@", diff --git a/tests/unit_tests/server/ut-server.cpp b/tests/unit_tests/server/ut-server.cpp index 75b894a..3ed7319 100644 --- a/tests/unit_tests/server/ut-server.cpp +++ b/tests/unit_tests/server/ut-server.cpp @@ -36,6 +36,7 @@ namespace { const std::string ZONES_PATH = "/tmp/ut-zones"; // the same as in daemon.conf +const bool AS_ROOT = true; struct Fixture { vasum::utils::ScopedDir mZonesPathGuard; @@ -65,12 +66,12 @@ BOOST_AUTO_TEST_CASE(ConstructorDestructorTest) BOOST_AUTO_TEST_CASE(BuggyConfigTest) { - BOOST_REQUIRE_THROW(Server(BUGGY_CONFIG_PATH).run(), ConfigException); + BOOST_REQUIRE_THROW(Server(BUGGY_CONFIG_PATH).run(AS_ROOT), ConfigException); } BOOST_AUTO_TEST_CASE(MissingConfigTest) { - BOOST_REQUIRE_THROW(Server(MISSING_CONFIG_PATH).run(), ConfigException); + BOOST_REQUIRE_THROW(Server(MISSING_CONFIG_PATH).run(AS_ROOT), ConfigException); } BOOST_AUTO_TEST_CASE(TerminateTest) @@ -83,13 +84,13 @@ BOOST_AUTO_TEST_CASE(TerminateRunTest) { Server s(TEST_CONFIG_PATH); s.terminate(); - s.run(); + s.run(AS_ROOT); } BOOST_AUTO_TEST_CASE(RunTerminateTest) { Server s(TEST_CONFIG_PATH); - std::future runFuture = std::async(std::launch::async, [&] {s.run();}); + std::future runFuture = std::async(std::launch::async, [&] {s.run(AS_ROOT);}); // give a chance to run a thread std::this_thread::sleep_for(std::chrono::milliseconds(200)); diff --git a/tests/unit_tests/server/ut-zones-manager.cpp b/tests/unit_tests/server/ut-zones-manager.cpp index 5b29340..5925052 100644 --- a/tests/unit_tests/server/ut-zones-manager.cpp +++ b/tests/unit_tests/server/ut-zones-manager.cpp @@ -1066,6 +1066,50 @@ BOOST_AUTO_TEST_CASE(CreateDestroyZoneTest) BOOST_CHECK_EQUAL(cm.getRunningForegroundZoneId(), ""); } +BOOST_AUTO_TEST_CASE(CreateDestroyZonePersistenceTest) +{ + const std::string zone = "test1"; + + Latch callDone; + auto resultCallback = [&]() { + callDone.set(); + }; + + auto getZoneIds = []() -> std::vector { + ZonesManager cm(EMPTY_DBUS_CONFIG_PATH); + cm.startAll(); + + DbusAccessory dbus(DbusAccessory::HOST_ID); + return dbus.callMethodGetZoneIds(); + }; + + BOOST_CHECK(getZoneIds().empty()); + + // create zone + { + ZonesManager cm(EMPTY_DBUS_CONFIG_PATH); + DbusAccessory dbus(DbusAccessory::HOST_ID); + dbus.callAsyncMethodCreateZone(zone, resultCallback); + BOOST_REQUIRE(callDone.wait(EVENT_TIMEOUT)); + } + + { + auto ids = getZoneIds(); + BOOST_CHECK_EQUAL(1, ids.size()); + BOOST_CHECK(ids.at(0) == zone); + } + + // destroy zone + { + ZonesManager cm(EMPTY_DBUS_CONFIG_PATH); + DbusAccessory dbus(DbusAccessory::HOST_ID); + dbus.callAsyncMethodDestroyZone(zone, resultCallback); + BOOST_REQUIRE(callDone.wait(EVENT_TIMEOUT)); + } + + BOOST_CHECK(getZoneIds().empty()); +} + BOOST_AUTO_TEST_CASE(StartShutdownZoneTest) { const std::string zone1 = "ut-zones-manager-console1-dbus"; -- 2.7.4 From dfde8584ca4722a4906926ba5b6444b8bb521c98 Mon Sep 17 00:00:00 2001 From: Mateusz Malicki Date: Thu, 15 Jan 2015 16:20:12 +0100 Subject: [PATCH 11/16] Prevent from call method on partially destroyed object [Bug] startAsync can cause call method on partially destroyed object [Cause] async call are processed while ZoneManager is being destroyed [Solution] Ensure that all async calls was ended [Verification] Set EVENT_TIMEOUT=5 (ut-zones-manager.cpp), build, install, run ZonesManagerSuite/StartShutdownZoneTest with 1000 times less speed. Change-Id: I00bc8c0926be8a9a62ec23b81f62a2827db4799b --- server/zone.cpp | 8 ++++++-- server/zones-manager.cpp | 2 ++ 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/server/zone.cpp b/server/zone.cpp index f6fb188..1c4d2b2 100644 --- a/server/zone.cpp +++ b/server/zone.cpp @@ -81,8 +81,12 @@ Zone::~Zone() // Make sure all OnNameLostCallbacks get finished and no new will // get called before proceeding further. This guarantees no race // condition on the reconnect thread. - Lock lock(mReconnectMutex); - disconnect(); + { + Lock lock(mReconnectMutex); + disconnect(); + } + // wait for all tasks to complete + mWorker.reset(); } const std::vector& Zone::getPermittedToSend() const diff --git a/server/zones-manager.cpp b/server/zones-manager.cpp index 840d8c4..42a3a48 100644 --- a/server/zones-manager.cpp +++ b/server/zones-manager.cpp @@ -187,6 +187,8 @@ ZonesManager::~ZonesManager() LOGE("Failed to stop all of the zones"); } } + // wait for all tasks to complete + mWorker.reset(); LOGD("ZonesManager object destroyed"); } -- 2.7.4 From 72cb396b3c7d9125bbc6cce643f453a17a7d98d2 Mon Sep 17 00:00:00 2001 From: Piotr Bartosiewicz Date: Fri, 16 Jan 2015 15:00:40 +0100 Subject: [PATCH 12/16] Add prefix to config db [Bug/Feature] Added prefix to config db. Adjusted to libConfig changes. [Cause] N/A [Solution] N/A [Verification] Build, run tests Change-Id: Ie2cbf4cd4c8b4ef6225b5a2bfa740e5f61129fcc --- server/server.cpp | 2 +- server/zone-provision.cpp | 6 +- server/zone.cpp | 2 +- server/zones-manager.cpp | 8 +- tests/unit_tests/config/ut-configuration.cpp | 120 +++++++++++++------------- tests/unit_tests/config/ut-dynvisit.cpp | 24 +++--- tests/unit_tests/server/ut-zone-admin.cpp | 2 +- tests/unit_tests/server/ut-zone-provision.cpp | 16 ++-- 8 files changed, 91 insertions(+), 89 deletions(-) diff --git a/server/server.cpp b/server/server.cpp index 2f30a55..874b02f 100644 --- a/server/server.cpp +++ b/server/server.cpp @@ -144,7 +144,7 @@ bool Server::prepareEnvironment(const std::string& configPath, bool runAsRoot) // TODO: currently this config is loaded twice: here and in ZonesManager ZonesManagerConfig config; - config::loadFromFile(configPath, config); + config::loadFromJsonFile(configPath, config); struct passwd* pwd = ::getpwnam(VASUM_USER); if (pwd == NULL) { diff --git a/server/zone-provision.cpp b/server/zone-provision.cpp index b9192a4..c04a32a 100644 --- a/server/zone-provision.cpp +++ b/server/zone-provision.cpp @@ -51,10 +51,10 @@ void declareUnit(const std::string& file, ZoneProvisioning::Unit&& unit) // TODO: Add to the dynamic configuration ZoneProvisioning config; if (fs::exists(file)) { - config::loadFromFile(file, config); + config::loadFromJsonFile(file, config); } config.units.push_back(std::move(unit)); - config::saveToFile(file, config); + config::saveToJsonFile(file, config); } } // namespace @@ -113,7 +113,7 @@ void ZoneProvision::declareLink(const std::string& source, void ZoneProvision::start() noexcept { if (fs::exists(mProvisionFile)) { - config::loadFromFile(mProvisionFile, mProvisioningConfig); + config::loadFromJsonFile(mProvisionFile, mProvisioningConfig); for (const auto& unit : mProvisioningConfig.units) { try { if (unit.is()) { diff --git a/server/zone.cpp b/server/zone.cpp index 1c4d2b2..d38a0c2 100644 --- a/server/zone.cpp +++ b/server/zone.cpp @@ -58,7 +58,7 @@ Zone::Zone(const utils::Worker::Pointer& worker, const std::string& baseRunMountPointPath) : mWorker(worker) { - config::loadFromFile(zoneConfigPath, mConfig); + config::loadFromJsonFile(zoneConfigPath, mConfig); for (std::string r: mConfig.permittedToSend) { mPermittedToSend.push_back(boost::regex(r)); diff --git a/server/zones-manager.cpp b/server/zones-manager.cpp index 42a3a48..af1da8d 100644 --- a/server/zones-manager.cpp +++ b/server/zones-manager.cpp @@ -64,8 +64,8 @@ bool regexMatchVector(const std::string& str, const std::vector& v return false; } +const std::string DB_PREFIX = "daemon"; const std::string HOST_ID = "host"; -const std::string ZONE_TEMPLATE_CONFIG_PATH = "template.conf"; const std::string ENABLED_FILE_NAME = "enabled"; const boost::regex ZONE_NAME_REGEX("~NAME~"); @@ -95,8 +95,8 @@ ZonesManager::ZonesManager(const std::string& configPath) { LOGD("Instantiating ZonesManager object..."); - config::loadFromFile(configPath, mConfig); - config::loadFromKVStoreWithJsonFile(mConfig.dbPath, configPath, mDynamicConfig); + config::loadFromJsonFile(configPath, mConfig); + config::loadFromKVStoreWithJsonFile(mConfig.dbPath, configPath, mDynamicConfig, DB_PREFIX); mProxyCallPolicy.reset(new ProxyCallPolicy(mConfig.proxyCallRules)); @@ -195,7 +195,7 @@ ZonesManager::~ZonesManager() void ZonesManager::saveDynamicConfig() { - config::saveToKVStore(mConfig.dbPath, mDynamicConfig); + config::saveToKVStore(mConfig.dbPath, mDynamicConfig, DB_PREFIX); } void ZonesManager::createZone(const std::string& zoneConfigPath) diff --git a/tests/unit_tests/config/ut-configuration.cpp b/tests/unit_tests/config/ut-configuration.cpp index 95fc52a..bbf9ed9 100644 --- a/tests/unit_tests/config/ut-configuration.cpp +++ b/tests/unit_tests/config/ut-configuration.cpp @@ -45,7 +45,7 @@ BOOST_AUTO_TEST_CASE(FromStringTest) { TestConfig testConfig; - BOOST_REQUIRE_NO_THROW(loadFromString(jsonTestString, testConfig)); + BOOST_REQUIRE_NO_THROW(loadFromJsonString(jsonTestString, testConfig)); BOOST_CHECK_EQUAL(12345, testConfig.intVal); BOOST_CHECK_EQUAL(-1234567890123456789ll, testConfig.int64Val); @@ -91,13 +91,13 @@ BOOST_AUTO_TEST_CASE(FromStringTest) BOOST_AUTO_TEST_CASE(ToStringTest) { TestConfig testConfig; - BOOST_REQUIRE_NO_THROW(loadFromString(jsonTestString, testConfig)); + BOOST_REQUIRE_NO_THROW(loadFromJsonString(jsonTestString, testConfig)); - std::string out = saveToString(testConfig); + std::string out = saveToJsonString(testConfig); BOOST_CHECK_EQUAL(out, jsonTestString); TestConfig::SubConfigOption unionConfig; - BOOST_CHECK_THROW(saveToString(unionConfig), ConfigException); + BOOST_CHECK_THROW(saveToJsonString(unionConfig), ConfigException); } namespace loadErrorsTest { @@ -129,70 +129,70 @@ BOOST_AUTO_TEST_CASE(LoadErrorsTest) using namespace loadErrorsTest; IntConfig config; - BOOST_REQUIRE_NO_THROW(loadFromString("{\"field\":1}", config)); + BOOST_REQUIRE_NO_THROW(loadFromJsonString("{\"field\":1}", config)); - BOOST_CHECK_THROW(loadFromString("", config), ConfigException); - BOOST_CHECK_THROW(loadFromString("{", config), ConfigException); // invalid json - BOOST_CHECK_THROW(loadFromString("{}", config), ConfigException); // missing field + BOOST_CHECK_THROW(loadFromJsonString("", config), ConfigException); + BOOST_CHECK_THROW(loadFromJsonString("{", config), ConfigException); // invalid json + BOOST_CHECK_THROW(loadFromJsonString("{}", config), ConfigException); // missing field // invalid type IntConfig intConfig; - BOOST_CHECK_NO_THROW(loadFromString("{\"field\": 1}", intConfig)); - BOOST_CHECK_THROW(loadFromString("{\"field\": \"1\"}", intConfig), ConfigException); - BOOST_CHECK_THROW(loadFromString("{\"field\": 1.0}", intConfig), ConfigException); - BOOST_CHECK_THROW(loadFromString("{\"field\": true}", intConfig), ConfigException); - BOOST_CHECK_THROW(loadFromString("{\"field\": []}", intConfig), ConfigException); - BOOST_CHECK_THROW(loadFromString("{\"field\": {}}", intConfig), ConfigException); - BOOST_CHECK_THROW(loadFromString("{\"field\": 1234567890123456789}", intConfig), ConfigException); - BOOST_CHECK_THROW(loadFromString("{\"field\": -1234567890123456789}", intConfig), ConfigException); + BOOST_CHECK_NO_THROW(loadFromJsonString("{\"field\": 1}", intConfig)); + BOOST_CHECK_THROW(loadFromJsonString("{\"field\": \"1\"}", intConfig), ConfigException); + BOOST_CHECK_THROW(loadFromJsonString("{\"field\": 1.0}", intConfig), ConfigException); + BOOST_CHECK_THROW(loadFromJsonString("{\"field\": true}", intConfig), ConfigException); + BOOST_CHECK_THROW(loadFromJsonString("{\"field\": []}", intConfig), ConfigException); + BOOST_CHECK_THROW(loadFromJsonString("{\"field\": {}}", intConfig), ConfigException); + BOOST_CHECK_THROW(loadFromJsonString("{\"field\": 1234567890123456789}", intConfig), ConfigException); + BOOST_CHECK_THROW(loadFromJsonString("{\"field\": -1234567890123456789}", intConfig), ConfigException); StringConfig stringConfig; - BOOST_CHECK_THROW(loadFromString("{\"field\": 1}", stringConfig), ConfigException); - BOOST_CHECK_NO_THROW(loadFromString("{\"field\": \"1\"}", stringConfig)); - BOOST_CHECK_THROW(loadFromString("{\"field\": 1.0}", stringConfig), ConfigException); - BOOST_CHECK_THROW(loadFromString("{\"field\": true}", stringConfig), ConfigException); - BOOST_CHECK_THROW(loadFromString("{\"field\": []}", stringConfig), ConfigException); - BOOST_CHECK_THROW(loadFromString("{\"field\": {}}", stringConfig), ConfigException); + BOOST_CHECK_THROW(loadFromJsonString("{\"field\": 1}", stringConfig), ConfigException); + BOOST_CHECK_NO_THROW(loadFromJsonString("{\"field\": \"1\"}", stringConfig)); + BOOST_CHECK_THROW(loadFromJsonString("{\"field\": 1.0}", stringConfig), ConfigException); + BOOST_CHECK_THROW(loadFromJsonString("{\"field\": true}", stringConfig), ConfigException); + BOOST_CHECK_THROW(loadFromJsonString("{\"field\": []}", stringConfig), ConfigException); + BOOST_CHECK_THROW(loadFromJsonString("{\"field\": {}}", stringConfig), ConfigException); DoubleConfig doubleConfig; - BOOST_CHECK_THROW(loadFromString("{\"field\": 1}", doubleConfig), ConfigException); - BOOST_CHECK_THROW(loadFromString("{\"field\": \"1\"}", doubleConfig), ConfigException); - BOOST_CHECK_NO_THROW(loadFromString("{\"field\": 1.0}", doubleConfig)); - BOOST_CHECK_THROW(loadFromString("{\"field\": true}", doubleConfig), ConfigException); - BOOST_CHECK_THROW(loadFromString("{\"field\": []}", doubleConfig), ConfigException); - BOOST_CHECK_THROW(loadFromString("{\"field\": {}}", doubleConfig), ConfigException); + BOOST_CHECK_THROW(loadFromJsonString("{\"field\": 1}", doubleConfig), ConfigException); + BOOST_CHECK_THROW(loadFromJsonString("{\"field\": \"1\"}", doubleConfig), ConfigException); + BOOST_CHECK_NO_THROW(loadFromJsonString("{\"field\": 1.0}", doubleConfig)); + BOOST_CHECK_THROW(loadFromJsonString("{\"field\": true}", doubleConfig), ConfigException); + BOOST_CHECK_THROW(loadFromJsonString("{\"field\": []}", doubleConfig), ConfigException); + BOOST_CHECK_THROW(loadFromJsonString("{\"field\": {}}", doubleConfig), ConfigException); BoolConfig boolConfig; - BOOST_CHECK_THROW(loadFromString("{\"field\": 1}", boolConfig), ConfigException); - BOOST_CHECK_THROW(loadFromString("{\"field\": \"1\"}", boolConfig), ConfigException); - BOOST_CHECK_THROW(loadFromString("{\"field\": 1.0}", boolConfig), ConfigException); - BOOST_CHECK_NO_THROW(loadFromString("{\"field\": true}", boolConfig)); - BOOST_CHECK_THROW(loadFromString("{\"field\": []}", boolConfig), ConfigException); - BOOST_CHECK_THROW(loadFromString("{\"field\": {}}", boolConfig), ConfigException); + BOOST_CHECK_THROW(loadFromJsonString("{\"field\": 1}", boolConfig), ConfigException); + BOOST_CHECK_THROW(loadFromJsonString("{\"field\": \"1\"}", boolConfig), ConfigException); + BOOST_CHECK_THROW(loadFromJsonString("{\"field\": 1.0}", boolConfig), ConfigException); + BOOST_CHECK_NO_THROW(loadFromJsonString("{\"field\": true}", boolConfig)); + BOOST_CHECK_THROW(loadFromJsonString("{\"field\": []}", boolConfig), ConfigException); + BOOST_CHECK_THROW(loadFromJsonString("{\"field\": {}}", boolConfig), ConfigException); ArrayConfig arrayConfig; - BOOST_CHECK_THROW(loadFromString("{\"field\": 1}", arrayConfig), ConfigException); - BOOST_CHECK_THROW(loadFromString("{\"field\": \"1\"}", arrayConfig), ConfigException); - BOOST_CHECK_THROW(loadFromString("{\"field\": 1.0}", arrayConfig), ConfigException); - BOOST_CHECK_THROW(loadFromString("{\"field\": true}", arrayConfig), ConfigException); - BOOST_CHECK_NO_THROW(loadFromString("{\"field\": []}", arrayConfig)); - BOOST_CHECK_THROW(loadFromString("{\"field\": {}}", arrayConfig), ConfigException); + BOOST_CHECK_THROW(loadFromJsonString("{\"field\": 1}", arrayConfig), ConfigException); + BOOST_CHECK_THROW(loadFromJsonString("{\"field\": \"1\"}", arrayConfig), ConfigException); + BOOST_CHECK_THROW(loadFromJsonString("{\"field\": 1.0}", arrayConfig), ConfigException); + BOOST_CHECK_THROW(loadFromJsonString("{\"field\": true}", arrayConfig), ConfigException); + BOOST_CHECK_NO_THROW(loadFromJsonString("{\"field\": []}", arrayConfig)); + BOOST_CHECK_THROW(loadFromJsonString("{\"field\": {}}", arrayConfig), ConfigException); ObjectConfig objectConfig; - BOOST_CHECK_THROW(loadFromString("{\"field\": 1}", objectConfig), ConfigException); - BOOST_CHECK_THROW(loadFromString("{\"field\": \"1\"}", objectConfig), ConfigException); - BOOST_CHECK_THROW(loadFromString("{\"field\": 1.0}", objectConfig), ConfigException); - BOOST_CHECK_THROW(loadFromString("{\"field\": true}", objectConfig), ConfigException); - BOOST_CHECK_THROW(loadFromString("{\"field\": []}", objectConfig), ConfigException); - BOOST_CHECK_THROW(loadFromString("{\"field\": {}}", objectConfig), ConfigException); - BOOST_CHECK_NO_THROW(loadFromString("{\"field\": {\"field\": 1}}", objectConfig)); + BOOST_CHECK_THROW(loadFromJsonString("{\"field\": 1}", objectConfig), ConfigException); + BOOST_CHECK_THROW(loadFromJsonString("{\"field\": \"1\"}", objectConfig), ConfigException); + BOOST_CHECK_THROW(loadFromJsonString("{\"field\": 1.0}", objectConfig), ConfigException); + BOOST_CHECK_THROW(loadFromJsonString("{\"field\": true}", objectConfig), ConfigException); + BOOST_CHECK_THROW(loadFromJsonString("{\"field\": []}", objectConfig), ConfigException); + BOOST_CHECK_THROW(loadFromJsonString("{\"field\": {}}", objectConfig), ConfigException); + BOOST_CHECK_NO_THROW(loadFromJsonString("{\"field\": {\"field\": 1}}", objectConfig)); UnionConfig unionConfig; - BOOST_CHECK_THROW(loadFromString("{\"type\": \"long\", \"value\": 1}", unionConfig), ConfigException); - BOOST_CHECK_THROW(loadFromString("{\"type\": \"int\"}", unionConfig), ConfigException); - BOOST_CHECK_NO_THROW(loadFromString("{\"type\": \"int\", \"value\": 1}", unionConfig)); - BOOST_CHECK_NO_THROW(loadFromString("{\"type\": \"bool\", \"value\": true}", unionConfig)); + BOOST_CHECK_THROW(loadFromJsonString("{\"type\": \"long\", \"value\": 1}", unionConfig), ConfigException); + BOOST_CHECK_THROW(loadFromJsonString("{\"type\": \"int\"}", unionConfig), ConfigException); + BOOST_CHECK_NO_THROW(loadFromJsonString("{\"type\": \"int\", \"value\": 1}", unionConfig)); + BOOST_CHECK_NO_THROW(loadFromJsonString("{\"type\": \"bool\", \"value\": true}", unionConfig)); } namespace hasVisitableTest { @@ -247,15 +247,15 @@ BOOST_AUTO_TEST_CASE(HasVisibleInternalHelperTest) BOOST_AUTO_TEST_CASE(FromToKVStoreTest) { TestConfig config; - loadFromString(jsonTestString, config); + loadFromJsonString(jsonTestString, config); std::string dbPath = fs::unique_path("/tmp/kvstore-%%%%.db3").string(); - saveToKVStore(dbPath, config); + saveToKVStore(dbPath, config, "prefix"); TestConfig outConfig; - loadFromKVStore(dbPath, outConfig); + loadFromKVStore(dbPath, outConfig, "prefix"); - std::string out = saveToString(outConfig); + std::string out = saveToJsonString(outConfig); BOOST_CHECK_EQUAL(out, jsonTestString); fs::remove(dbPath); @@ -265,7 +265,7 @@ BOOST_AUTO_TEST_CASE(FromToKVStoreTest) BOOST_AUTO_TEST_CASE(FromToFDTest) { TestConfig config; - loadFromString(jsonTestString, config); + loadFromJsonString(jsonTestString, config); // Setup fd std::string fifoPath = fs::unique_path("/tmp/fdstore-%%%%").string(); BOOST_CHECK(::mkfifo(fifoPath.c_str(), S_IWUSR | S_IRUSR) >= 0); @@ -276,7 +276,7 @@ BOOST_AUTO_TEST_CASE(FromToFDTest) saveToFD(fd, config); TestConfig outConfig; loadFromFD(fd, outConfig); - std::string out = saveToString(outConfig); + std::string out = saveToJsonString(outConfig); BOOST_CHECK_EQUAL(out, jsonTestString); // Cleanup @@ -287,7 +287,7 @@ BOOST_AUTO_TEST_CASE(FromToFDTest) BOOST_AUTO_TEST_CASE(ConfigUnionTest) { TestConfig testConfig; - BOOST_REQUIRE_NO_THROW(loadFromString(jsonTestString, testConfig)); + BOOST_REQUIRE_NO_THROW(loadFromJsonString(jsonTestString, testConfig)); BOOST_CHECK(testConfig.union1.is()); BOOST_CHECK(!testConfig.union1.is()); @@ -298,7 +298,7 @@ BOOST_AUTO_TEST_CASE(ConfigUnionTest) BOOST_CHECK_EQUAL(subConfig.intVal, 54321); BOOST_CHECK(testConfig.unions[0].is()); BOOST_CHECK(testConfig.unions[1].is()); - std::string out = saveToString(testConfig); + std::string out = saveToJsonString(testConfig); BOOST_CHECK_EQUAL(out, jsonTestString); //Check copy @@ -326,7 +326,7 @@ BOOST_AUTO_TEST_CASE(ConfigUnionTest) testConfig.unions.clear(); testConfig.unions = unions; - out = saveToString(testConfig); + out = saveToJsonString(testConfig); BOOST_CHECK_EQUAL(out, jsonTestString); } diff --git a/tests/unit_tests/config/ut-dynvisit.cpp b/tests/unit_tests/config/ut-dynvisit.cpp index 0a24d5c..dd782f3 100644 --- a/tests/unit_tests/config/ut-dynvisit.cpp +++ b/tests/unit_tests/config/ut-dynvisit.cpp @@ -37,9 +37,11 @@ namespace fs = boost::filesystem; struct Fixture { std::string dbPath; + std::string dbPrefix; Fixture() : dbPath(fs::unique_path("/tmp/kvstore-%%%%.db3").string()) + , dbPrefix("conf") { fs::remove(dbPath); } @@ -54,7 +56,7 @@ BOOST_FIXTURE_TEST_SUITE(DynVisitSuite, Fixture) void checkJsonConfig(const TestConfig& cfg, const std::string& json) { TestConfig cfg2; - loadFromString(json, cfg2); + loadFromJsonString(json, cfg2); BOOST_CHECK_EQUAL(cfg2.intVal, cfg.intVal); BOOST_CHECK_EQUAL(cfg.int64Val, cfg.int64Val); BOOST_CHECK_EQUAL(cfg2.boolVal, cfg.boolVal); @@ -66,34 +68,34 @@ void checkJsonConfig(const TestConfig& cfg, const std::string& json) void checkKVConfig(const TestConfig& cfg, const std::string& db) { KVStore store(db); - BOOST_CHECK_EQUAL(store.get(".intVal"), cfg.intVal); - BOOST_CHECK_EQUAL(store.get(".int64Val"), cfg.int64Val); - BOOST_CHECK_EQUAL(store.get(".boolVal"), cfg.boolVal); - BOOST_CHECK_EQUAL(store.get(".stringVal"), cfg.stringVal); - BOOST_CHECK_EQUAL(store.get(".intVector"), cfg.intVector.size()); - BOOST_CHECK_EQUAL(store.get(".subObj.intVal"), cfg.subObj.intVal); + BOOST_CHECK_EQUAL(store.get("conf.intVal"), cfg.intVal); + BOOST_CHECK_EQUAL(store.get("conf.int64Val"), cfg.int64Val); + BOOST_CHECK_EQUAL(store.get("conf.boolVal"), cfg.boolVal); + BOOST_CHECK_EQUAL(store.get("conf.stringVal"), cfg.stringVal); + BOOST_CHECK_EQUAL(store.get("conf.intVector"), cfg.intVector.size()); + BOOST_CHECK_EQUAL(store.get("conf.subObj.intVal"), cfg.subObj.intVal); } BOOST_AUTO_TEST_CASE(ReadConfigDefaults) { TestConfig cfg; - loadFromKVStoreWithJson(dbPath, jsonTestString, cfg); + loadFromKVStoreWithJson(dbPath, jsonTestString, cfg, dbPrefix); checkJsonConfig(cfg, jsonTestString); } BOOST_AUTO_TEST_CASE(ReadConfigNoDefaults) { TestConfig cfg; - loadFromKVStoreWithJson(dbPath, jsonTestString, cfg); + loadFromKVStoreWithJson(dbPath, jsonTestString, cfg, dbPrefix); // modify and save config cfg.intVal += 5; cfg.int64Val += 7777; cfg.boolVal = !cfg.boolVal; cfg.stringVal += "-changed"; - config::saveToKVStore(dbPath, cfg); + config::saveToKVStore(dbPath, cfg, dbPrefix); TestConfig cfg2; - loadFromKVStoreWithJson(dbPath, jsonTestString, cfg2); + loadFromKVStoreWithJson(dbPath, jsonTestString, cfg2, dbPrefix); checkKVConfig(cfg2, dbPath); } diff --git a/tests/unit_tests/server/ut-zone-admin.cpp b/tests/unit_tests/server/ut-zone-admin.cpp index 0cd2ddf..8dc3eb8 100644 --- a/tests/unit_tests/server/ut-zone-admin.cpp +++ b/tests/unit_tests/server/ut-zone-admin.cpp @@ -56,7 +56,7 @@ struct Fixture { std::unique_ptr create(const std::string& configPath) { - config::loadFromFile(configPath, mConfig); + config::loadFromJsonFile(configPath, mConfig); return std::unique_ptr(new ZoneAdmin(ZONES_PATH, LXC_TEMPLATES_PATH, mConfig)); diff --git a/tests/unit_tests/server/ut-zone-provision.cpp b/tests/unit_tests/server/ut-zone-provision.cpp index 239d53e..8d1738f 100644 --- a/tests/unit_tests/server/ut-zone-provision.cpp +++ b/tests/unit_tests/server/ut-zone-provision.cpp @@ -92,7 +92,7 @@ BOOST_AUTO_TEST_CASE(DestructorTest) ""})); config.units.push_back(unit); - config::saveToFile(PROVISION_FILE_PATH.string(), config); + config::saveToJsonFile(PROVISION_FILE_PATH.string(), config); ZoneProvision zoneProvision(ZONE_PATH.string(), {}); zoneProvision.start(); } @@ -129,7 +129,7 @@ BOOST_AUTO_TEST_CASE(FileTest) 0, 0777})); config.units.push_back(unit); - config::saveToFile(PROVISION_FILE_PATH.string(), config); + config::saveToJsonFile(PROVISION_FILE_PATH.string(), config); ZoneProvision zoneProvision(ZONE_PATH.string(), {}); zoneProvision.start(); @@ -170,7 +170,7 @@ BOOST_AUTO_TEST_CASE(MountTest) O_CREAT, 0777})); config.units.push_back(unit); - config::saveToFile(PROVISION_FILE_PATH.string(), config); + config::saveToJsonFile(PROVISION_FILE_PATH.string(), config); ZoneProvision zoneProvision(ZONE_PATH.string(), {}); zoneProvision.start(); @@ -192,7 +192,7 @@ BOOST_AUTO_TEST_CASE(LinkTest) unit.set(ZoneProvisioning::Link({PROVISION_FILE_PATH.string(), linkFile.string()})); config.units.push_back(unit); - config::saveToFile(PROVISION_FILE_PATH.string(), config); + config::saveToJsonFile(PROVISION_FILE_PATH.string(), config); { ZoneProvision zoneProvision(ZONE_PATH.string(), {}); zoneProvision.start(); @@ -218,7 +218,7 @@ BOOST_AUTO_TEST_CASE(DeclareFileTest) zoneProvision.declareFile(2, "path", 0747, 0777); ZoneProvisioning config; - BOOST_REQUIRE_NO_THROW(loadFromFile(PROVISION_FILE_PATH.string(), config)); + BOOST_REQUIRE_NO_THROW(loadFromJsonFile(PROVISION_FILE_PATH.string(), config)); BOOST_REQUIRE_EQUAL(config.units.size(), 2); BOOST_REQUIRE(config.units[0].is()); BOOST_REQUIRE(config.units[1].is()); @@ -236,7 +236,7 @@ BOOST_AUTO_TEST_CASE(DeclareMountTest) zoneProvision.declareMount("/fake/path2", "/fake/path2", "tmpfs", 077, "fake"); ZoneProvisioning config; - BOOST_REQUIRE_NO_THROW(loadFromFile(PROVISION_FILE_PATH.string(), config)); + BOOST_REQUIRE_NO_THROW(loadFromJsonFile(PROVISION_FILE_PATH.string(), config)); BOOST_REQUIRE_EQUAL(config.units.size(), 2); BOOST_REQUIRE(config.units[0].is()); BOOST_REQUIRE(config.units[1].is()); @@ -255,7 +255,7 @@ BOOST_AUTO_TEST_CASE(DeclareLinkTest) zoneProvision.declareLink("/fake/path2", "/fake/path2"); ZoneProvisioning config; - BOOST_REQUIRE_NO_THROW(loadFromFile(PROVISION_FILE_PATH.string(), config)); + BOOST_REQUIRE_NO_THROW(loadFromJsonFile(PROVISION_FILE_PATH.string(), config)); BOOST_REQUIRE_EQUAL(config.units.size(), 2); BOOST_REQUIRE(config.units[0].is()); BOOST_REQUIRE(config.units[1].is()); @@ -286,7 +286,7 @@ BOOST_AUTO_TEST_CASE(ProvisionedAlreadyTest) 0777})); config.units.push_back(unit); - config::saveToFile(PROVISION_FILE_PATH.string(), config); + config::saveToJsonFile(PROVISION_FILE_PATH.string(), config); ZoneProvision zoneProvision(ZONE_PATH.string(), {"/tmp/"}); zoneProvision.start(); -- 2.7.4 From e5af6bc538db0cfc3995169f801430df9fb6fc2a Mon Sep 17 00:00:00 2001 From: Jan Olszak Date: Thu, 15 Jan 2015 16:37:06 +0100 Subject: [PATCH 13/16] IPC: Single request queue [Bug/Feature] Single queue for passing data between threads Prefixes in loggs inside Processor Destructor always waits till Processor ends [Cause] N/A [Solution] N/A [Verification] Build, install, run tests, run tests under valgrind Change-Id: Idc31496559b46e836528843dfc411cbdeaf259e0 --- common/ipc/client.cpp | 14 +- common/ipc/client.hpp | 11 +- common/ipc/internals/add-peer-request.hpp | 52 ++++ common/ipc/internals/call-queue.cpp | 81 ----- common/ipc/internals/call-queue.hpp | 163 ---------- common/ipc/internals/finish-request.hpp | 48 +++ common/ipc/internals/method-request.hpp | 97 ++++++ common/ipc/internals/processor.cpp | 448 ++++++++++++++------------- common/ipc/internals/processor.hpp | 133 ++++---- common/ipc/internals/remove-peer-request.hpp | 55 ++++ common/ipc/internals/request-queue.hpp | 161 ++++++++++ common/ipc/internals/signal-request.hpp | 82 +++++ common/ipc/service.cpp | 10 +- common/ipc/service.hpp | 4 +- common/ipc/types.cpp | 10 + common/ipc/types.hpp | 4 + tests/unit_tests/ipc/ut-ipc.cpp | 6 +- 17 files changed, 821 insertions(+), 558 deletions(-) create mode 100644 common/ipc/internals/add-peer-request.hpp delete mode 100644 common/ipc/internals/call-queue.cpp delete mode 100644 common/ipc/internals/call-queue.hpp create mode 100644 common/ipc/internals/finish-request.hpp create mode 100644 common/ipc/internals/method-request.hpp create mode 100644 common/ipc/internals/remove-peer-request.hpp create mode 100644 common/ipc/internals/request-queue.hpp create mode 100644 common/ipc/internals/signal-request.hpp diff --git a/common/ipc/client.cpp b/common/ipc/client.cpp index 8455b19..16f77e6 100644 --- a/common/ipc/client.cpp +++ b/common/ipc/client.cpp @@ -32,7 +32,8 @@ namespace vasum { namespace ipc { Client::Client(const std::string& socketPath) - : mSocketPath(socketPath) + : mProcessor("[CLIENT] "), + mSocketPath(socketPath) { LOGS("Client Constructor"); } @@ -47,19 +48,14 @@ Client::~Client() } } -void Client::connect() +void Client::start(const bool usesExternalPolling) { + LOGS("Client start"); // Initialize the connection with the server LOGD("Connecting to " + mSocketPath); auto socketPtr = std::make_shared(Socket::connectSocket(mSocketPath)); mServiceFD = mProcessor.addPeer(socketPtr); -} - -void Client::start() -{ - LOGS("Client start"); - connect(); - mProcessor.start(); + mProcessor.start(usesExternalPolling); } bool Client::isStarted() diff --git a/common/ipc/client.hpp b/common/ipc/client.hpp index de847a9..7b86198 100644 --- a/common/ipc/client.hpp +++ b/common/ipc/client.hpp @@ -55,16 +55,11 @@ public: Client& operator=(const Client&) = delete; /** - * Places a connection request in the internal event queue. - * - * Used with an external polling loop. - */ - void connect(); - - /** * Starts the worker thread + * + * @param usesExternalPolling internal or external polling is used */ - void start(); + void start(const bool usesExternalPolling = false); /** * @return is the communication thread running diff --git a/common/ipc/internals/add-peer-request.hpp b/common/ipc/internals/add-peer-request.hpp new file mode 100644 index 0000000..05c5524 --- /dev/null +++ b/common/ipc/internals/add-peer-request.hpp @@ -0,0 +1,52 @@ +/* +* Copyright (c) 2014 Samsung Electronics Co., Ltd All Rights Reserved +* +* Contact: Jan Olszak +* +* 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 Jan Olszak (j.olszak@samsung.com) + * @brief Processor's request to add a peer + */ + +#ifndef COMMON_IPC_INTERNALS_ADD_PEER_REQUEST_HPP +#define COMMON_IPC_INTERNALS_ADD_PEER_REQUEST_HPP + +#include "ipc/types.hpp" +#include "ipc/internals/socket.hpp" + +namespace vasum { +namespace ipc { + +class AddPeerRequest { +public: + AddPeerRequest(const AddPeerRequest&) = delete; + AddPeerRequest& operator=(const AddPeerRequest&) = delete; + + AddPeerRequest(const FileDescriptor peerFD, const std::shared_ptr& socketPtr) + : peerFD(peerFD), + socketPtr(socketPtr) + { + } + + FileDescriptor peerFD; + std::shared_ptr socketPtr; +}; + +} // namespace ipc +} // namespace vasum + +#endif // COMMON_IPC_INTERNALS_ADD_PEER_REQUEST_HPP diff --git a/common/ipc/internals/call-queue.cpp b/common/ipc/internals/call-queue.cpp deleted file mode 100644 index 70871e5..0000000 --- a/common/ipc/internals/call-queue.cpp +++ /dev/null @@ -1,81 +0,0 @@ -/* -* Copyright (c) 2014 Samsung Electronics Co., Ltd All Rights Reserved -* -* Contact: Jan Olszak -* -* 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 Jan Olszak (j.olszak@samsung.com) - * @brief Managing the queue with calls - */ - -#include "config.hpp" - -#include "ipc/internals/call-queue.hpp" -#include "ipc/exception.hpp" -#include "logger/logger.hpp" -#include - -namespace vasum { -namespace ipc { - -CallQueue::CallQueue() - : mMessageIDCounter(0) -{ -} - -CallQueue::~CallQueue() -{ -} - -bool CallQueue::isEmpty() const -{ - return mCalls.empty(); -} - -MessageID CallQueue::getNextMessageID() -{ - // TODO: This method of generating UIDs is buggy. To be changed. - return ++mMessageIDCounter; -} - -bool CallQueue::erase(const MessageID messageID) -{ - LOGT("Erase messgeID: " << messageID); - auto it = std::find(mCalls.begin(), mCalls.end(), messageID); - if (it == mCalls.end()) { - LOGT("No such messgeID"); - return false; - } - - mCalls.erase(it); - LOGT("Erased"); - return true; -} - -CallQueue::Call CallQueue::pop() -{ - if (isEmpty()) { - LOGE("CallQueue is empty"); - throw IPCException("CallQueue is empty"); - } - Call call = std::move(mCalls.front()); - mCalls.pop_front(); - return call; -} - -} // namespace ipc -} // namespace vasum diff --git a/common/ipc/internals/call-queue.hpp b/common/ipc/internals/call-queue.hpp deleted file mode 100644 index a6e45ed..0000000 --- a/common/ipc/internals/call-queue.hpp +++ /dev/null @@ -1,163 +0,0 @@ -/* -* Copyright (c) 2014 Samsung Electronics Co., Ltd All Rights Reserved -* -* Contact: Jan Olszak -* -* 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 Jan Olszak (j.olszak@samsung.com) - * @brief Managing the queue with calls - */ - -#ifndef COMMON_IPC_INTERNALS_CALL_QUEUE_HPP -#define COMMON_IPC_INTERNALS_CALL_QUEUE_HPP - -#include "ipc/types.hpp" -#include "config/manager.hpp" -#include "logger/logger-scope.hpp" - -#include -#include - -namespace vasum { -namespace ipc { - -/** -* Class for managing a queue of calls in the Processor -*/ -class CallQueue { -public: - typedef std::function& data)> SerializeCallback; - typedef std::function(int fd)> ParseCallback; - - struct Call { - Call(const Call& other) = delete; - Call& operator=(const Call&) = delete; - Call& operator=(Call&&) = default; - Call() = default; - Call(Call&&) = default; - - bool operator==(const MessageID m) - { - return m == messageID; - } - - FileDescriptor peerFD; - MethodID methodID; - MessageID messageID; - std::shared_ptr data; - SerializeCallback serialize; - ParseCallback parse; - ResultHandler::type process; - }; - - CallQueue(); - ~CallQueue(); - - CallQueue(const CallQueue&) = delete; - CallQueue(CallQueue&&) = delete; - CallQueue& operator=(const CallQueue&) = delete; - - template - MessageID push(const MethodID methodID, - const FileDescriptor peerFD, - const std::shared_ptr& data, - const typename ResultHandler::type& process); - - - template - MessageID push(const MethodID methodID, - const FileDescriptor peerFD, - const std::shared_ptr& data); - - Call pop(); - - bool erase(const MessageID messageID); - - bool isEmpty() const; - -private: - std::list mCalls; - std::atomic mMessageIDCounter; - - MessageID getNextMessageID(); -}; - - -template -MessageID CallQueue::push(const MethodID methodID, - const FileDescriptor peerFD, - const std::shared_ptr& data, - const typename ResultHandler::type& process) -{ - Call call; - call.methodID = methodID; - call.peerFD = peerFD; - call.data = data; - - MessageID messageID = getNextMessageID(); - call.messageID = messageID; - - call.serialize = [](const int fd, std::shared_ptr& data)->void { - LOGS("Method serialize, peerFD: " << fd); - config::saveToFD(fd, *std::static_pointer_cast(data)); - }; - - call.parse = [](const int fd)->std::shared_ptr { - LOGS("Method parse, peerFD: " << fd); - std::shared_ptr data(new ReceivedDataType()); - config::loadFromFD(fd, *data); - return data; - }; - - call.process = [process](Status status, std::shared_ptr& data)->void { - LOGS("Method process, status: " << toString(status)); - std::shared_ptr tmpData = std::static_pointer_cast(data); - return process(status, tmpData); - }; - - mCalls.push_back(std::move(call)); - - return messageID; -} - -template -MessageID CallQueue::push(const MethodID methodID, - const FileDescriptor peerFD, - const std::shared_ptr& data) -{ - Call call; - call.methodID = methodID; - call.peerFD = peerFD; - call.data = data; - - MessageID messageID = getNextMessageID(); - call.messageID = messageID; - - call.serialize = [](const int fd, std::shared_ptr& data)->void { - LOGS("Signal serialize, peerFD: " << fd); - config::saveToFD(fd, *std::static_pointer_cast(data)); - }; - - mCalls.push_back(std::move(call)); - - return messageID; -} - -} // namespace ipc -} // namespace vasum - -#endif // COMMON_IPC_INTERNALS_CALL_QUEUE_HPP diff --git a/common/ipc/internals/finish-request.hpp b/common/ipc/internals/finish-request.hpp new file mode 100644 index 0000000..3019475 --- /dev/null +++ b/common/ipc/internals/finish-request.hpp @@ -0,0 +1,48 @@ +/* +* Copyright (c) 2014 Samsung Electronics Co., Ltd All Rights Reserved +* +* Contact: Jan Olszak +* +* 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 Jan Olszak (j.olszak@samsung.com) + * @brief Managing the queue with requests + */ + +#ifndef COMMON_IPC_INTERNALS_FINISH_REQUEST_HPP +#define COMMON_IPC_INTERNALS_FINISH_REQUEST_HPP + +#include + +namespace vasum { +namespace ipc { + +class FinishRequest { +public: + FinishRequest(const FinishRequest&) = delete; + FinishRequest& operator=(const FinishRequest&) = delete; + + FinishRequest(const std::shared_ptr& conditionPtr) + : conditionPtr(conditionPtr) + {} + + std::shared_ptr conditionPtr; +}; + +} // namespace ipc +} // namespace vasum + +#endif // COMMON_IPC_INTERNALS_FINISH_REQUEST_HPP diff --git a/common/ipc/internals/method-request.hpp b/common/ipc/internals/method-request.hpp new file mode 100644 index 0000000..f9860f7 --- /dev/null +++ b/common/ipc/internals/method-request.hpp @@ -0,0 +1,97 @@ +/* +* Copyright (c) 2014 Samsung Electronics Co., Ltd All Rights Reserved +* +* Contact: Jan Olszak +* +* 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 Jan Olszak (j.olszak@samsung.com) + * @brief Processor's request to call a method + */ + +#ifndef COMMON_IPC_INTERNALS_METHOD_REQUEST_HPP +#define COMMON_IPC_INTERNALS_METHOD_REQUEST_HPP + +#include "ipc/types.hpp" +#include "logger/logger-scope.hpp" +#include "config/manager.hpp" + +namespace vasum { +namespace ipc { + +class MethodRequest { +public: + MethodRequest(const MethodRequest&) = delete; + MethodRequest& operator=(const MethodRequest&) = delete; + + template + static std::shared_ptr create(const MethodID methodID, + const FileDescriptor peerFD, + const std::shared_ptr& data, + const typename ResultHandler::type& process); + + MethodID methodID; + FileDescriptor peerFD; + MessageID messageID; + std::shared_ptr data; + SerializeCallback serialize; + ParseCallback parse; + ResultHandler::type process; + +private: + MethodRequest(const MethodID methodID, const FileDescriptor peerFD) + : methodID(methodID), + peerFD(peerFD), + messageID(getNextMessageID()) + {} +}; + + +template +std::shared_ptr MethodRequest::create(const MethodID methodID, + const FileDescriptor peerFD, + const std::shared_ptr& data, + const typename ResultHandler::type& process) +{ + std::shared_ptr request(new MethodRequest(methodID, peerFD)); + + request->data = data; + + request->serialize = [](const int fd, std::shared_ptr& data)->void { + LOGS("Method serialize, peerFD: " << fd); + config::saveToFD(fd, *std::static_pointer_cast(data)); + }; + + request->parse = [](const int fd)->std::shared_ptr { + LOGS("Method parse, peerFD: " << fd); + std::shared_ptr data(new ReceivedDataType()); + config::loadFromFD(fd, *data); + return data; + }; + + request->process = [process](Status status, std::shared_ptr& data)->void { + LOGS("Method process, status: " << toString(status)); + std::shared_ptr tmpData = std::static_pointer_cast(data); + return process(status, tmpData); + }; + + return request; +} + +} // namespace ipc +} // namespace vasum + +#endif // COMMON_IPC_INTERNALS_METHOD_REQUEST_HPP diff --git a/common/ipc/internals/processor.cpp b/common/ipc/internals/processor.cpp index d1e829e..05c97aa 100644 --- a/common/ipc/internals/processor.cpp +++ b/common/ipc/internals/processor.cpp @@ -46,20 +46,23 @@ namespace ipc { expr; \ } \ catch (const std::exception& e){ \ - LOGE("Callback threw an error: " << e.what()); \ + LOGE(mLogPrefix + "Callback threw an error: " << e.what()); \ } const MethodID Processor::RETURN_METHOD_ID = std::numeric_limits::max(); const MethodID Processor::REGISTER_SIGNAL_METHOD_ID = std::numeric_limits::max() - 1; -Processor::Processor(const PeerCallback& newPeerCallback, +Processor::Processor(const std::string& logName, + const PeerCallback& newPeerCallback, const PeerCallback& removedPeerCallback, const unsigned int maxNumberOfPeers) - : mNewPeerCallback(newPeerCallback), + : mLogPrefix(logName), + mIsRunning(false), + mNewPeerCallback(newPeerCallback), mRemovedPeerCallback(removedPeerCallback), mMaxNumberOfPeers(maxNumberOfPeers) { - LOGS("Processor Constructor"); + LOGS(mLogPrefix + "Processor Constructor"); utils::signalBlock(SIGPIPE); using namespace std::placeholders; @@ -69,39 +72,57 @@ Processor::Processor(const PeerCallback& newPeerCallback, Processor::~Processor() { - LOGS("Processor Destructor"); + LOGS(mLogPrefix + "Processor Destructor"); try { stop(); } catch (IPCException& e) { - LOGE("Error in Processor's destructor: " << e.what()); + LOGE(mLogPrefix + "Error in Processor's destructor: " << e.what()); } } bool Processor::isStarted() { - return mThread.joinable(); + Lock lock(mStateMutex); + return mIsRunning; } -void Processor::start() +void Processor::start(bool usesExternalPolling) { - LOGS("Processor start"); + LOGS(mLogPrefix + "Processor start"); + Lock lock(mStateMutex); if (!isStarted()) { - mThread = std::thread(&Processor::run, this); + LOGI(mLogPrefix + "Processor start"); + mIsRunning = true; + if (!usesExternalPolling) { + mThread = std::thread(&Processor::run, this); + } } } void Processor::stop() { - LOGS("Processor stop"); + LOGS(mLogPrefix + "Processor stop"); if (isStarted()) { + auto conditionPtr = std::make_shared(); { Lock lock(mStateMutex); - mEventQueue.send(Event::FINISH); + auto request = std::make_shared(conditionPtr); + mRequestQueue.push(Event::FINISH, request); + } + + LOGD(mLogPrefix + "Waiting for the Processor to stop"); + + if (mThread.joinable()) { + mThread.join(); + } else { + // Wait till the FINISH request is served + Lock lock(mStateMutex); + conditionPtr->wait(lock, [this]() { + return !isStarted(); + }); } - LOGT("Waiting for the Processor to stop"); - mThread.join(); } } @@ -120,7 +141,7 @@ void Processor::setRemovedPeerCallback(const PeerCallback& removedPeerCallback) FileDescriptor Processor::getEventFD() { Lock lock(mStateMutex); - return mEventQueue.getFD(); + return mRequestQueue.getFD(); } void Processor::removeMethod(const MethodID methodID) @@ -131,51 +152,53 @@ void Processor::removeMethod(const MethodID methodID) FileDescriptor Processor::addPeer(const std::shared_ptr& socketPtr) { - LOGS("Processor addPeer"); + LOGS(mLogPrefix + "Processor addPeer"); Lock lock(mStateMutex); + FileDescriptor peerFD = socketPtr->getFD(); - SocketInfo socketInfo(peerFD, std::move(socketPtr)); - mNewSockets.push(std::move(socketInfo)); - mEventQueue.send(Event::ADD_PEER); + auto request = std::make_shared(peerFD, socketPtr); + mRequestQueue.push(Event::ADD_PEER, request); - LOGI("New peer added. Id: " << peerFD); + LOGI(mLogPrefix + "Add Peer Request. Id: " << peerFD); return peerFD; } void Processor::removePeer(const FileDescriptor peerFD) { - LOGS("Processor removePeer peerFD: " << peerFD); - - // TODO: Remove ADD_PEER event if it's not processed + LOGS(mLogPrefix + "Processor removePeer peerFD: " << peerFD); + { + Lock lock(mStateMutex); + mRequestQueue.removeIf([peerFD](Request & request) { + return request.requestID == Event::ADD_PEER && + request.get()->peerFD == peerFD; + }); + } // Remove peer and wait till he's gone - std::shared_ptr conditionPtr(new std::condition_variable()); + std::shared_ptr conditionPtr(new std::condition_variable_any()); { Lock lock(mStateMutex); - RemovePeerRequest request(peerFD, conditionPtr); - mPeersToDelete.push(std::move(request)); - mEventQueue.send(Event::REMOVE_PEER); + auto request = std::make_shared(peerFD, conditionPtr); + mRequestQueue.push(Event::REMOVE_PEER, request); } auto isPeerDeleted = [&peerFD, this]()->bool { - Lock lock(mStateMutex); return mSockets.count(peerFD) == 0; }; - std::mutex mutex; - std::unique_lock lock(mutex); + Lock lock(mStateMutex); conditionPtr->wait(lock, isPeerDeleted); } void Processor::removePeerInternal(const FileDescriptor peerFD, Status status) { - LOGS("Processor removePeerInternal peerFD: " << peerFD); - LOGI("Removing peer. peerFD: " << peerFD); + LOGS(mLogPrefix + "Processor removePeerInternal peerFD: " << peerFD); + LOGI(mLogPrefix + "Removing peer. peerFD: " << peerFD); if (!mSockets.erase(peerFD)) { - LOGW("No such peer. Another thread called removePeerInternal"); + LOGW(mLogPrefix + "No such peer. Another thread called removePeerInternal"); return; } @@ -211,21 +234,25 @@ void Processor::removePeerInternal(const FileDescriptor peerFD, Status status) void Processor::resetPolling() { + LOGS(mLogPrefix + "Processor resetPolling"); + if (!isStarted()) { + LOGW(mLogPrefix + "Processor not started! Polling not reset!"); return; } { Lock lock(mStateMutex); - + LOGI(mLogPrefix + "Reseting mFDS.size: " << mSockets.size()); // Setup polling on eventfd and sockets mFDs.resize(mSockets.size() + 1); - mFDs[0].fd = mEventQueue.getFD(); + mFDs[0].fd = mRequestQueue.getFD(); mFDs[0].events = POLLIN; auto socketIt = mSockets.begin(); for (unsigned int i = 1; i < mFDs.size(); ++i) { + LOGI(mLogPrefix + "Reseting fd: " << socketIt->second->getFD()); mFDs[i].fd = socketIt->second->getFD(); mFDs[i].events = POLLIN | POLLHUP; // Listen for input events ++socketIt; @@ -236,20 +263,19 @@ void Processor::resetPolling() void Processor::run() { - LOGS("Processor run"); + LOGS(mLogPrefix + "Processor run"); resetPolling(); - mIsRunning = true; - while (mIsRunning) { - LOGT("Waiting for communication..."); + while (isStarted()) { + LOGT(mLogPrefix + "Waiting for communication..."); int ret = poll(mFDs.data(), mFDs.size(), -1 /*blocking call*/); - LOGT("... incoming communication!"); + LOGT(mLogPrefix + "... incoming communication!"); if (ret == -1 || ret == 0) { if (errno == EINTR) { continue; } - LOGE("Error in poll: " << std::string(strerror(errno))); + LOGE(mLogPrefix + "Error in poll: " << std::string(strerror(errno))); throw IPCException("Error in poll: " + std::string(strerror(errno))); } @@ -285,7 +311,7 @@ bool Processor::handleLostConnections() { for (unsigned int i = 1; i < mFDs.size(); ++i) { if (mFDs[i].revents & POLLHUP) { - LOGI("Lost connection to peer: " << mFDs[i].fd); + LOGI(mLogPrefix + "Lost connection to peer: " << mFDs[i].fd); mFDs[i].revents &= ~(POLLHUP); removePeerInternal(mFDs[i].fd, Status::PEER_DISCONNECTED); isPeerRemoved = true; @@ -320,7 +346,7 @@ bool Processor::handleInputs() bool Processor::handleInput(const FileDescriptor peerFD) { - LOGS("Processor handleInput peerFD: " << peerFD); + LOGS(mLogPrefix + "Processor handleInput peerFD: " << peerFD); Lock lock(mStateMutex); std::shared_ptr socketPtr; @@ -328,7 +354,7 @@ bool Processor::handleInput(const FileDescriptor peerFD) // Get the peer's socket socketPtr = mSockets.at(peerFD); } catch (const std::out_of_range&) { - LOGE("No such peer: " << peerFD); + LOGE(mLogPrefix + "No such peer: " << peerFD); return false; } @@ -341,7 +367,7 @@ bool Processor::handleInput(const FileDescriptor peerFD) socketPtr->read(&messageID, sizeof(messageID)); } catch (const IPCException& e) { - LOGE("Error during reading the socket"); + LOGE(mLogPrefix + "Error during reading the socket"); removePeerInternal(socketPtr->getFD(), Status::NAUGHTY_PEER); return true; } @@ -362,7 +388,7 @@ bool Processor::handleInput(const FileDescriptor peerFD) } else { // Nothing - LOGW("No method or signal callback for methodID: " << methodID); + LOGW(mLogPrefix + "No method or signal callback for methodID: " << methodID); removePeerInternal(socketPtr->getFD(), Status::NAUGHTY_PEER); return true; } @@ -373,7 +399,7 @@ bool Processor::handleInput(const FileDescriptor peerFD) std::shared_ptr Processor::onNewSignals(const FileDescriptor peerFD, std::shared_ptr& data) { - LOGS("Processor onNewSignals peerFD: " << peerFD); + LOGS(mLogPrefix + "Processor onNewSignals peerFD: " << peerFD); for (const MethodID methodID : data->ids) { mSignalsPeers[methodID].push_back(peerFD); @@ -385,35 +411,35 @@ std::shared_ptr Processor::onNewSignals(const FileDescript bool Processor::onReturnValue(const Socket& socket, const MessageID messageID) { - LOGS("Processor onReturnValue messageID: " << messageID); + LOGS(mLogPrefix + "Processor onReturnValue messageID: " << messageID); - // LOGI("Return value for messageID: " << messageID); + // LOGI(mLogPrefix + "Return value for messageID: " << messageID); ReturnCallbacks returnCallbacks; try { - LOGT("Getting the return callback"); + LOGT(mLogPrefix + "Getting the return callback"); returnCallbacks = std::move(mReturnCallbacks.at(messageID)); mReturnCallbacks.erase(messageID); } catch (const std::out_of_range&) { - LOGW("No return callback for messageID: " << messageID); + LOGW(mLogPrefix + "No return callback for messageID: " << messageID); removePeerInternal(socket.getFD(), Status::NAUGHTY_PEER); return true; } std::shared_ptr data; try { - LOGT("Parsing incoming return data"); + LOGT(mLogPrefix + "Parsing incoming return data"); data = returnCallbacks.parse(socket.getFD()); } catch (const std::exception& e) { - LOGE("Exception during parsing: " << e.what()); + LOGE(mLogPrefix + "Exception during parsing: " << e.what()); IGNORE_EXCEPTIONS(returnCallbacks.process(Status::PARSING_ERROR, data)); removePeerInternal(socket.getFD(), Status::PARSING_ERROR); return true; } - // LOGT("Process return value callback for messageID: " << messageID); + // LOGT(mLogPrefix + "Process return value callback for messageID: " << messageID); IGNORE_EXCEPTIONS(returnCallbacks.process(Status::OK, data)); - // LOGT("Return value for messageID: " << messageID << " processed"); + // LOGT(mLogPrefix + "Return value for messageID: " << messageID << " processed"); return false; } @@ -422,25 +448,25 @@ bool Processor::onRemoteSignal(const Socket& socket, const MessageID messageID, std::shared_ptr signalCallbacks) { - LOGS("Processor onRemoteSignal; methodID: " << methodID << " messageID: " << messageID); + LOGS(mLogPrefix + "Processor onRemoteSignal; methodID: " << methodID << " messageID: " << messageID); - // LOGI("Processor onRemoteSignal; methodID: " << methodID << " messageID: " << messageID); + // LOGI(mLogPrefix + "Processor onRemoteSignal; methodID: " << methodID << " messageID: " << messageID); std::shared_ptr data; try { - LOGT("Parsing incoming data"); + LOGT(mLogPrefix + "Parsing incoming data"); data = signalCallbacks->parse(socket.getFD()); } catch (const std::exception& e) { - LOGE("Exception during parsing: " << e.what()); + LOGE(mLogPrefix + "Exception during parsing: " << e.what()); removePeerInternal(socket.getFD(), Status::PARSING_ERROR); return true; } - // LOGT("Signal callback for methodID: " << methodID << "; messageID: " << messageID); + // LOGT(mLogPrefix + "Signal callback for methodID: " << methodID << "; messageID: " << messageID); try { signalCallbacks->signal(socket.getFD(), data); } catch (const std::exception& e) { - LOGE("Exception in method handler: " << e.what()); + LOGE(mLogPrefix + "Exception in method handler: " << e.what()); removePeerInternal(socket.getFD(), Status::NAUGHTY_PEER); return true; } @@ -453,30 +479,30 @@ bool Processor::onRemoteCall(const Socket& socket, const MessageID messageID, std::shared_ptr methodCallbacks) { - LOGS("Processor onRemoteCall; methodID: " << methodID << " messageID: " << messageID); - // LOGI("Remote call; methodID: " << methodID << " messageID: " << messageID); + LOGS(mLogPrefix + "Processor onRemoteCall; methodID: " << methodID << " messageID: " << messageID); + // LOGI(mLogPrefix + "Remote call; methodID: " << methodID << " messageID: " << messageID); std::shared_ptr data; try { - LOGT("Parsing incoming data"); + LOGT(mLogPrefix + "Parsing incoming data"); data = methodCallbacks->parse(socket.getFD()); } catch (const std::exception& e) { - LOGE("Exception during parsing: " << e.what()); + LOGE(mLogPrefix + "Exception during parsing: " << e.what()); removePeerInternal(socket.getFD(), Status::PARSING_ERROR); return true; } - LOGT("Process callback for methodID: " << methodID << "; messageID: " << messageID); + LOGT(mLogPrefix + "Process callback for methodID: " << methodID << "; messageID: " << messageID); std::shared_ptr returnData; try { returnData = methodCallbacks->method(socket.getFD(), data); } catch (const std::exception& e) { - LOGE("Exception in method handler: " << e.what()); + LOGE(mLogPrefix + "Exception in method handler: " << e.what()); removePeerInternal(socket.getFD(), Status::NAUGHTY_PEER); return true; } - LOGT("Sending return data; methodID: " << methodID << "; messageID: " << messageID); + LOGT(mLogPrefix + "Sending return data; methodID: " << methodID << "; messageID: " << messageID); try { // Send the call with the socket Socket::Guard guard = socket.getGuard(); @@ -484,7 +510,7 @@ bool Processor::onRemoteCall(const Socket& socket, socket.write(&messageID, sizeof(messageID)); methodCallbacks->serialize(socket.getFD(), returnData); } catch (const std::exception& e) { - LOGE("Exception during serialization: " << e.what()); + LOGE(mLogPrefix + "Exception during serialization: " << e.what()); removePeerInternal(socket.getFD(), Status::SERIALIZATION_ERROR); return true; } @@ -494,225 +520,213 @@ bool Processor::onRemoteCall(const Socket& socket, bool Processor::handleEvent() { - LOGS("Processor handleEvent"); + LOGS(mLogPrefix + "Processor handleEvent"); Lock lock(mStateMutex); - switch (mEventQueue.receive()) { - - case Event::FINISH: { - LOGD("Event FINISH"); - mIsRunning = false; - cleanCommunication(); - return false; - } - - case Event::CALL: { - LOGD("Event CALL"); - return onCall(); - } + auto request = mRequestQueue.pop(); + LOGD(mLogPrefix + "Got: " << request.requestID); - case Event::ADD_PEER: { - LOGD("Event ADD_PEER"); - return onNewPeer(); - } - - case Event::REMOVE_PEER: { - LOGD("Event REMOVE_PEER"); - return onRemovePeer(); - } + switch (request.requestID) { + case Event::METHOD: return onMethodRequest(*request.get()); + case Event::SIGNAL: return onSignalRequest(*request.get()); + case Event::ADD_PEER: return onAddPeerRequest(*request.get()); + case Event::REMOVE_PEER: return onRemovePeerRequest(*request.get()); + case Event::FINISH: return onFinishRequest(*request.get()); } return false; } -bool Processor::onNewPeer() +bool Processor::onMethodRequest(MethodRequest& request) { - LOGS("Processor onNewPeer"); + LOGS(mLogPrefix + "Processor onMethodRequest"); + std::shared_ptr socketPtr; - // TODO: What if there is no newSocket? (request removed in the mean time) - // Add new socket of the peer - SocketInfo socketInfo = std::move(mNewSockets.front()); - mNewSockets.pop(); + try { + // Get the peer's socket + socketPtr = mSockets.at(request.peerFD); + } catch (const std::out_of_range&) { + LOGE(mLogPrefix + "Peer disconnected. No socket with a peerFD: " << request.peerFD); + + // Pass the error to the processing callback + IGNORE_EXCEPTIONS(request.process(Status::PEER_DISCONNECTED, request.data)); - if (mSockets.size() > mMaxNumberOfPeers) { - LOGE("There are too many peers. I don't accept the connection with " << socketInfo.peerFD); - return false; - } - if (mSockets.count(socketInfo.peerFD) != 0) { - LOGE("There already was a socket for peerFD: " << socketInfo.peerFD); return false; } - mSockets[socketInfo.peerFD] = std::move(socketInfo.socketPtr); - - - // LOGW("Sending handled signals"); - std::vector ids; - for (const auto kv : mSignalsCallbacks) { - ids.push_back(kv.first); + if (mReturnCallbacks.count(request.messageID) != 0) { + LOGE(mLogPrefix + "There already was a return callback for messageID: " << request.messageID); } - auto data = std::make_shared(ids); - callAsync(REGISTER_SIGNAL_METHOD_ID, - socketInfo.peerFD, - data, - discardResultHandler); - // LOGW("Sent handled signals"); + mReturnCallbacks[request.messageID] = std::move(ReturnCallbacks(request.peerFD, + std::move(request.parse), + std::move(request.process))); - resetPolling(); - - if (mNewPeerCallback) { - // Notify about the new user. - LOGT("Calling NewPeerCallback"); - mNewPeerCallback(socketInfo.peerFD); - } + try { + // Send the call with the socket + Socket::Guard guard = socketPtr->getGuard(); + socketPtr->write(&request.methodID, sizeof(request.methodID)); + socketPtr->write(&request.messageID, sizeof(request.messageID)); + LOGT(mLogPrefix + "Serializing the message"); + request.serialize(socketPtr->getFD(), request.data); + } catch (const std::exception& e) { + LOGE(mLogPrefix + "Error during sending a method: " << e.what()); - return true; -} + // Inform about the error, + IGNORE_EXCEPTIONS(mReturnCallbacks[request.messageID].process(Status::SERIALIZATION_ERROR, request.data)); -bool Processor::onRemovePeer() -{ - LOGS("Processor onRemovePeer"); - removePeerInternal(mPeersToDelete.front().peerFD, Status::REMOVED_PEER); + mReturnCallbacks.erase(request.messageID); + removePeerInternal(request.peerFD, Status::SERIALIZATION_ERROR); - mPeersToDelete.front().conditionPtr->notify_all(); - mPeersToDelete.pop(); - return true; -} + return true; -bool Processor::onCall() -{ - LOGS("Processor onCall"); - CallQueue::Call call; - try { - call = std::move(mCalls.pop()); - } catch (const IPCException&) { - LOGE("No calls to serve, but got an EVENT::CALL. Event got removed before serving"); - return false; } - if (call.parse && call.process) { - return onMethodCall(call); - } else { - return onSignalCall(call); - } + return false; } -bool Processor::onSignalCall(CallQueue::Call& call) +bool Processor::onSignalRequest(SignalRequest& request) { - LOGS("Processor onSignalCall"); + LOGS(mLogPrefix + "Processor onSignalRequest"); std::shared_ptr socketPtr; try { // Get the peer's socket - socketPtr = mSockets.at(call.peerFD); + socketPtr = mSockets.at(request.peerFD); } catch (const std::out_of_range&) { - LOGE("Peer disconnected. No socket with a peerFD: " << call.peerFD); + LOGE(mLogPrefix + "Peer disconnected. No socket with a peerFD: " << request.peerFD); return false; } try { // Send the call with the socket Socket::Guard guard = socketPtr->getGuard(); - socketPtr->write(&call.methodID, sizeof(call.methodID)); - socketPtr->write(&call.messageID, sizeof(call.messageID)); - call.serialize(socketPtr->getFD(), call.data); + socketPtr->write(&request.methodID, sizeof(request.methodID)); + socketPtr->write(&request.messageID, sizeof(request.messageID)); + request.serialize(socketPtr->getFD(), request.data); } catch (const std::exception& e) { - LOGE("Error during sending a signal: " << e.what()); + LOGE(mLogPrefix + "Error during sending a signal: " << e.what()); - removePeerInternal(call.peerFD, Status::SERIALIZATION_ERROR); + removePeerInternal(request.peerFD, Status::SERIALIZATION_ERROR); return true; } return false; } -bool Processor::onMethodCall(CallQueue::Call& call) +bool Processor::onAddPeerRequest(AddPeerRequest& request) { - LOGS("Processor onMethodCall"); - std::shared_ptr socketPtr; - - - try { - // Get the peer's socket - socketPtr = mSockets.at(call.peerFD); - } catch (const std::out_of_range&) { - LOGE("Peer disconnected. No socket with a peerFD: " << call.peerFD); - - // Pass the error to the processing callback - IGNORE_EXCEPTIONS(call.process(Status::PEER_DISCONNECTED, call.data)); + LOGS(mLogPrefix + "Processor onAddPeerRequest"); + if (mSockets.size() > mMaxNumberOfPeers) { + LOGE(mLogPrefix + "There are too many peers. I don't accept the connection with " << request.peerFD); return false; } - - if (mReturnCallbacks.count(call.messageID) != 0) { - LOGE("There already was a return callback for messageID: " << call.messageID); + if (mSockets.count(request.peerFD) != 0) { + LOGE(mLogPrefix + "There already was a socket for peerFD: " << request.peerFD); + return false; } - mReturnCallbacks[call.messageID] = std::move(ReturnCallbacks(call.peerFD, - std::move(call.parse), - std::move(call.process))); - try { - // Send the call with the socket - Socket::Guard guard = socketPtr->getGuard(); - socketPtr->write(&call.methodID, sizeof(call.methodID)); - socketPtr->write(&call.messageID, sizeof(call.messageID)); - LOGT("Serializing the message"); - call.serialize(socketPtr->getFD(), call.data); - } catch (const std::exception& e) { - LOGE("Error during sending a method: " << e.what()); + mSockets[request.peerFD] = std::move(request.socketPtr); - // Inform about the error, - IGNORE_EXCEPTIONS(mReturnCallbacks[call.messageID].process(Status::SERIALIZATION_ERROR, call.data)); + // Sending handled signals + std::vector ids; + for (const auto kv : mSignalsCallbacks) { + ids.push_back(kv.first); + } + auto data = std::make_shared(ids); + callAsync(REGISTER_SIGNAL_METHOD_ID, + request.peerFD, + data, + discardResultHandler); - mReturnCallbacks.erase(call.messageID); - removePeerInternal(call.peerFD, Status::SERIALIZATION_ERROR); - return true; + resetPolling(); + if (mNewPeerCallback) { + // Notify about the new user. + LOGT(mLogPrefix + "Calling NewPeerCallback"); + mNewPeerCallback(request.peerFD); } - return false; + LOGI(mLogPrefix + "New peer: " << request.peerFD); + return true; } -void Processor::cleanCommunication() +bool Processor::onRemovePeerRequest(RemovePeerRequest& request) { - LOGS("Processor cleanCommunication"); + LOGS(mLogPrefix + "Processor onRemovePeer"); - while (!mEventQueue.isEmpty()) { - switch (mEventQueue.receive()) { - case Event::FINISH: { - LOGE("Event FINISH after FINISH"); - break; - } - case Event::CALL: { - LOGW("Event CALL after FINISH"); - try { - CallQueue::Call call = mCalls.pop(); - if (call.process) { - IGNORE_EXCEPTIONS(call.process(Status::CLOSING, call.data)); - } - } catch (const IPCException&) { - // No more calls - } - break; - } + removePeerInternal(request.peerFD, Status::REMOVED_PEER); + request.conditionPtr->notify_all(); + + return true; +} + +bool Processor::onFinishRequest(FinishRequest& request) +{ + LOGS(mLogPrefix + "Processor onFinishRequest"); + + // Clean the mRequestQueue + while (!mRequestQueue.isEmpty()) { + auto request = mRequestQueue.pop(); + LOGE(mLogPrefix + "Got: " << request.requestID << " after FINISH"); - case Event::ADD_PEER: { - LOGW("Event ADD_PEER after FINISH"); + switch (request.requestID) { + case Event::METHOD: { + auto requestPtr = request.get(); + IGNORE_EXCEPTIONS(requestPtr->process(Status::CLOSING, requestPtr->data)); break; } - case Event::REMOVE_PEER: { - LOGW("Event REMOVE_PEER after FINISH"); - mPeersToDelete.front().conditionPtr->notify_all(); - mPeersToDelete.pop(); + request.get()->conditionPtr->notify_all(); break; } + case Event::SIGNAL: + case Event::ADD_PEER: + case Event::FINISH: + break; } } + + mIsRunning = false; + request.conditionPtr->notify_all(); + return true; +} + +std::ostream& operator<<(std::ostream& os, const Processor::Event& event) +{ + switch (event) { + + case Processor::Event::FINISH: { + os << "Event::FINISH"; + break; + } + + case Processor::Event::METHOD: { + os << "Event::METHOD"; + break; + } + + case Processor::Event::SIGNAL: { + os << "Event::SIGNAL"; + break; + } + + case Processor::Event::ADD_PEER: { + os << "Event::ADD_PEER"; + break; + } + + case Processor::Event::REMOVE_PEER: { + os << "Event::REMOVE_PEER"; + break; + } + } + + return os; } } // namespace ipc diff --git a/common/ipc/internals/processor.hpp b/common/ipc/internals/processor.hpp index b0f7ea0..157f39c 100644 --- a/common/ipc/internals/processor.hpp +++ b/common/ipc/internals/processor.hpp @@ -26,8 +26,12 @@ #define COMMON_IPC_INTERNALS_PROCESSOR_HPP #include "ipc/internals/socket.hpp" -#include "ipc/internals/event-queue.hpp" -#include "ipc/internals/call-queue.hpp" +#include "ipc/internals/request-queue.hpp" +#include "ipc/internals/method-request.hpp" +#include "ipc/internals/signal-request.hpp" +#include "ipc/internals/add-peer-request.hpp" +#include "ipc/internals/remove-peer-request.hpp" +#include "ipc/internals/finish-request.hpp" #include "ipc/exception.hpp" #include "ipc/types.hpp" #include "config/manager.hpp" @@ -35,9 +39,9 @@ #include "logger/logger.hpp" #include "logger/logger-scope.hpp" +#include #include #include -#include #include #include #include @@ -67,7 +71,6 @@ const unsigned int DEFAULT_METHOD_TIMEOUT = 1000; * - Rest: The data written in a callback. One type per method.ReturnCallbacks * * TODO: -* - some mutexes may not be needed * - synchronous call to many peers * - implement HandlerStore class for storing both signals and methods * - API for removing signals @@ -81,12 +84,22 @@ const unsigned int DEFAULT_METHOD_TIMEOUT = 1000; * - no new events added after stop() called * - when using IPCGSource: addFD and removeFD can be called from addPeer removePeer callbacks, but * there is no mechanism to ensure the IPCSource exists.. therefore SIGSEGV :) -* - EventQueue should store std::shared_ptr and it should be the only queue to the Processor thread. -* It should have an API for removing events from the middle of the queue * */ class Processor { +private: + enum class Event { + FINISH, // Shutdown request + METHOD, // New method call in the queue + SIGNAL, // New signal call in the queue + ADD_PEER, // New peer in the queue + REMOVE_PEER // Remove peer + }; + public: + + friend std::ostream& operator<<(std::ostream& os, const Processor::Event& event); + /** * Used to indicate a message with the return value. */ @@ -104,7 +117,8 @@ public: * @param newPeerCallback called when a new peer arrives * @param removedPeerCallback called when the Processor stops listening for this peer */ - Processor(const PeerCallback& newPeerCallback = nullptr, + Processor(const std::string& logName = "", + const PeerCallback& newPeerCallback = nullptr, const PeerCallback& removedPeerCallback = nullptr, const unsigned int maxNumberOfPeers = DEFAULT_MAX_NUMBER_OF_PEERS); ~Processor(); @@ -113,11 +127,14 @@ public: Processor(Processor&&) = delete; Processor& operator=(const Processor&) = delete; + /** * Start the processing thread. * Quits immediately after starting the thread. + * + * @param usesExternalPolling internal or external polling is used */ - void start(); + void start(const bool usesExternalPolling); /** * @return is processor running @@ -281,6 +298,7 @@ private: typedef std::function& data)> SerializeCallback; typedef std::function(int fd)> ParseCallback; typedef std::unique_lock Lock; + typedef RequestQueue::Request Request; struct EmptyData { CONFIG_REGISTER_EMPTY @@ -337,56 +355,18 @@ private: ResultHandler::type process; }; - struct SocketInfo { - SocketInfo(const SocketInfo& other) = delete; - SocketInfo& operator=(const SocketInfo&) = delete; - SocketInfo() = default; - SocketInfo(SocketInfo&&) = default; - SocketInfo& operator=(SocketInfo &&) = default; - - SocketInfo(const FileDescriptor peerFD, const std::shared_ptr& socketPtr) - : peerFD(peerFD), socketPtr(socketPtr) {} - - FileDescriptor peerFD; - std::shared_ptr socketPtr; - }; - - struct RemovePeerRequest { - RemovePeerRequest(const RemovePeerRequest& other) = delete; - RemovePeerRequest& operator=(const RemovePeerRequest&) = delete; - RemovePeerRequest() = default; - RemovePeerRequest(RemovePeerRequest&&) = default; - RemovePeerRequest& operator=(RemovePeerRequest &&) = default; - - RemovePeerRequest(const FileDescriptor peerFD, - const std::shared_ptr& conditionPtr) - : peerFD(peerFD), conditionPtr(conditionPtr) {} - - FileDescriptor peerFD; - std::shared_ptr conditionPtr; - }; - - enum class Event : int { - FINISH, // Shutdown request - CALL, // New method call in the queue - ADD_PEER, // New peer in the queue - REMOVE_PEER // Remove peer - }; - EventQueue mEventQueue; + std::string mLogPrefix; + RequestQueue mRequestQueue; bool mIsRunning; - - CallQueue mCalls; std::unordered_map> mMethodsCallbacks; std::unordered_map> mSignalsCallbacks; std::unordered_map> mSignalsPeers; std::unordered_map > mSockets; std::vector mFDs; - std::queue mNewSockets; - std::queue mPeersToDelete; std::unordered_map mReturnCallbacks; @@ -408,11 +388,14 @@ private: static void discardResultHandler(Status, std::shared_ptr&) {} void run(); - bool onCall(); - bool onSignalCall(CallQueue::Call& call); - bool onMethodCall(CallQueue::Call& call); - bool onNewPeer(); - bool onRemovePeer(); + + // Request handlers + bool onMethodRequest(MethodRequest& request); + bool onSignalRequest(SignalRequest& request); + bool onAddPeerRequest(AddPeerRequest& request); + bool onRemovePeerRequest(RemovePeerRequest& request); + bool onFinishRequest(FinishRequest& request); + bool handleLostConnections(); bool handleInputs(); @@ -434,7 +417,6 @@ private: std::shared_ptr& data); - void cleanCommunication(); }; template @@ -469,7 +451,7 @@ void Processor::addMethodHandler(const MethodID methodID, const typename MethodHandler::type& method) { if (methodID == RETURN_METHOD_ID || methodID == REGISTER_SIGNAL_METHOD_ID) { - LOGE("Forbidden methodID: " << methodID); + LOGE(mLogPrefix + "Forbidden methodID: " << methodID); throw IPCException("Forbidden methodID: " + std::to_string(methodID)); } @@ -477,7 +459,7 @@ void Processor::addMethodHandler(const MethodID methodID, Lock lock(mStateMutex); if (mSignalsCallbacks.count(methodID)) { - LOGE("MethodID used by a signal: " << methodID); + LOGE(mLogPrefix + "MethodID used by a signal: " << methodID); throw IPCException("MethodID used by a signal: " + std::to_string(methodID)); } @@ -491,7 +473,7 @@ void Processor::addSignalHandler(const MethodID methodID, const typename SignalHandler::type& handler) { if (methodID == RETURN_METHOD_ID || methodID == REGISTER_SIGNAL_METHOD_ID) { - LOGE("Forbidden methodID: " << methodID); + LOGE(mLogPrefix + "Forbidden methodID: " << methodID); throw IPCException("Forbidden methodID: " + std::to_string(methodID)); } @@ -502,7 +484,7 @@ void Processor::addSignalHandler(const MethodID methodID, // Andd the signal handler: if (mMethodsCallbacks.count(methodID)) { - LOGE("MethodID used by a method: " << methodID); + LOGE(mLogPrefix + "MethodID used by a method: " << methodID); throw IPCException("MethodID used by a method: " + std::to_string(methodID)); } @@ -546,10 +528,9 @@ MessageID Processor::callAsync(const MethodID methodID, const typename ResultHandler::type& process) { Lock lock(mStateMutex); - MessageID messageID = mCalls.push(methodID, peerFD, data, process); - mEventQueue.send(Event::CALL); - - return messageID; + auto request = MethodRequest::create(methodID, peerFD, data, process); + mRequestQueue.push(Event::METHOD, request); + return request->messageID; } @@ -581,24 +562,31 @@ std::shared_ptr Processor::callSync(const MethodID methodID, }; std::unique_lock lock(mutex); - LOGT("Waiting for the response..."); + LOGT(mLogPrefix + "Waiting for the response..."); if (!cv.wait_for(lock, std::chrono::milliseconds(timeoutMS), isResultInitialized)) { - LOGW("Probably a timeout in callSync. Checking..."); + LOGW(mLogPrefix + "Probably a timeout in callSync. Checking..."); bool isTimeout; { Lock lock(mStateMutex); // Call isn't sent or call is sent but there is no reply - isTimeout = mCalls.erase(messageID) || 1 == mReturnCallbacks.erase(messageID); + isTimeout = mRequestQueue.removeIf([messageID](Request & request) { + return request.requestID == Event::METHOD && + request.get()->messageID == messageID; + }) + || mRequestQueue.removeIf([messageID](Request & request) { + return request.requestID == Event::SIGNAL && + request.get()->messageID == messageID; + }) + || 1 == mReturnCallbacks.erase(messageID); } - if (isTimeout) { - LOGE("Function call timeout; methodID: " << methodID); + LOGE(mLogPrefix + "Function call timeout; methodID: " << methodID); removePeer(peerFD); throw IPCTimeoutException("Function call timeout; methodID: " + std::to_string(methodID)); } else { - LOGW("Timeout started during the return value processing, so wait for it to finish"); + LOGW(mLogPrefix + "Timeout started during the return value processing, so wait for it to finish"); if (!cv.wait_for(lock, std::chrono::milliseconds(timeoutMS), isResultInitialized)) { - LOGE("Function call timeout; methodID: " << methodID); + LOGE(mLogPrefix + "Function call timeout; methodID: " << methodID); throw IPCTimeoutException("Function call timeout; methodID: " + std::to_string(methodID)); } } @@ -616,16 +604,17 @@ void Processor::signal(const MethodID methodID, Lock lock(mStateMutex); const auto it = mSignalsPeers.find(methodID); if (it == mSignalsPeers.end()) { - LOGW("No peer is handling signal with methodID: " << methodID); + LOGW(mLogPrefix + "No peer is handling signal with methodID: " << methodID); return; } for (const FileDescriptor peerFD : it->second) { - mCalls.push(methodID, peerFD, data); - mEventQueue.send(Event::CALL); + auto request = SignalRequest::create(methodID, peerFD, data); + mRequestQueue.push(Event::SIGNAL, request); } } + } // namespace ipc } // namespace vasum diff --git a/common/ipc/internals/remove-peer-request.hpp b/common/ipc/internals/remove-peer-request.hpp new file mode 100644 index 0000000..ec01ac4 --- /dev/null +++ b/common/ipc/internals/remove-peer-request.hpp @@ -0,0 +1,55 @@ +/* +* Copyright (c) 2014 Samsung Electronics Co., Ltd All Rights Reserved +* +* Contact: Jan Olszak +* +* 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 Jan Olszak (j.olszak@samsung.com) + * @brief Processor's request to remove a peer + */ + +#ifndef COMMON_IPC_INTERNALS_REMOVE_PEER_REQUEST_HPP +#define COMMON_IPC_INTERNALS_REMOVE_PEER_REQUEST_HPP + +#include "ipc/types.hpp" +#include "ipc/internals/socket.hpp" +#include + + +namespace vasum { +namespace ipc { + +class RemovePeerRequest { +public: + RemovePeerRequest(const RemovePeerRequest&) = delete; + RemovePeerRequest& operator=(const RemovePeerRequest&) = delete; + + RemovePeerRequest(const FileDescriptor peerFD, + const std::shared_ptr& conditionPtr) + : peerFD(peerFD), + conditionPtr(conditionPtr) + { + } + + FileDescriptor peerFD; + std::shared_ptr conditionPtr; +}; + +} // namespace ipc +} // namespace vasum + +#endif // COMMON_IPC_INTERNALS_REMOVE_PEER_REQUEST_HPP diff --git a/common/ipc/internals/request-queue.hpp b/common/ipc/internals/request-queue.hpp new file mode 100644 index 0000000..35b5120 --- /dev/null +++ b/common/ipc/internals/request-queue.hpp @@ -0,0 +1,161 @@ +/* +* Copyright (c) 2014 Samsung Electronics Co., Ltd All Rights Reserved +* +* Contact: Jan Olszak +* +* 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 Jan Olszak (j.olszak@samsung.com) + * @brief Managing the queue of messages carrying any kind of data + */ + +#ifndef COMMON_IPC_INTERNALS_MESSAGE_QUEUE_HPP +#define COMMON_IPC_INTERNALS_MESSAGE_QUEUE_HPP + +#include "ipc/exception.hpp" +#include "ipc/internals/eventfd.hpp" +#include "logger/logger.hpp" + +#include +#include +#include + +namespace vasum { +namespace ipc { + +/** +* Class for managing a queue of Requests carrying any data +*/ +template +class RequestQueue { +public: + RequestQueue() = default; + + RequestQueue(const RequestQueue&) = delete; + RequestQueue& operator=(const RequestQueue&) = delete; + + struct Request { + Request(const Request& other) = delete; + Request& operator=(const Request&) = delete; + + Request(Request&&) = default; + Request(const RequestIdType requestID, const std::shared_ptr& data) + : requestID(requestID), + data(data) + {} + + template + std::shared_ptr get() + { + return std::static_pointer_cast(data); + } + + RequestIdType requestID; + std::shared_ptr data; + }; + + /** + * @return event's file descriptor + */ + int getFD() const; + + /** + * @return is the queue empty + */ + bool isEmpty() const; + + /** + * Push data to the queue + * + * @param requestID request type + * @param data data corresponding to the request + */ + void push(const RequestIdType requestID, + const std::shared_ptr& data = nullptr); + + /** + * @return get the data from the next request + */ + Request pop(); + + /** + * Remove elements from the queue when the predicate returns true + * + * @param predicate condition + * @return was anything removed + */ + template + bool removeIf(Predicate predicate); + +private: + std::list mRequests; + EventFD mEventFD; +}; + +template +int RequestQueue::getFD() const +{ + return mEventFD.getFD(); +} + +template +bool RequestQueue::isEmpty() const +{ + return mRequests.empty(); +} + +template +void RequestQueue::push(const RequestIdType requestID, + const std::shared_ptr& data) +{ + Request request(requestID, data); + mRequests.push_back(std::move(request)); + mEventFD.send(); +} + +template +typename RequestQueue::Request RequestQueue::pop() +{ + mEventFD.receive(); + if (mRequests.empty()) { + LOGE("Request queue is empty"); + throw IPCException("Request queue is empty"); + } + Request request = std::move(mRequests.front()); + mRequests.pop_front(); + return request; +} + +template +template +bool RequestQueue::removeIf(Predicate predicate) +{ + auto it = std::find_if(mRequests.begin(), mRequests.end(), predicate); + if (it == mRequests.end()) { + return false; + } + + do { + it = mRequests.erase(it); + it = std::find_if(it, mRequests.end(), predicate); + } while (it != mRequests.end()); + + return true; +} +} // namespace ipc +} // namespace vasum + +#endif // COMMON_IPC_INTERNALS_MESSAGE_QUEUE_HPP diff --git a/common/ipc/internals/signal-request.hpp b/common/ipc/internals/signal-request.hpp new file mode 100644 index 0000000..4cf62c2 --- /dev/null +++ b/common/ipc/internals/signal-request.hpp @@ -0,0 +1,82 @@ +/* +* Copyright (c) 2014 Samsung Electronics Co., Ltd All Rights Reserved +* +* Contact: Jan Olszak +* +* 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 Jan Olszak (j.olszak@samsung.com) + * @brief Processor's request to send a signal + */ + +#ifndef COMMON_IPC_INTERNALS_SIGNAL_REQUEST_HPP +#define COMMON_IPC_INTERNALS_SIGNAL_REQUEST_HPP + +#include "ipc/types.hpp" +#include "config/manager.hpp" +#include "logger/logger-scope.hpp" + +namespace vasum { +namespace ipc { + +class SignalRequest { +public: + SignalRequest(const SignalRequest&) = delete; + SignalRequest& operator=(const SignalRequest&) = delete; + + + + template + static std::shared_ptr create(const MethodID methodID, + const FileDescriptor peerFD, + const std::shared_ptr& data); + + MethodID methodID; + FileDescriptor peerFD; + MessageID messageID; + std::shared_ptr data; + SerializeCallback serialize; + +private: + SignalRequest(const MethodID methodID, const FileDescriptor peerFD) + : methodID(methodID), + peerFD(peerFD), + messageID(getNextMessageID()) + {} + +}; + +template +std::shared_ptr SignalRequest::create(const MethodID methodID, + const FileDescriptor peerFD, + const std::shared_ptr& data) +{ + std::shared_ptr request(new SignalRequest(methodID, peerFD)); + + request->data = data; + + request->serialize = [](const int fd, std::shared_ptr& data)->void { + LOGS("Signal serialize, peerFD: " << fd); + config::saveToFD(fd, *std::static_pointer_cast(data)); + }; + + return request; +} + +} // namespace ipc +} // namespace vasum + +#endif // COMMON_IPC_INTERNALS_SIGNAL_REQUEST_HPP diff --git a/common/ipc/service.cpp b/common/ipc/service.cpp index ef46346..b96bcd4 100644 --- a/common/ipc/service.cpp +++ b/common/ipc/service.cpp @@ -36,7 +36,7 @@ namespace ipc { Service::Service(const std::string& socketPath, const PeerCallback& addPeerCallback, const PeerCallback& removePeerCallback) - : mProcessor(addPeerCallback, removePeerCallback), + : mProcessor("[SERVICE] ", addPeerCallback, removePeerCallback), mAcceptor(socketPath, std::bind(&Processor::addPeer, &mProcessor, _1)) { @@ -53,14 +53,16 @@ Service::~Service() } } -void Service::start() +void Service::start(const bool usesExternalPolling) { LOGS("Service start"); - mProcessor.start(); + mProcessor.start(usesExternalPolling); // There can be an incoming connection from mAcceptor before mProcessor is listening, // but it's OK. It will handle the connection when ready. So no need to wait for mProcessor. - mAcceptor.start(); + if (!usesExternalPolling) { + mAcceptor.start(); + } } bool Service::isStarted() diff --git a/common/ipc/service.hpp b/common/ipc/service.hpp index fa12e30..9392a42 100644 --- a/common/ipc/service.hpp +++ b/common/ipc/service.hpp @@ -61,8 +61,10 @@ public: /** * Starts the worker and acceptor threads + * + * @param usesExternalPolling internal or external polling is used */ - void start(); + void start(const bool usesExternalPolling = false); /** * @return is the communication thread running diff --git a/common/ipc/types.cpp b/common/ipc/types.cpp index fa57648..ba4c1c4 100644 --- a/common/ipc/types.cpp +++ b/common/ipc/types.cpp @@ -27,10 +27,20 @@ #include "ipc/types.hpp" #include "logger/logger.hpp" +#include namespace vasum { namespace ipc { +namespace { +std::atomic gLastMessageID(0); +} // namespace + +MessageID getNextMessageID() +{ + return ++gLastMessageID; +} + std::string toString(const Status status) { switch (status) { diff --git a/common/ipc/types.hpp b/common/ipc/types.hpp index 5132911..10b87df 100644 --- a/common/ipc/types.hpp +++ b/common/ipc/types.hpp @@ -39,6 +39,8 @@ typedef unsigned int MethodID; typedef unsigned int MessageID; typedef std::function PeerCallback; +typedef std::function& data)> SerializeCallback; +typedef std::function(int fd)> ParseCallback; enum class Status : int { OK = 0, @@ -53,6 +55,8 @@ enum class Status : int { std::string toString(const Status status); void throwOnError(const Status status); +MessageID getNextMessageID(); + template struct MethodHandler { diff --git a/tests/unit_tests/ipc/ut-ipc.cpp b/tests/unit_tests/ipc/ut-ipc.cpp index 7c9df6c..caf59d2 100644 --- a/tests/unit_tests/ipc/ut-ipc.cpp +++ b/tests/unit_tests/ipc/ut-ipc.cpp @@ -63,7 +63,7 @@ const int TIMEOUT = 1000 /*ms*/; const int SHORT_OPERATION_TIME = TIMEOUT / 100; // Time that will cause "TIMEOUT" methods to throw -const int LONG_OPERATION_TIME = 500 + TIMEOUT; +const int LONG_OPERATION_TIME = 1000 + TIMEOUT; struct Fixture { std::string socketPath; @@ -204,7 +204,7 @@ std::pair connectServiceGSource(Service& s, // TODO: On timeout remove the callback s.setNewPeerCallback(newPeerCallback); s.setRemovedPeerCallback(std::bind(&IPCGSource::removeFD, ipcGSourcePtr, _1)); - + s.start(true); // Service starts to process ipcGSourcePtr->attach(); @@ -239,7 +239,7 @@ std::pair connectClientGSource(Service& s, } - c.connect(); + c.start(true); IPCGSource::Pointer ipcGSourcePtr = IPCGSource::create(c.getFDs(), std::bind(&Client::handle, &c, _1, _2)); -- 2.7.4 From 4a0d73ee307ae50aae3e0df63df897b1309d6183 Mon Sep 17 00:00:00 2001 From: Mateusz Malicki Date: Mon, 19 Jan 2015 15:59:23 +0100 Subject: [PATCH 14/16] Fix: Abort tests on timeout [Bug/Feature] Abort testing if timeout was exceeded [Cause] Access to expired memory (stack) or unset memory [Solution] Move shared data on heap or change expiring order [Verification] Run tests (IPCSuite) in slow machine Change-Id: Ib2621d1700cd7c53f9d0dfa29e4b603006525a81 --- tests/unit_tests/ipc/ut-ipc.cpp | 84 ++++++++++++++++++++++++----------------- 1 file changed, 49 insertions(+), 35 deletions(-) diff --git a/tests/unit_tests/ipc/ut-ipc.cpp b/tests/unit_tests/ipc/ut-ipc.cpp index caf59d2..de420df 100644 --- a/tests/unit_tests/ipc/ut-ipc.cpp +++ b/tests/unit_tests/ipc/ut-ipc.cpp @@ -160,7 +160,6 @@ FileDescriptor connect(Service& s, Client& c) cv.notify_all(); }; - // TODO: On timeout remove the callback s.setNewPeerCallback(newPeerCallback); if (!s.isStarted()) { @@ -171,9 +170,12 @@ FileDescriptor connect(Service& s, Client& c) std::unique_lock lock(mutex); - BOOST_REQUIRE(cv.wait_for(lock, std::chrono::milliseconds(3 * TIMEOUT), [&peerFD]() { + BOOST_CHECK(cv.wait_for(lock, std::chrono::milliseconds(3 * TIMEOUT), [&peerFD]() { return peerFD != 0; })); + // Remove the callback + s.setNewPeerCallback(nullptr); + BOOST_REQUIRE(peerFD != 0); return peerFD; } @@ -201,7 +203,6 @@ std::pair connectServiceGSource(Service& s, }; - // TODO: On timeout remove the callback s.setNewPeerCallback(newPeerCallback); s.setRemovedPeerCallback(std::bind(&IPCGSource::removeFD, ipcGSourcePtr, _1)); s.start(true); @@ -211,9 +212,12 @@ std::pair connectServiceGSource(Service& s, c.start(); std::unique_lock lock(mutex); - BOOST_REQUIRE(cv.wait_for(lock, std::chrono::milliseconds(3 * TIMEOUT), [&peerFD]() { + BOOST_CHECK(cv.wait_for(lock, std::chrono::milliseconds(3 * TIMEOUT), [&peerFD]() { return peerFD != 0; })); + // remove the callback + s.setNewPeerCallback(nullptr); + BOOST_REQUIRE(peerFD != 0); return std::make_pair(peerFD, ipcGSourcePtr); } @@ -230,7 +234,6 @@ std::pair connectClientGSource(Service& s, peerFD = newFD; cv.notify_all(); }; - // TODO: On timeout remove the callback s.setNewPeerCallback(newPeerCallback); if (!s.isStarted()) { @@ -246,9 +249,12 @@ std::pair connectClientGSource(Service& s, ipcGSourcePtr->attach(); std::unique_lock lock(mutex); - BOOST_REQUIRE(cv.wait_for(lock, std::chrono::milliseconds(3 * TIMEOUT), [&peerFD]() { + BOOST_CHECK(cv.wait_for(lock, std::chrono::milliseconds(3 * TIMEOUT), [&peerFD]() { return peerFD != 0; })); + // Remove the callback + s.setNewPeerCallback(nullptr); + BOOST_REQUIRE(peerFD != 0); return std::make_pair(peerFD, ipcGSourcePtr); } @@ -407,6 +413,12 @@ BOOST_AUTO_TEST_CASE(SyncServiceToClientEcho) BOOST_AUTO_TEST_CASE(AsyncClientToServiceEcho) { + std::mutex mutex; + std::condition_variable cv; + + std::shared_ptr sentData(new SendData(34)); + std::shared_ptr recvData; + // Setup Service and Client Service s(socketPath); s.addMethodHandler(1, echoCallback); @@ -414,46 +426,44 @@ BOOST_AUTO_TEST_CASE(AsyncClientToServiceEcho) Client c(socketPath); c.start(); - std::mutex mutex; - std::condition_variable cv; - //Async call - std::shared_ptr sentData(new SendData(34)); - std::shared_ptr recvData; auto dataBack = [&cv, &recvData, &mutex](ipc::Status status, std::shared_ptr& data) { - BOOST_CHECK(status == ipc::Status::OK); - std::unique_lock lock(mutex); - recvData = data; + if (status == ipc::Status::OK) { + std::unique_lock lock(mutex); + recvData = data; + } cv.notify_one(); }; c.callAsync(1, sentData, dataBack); // Wait for the response std::unique_lock lock(mutex); - BOOST_CHECK(cv.wait_for(lock, std::chrono::milliseconds(TIMEOUT), [&recvData]() { + BOOST_REQUIRE(cv.wait_for(lock, std::chrono::milliseconds(TIMEOUT), [&recvData]() { return static_cast(recvData); })); - + BOOST_REQUIRE(recvData); BOOST_CHECK_EQUAL(recvData->intVal, sentData->intVal); } BOOST_AUTO_TEST_CASE(AsyncServiceToClientEcho) { + std::shared_ptr sentData(new SendData(56)); + std::shared_ptr recvData; + + std::mutex mutex; + std::condition_variable cv; + Service s(socketPath); Client c(socketPath); c.addMethodHandler(1, echoCallback); FileDescriptor peerFD = connect(s, c); // Async call - std::shared_ptr sentData(new SendData(56)); - std::shared_ptr recvData; - - std::mutex mutex; - std::condition_variable cv; auto dataBack = [&cv, &recvData, &mutex](ipc::Status status, std::shared_ptr& data) { - BOOST_CHECK(status == ipc::Status::OK); - std::unique_lock lock(mutex); - recvData = data; + if (status == ipc::Status::OK) { + std::unique_lock lock(mutex); + recvData = data; + } cv.notify_one(); }; @@ -461,10 +471,11 @@ BOOST_AUTO_TEST_CASE(AsyncServiceToClientEcho) // Wait for the response std::unique_lock lock(mutex); - BOOST_CHECK(cv.wait_for(lock, std::chrono::milliseconds(TIMEOUT), [&recvData]() { + BOOST_REQUIRE(cv.wait_for(lock, std::chrono::milliseconds(TIMEOUT), [&recvData]() { return recvData.get() != nullptr; })); + BOOST_REQUIRE(recvData); BOOST_CHECK_EQUAL(recvData->intVal, sentData->intVal); } @@ -520,13 +531,13 @@ BOOST_AUTO_TEST_CASE(DisconnectedPeerError) s.addMethodHandler(1, method); s.start(); - Client c(socketPath); - c.start(); - std::mutex mutex; std::condition_variable cv; ipc::Status retStatus = ipc::Status::UNDEFINED; + Client c(socketPath); + c.start(); + auto dataBack = [&cv, &retStatus, &mutex](ipc::Status status, std::shared_ptr&) { std::unique_lock lock(mutex); retStatus = status; @@ -538,7 +549,7 @@ BOOST_AUTO_TEST_CASE(DisconnectedPeerError) // Wait for the response std::unique_lock lock(mutex); - BOOST_CHECK(cv.wait_for(lock, std::chrono::milliseconds(TIMEOUT), [&retStatus]() { + BOOST_REQUIRE(cv.wait_for(lock, std::chrono::milliseconds(TIMEOUT), [&retStatus]() { return retStatus != ipc::Status::UNDEFINED; })); @@ -588,16 +599,17 @@ BOOST_AUTO_TEST_CASE(WriteTimeout) BOOST_AUTO_TEST_CASE(AddSignalInRuntime) { + utils::Latch latchA; + utils::Latch latchB; + Service s(socketPath); Client c(socketPath); connect(s, c); - utils::Latch latchA; auto handlerA = [&latchA](const FileDescriptor, std::shared_ptr&) { latchA.set(); }; - utils::Latch latchB; auto handlerB = [&latchB](const FileDescriptor, std::shared_ptr&) { latchB.set(); }; @@ -619,15 +631,16 @@ BOOST_AUTO_TEST_CASE(AddSignalInRuntime) BOOST_AUTO_TEST_CASE(AddSignalOffline) { + utils::Latch latchA; + utils::Latch latchB; + Service s(socketPath); Client c(socketPath); - utils::Latch latchA; auto handlerA = [&latchA](const FileDescriptor, std::shared_ptr&) { latchA.set(); }; - utils::Latch latchB; auto handlerB = [&latchB](const FileDescriptor, std::shared_ptr&) { latchB.set(); }; @@ -652,8 +665,9 @@ BOOST_AUTO_TEST_CASE(AddSignalOffline) BOOST_AUTO_TEST_CASE(ServiceGSource) { - ScopedGlibLoop loop; utils::Latch l; + ScopedGlibLoop loop; + auto signalHandler = [&l](const FileDescriptor, std::shared_ptr&) { l.set(); }; @@ -678,9 +692,9 @@ BOOST_AUTO_TEST_CASE(ServiceGSource) BOOST_AUTO_TEST_CASE(ClientGSource) { + utils::Latch l; ScopedGlibLoop loop; - utils::Latch l; auto signalHandler = [&l](const FileDescriptor, std::shared_ptr&) { l.set(); }; -- 2.7.4 From ba2db8f5ef416d73c23023e6c2fe51b24e25cb62 Mon Sep 17 00:00:00 2001 From: Lukasz Kostyra Date: Tue, 13 Jan 2015 11:13:44 +0100 Subject: [PATCH 15/16] Add ValueLatch interface and use it in IPC tests [Feature] New ValueLatch interface. [Cause] N/A [Solution] N/A [Verification] Build, install, run tests Change-Id: I90e4f7523f57b2f5cb8543e932e0fadffff5b824 --- common/utils/value-latch.hpp | 126 +++++++++++++++++++++++++++++ tests/unit_tests/ipc/ut-ipc.cpp | 116 +++++++-------------------- tests/unit_tests/utils/ut-value-latch.cpp | 128 ++++++++++++++++++++++++++++++ 3 files changed, 283 insertions(+), 87 deletions(-) create mode 100644 common/utils/value-latch.hpp create mode 100644 tests/unit_tests/utils/ut-value-latch.cpp diff --git a/common/utils/value-latch.hpp b/common/utils/value-latch.hpp new file mode 100644 index 0000000..72c7873 --- /dev/null +++ b/common/utils/value-latch.hpp @@ -0,0 +1,126 @@ +/* +* Copyright (c) 2015 Samsung Electronics Co., Ltd All Rights Reserved +* +* Contact: Lukasz Kostyra +* +* 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 Kostyra (l.kostyra@samsung.com) + * @brief Definition of ValueLatch template, used to wait for variable to be set. + */ + +#ifndef COMMON_UTILS_VALUE_LATCH_H +#define COMMON_UTILS_VALUE_LATCH_H + +#include "utils/exception.hpp" + +#include +#include +#include + +namespace vasum { +namespace utils { + +template +class ValueLatch { +public: + /** + * Assigns value to kept variable and sets Latch. + * + * @param value Value to set. + */ + void set(const T& value); + + /** + * Assigns value to kept variable and sets Latch. + * + * @param value Value to set. + */ + void set(T&& value); + + /** + * Waits until set() is called, then set value is moved to caller. + * + * @return Value provided by set(). + */ + T get(); + + /** + * Waits until set() is called, or until timeout occurs. Then, set value is moved to caller. + * + * @param timeoutMs Maximum time to wait for value to be set. + * + * @return Value provided by set(). + */ + T get(const unsigned int timeoutMs); + +private: + std::mutex mMutex; + std::condition_variable mCondition; + std::unique_ptr mValue; +}; + +template +void ValueLatch::set(const T& value) +{ + std::unique_lock lock(mMutex); + if (mValue) { + throw UtilsException("Cannot set ValueLatch multiple times!"); + } + mValue.reset(new T(value)); + mCondition.notify_one(); +} + +template +void ValueLatch::set(T&& value) +{ + std::unique_lock lock(mMutex); + if (mValue) { + throw UtilsException("Cannot set ValueLatch multiple times!"); + } + mValue.reset(new T(std::move(value))); + mCondition.notify_one(); +} + +template +T ValueLatch::get() +{ + std::unique_lock lock(mMutex); + mCondition.wait(lock, [this]() { + return (bool)mValue; + }); + std::unique_ptr retValue(std::move(mValue)); + return T(std::move(*retValue)); +} + +template +T ValueLatch::get(const unsigned int timeoutMs) +{ + std::unique_lock lock(mMutex); + if (mCondition.wait_for(lock, std::chrono::milliseconds(timeoutMs), [this]() { + return (bool)mValue; + }) ) { + std::unique_ptr retValue(std::move(mValue)); + return T(std::move(*retValue)); + } else { + throw UtilsException("Timeout occured."); + } +} + +} // namespace utils +} // namespace vasum + +#endif // COMMON_UTILS_VALUE_LATCH_H diff --git a/tests/unit_tests/ipc/ut-ipc.cpp b/tests/unit_tests/ipc/ut-ipc.cpp index de420df..65ee470 100644 --- a/tests/unit_tests/ipc/ut-ipc.cpp +++ b/tests/unit_tests/ipc/ut-ipc.cpp @@ -37,6 +37,7 @@ #include "ipc/types.hpp" #include "utils/glib-loop.hpp" #include "utils/latch.hpp" +#include "utils/value-latch.hpp" #include "config/fields.hpp" #include "logger/logger.hpp" @@ -150,14 +151,9 @@ std::shared_ptr longEchoCallback(const FileDescriptor, std::shared_ptr FileDescriptor connect(Service& s, Client& c) { // Connects the Client to the Service and returns Clients FileDescriptor - std::mutex mutex; - std::condition_variable cv; - - FileDescriptor peerFD = 0; - auto newPeerCallback = [&cv, &peerFD, &mutex](const FileDescriptor newFD) { - std::unique_lock lock(mutex); - peerFD = newFD; - cv.notify_all(); + ValueLatch peerFDLatch; + auto newPeerCallback = [&peerFDLatch](const FileDescriptor newFD) { + peerFDLatch.set(newFD); }; s.setNewPeerCallback(newPeerCallback); @@ -168,38 +164,25 @@ FileDescriptor connect(Service& s, Client& c) c.start(); - - std::unique_lock lock(mutex); - BOOST_CHECK(cv.wait_for(lock, std::chrono::milliseconds(3 * TIMEOUT), [&peerFD]() { - return peerFD != 0; - })); - // Remove the callback + FileDescriptor peerFD = peerFDLatch.get(TIMEOUT); s.setNewPeerCallback(nullptr); - BOOST_REQUIRE(peerFD != 0); - + BOOST_REQUIRE_NE(peerFD, 0); return peerFD; } - - #if GLIB_CHECK_VERSION(2,36,0) std::pair connectServiceGSource(Service& s, Client& c) { - std::mutex mutex; - std::condition_variable cv; - - FileDescriptor peerFD = 0; + ValueLatch peerFDLatch; IPCGSource::Pointer ipcGSourcePtr = IPCGSource::create(s.getFDs(), std::bind(&Service::handle, &s, _1, _2)); - auto newPeerCallback = [&cv, &peerFD, &mutex, ipcGSourcePtr](const FileDescriptor newFD) { + auto newPeerCallback = [&peerFDLatch, ipcGSourcePtr](const FileDescriptor newFD) { if (ipcGSourcePtr) { //TODO: Remove this if ipcGSourcePtr->addFD(newFD); } - std::unique_lock lock(mutex); - peerFD = newFD; - cv.notify_all(); + peerFDLatch.set(newFD); }; @@ -211,28 +194,18 @@ std::pair connectServiceGSource(Service& s, c.start(); - std::unique_lock lock(mutex); - BOOST_CHECK(cv.wait_for(lock, std::chrono::milliseconds(3 * TIMEOUT), [&peerFD]() { - return peerFD != 0; - })); - // remove the callback + FileDescriptor peerFD = peerFDLatch.get(TIMEOUT); s.setNewPeerCallback(nullptr); - BOOST_REQUIRE(peerFD != 0); - + BOOST_REQUIRE_NE(peerFD, 0); return std::make_pair(peerFD, ipcGSourcePtr); } std::pair connectClientGSource(Service& s, Client& c) { // Connects the Client to the Service and returns Clients FileDescriptor - std::mutex mutex; - std::condition_variable cv; - - FileDescriptor peerFD = 0; - auto newPeerCallback = [&cv, &peerFD, &mutex](const FileDescriptor newFD) { - std::unique_lock lock(mutex); - peerFD = newFD; - cv.notify_all(); + ValueLatch peerFDLatch; + auto newPeerCallback = [&peerFDLatch](const FileDescriptor newFD) { + peerFDLatch.set(newFD); }; s.setNewPeerCallback(newPeerCallback); @@ -248,14 +221,9 @@ std::pair connectClientGSource(Service& s, ipcGSourcePtr->attach(); - std::unique_lock lock(mutex); - BOOST_CHECK(cv.wait_for(lock, std::chrono::milliseconds(3 * TIMEOUT), [&peerFD]() { - return peerFD != 0; - })); - // Remove the callback + FileDescriptor peerFD = peerFDLatch.get(TIMEOUT); s.setNewPeerCallback(nullptr); - BOOST_REQUIRE(peerFD != 0); - + BOOST_REQUIRE_NE(peerFD, 0); return std::make_pair(peerFD, ipcGSourcePtr); } @@ -413,11 +381,8 @@ BOOST_AUTO_TEST_CASE(SyncServiceToClientEcho) BOOST_AUTO_TEST_CASE(AsyncClientToServiceEcho) { - std::mutex mutex; - std::condition_variable cv; - std::shared_ptr sentData(new SendData(34)); - std::shared_ptr recvData; + ValueLatch recvDataLatch; // Setup Service and Client Service s(socketPath); @@ -427,31 +392,22 @@ BOOST_AUTO_TEST_CASE(AsyncClientToServiceEcho) c.start(); //Async call - auto dataBack = [&cv, &recvData, &mutex](ipc::Status status, std::shared_ptr& data) { + auto dataBack = [&recvDataLatch](ipc::Status status, std::shared_ptr& data) { if (status == ipc::Status::OK) { - std::unique_lock lock(mutex); - recvData = data; + recvDataLatch.set(*data); } - cv.notify_one(); }; c.callAsync(1, sentData, dataBack); // Wait for the response - std::unique_lock lock(mutex); - BOOST_REQUIRE(cv.wait_for(lock, std::chrono::milliseconds(TIMEOUT), [&recvData]() { - return static_cast(recvData); - })); - BOOST_REQUIRE(recvData); + std::shared_ptr recvData(new SendData(recvDataLatch.get(TIMEOUT))); BOOST_CHECK_EQUAL(recvData->intVal, sentData->intVal); } BOOST_AUTO_TEST_CASE(AsyncServiceToClientEcho) { std::shared_ptr sentData(new SendData(56)); - std::shared_ptr recvData; - - std::mutex mutex; - std::condition_variable cv; + ValueLatch recvDataLatch; Service s(socketPath); Client c(socketPath); @@ -459,23 +415,16 @@ BOOST_AUTO_TEST_CASE(AsyncServiceToClientEcho) FileDescriptor peerFD = connect(s, c); // Async call - auto dataBack = [&cv, &recvData, &mutex](ipc::Status status, std::shared_ptr& data) { + auto dataBack = [&recvDataLatch](ipc::Status status, std::shared_ptr& data) { if (status == ipc::Status::OK) { - std::unique_lock lock(mutex); - recvData = data; + recvDataLatch.set(*data); } - cv.notify_one(); }; s.callAsync(1, peerFD, sentData, dataBack); // Wait for the response - std::unique_lock lock(mutex); - BOOST_REQUIRE(cv.wait_for(lock, std::chrono::milliseconds(TIMEOUT), [&recvData]() { - return recvData.get() != nullptr; - })); - - BOOST_REQUIRE(recvData); + std::shared_ptr recvData(new SendData(recvDataLatch.get(TIMEOUT))); BOOST_CHECK_EQUAL(recvData->intVal, sentData->intVal); } @@ -521,6 +470,7 @@ BOOST_AUTO_TEST_CASE(ParseError) BOOST_AUTO_TEST_CASE(DisconnectedPeerError) { + ValueLatch retStatusLatch; Service s(socketPath); auto method = [](const FileDescriptor, std::shared_ptr&) { @@ -531,27 +481,18 @@ BOOST_AUTO_TEST_CASE(DisconnectedPeerError) s.addMethodHandler(1, method); s.start(); - std::mutex mutex; - std::condition_variable cv; - ipc::Status retStatus = ipc::Status::UNDEFINED; - Client c(socketPath); c.start(); - auto dataBack = [&cv, &retStatus, &mutex](ipc::Status status, std::shared_ptr&) { - std::unique_lock lock(mutex); - retStatus = status; - cv.notify_one(); + auto dataBack = [&retStatusLatch](ipc::Status status, std::shared_ptr&) { + retStatusLatch.set(status); }; std::shared_ptr sentData(new SendData(78)); c.callAsync(1, sentData, dataBack); // Wait for the response - std::unique_lock lock(mutex); - BOOST_REQUIRE(cv.wait_for(lock, std::chrono::milliseconds(TIMEOUT), [&retStatus]() { - return retStatus != ipc::Status::UNDEFINED; - })); + ipc::Status retStatus = retStatusLatch.get(TIMEOUT); // The disconnection might have happened: // - after sending the message (PEER_DISCONNECTED) @@ -663,6 +604,7 @@ BOOST_AUTO_TEST_CASE(AddSignalOffline) #if GLIB_CHECK_VERSION(2,36,0) +// FIXME This test causes segfault, however it should work in GDB. BOOST_AUTO_TEST_CASE(ServiceGSource) { utils::Latch l; diff --git a/tests/unit_tests/utils/ut-value-latch.cpp b/tests/unit_tests/utils/ut-value-latch.cpp new file mode 100644 index 0000000..a16128e --- /dev/null +++ b/tests/unit_tests/utils/ut-value-latch.cpp @@ -0,0 +1,128 @@ +/* + * Copyright (c) 2015 Samsung Electronics Co., Ltd All Rights Reserved + * + * Contact: Lukasz Kostyra + * + * 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 Kostyra (l.kostyra@samsung.com) + * @brief Unit tests of ValueLatch interface + */ + +#include "config.hpp" +#include "ut.hpp" + +#include "utils/value-latch.hpp" + +#include +#include + +BOOST_AUTO_TEST_SUITE(ValueLatchSuite) + +using namespace vasum::utils; + +namespace +{ + const int TIMEOUT = 1000; // ms + const int EXPECTED_TIMEOUT = 200; // ms + const std::string TEST_STRING = "some_random text"; + + struct ComplexType + { + float value; + std::string str; + }; + + struct ComplexMovableType + { + explicit ComplexMovableType(const ComplexType& val) + : value(val) {}; + ComplexMovableType(const ComplexMovableType&) = delete; + ComplexMovableType(ComplexMovableType&&) = default; + + ComplexType value; + }; +} // namespace + +BOOST_AUTO_TEST_CASE(SimpleTypeTest) +{ + ValueLatch testLatch; + + std::thread testThread([&testLatch]() { + testLatch.set(3); + }); + + testThread.join(); + + BOOST_REQUIRE_EQUAL(testLatch.get(TIMEOUT), 3); +} + +BOOST_AUTO_TEST_CASE(ComplexTypeTest) +{ + ValueLatch testLatch; + + std::thread testThread([&testLatch]() { + testLatch.set({ 2.5f, TEST_STRING }); + }); + + testThread.join(); + + ComplexType test(testLatch.get(TIMEOUT)); + BOOST_REQUIRE_EQUAL(test.value, 2.5f); + BOOST_REQUIRE_EQUAL(test.str, TEST_STRING); +} + +BOOST_AUTO_TEST_CASE(ComplexMovableTypeTest) +{ + ValueLatch testLatch; + + std::thread testThread([&testLatch]() { + testLatch.set( ComplexMovableType({ 2.5f, TEST_STRING }) ); + }); + + testThread.join(); + + ComplexMovableType test(testLatch.get(TIMEOUT)); + BOOST_REQUIRE_EQUAL(test.value.value, 2.5f); + BOOST_REQUIRE_EQUAL(test.value.str, TEST_STRING); +} + +BOOST_AUTO_TEST_CASE(TimeoutTest) +{ + ValueLatch testLatch; + + BOOST_REQUIRE_THROW(testLatch.get(EXPECTED_TIMEOUT), vasum::UtilsException); +} + +BOOST_AUTO_TEST_CASE(MultipleSetTest) +{ + ValueLatch testLatch; + + testLatch.set(3); + BOOST_REQUIRE_THROW(testLatch.set(2), vasum::UtilsException); +} + +BOOST_AUTO_TEST_CASE(MultipleGetTest) +{ + ValueLatch testLatch; + + testLatch.set(3); + testLatch.get(TIMEOUT); + BOOST_REQUIRE_THROW(testLatch.get(EXPECTED_TIMEOUT), vasum::UtilsException); +} + +BOOST_AUTO_TEST_SUITE_END() -- 2.7.4 From 48e65e93bbc02002fdfe8ca1f4467306a740f7ec Mon Sep 17 00:00:00 2001 From: Mateusz Malicki Date: Wed, 21 Jan 2015 12:53:38 +0100 Subject: [PATCH 16/16] Some synchronizations [Bug/Feature] Segfault may happen [Cause] Access to expired memory (stack), unset memory or data races [Solution] Synchronizations [Verification] Run tests in slow machine Change-Id: Iebf1a3d7dc93341d4200f6380c0110ecd2d96075 --- server/zone.cpp | 20 -------------------- server/zone.hpp | 8 -------- server/zones-manager.cpp | 17 +++++++++++------ server/zones-manager.hpp | 2 +- tests/unit_tests/client/ut-client.cpp | 1 + tests/unit_tests/dbus/ut-connection.cpp | 6 +++--- tests/unit_tests/ipc/ut-ipc.cpp | 4 ++++ 7 files changed, 20 insertions(+), 38 deletions(-) diff --git a/server/zone.cpp b/server/zone.cpp index d38a0c2..f35a360 100644 --- a/server/zone.cpp +++ b/server/zone.cpp @@ -127,26 +127,6 @@ void Zone::start() goBackground(); } -void Zone::startAsync(const StartAsyncResultCallback& callback) -{ - auto startWrapper = [this, callback]() { - bool succeeded = false; - - try { - start(); - succeeded = true; - } catch(std::exception& e) { - LOGE(getId() << ": failed to start: " << e.what()); - } - - if (callback) { - callback(succeeded); - } - }; - - mWorker->addTask(startWrapper); -} - void Zone::stop() { Lock lock(mReconnectMutex); diff --git a/server/zone.hpp b/server/zone.hpp index dd0daca..7eb34cf 100644 --- a/server/zone.hpp +++ b/server/zone.hpp @@ -98,14 +98,6 @@ public: void start(); /** - * Boot the zone to the background in separate thread. This function immediately exits - * after zone booting is started in another thread. - * - * @param callback Called after starting the zone. Passes bool with result of starting. - */ - void startAsync(const StartAsyncResultCallback& callback); - - /** * Try to shutdown the zone, if failed, destroy it. */ void stop(); diff --git a/server/zones-manager.cpp b/server/zones-manager.cpp index af1da8d..f65f9e3 100644 --- a/server/zones-manager.cpp +++ b/server/zones-manager.cpp @@ -965,17 +965,22 @@ void ZonesManager::handleStartZoneCall(const std::string& id, LOGT("Start zone " << id); - auto resultCallback = [this, id, result](bool succeeded) { - if (succeeded) { + auto startAsync = [this, id, result]() { + try { + ZoneMap::mapped_type zone; + { + Lock lock(mMutex); + zone = mZones.at(id); + } + zone->start(); focus(id); result->setVoid(); - } else { - LOGE("Failed to start zone."); + } catch (const std::exception& e) { + LOGE(id << ": failed to start: " << e.what()); result->setError(api::ERROR_INTERNAL, "Failed to start zone"); } }; - - mZones[id]->startAsync(resultCallback); + mWorker->addTask(startAsync); } void ZonesManager::handleLockZoneCall(const std::string& id, diff --git a/server/zones-manager.hpp b/server/zones-manager.hpp index 5bdb6b5..e074fc9 100644 --- a/server/zones-manager.hpp +++ b/server/zones-manager.hpp @@ -117,7 +117,7 @@ private: // to hold InputMonitor pointer to monitor if zone switching sequence is recognized std::unique_ptr mSwitchingSequenceMonitor; std::unique_ptr mProxyCallPolicy; - typedef std::unordered_map> ZoneMap; + typedef std::unordered_map> ZoneMap; ZoneMap mZones; // map of zones, id is the key bool mDetachOnExit; diff --git a/tests/unit_tests/client/ut-client.cpp b/tests/unit_tests/client/ut-client.cpp index 9536ccb..49e18d2 100644 --- a/tests/unit_tests/client/ut-client.cpp +++ b/tests/unit_tests/client/ut-client.cpp @@ -158,6 +158,7 @@ BOOST_AUTO_TEST_CASE(GetZoneDbusesTest) BOOST_REQUIRE_EQUAL(VSMCLIENT_SUCCESS, status); VsmArrayString keys, values; status = vsm_get_zone_dbuses(client, &keys, &values); + //TODO: Clean up if BOOST_REQUIRE_EQUAL fail (remove client). Same in other client tests. BOOST_REQUIRE_EQUAL(VSMCLIENT_SUCCESS, status); BOOST_CHECK_EQUAL(getArrayStringLength(keys, EXPECTED_DBUSES_STARTED.size() + 1u), diff --git a/tests/unit_tests/dbus/ut-connection.cpp b/tests/unit_tests/dbus/ut-connection.cpp index d4eb1de..96175ee 100644 --- a/tests/unit_tests/dbus/ut-connection.cpp +++ b/tests/unit_tests/dbus/ut-connection.cpp @@ -42,6 +42,7 @@ #include #include +//TODO: BOOST_* macros aren't thread-safe. Remove it from callbacks BOOST_AUTO_TEST_SUITE(DbusSuite) @@ -461,6 +462,8 @@ BOOST_AUTO_TEST_CASE(MethodAsyncCallAsyncHandlerTest) Latch nameAcquired; Latch handlerDone; Latch callDone; + std::string strResult; + MethodResultBuilder::Pointer deferredResult; DbusConnection::Pointer conn1 = DbusConnection::create(DBUS_ADDRESS); DbusConnection::Pointer conn2 = DbusConnection::create(DBUS_ADDRESS); @@ -470,9 +473,6 @@ BOOST_AUTO_TEST_CASE(MethodAsyncCallAsyncHandlerTest) [] {}); BOOST_REQUIRE(nameAcquired.wait(EVENT_TIMEOUT)); - std::string strResult; - MethodResultBuilder::Pointer deferredResult; - auto handler = [&](const std::string& objectPath, const std::string& interface, const std::string& methodName, diff --git a/tests/unit_tests/ipc/ut-ipc.cpp b/tests/unit_tests/ipc/ut-ipc.cpp index 65ee470..c14b2a0 100644 --- a/tests/unit_tests/ipc/ut-ipc.cpp +++ b/tests/unit_tests/ipc/ut-ipc.cpp @@ -234,6 +234,7 @@ void testEcho(Client& c, const MethodID methodID) { std::shared_ptr sentData(new SendData(34)); std::shared_ptr recvData = c.callSync(methodID, sentData, TIMEOUT); + BOOST_REQUIRE(recvData); BOOST_CHECK_EQUAL(recvData->intVal, sentData->intVal); } @@ -241,6 +242,7 @@ void testEcho(Service& s, const MethodID methodID, const FileDescriptor peerFD) { std::shared_ptr sentData(new SendData(56)); std::shared_ptr recvData = s.callSync(methodID, peerFD, sentData, TIMEOUT); + BOOST_REQUIRE(recvData); BOOST_CHECK_EQUAL(recvData->intVal, sentData->intVal); } @@ -376,6 +378,7 @@ BOOST_AUTO_TEST_CASE(SyncServiceToClientEcho) std::shared_ptr sentData(new SendData(56)); std::shared_ptr recvData = s.callSync(1, peerFD, sentData); + BOOST_REQUIRE(recvData); BOOST_CHECK_EQUAL(recvData->intVal, sentData->intVal); } @@ -530,6 +533,7 @@ BOOST_AUTO_TEST_CASE(WriteTimeout) // Test echo with a minimal timeout std::shared_ptr sentDataA(new LongSendData(34, SHORT_OPERATION_TIME)); std::shared_ptr recvData = c.callSync(1, sentDataA, TIMEOUT); + BOOST_REQUIRE(recvData); BOOST_CHECK_EQUAL(recvData->intVal, sentDataA->intVal); // Test timeout on write -- 2.7.4