Use unique_ptr to signal ownership correctly

8 files changed, 56 insertions, 59 deletions

diff --git a/slobrok/src/vespa/slobrok/server/remote_slobrok.cpp b/slobrok/src/vespa/slobrok/server/remote_slobrok.cpp
index 345a96ec9f1..d80faa695ca 100644
--- a/slobrok/src/vespa/slobrok/server/remote_slobrok.cpp
+++ b/slobrok/src/vespa/slobrok/server/remote_slobrok.cpp
@@ -79,7 +79,7 @@ RemoteSlobrok::doPending()
         NamedService *todo = _pending.front();
         _pending.pop_front();
 
-        NamedService *rpcsrv = _exchanger._rpcsrvmap.lookup(todo->getName());
+        const NamedService *rpcsrv = _exchanger._rpcsrvmap.lookup(todo->getName());
 
         if (rpcsrv == nullptr) {
             _remRemReq = getSupervisor()->AllocRPCRequest();
diff --git a/slobrok/src/vespa/slobrok/server/rpc_server_manager.cpp b/slobrok/src/vespa/slobrok/server/rpc_server_manager.cpp
index e18e13d8d59..ba7d94e3885 100644
--- a/slobrok/src/vespa/slobrok/server/rpc_server_manager.cpp
+++ b/slobrok/src/vespa/slobrok/server/rpc_server_manager.cpp
@@ -54,7 +54,7 @@ RpcServerManager::checkPartner(const std::string & remslobrok)
     if (remslobrok == _env.mySpec()) {
         return OkState(13, "remote slobrok using my rpcserver name");
     }
-    RemoteSlobrok *partner = _exchanger.lookupPartner(remslobrok);
+    const RemoteSlobrok *partner = _exchanger.lookupPartner(remslobrok);
     if (partner == nullptr) {
         return OkState(13, "remote slobrok not a partner");
     }
@@ -70,7 +70,7 @@ RpcServerManager::addRemReservation(const std::string & remslobrok, const std::s
     OkState valid = validateName(name);
     if (valid.failed()) return valid;
 
-    NamedService *old = _rpcsrvmap.lookupManaged(name);
+    const NamedService *old = _rpcsrvmap.lookupManaged(name);
     if (old != nullptr) {
         if (old->getSpec() == spec) {
             // was alright already
@@ -83,7 +83,7 @@ RpcServerManager::addRemReservation(const std::string & remslobrok, const std::s
     if (_rpcsrvmap.conflictingReservation(name, spec)) {
         return OkState(FRTE_RPC_METHOD_FAILED, "registration for name already in progress");
     }
-    _rpcsrvmap.addReservation(new ReservedName(name, spec, false));
+    _rpcsrvmap.addReservation(std::make_unique<ReservedName>(name, spec, false));
     return OkState(0, "done");
 }
 
@@ -104,7 +104,7 @@ RpcServerManager::removePeer(const std::string & remsbname, const std::string &
     if (remsbname == _env.mySpec()) {
         return OkState(13, "cannot remove my own rpcserver name");
     }
-    RemoteSlobrok *partner = _exchanger.lookupPartner(remsbname);
+    const RemoteSlobrok *partner = _exchanger.lookupPartner(remsbname);
     if (partner == nullptr) {
         return OkState(0, "remote slobrok not a partner");
     }
@@ -122,7 +122,7 @@ RpcServerManager::addMyReservation(const std::string & name, const std::string &
     OkState valid = validateName(name);
     if (valid.failed()) return valid;
 
-    NamedService *old = _rpcsrvmap.lookupManaged(name);
+    const NamedService *old = _rpcsrvmap.lookupManaged(name);
     if (old != nullptr) {
         if (old->getSpec() == spec) {
             // was alright already
@@ -143,8 +143,7 @@ RpcServerManager::addMyReservation(const std::string & name, const std::string &
                        "registration for name already in progress with a different spec");
     }
     _rpcsrvmap.removeReservation(name);
-    ReservedName *rpcsrv = new ReservedName(name, spec, true);
-    _rpcsrvmap.addReservation(rpcsrv);
+    _rpcsrvmap.addReservation(std::make_unique<ReservedName>(name, spec, true));
     return OkState(0, "done");
 }
 
@@ -158,7 +157,7 @@ RpcServerManager::addRemote(const std::string & name, const std::string &spec)
     if (alreadyManaged(name, spec)) {
         return OkState(0, "already correct");
     }
-    NamedService *old = _rpcsrvmap.lookup(name);
+    const NamedService *old = _rpcsrvmap.lookup(name);
     if (old != nullptr) {
         if (old->getSpec() !=  spec) {
             LOG(warning, "collision on remote add: name %s registered to %s locally, "
@@ -172,16 +171,17 @@ RpcServerManager::addRemote(const std::string & name, const std::string &spec)
         return OkState(0, "already correct");
     }
     _rpcsrvmap.removeReservation(name);
-    ManagedRpcServer *rpcsrv = new ManagedRpcServer(name, spec, *this);
-    _rpcsrvmap.addNew(rpcsrv);
-    rpcsrv->healthCheck();
+    auto rpcsrv = std::make_unique<ManagedRpcServer>(name, spec, *this);
+    ManagedRpcServer & rpcServer = *rpcsrv;
+    _rpcsrvmap.addNew(std::move(rpcsrv));
+    rpcServer.healthCheck();
     return OkState(0, "done");
 }
 
 OkState
 RpcServerManager::remove(ManagedRpcServer *rpcsrv)
 {
-    NamedService *td = _rpcsrvmap.lookup(rpcsrv->getName());
+    const NamedService *td = _rpcsrvmap.lookup(rpcsrv->getName());
     if (td == rpcsrv) {
         return removeLocal(rpcsrv->getName(), rpcsrv->getSpec());
     } else {
@@ -193,7 +193,7 @@ RpcServerManager::remove(ManagedRpcServer *rpcsrv)
 OkState
 RpcServerManager::removeRemote(const std::string &name, const std::string &spec)
 {
-    NamedService *old = _rpcsrvmap.lookup(name);
+    const NamedService *old = _rpcsrvmap.lookup(name);
     if (old == nullptr) {
         // was alright already, remove any reservation too
         _rpcsrvmap.removeReservation(name);
@@ -202,27 +202,26 @@ RpcServerManager::removeRemote(const std::string &name, const std::string &spec)
     if (old->getSpec() != spec) {
         return OkState(1, "name registered, but with different spec");
     }
-    NamedService *td = _rpcsrvmap.remove(name);
-    LOG_ASSERT(td == old);
-    delete td;
+    std::unique_ptr<NamedService> td = _rpcsrvmap.remove(name);
+    LOG_ASSERT(td.get() == old);
     return OkState(0, "done");
 }
 
 OkState
 RpcServerManager::removeLocal(const std::string & name, const std::string &spec)
 {
-    NamedService *td = _rpcsrvmap.lookup(name);
+    const NamedService *td = _rpcsrvmap.lookup(name);
     if (td == nullptr) {
         // already removed, nop
         return OkState();
     }
 
-    RemoteSlobrok *partner = _exchanger.lookupPartner(name);
+    const RemoteSlobrok *partner = _exchanger.lookupPartner(name);
     if (partner != nullptr) {
         return OkState(13, "cannot unregister partner slobrok");
     }
 
-    ManagedRpcServer *rpcsrv = _rpcsrvmap.lookupManaged(name);
+    const ManagedRpcServer *rpcsrv = _rpcsrvmap.lookupManaged(name);
     if (rpcsrv == nullptr) {
         return OkState(13, "not a local rpcserver");
     }
@@ -232,9 +231,8 @@ RpcServerManager::removeLocal(const std::string & name, const std::string &spec)
         // or log it on level INFO?
         return OkState(1, "name registered, but with different spec");
     }
-    td = _rpcsrvmap.remove(name);
-    LOG_ASSERT(td == rpcsrv);
-    delete rpcsrv;
+    auto tdUP = _rpcsrvmap.remove(name);
+    LOG_ASSERT(tdUP.get() == rpcsrv);
     _exchanger.forwardRemove(name, spec);
     return OkState();
 }
@@ -243,19 +241,20 @@ RpcServerManager::removeLocal(const std::string & name, const std::string &spec)
 void
 RpcServerManager::addManaged(const std::string &name, const std::string &spec, RegRpcSrvCommand rdc)
 {
-    ManagedRpcServer *rpcsrv = new ManagedRpcServer(name, spec, *this);
-    _rpcsrvmap.addNew(rpcsrv);
+    auto newRpcServer = std::make_unique<ManagedRpcServer>(name, spec, *this);
+    ManagedRpcServer & rpcsrv = *newRpcServer;
+    _rpcsrvmap.addNew(std::move(newRpcServer));
     for (size_t i = 0; i < _addManageds.size(); i++) {
         if (_addManageds[i].rpcsrv == nullptr) {
-            _addManageds[i].rpcsrv = rpcsrv;
+            _addManageds[i].rpcsrv = &rpcsrv;
             _addManageds[i].handler = rdc;
-            rpcsrv->healthCheck();
+            rpcsrv.healthCheck();
             return;
         }
     }
-    MRSandRRSC pair(rpcsrv, rdc);
+    MRSandRRSC pair(&rpcsrv, rdc);
     _addManageds.push_back(pair);
-    rpcsrv->healthCheck();
+    rpcsrv.healthCheck();
     return;
 }
 
@@ -264,7 +263,7 @@ RpcServerManager::addManaged(const std::string &name, const std::string &spec, R
 bool
 RpcServerManager::alreadyManaged(const std::string &name, const std::string &spec)
 {
-    ManagedRpcServer *rpcsrv = _rpcsrvmap.lookupManaged(name);
+    const ManagedRpcServer *rpcsrv = _rpcsrvmap.lookupManaged(name);
     if (rpcsrv != nullptr) {
         if (rpcsrv->getSpec() == spec) {
             return true;
@@ -297,9 +296,9 @@ RpcServerManager::notifyFailedRpcSrv(ManagedRpcServer *rpcsrv, std::string errms
 {
     _env.countFailedHeartbeat();
     bool logged = false;
-    NamedService *old = _rpcsrvmap.lookup(rpcsrv->getName());
+    const NamedService *old = _rpcsrvmap.lookup(rpcsrv->getName());
     if (old == rpcsrv) {
-        old = _rpcsrvmap.remove(rpcsrv->getName());
+        old = _rpcsrvmap.remove(rpcsrv->getName()).release();
         LOG_ASSERT(old == rpcsrv);
         LOG(info, "managed server %s at %s failed: %s",
             rpcsrv->getName().c_str(), rpcsrv->getSpec().c_str(), errmsg.c_str());
diff --git a/slobrok/src/vespa/slobrok/server/rpc_server_map.cpp b/slobrok/src/vespa/slobrok/server/rpc_server_map.cpp
index 34f8f7b9135..e19a91cd78d 100644
--- a/slobrok/src/vespa/slobrok/server/rpc_server_map.cpp
+++ b/slobrok/src/vespa/slobrok/server/rpc_server_map.cpp
@@ -18,19 +18,19 @@ RpcServerMap::lookupManaged(const std::string & name) const {
     return (found == _myrpcsrv_map.end()) ? nullptr : found->second.get();
 }
 
-NamedService *
+const NamedService *
 RpcServerMap::lookup(const std::string & name) const
 {
     return lookupManaged(name);
 }
 
-NamedService *
+std::unique_ptr<NamedService>
 RpcServerMap::remove(const std::string & name)
 {
     _visible_map.remove(name);
     auto service = std::move(_myrpcsrv_map[name]);
     _myrpcsrv_map.erase(name);
-    return service.release();
+    return service;
 }
 
 std::vector<const NamedService *>
@@ -73,7 +73,7 @@ RpcServerMap::add(NamedService *rpcsrv)
 }
 
 void
-RpcServerMap::addNew(ManagedRpcServer *rpcsrv)
+RpcServerMap::addNew(std::unique_ptr<ManagedRpcServer> rpcsrv)
 {
     const std::string &name = rpcsrv->getName();
 
@@ -102,13 +102,13 @@ RpcServerMap::addNew(ManagedRpcServer *rpcsrv)
             }
         }
     }
-    add(rpcsrv);
-    _myrpcsrv_map[name].reset(rpcsrv);
+    add(rpcsrv.get());
+    _myrpcsrv_map[name] = std::move(rpcsrv);
 }
 
 
 void
-RpcServerMap::addReservation(ReservedName *rpcsrv)
+RpcServerMap::addReservation(std::unique_ptr<ReservedName> rpcsrv)
 {
     LOG_ASSERT(rpcsrv != nullptr);
     LOG_ASSERT(_myrpcsrv_map.find(rpcsrv->getName()) == _myrpcsrv_map.end());
@@ -117,10 +117,10 @@ RpcServerMap::addReservation(ReservedName *rpcsrv)
     // this should have been checked already, so assert
     LOG_ASSERT(! conflictingReservation(rpcsrv->getName(), rpcsrv->getSpec()));
     auto old = std::move(_reservations[rpcsrv->getName()]);
-    _reservations[rpcsrv->getName()].reset(rpcsrv);
     LOG_ASSERT(!old
                || old->getSpec() == rpcsrv->getSpec()
                || ! old->stillReserved());
+    _reservations[rpcsrv->getName()] = std::move(rpcsrv);
 }
 
 
diff --git a/slobrok/src/vespa/slobrok/server/rpc_server_map.h b/slobrok/src/vespa/slobrok/server/rpc_server_map.h
index d458df6cb1b..2c69c1b8cde 100644
--- a/slobrok/src/vespa/slobrok/server/rpc_server_map.h
+++ b/slobrok/src/vespa/slobrok/server/rpc_server_map.h
@@ -38,15 +38,14 @@ public:
 
     ManagedRpcServer *lookupManaged(const std::string & name) const;
 
-    NamedService *    lookup(const std::string & name) const;
+    const NamedService *    lookup(const std::string & name) const;
     RpcSrvlist        lookupPattern(const char *pattern) const;
-    RpcSrvlist        allVisible() const;
     RpcSrvlist        allManaged() const;
 
-    void              addNew(ManagedRpcServer *rpcsrv);
-    NamedService *    remove(const std::string & name);
+    void              addNew(std::unique_ptr<ManagedRpcServer> rpcsrv);
+    std::unique_ptr<NamedService> remove(const std::string & name);
 
-    void          addReservation(ReservedName *rpcsrv);
+    void          addReservation(std::unique_ptr<ReservedName>rpcsrv);
     bool          conflictingReservation(const std::string & name, const std::string &spec);
 
     const ReservedName *getReservation(const std::string & name) const;
diff --git a/slobrok/src/vespa/slobrok/server/rpchooks.cpp b/slobrok/src/vespa/slobrok/server/rpchooks.cpp
index 133de63c895..9abbf6ab3fe 100644
--- a/slobrok/src/vespa/slobrok/server/rpchooks.cpp
+++ b/slobrok/src/vespa/slobrok/server/rpchooks.cpp
@@ -466,7 +466,7 @@ RPCHooks::rpc_lookupManaged(FRT_RPCRequest *req)
     FRT_Values &args = *req->GetParams();
     const char *name = args[0]._string._str;
     LOG(debug, "RPC: lookupManaged(%s)", name);
-    ManagedRpcServer *found = _rpcsrvmap.lookupManaged(name);
+    const ManagedRpcServer *found = _rpcsrvmap.lookupManaged(name);
 
     if (found == nullptr) {
         req->SetError(FRTE_RPC_METHOD_FAILED, "Not found");
diff --git a/slobrok/src/vespa/slobrok/server/selfcheck.cpp b/slobrok/src/vespa/slobrok/server/selfcheck.cpp
index a5e6870882e..7fc639ec028 100644
--- a/slobrok/src/vespa/slobrok/server/selfcheck.cpp
+++ b/slobrok/src/vespa/slobrok/server/selfcheck.cpp
@@ -51,5 +51,4 @@ SelfCheck::PerformTask()
     Schedule(seconds);
 }
 
-
 } // namespace slobrok
diff --git a/slobrok/src/vespa/slobrok/server/visible_map.cpp b/slobrok/src/vespa/slobrok/server/visible_map.cpp
index a9257dbb391..d26bb49d874 100644
--- a/slobrok/src/vespa/slobrok/server/visible_map.cpp
+++ b/slobrok/src/vespa/slobrok/server/visible_map.cpp
@@ -54,7 +54,7 @@ VisibleMap::removeUpdateListener(IUpdateListener *l)
 
 //-----------------------------------------------------------------------------
 
-NamedService *
+const NamedService *
 VisibleMap::lookup(const std::string &name) const {
     auto found = _map.find(name);
     return (found == _map.end()) ? nullptr : found->second;
@@ -74,7 +74,7 @@ VisibleMap::allVisible() const
 
 
 void
-VisibleMap::addNew(NamedService *rpcsrv)
+VisibleMap::addNew(const NamedService *rpcsrv)
 {
     LOG_ASSERT(rpcsrv != nullptr);
     LOG_ASSERT(_map.find(rpcsrv->getName()) == _map.end());
@@ -85,10 +85,10 @@ VisibleMap::addNew(NamedService *rpcsrv)
 }
 
 
-NamedService *
+const NamedService *
 VisibleMap::remove(const std::string &name) {
 
-    NamedService *d = _map[name];
+    const NamedService *d = _map[name];
     _map.erase(name);
     if (d != nullptr) {
         _history.add(name, _genCnt);
@@ -98,11 +98,11 @@ VisibleMap::remove(const std::string &name) {
 }
 
 
-NamedService *
-VisibleMap::update(NamedService *rpcsrv) {
+const NamedService *
+VisibleMap::update(const NamedService *rpcsrv) {
     LOG_ASSERT(rpcsrv != nullptr);
 
-    NamedService *d = rpcsrv;
+    const NamedService *d = rpcsrv;
     std::swap(d, _map[rpcsrv->getName()]);
     LOG_ASSERT(d != nullptr);
 
diff --git a/slobrok/src/vespa/slobrok/server/visible_map.h b/slobrok/src/vespa/slobrok/server/visible_map.h
index 5d2440cd176..6ea2d5e2a46 100644
--- a/slobrok/src/vespa/slobrok/server/visible_map.h
+++ b/slobrok/src/vespa/slobrok/server/visible_map.h
@@ -45,7 +45,7 @@ public:
     };
 
 private:
-    using Map = std::unordered_map<std::string, NamedService *>;
+    using Map = std::unordered_map<std::string, const NamedService *>;
     using WaitList = std::vector<IUpdateListener *>;
 
     Map              _map;
@@ -62,11 +62,11 @@ public:
     void addUpdateListener(IUpdateListener *l);
     void removeUpdateListener(IUpdateListener *l);
 
-    void       addNew(NamedService *rpcsrv);
-    NamedService *remove(const std::string &name);
-    NamedService *update(NamedService *rpcsrv);
+    void       addNew(const NamedService *rpcsrv);
+    const NamedService *remove(const std::string &name);
+    const NamedService *update(const NamedService *rpcsrv);
 
-    NamedService *lookup(const std::string &name) const;
+    const NamedService *lookup(const std::string &name) const;
     RpcSrvlist allVisible() const;
 
     const vespalib::GenCnt& genCnt() { return _genCnt; }