From 94da82801334a586aa1581a8e5b8591aa90d8e94 Mon Sep 17 00:00:00 2001 From: Henning Baldersheim Date: Thu, 27 Sep 2018 20:34:06 +0200 Subject: Modernise code and use smart pointers and favor std containers over homegrown specialized classes. --- slobrok/src/vespa/slobrok/server/cmd.cpp | 15 ++-- slobrok/src/vespa/slobrok/server/reserved_name.h | 4 +- .../vespa/slobrok/server/rpc_server_manager.cpp | 2 +- .../src/vespa/slobrok/server/rpc_server_map.cpp | 98 ++++++++++------------ slobrok/src/vespa/slobrok/server/rpc_server_map.h | 35 +++----- slobrok/src/vespa/slobrok/server/visible_map.cpp | 20 ++--- slobrok/src/vespa/slobrok/server/visible_map.h | 5 +- 7 files changed, 80 insertions(+), 99 deletions(-) (limited to 'slobrok/src') diff --git a/slobrok/src/vespa/slobrok/server/cmd.cpp b/slobrok/src/vespa/slobrok/server/cmd.cpp index 6ba4f9a6e98..63a59eb0a8d 100644 --- a/slobrok/src/vespa/slobrok/server/cmd.cpp +++ b/slobrok/src/vespa/slobrok/server/cmd.cpp @@ -45,10 +45,9 @@ RegRpcSrvCommand::makeRegRpcSrvCmd(SBEnv &env, } RegRpcSrvCommand -RegRpcSrvCommand::makeRemRemCmd(SBEnv &env, - const char *name, const char *spec) +RegRpcSrvCommand::makeRemRemCmd(SBEnv &env, const char *name, const char *spec) { - RegRpcSrvData *data = new RegRpcSrvData(env, name, spec, NULL); + RegRpcSrvData *data = new RegRpcSrvData(env, name, spec, nullptr); data->_state = RegRpcSrvData::XCH_IGNORE; return RegRpcSrvCommand(data); } @@ -65,8 +64,8 @@ void RegRpcSrvCommand::cleanupReservation() { RpcServerMap &map = _data->env._rpcsrvmap; - ReservedName *rsvp = map.getReservation(_data->name.c_str()); - if (rsvp != NULL && rsvp->isLocal) { + const ReservedName *rsvp = map.getReservation(_data->name.c_str()); + if (rsvp != nullptr && rsvp->isLocal) { map.removeReservation(_data->name.c_str()); } } @@ -74,14 +73,14 @@ RegRpcSrvCommand::cleanupReservation() void RegRpcSrvCommand::doneHandler(OkState result) { - LOG_ASSERT(_data != NULL); + LOG_ASSERT(_data != nullptr); if (result.failed()) { LOG(warning, "failed in state %d: %s", _data->_state, result.errorMsg.c_str()); cleanupReservation(); // XXX should handle different state errors differently? - if (_data->registerRequest != NULL) { + if (_data->registerRequest != nullptr) { _data->registerRequest->SetError(FRTE_RPC_METHOD_FAILED, result.errorMsg.c_str()); _data->registerRequest->Return(); @@ -121,7 +120,7 @@ RegRpcSrvCommand::doneHandler(OkState result) alldone: cleanupReservation(); delete _data; - _data = NULL; + _data = nullptr; } //----------------------------------------------------------------------------- diff --git a/slobrok/src/vespa/slobrok/server/reserved_name.h b/slobrok/src/vespa/slobrok/server/reserved_name.h index 7761dc37918..ee533626f72 100644 --- a/slobrok/src/vespa/slobrok/server/reserved_name.h +++ b/slobrok/src/vespa/slobrok/server/reserved_name.h @@ -27,10 +27,10 @@ public: { _reservedTime.SetNow(); } - bool stillReserved() { + bool stillReserved() const { return (_reservedTime.MilliSecsToNow() < 15000); } - int seconds() { return _reservedTime.MilliSecsToNow() / 1000; } + int seconds() const { return _reservedTime.MilliSecsToNow() / 1000; } }; //----------------------------------------------------------------------------- diff --git a/slobrok/src/vespa/slobrok/server/rpc_server_manager.cpp b/slobrok/src/vespa/slobrok/server/rpc_server_manager.cpp index 293975cc82a..97749de645c 100644 --- a/slobrok/src/vespa/slobrok/server/rpc_server_manager.cpp +++ b/slobrok/src/vespa/slobrok/server/rpc_server_manager.cpp @@ -142,7 +142,7 @@ RpcServerManager::addMyReservation(const char *name, const char *spec) // check if we already are in the progress of adding this if (_rpcsrvmap.conflictingReservation(name, spec)) { - ReservedName * rsv = _rpcsrvmap.getReservation(name); + const ReservedName * rsv = _rpcsrvmap.getReservation(name); LOG(warning, "conflicting registrations: wanted [%s -> %s] but [%s -> %s] already reserved", name, spec, rsv->getName(), rsv->getSpec()); return OkState(FRTE_RPC_METHOD_FAILED, diff --git a/slobrok/src/vespa/slobrok/server/rpc_server_map.cpp b/slobrok/src/vespa/slobrok/server/rpc_server_map.cpp index 06c5698ec09..47fb7b2516e 100644 --- a/slobrok/src/vespa/slobrok/server/rpc_server_map.cpp +++ b/slobrok/src/vespa/slobrok/server/rpc_server_map.cpp @@ -12,33 +12,34 @@ namespace slobrok { //----------------------------------------------------------------------------- +ManagedRpcServer * +RpcServerMap::lookupManaged(const char *name) const { + auto found = _myrpcsrv_map.find(name); + return (found == _myrpcsrv_map.end()) ? nullptr : found->second.get(); +} + NamedService * RpcServerMap::lookup(const char *name) const { - NamedService *d = _myrpcsrv_map[name]; - return d; + return lookupManaged(name); } - NamedService * RpcServerMap::remove(const char *name) { _visible_map.remove(name); - NamedService *d = _myrpcsrv_map.remove(name); - return d; + auto service = std::move(_myrpcsrv_map[name]); + _myrpcsrv_map.erase(name); + return service.release(); } - - std::vector RpcServerMap::lookupPattern(const char *pattern) const { std::vector retval; - for (HashMap::Iterator it = _myrpcsrv_map.iterator(); - it.valid(); it.next()) - { - if (match(it.key(), pattern)) { - retval.push_back(it.value()); + for (const auto & entry : _myrpcsrv_map) { + if (match(entry.first.c_str(), pattern)) { + retval.push_back(entry.second.get()); } } return retval; @@ -50,10 +51,8 @@ RpcServerMap::allManaged() const { std::vector retval; // get list of all names in myrpcsrv_map - for (HashMap::Iterator it = _myrpcsrv_map.iterator(); - it.valid(); it.next()) - { - retval.push_back(it.value()); + for (const auto & entry : _myrpcsrv_map) { + retval.push_back(entry.second.get()); } return retval; } @@ -64,12 +63,12 @@ RpcServerMap::add(NamedService *rpcsrv) { const char *name = rpcsrv->getName(); - LOG_ASSERT(rpcsrv != NULL); - LOG_ASSERT(_myrpcsrv_map.isSet(name) == false); + LOG_ASSERT(rpcsrv != nullptr); + LOG_ASSERT(_myrpcsrv_map.find(name) == _myrpcsrv_map.end()); removeReservation(name); - LOG_ASSERT(_visible_map.lookup(name) == NULL); + LOG_ASSERT(_visible_map.lookup(name) == nullptr); _visible_map.addNew(rpcsrv); } @@ -78,11 +77,12 @@ RpcServerMap::addNew(ManagedRpcServer *rpcsrv) { const char *name = rpcsrv->getName(); - ManagedRpcServer *oldman = _myrpcsrv_map.remove(name); + auto oldman = std::move(_myrpcsrv_map[name]); + _myrpcsrv_map.erase(name); - if (oldman != NULL) { - ReservedName *oldres = _reservations[name]; - NamedService *oldvis = _visible_map.remove(name); + if (oldman) { + const ReservedName *oldres = _reservations[name].get(); + const NamedService *oldvis = _visible_map.remove(name); const char *spec = rpcsrv->getSpec(); const char *oldname = oldman->getName(); @@ -90,38 +90,37 @@ RpcServerMap::addNew(ManagedRpcServer *rpcsrv) if (strcmp(spec, oldspec) != 0) { LOG(warning, "internal state problem: adding [%s at %s] but already had [%s at %s]", name, spec, oldname, oldspec); - if (oldvis != oldman) { + if (oldvis != oldman.get()) { const char *n = oldvis->getName(); const char *s = oldvis->getSpec(); LOG(warning, "BAD: different old visible: [%s at %s]", n, s); } - if (oldres != NULL) { + if (oldres != nullptr) { const char *n = oldres->getName(); const char *s = oldres->getSpec(); LOG(warning, "old reservation: [%s at %s]", n, s); } } - delete oldman; } add(rpcsrv); - _myrpcsrv_map.set(name, rpcsrv); + _myrpcsrv_map[name].reset(rpcsrv); } void RpcServerMap::addReservation(ReservedName *rpcsrv) { - LOG_ASSERT(rpcsrv != NULL); - LOG_ASSERT(_myrpcsrv_map.isSet(rpcsrv->getName()) == false); + LOG_ASSERT(rpcsrv != nullptr); + LOG_ASSERT(_myrpcsrv_map.find(rpcsrv->getName()) == _myrpcsrv_map.end()); // must not be reserved for something else already // this should have been checked already, so assert LOG_ASSERT(! conflictingReservation(rpcsrv->getName(), rpcsrv->getSpec())); - ReservedName *old = _reservations.set(rpcsrv->getName(), rpcsrv); - LOG_ASSERT(old == NULL + auto old = std::move(_reservations[rpcsrv->getName()]); + _reservations[rpcsrv->getName()].reset(rpcsrv); + LOG_ASSERT(!old || strcmp(old->getSpec(), rpcsrv->getSpec()) == 0 || ! old->stillReserved()); - delete old; } @@ -129,37 +128,31 @@ RpcServerMap::addReservation(ReservedName *rpcsrv) bool RpcServerMap::conflictingReservation(const char *name, const char *spec) { - ReservedName *resv = _reservations[name]; - return (resv != NULL && + const ReservedName *resv = _reservations[name].get(); + return (resv != nullptr && resv->stillReserved() && strcmp(resv->getSpec(), spec) != 0); } +const ReservedName * +RpcServerMap::getReservation(const char *name) const { + auto found = _reservations.find(name); + return (found == _reservations.end()) ? nullptr : found->second.get(); +} -RpcServerMap::~RpcServerMap() +RpcServerMap::RpcServerMap() + : _myrpcsrv_map(), + _reservations() { - // get list of names in rpcsrv_map - std::vector names; - for (HashMap::Iterator it = _myrpcsrv_map.iterator(); - it.valid(); it.next()) - { - names.push_back(it.key()); - } - - for (uint32_t i = 0; i < names.size(); i++) { - NamedService *rpcsrv = _myrpcsrv_map.remove(names[i]); - LOG_ASSERT(rpcsrv != NULL); - delete rpcsrv; - } - LOG_ASSERT(_myrpcsrv_map.size() == 0); } +RpcServerMap::~RpcServerMap() = default; bool RpcServerMap::match(const char *name, const char *pattern) { - LOG_ASSERT(name != NULL); - LOG_ASSERT(pattern != NULL); + LOG_ASSERT(name != nullptr); + LOG_ASSERT(pattern != nullptr); while (*pattern != '\0') { if (*name == *pattern) { ++name; @@ -179,8 +172,7 @@ RpcServerMap::match(const char *name, const char *pattern) void RpcServerMap::removeReservation(const char *name) { - delete _reservations.remove(name); + _reservations.erase(name); } - } // namespace slobrok diff --git a/slobrok/src/vespa/slobrok/server/rpc_server_map.h b/slobrok/src/vespa/slobrok/server/rpc_server_map.h index f25b9989378..4ebccd12b36 100644 --- a/slobrok/src/vespa/slobrok/server/rpc_server_map.h +++ b/slobrok/src/vespa/slobrok/server/rpc_server_map.h @@ -2,12 +2,14 @@ #pragma once #include "visible_map.h" +#include +#include +#include namespace slobrok { class NamedService; class ManagedRpcServer; -class RemoteRpcServer; class ReservedName; /** @@ -19,22 +21,17 @@ class ReservedName; * three seperate hashmaps. **/ -using vespalib::HashMap; - class RpcServerMap { private: - VisibleMap _visible_map; - - HashMap _myrpcsrv_map; - - HashMap _reservations; + using ManagedRpcServerMap = std::unordered_map>; + using ReservedNameMap = std::unordered_map>; + VisibleMap _visible_map; + ManagedRpcServerMap _myrpcsrv_map; + ReservedNameMap _reservations; static bool match(const char *name, const char *pattern); - RpcServerMap(const RpcServerMap &); // Not used - RpcServerMap &operator=(const RpcServerMap &); // Not use - void add(NamedService *rpcsrv); public: @@ -42,9 +39,7 @@ public: VisibleMap& visibleMap() { return _visible_map; } - ManagedRpcServer *lookupManaged(const char *name) const { - return _myrpcsrv_map[name]; - } + ManagedRpcServer *lookupManaged(const char *name) const; NamedService * lookup(const char *name) const; RpcSrvlist lookupPattern(const char *pattern) const; @@ -57,16 +52,12 @@ public: void addReservation(ReservedName *rpcsrv); bool conflictingReservation(const char *name, const char *spec); - ReservedName *getReservation(const char *name) const { - return _reservations[name]; - } + const ReservedName *getReservation(const char *name) const; void removeReservation(const char *name); - RpcServerMap() - : _myrpcsrv_map(NULL), - _reservations(NULL) - { - } + RpcServerMap(const RpcServerMap &) = delete; + RpcServerMap &operator=(const RpcServerMap &) = delete; + RpcServerMap(); ~RpcServerMap(); }; diff --git a/slobrok/src/vespa/slobrok/server/visible_map.cpp b/slobrok/src/vespa/slobrok/server/visible_map.cpp index d4487fd0884..4188d1603e3 100644 --- a/slobrok/src/vespa/slobrok/server/visible_map.cpp +++ b/slobrok/src/vespa/slobrok/server/visible_map.cpp @@ -87,7 +87,7 @@ VisibleMap::allVisible() const void VisibleMap::addNew(NamedService *rpcsrv) { - LOG_ASSERT(rpcsrv != NULL); + LOG_ASSERT(rpcsrv != nullptr); LOG_ASSERT(_map.isSet(rpcsrv->getName()) == false); _map.set(rpcsrv->getName(), rpcsrv); @@ -100,7 +100,7 @@ NamedService * VisibleMap::remove(const char *name) { NamedService *d = _map.remove(name); - if (d != NULL) { + if (d != nullptr) { _history.add(name, _genCnt); updated(); } @@ -110,10 +110,10 @@ VisibleMap::remove(const char *name) { NamedService * VisibleMap::update(NamedService *rpcsrv) { - LOG_ASSERT(rpcsrv != NULL); + LOG_ASSERT(rpcsrv != nullptr); NamedService *d = _map.remove(rpcsrv->getName()); - LOG_ASSERT(d != NULL); + LOG_ASSERT(d != nullptr); _map.set(rpcsrv->getName(), rpcsrv); @@ -133,7 +133,7 @@ VisibleMap::history(const vespalib::GenCnt& gen) const ++it) { const NamedService *val = lookup(it->c_str()); - if (val == NULL) { + if (val == nullptr) { retval.removed.push_back(*it); } else { retval.updated.push_back(val); @@ -142,11 +142,11 @@ VisibleMap::history(const vespalib::GenCnt& gen) const return retval; } -VisibleMap::MapDiff::MapDiff() {} -VisibleMap::MapDiff::~MapDiff() {} +VisibleMap::MapDiff::MapDiff() = default; +VisibleMap::MapDiff::~MapDiff() = default; VisibleMap::VisibleMap() - : _map(NULL), + : _map(nullptr), _waitList(), _genCnt(1) { @@ -160,8 +160,8 @@ VisibleMap::~VisibleMap() bool VisibleMap::match(const char *name, const char *pattern) { - LOG_ASSERT(name != NULL); - LOG_ASSERT(pattern != NULL); + LOG_ASSERT(name != nullptr); + LOG_ASSERT(pattern != nullptr); while (*pattern != '\0') { if (*name == *pattern) { ++name; diff --git a/slobrok/src/vespa/slobrok/server/visible_map.h b/slobrok/src/vespa/slobrok/server/visible_map.h index 9ad6c13872e..0bd809bf4ef 100644 --- a/slobrok/src/vespa/slobrok/server/visible_map.h +++ b/slobrok/src/vespa/slobrok/server/visible_map.h @@ -56,9 +56,6 @@ private: static bool match(const char *name, const char *pattern); - VisibleMap(const VisibleMap &); // Not used - VisibleMap &operator=(const VisibleMap &); // Not use - void updated(); void aborted(); @@ -80,6 +77,8 @@ public: MapDiff history(const vespalib::GenCnt& gen) const; + VisibleMap(const VisibleMap &) = delete; + VisibleMap &operator=(const VisibleMap &) = delete; VisibleMap(); ~VisibleMap(); }; -- cgit v1.2.3