From 6614009474c5b9ea24ccd35da461f67346c00831 Mon Sep 17 00:00:00 2001 From: Guillaume Roguez Date: Wed, 11 Jan 2017 10:43:33 -0500 Subject: [PATCH] fix bad usage of shared_ptr as function argument Many function/method pass shared_ptr by copy and not by (const) reference. As explained here [1], if the smast pointer is not supposed to be modified and used as an output argument, it's a bad usage as we cause an uneeded copy. As this copy causes a memory lock this lead to CPU and memory cache lock bad usage. This patch converts all these by-value into by-const-reference on all shared_ptr arguments over the daemon sources. [1] https://herbsutter.com/2013/06/05/gotw-91-solution-smart-pointer-parameters/ Change-Id: Ic1e68d93a6917f5e2865b6818b7c1210ff98b1a8 --- src/account.cpp | 4 ++-- src/call.cpp | 2 +- src/call.h | 2 +- src/dring/dring.h | 2 +- src/manager.cpp | 4 ++-- src/manager.h | 4 ++-- src/media/audio/ringbufferpool.cpp | 4 ++-- src/media/audio/ringbufferpool.h | 4 ++-- src/media/video/sinkclient.cpp | 2 +- src/media/video/sinkclient.h | 2 +- src/media/video/video_base.h | 2 +- src/media/video/video_mixer.cpp | 2 +- src/media/video/video_mixer.h | 2 +- src/media/video/video_sender.cpp | 2 +- src/media/video/video_sender.h | 2 +- src/ringdht/namedirectory.cpp | 12 ++++++------ src/ringdht/ringaccount.cpp | 18 +++++++++--------- src/ringdht/ringaccount.h | 8 ++++---- src/security/certstore.cpp | 6 +++--- src/security/certstore.h | 4 ++-- src/security/tls_session.cpp | 2 +- src/security/tls_session.h | 2 +- src/sip/sipcall.cpp | 4 ++-- src/sip/sipcall.h | 6 +++--- src/sip/siptransport.cpp | 4 ++-- src/sip/siptransport.h | 4 ++-- 26 files changed, 55 insertions(+), 55 deletions(-) diff --git a/src/account.cpp b/src/account.cpp index 526ef58b6..244d47059 100644 --- a/src/account.cpp +++ b/src/account.cpp @@ -331,8 +331,8 @@ Account::setActiveCodecs(const std::vector& list) std::sort(std::begin(accountCodecInfoList_), std::end (accountCodecInfoList_), - [](std::shared_ptr a, - std::shared_ptr b) { + [](const std::shared_ptr& a, + const std::shared_ptr& b) { return a->order < b->order; }); diff --git a/src/call.cpp b/src/call.cpp index e7959cfd8..d98aac8df 100644 --- a/src/call.cpp +++ b/src/call.cpp @@ -464,7 +464,7 @@ Call::addSubCall(const std::shared_ptr& call) } void -Call::merge(std::shared_ptr scall) +Call::merge(const std::shared_ptr& scall) { RING_WARN("[call:%s] merge to -> [call:%s]", scall->getCallId().c_str(), getCallId().c_str()); auto& call = *scall; diff --git a/src/call.h b/src/call.h index 0a47ccc7c..b139f4a81 100644 --- a/src/call.h +++ b/src/call.h @@ -344,7 +344,7 @@ class Call : public Recordable, public std::enable_shared_from_this { } void addSubCall(const std::shared_ptr& call); - virtual void merge(std::shared_ptr scall); + virtual void merge(const std::shared_ptr& scall); protected: /** diff --git a/src/dring/dring.h b/src/dring/dring.h index 5db5255c3..1662cc9d3 100644 --- a/src/dring/dring.h +++ b/src/dring/dring.h @@ -118,7 +118,7 @@ class CallbackWrapper : public CallbackWrapperBase { // Create and initialize a wrapper from a generic CallbackWrapperBase // shared pointer. // Note: the given callback is copied into internal storage. - CallbackWrapper(std::shared_ptr p) noexcept { + CallbackWrapper(const std::shared_ptr& p) noexcept { if (p) cb_ = ((CallbackWrapper*)p.get())->cb_; } diff --git a/src/manager.cpp b/src/manager.cpp index 7890e99be..c67858f57 100644 --- a/src/manager.cpp +++ b/src/manager.cpp @@ -453,7 +453,7 @@ Manager::switchCall(const std::string& id) } void -Manager::switchCall(std::shared_ptr call) +Manager::switchCall(const std::shared_ptr& call) { switchCall(call->getCallId()); } @@ -1445,7 +1445,7 @@ Manager::scheduleTask(const std::function&& task, std::chrono::steady_cl void -Manager::scheduleTask(std::shared_ptr task, std::chrono::steady_clock::time_point when) +Manager::scheduleTask(const std::shared_ptr& task, std::chrono::steady_clock::time_point when) { std::lock_guard lock(scheduledTasksMutex_); scheduledTasks_.emplace(when, task); diff --git a/src/manager.h b/src/manager.h index b8461425e..aaaba6e92 100644 --- a/src/manager.h +++ b/src/manager.h @@ -809,7 +809,7 @@ class Manager { void unsetCurrentCall(); void switchCall(const std::string& id); - void switchCall(std::shared_ptr call); + void switchCall(const std::shared_ptr& call); /** Application wide tone controler */ ToneControl toneCtrl_; @@ -998,7 +998,7 @@ class Manager { Runnable(const std::function&& t) : cb(std::move(t)) {} }; std::shared_ptr scheduleTask(const std::function&& task, std::chrono::steady_clock::time_point when); - void scheduleTask(std::shared_ptr task, std::chrono::steady_clock::time_point when); + void scheduleTask(const std::shared_ptr& task, std::chrono::steady_clock::time_point when); #ifdef RING_VIDEO /** diff --git a/src/media/audio/ringbufferpool.cpp b/src/media/audio/ringbufferpool.cpp index e5da15631..25ce9c188 100644 --- a/src/media/audio/ringbufferpool.cpp +++ b/src/media/audio/ringbufferpool.cpp @@ -142,7 +142,7 @@ RingBufferPool::removeReadBindings(const std::string& call_id) * Make given call ID a reader of given ring buffer */ void -RingBufferPool::addReaderToRingBuffer(std::shared_ptr rbuf, +RingBufferPool::addReaderToRingBuffer(const std::shared_ptr& rbuf, const std::string& call_id) { if (call_id != DEFAULT_ID and rbuf->id == call_id) @@ -154,7 +154,7 @@ RingBufferPool::addReaderToRingBuffer(std::shared_ptr rbuf, } void -RingBufferPool::removeReaderFromRingBuffer(std::shared_ptr rbuf, +RingBufferPool::removeReaderFromRingBuffer(const std::shared_ptr& rbuf, const std::string& call_id) { if (auto bindings = getReadBindings(call_id)) { diff --git a/src/media/audio/ringbufferpool.h b/src/media/audio/ringbufferpool.h index 4625391a8..1870970be 100644 --- a/src/media/audio/ringbufferpool.h +++ b/src/media/audio/ringbufferpool.h @@ -135,10 +135,10 @@ class RingBufferPool { void removeReadBindings(const std::string& call_id); - void addReaderToRingBuffer(std::shared_ptr rbuf, + void addReaderToRingBuffer(const std::shared_ptr& rbuf, const std::string& call_id); - void removeReaderFromRingBuffer(std::shared_ptr rbuf, + void removeReaderFromRingBuffer(const std::shared_ptr& rbuf, const std::string& call_id); // A cache of created RingBuffers listed by IDs. diff --git a/src/media/video/sinkclient.cpp b/src/media/video/sinkclient.cpp index a3699b43c..d8737723c 100644 --- a/src/media/video/sinkclient.cpp +++ b/src/media/video/sinkclient.cpp @@ -314,7 +314,7 @@ SinkClient::SinkClient(const std::string& id, bool mixer) void SinkClient::update(Observable>* /*obs*/, - std::shared_ptr frame_p) + const std::shared_ptr& frame_p) { auto& f = *frame_p; diff --git a/src/media/video/sinkclient.h b/src/media/video/sinkclient.h index f239af53d..0039220bc 100644 --- a/src/media/video/sinkclient.h +++ b/src/media/video/sinkclient.h @@ -56,7 +56,7 @@ class SinkClient : public VideoFramePassiveReader // as VideoFramePassiveReader void update(Observable>*, - std::shared_ptr) override; + const std::shared_ptr&) override; bool start() noexcept; bool stop() noexcept; diff --git a/src/media/video/video_base.h b/src/media/video/video_base.h index 23473ef6e..22abb0cfc 100644 --- a/src/media/video/video_base.h +++ b/src/media/video/video_base.h @@ -116,7 +116,7 @@ class Observer { public: virtual ~Observer() {}; - virtual void update(Observable*, T) = 0; + virtual void update(Observable*, const T&) = 0; virtual void attached(Observable*) {}; virtual void detached(Observable*) {}; }; diff --git a/src/media/video/video_mixer.cpp b/src/media/video/video_mixer.cpp index 335d08c21..3559dc2a0 100644 --- a/src/media/video/video_mixer.cpp +++ b/src/media/video/video_mixer.cpp @@ -101,7 +101,7 @@ VideoMixer::detached(Observable>* ob) void VideoMixer::update(Observable>* ob, - std::shared_ptr frame_p) + const std::shared_ptr& frame_p) { auto lock(rwMutex_.read()); diff --git a/src/media/video/video_mixer.h b/src/media/video/video_mixer.h index db791644b..ed037a048 100644 --- a/src/media/video/video_mixer.h +++ b/src/media/video/video_mixer.h @@ -49,7 +49,7 @@ public: int getPixelFormat() const override; // as VideoFramePassiveReader - void update(Observable>* ob, std::shared_ptr v) override; + void update(Observable>* ob, const std::shared_ptr& v) override; void attached(Observable>* ob) override; void detached(Observable>* ob) override; diff --git a/src/media/video/video_sender.cpp b/src/media/video/video_sender.cpp index 42914c2b8..d9531c01f 100644 --- a/src/media/video/video_sender.cpp +++ b/src/media/video/video_sender.cpp @@ -75,7 +75,7 @@ VideoSender::encodeAndSendVideo(VideoFrame& input_frame) void VideoSender::update(Observable>* /*obs*/, - std::shared_ptr frame_p) + const std::shared_ptr& frame_p) { encodeAndSendVideo(*frame_p); } diff --git a/src/media/video/video_sender.h b/src/media/video/video_sender.h index f0019b4a7..151a8c68b 100644 --- a/src/media/video/video_sender.h +++ b/src/media/video/video_sender.h @@ -54,7 +54,7 @@ public: // as VideoFramePassiveReader void update(Observable>* obs, - std::shared_ptr frame_p) override; + const std::shared_ptr& frame_p) override; void setMuted(bool isMuted); uint16_t getLastSeqValue(); diff --git a/src/ringdht/namedirectory.cpp b/src/ringdht/namedirectory.cpp index 09a633aaf..f40b2539d 100644 --- a/src/ringdht/namedirectory.cpp +++ b/src/ringdht/namedirectory.cpp @@ -113,8 +113,8 @@ void NameDirectory::lookupAddress(const std::string& addr, LookupCallback cb) RING_DBG("Address lookup for %s: %s", addr.c_str(), uri.to_string().c_str()); - auto ret = restbed::Http::async(req, [this,cb,addr](const std::shared_ptr, - const std::shared_ptr reply) { + auto ret = restbed::Http::async(req, [this,cb,addr](const std::shared_ptr&, + const std::shared_ptr& reply) { if (reply->get_status_code() == 200) { size_t length = getContentLength(*reply); if (length > MAX_RESPONSE_SIZE) { @@ -180,8 +180,8 @@ void NameDirectory::lookupName(const std::string& n, LookupCallback cb) RING_DBG("Name lookup for %s: %s", name.c_str(), uri.to_string().c_str()); - auto ret = restbed::Http::async(request, [this,cb,name](const std::shared_ptr, - const std::shared_ptr reply) { + auto ret = restbed::Http::async(request, [this,cb,name](const std::shared_ptr&, + const std::shared_ptr& reply) { auto code = reply->get_status_code(); if (code != 200) RING_DBG("Name lookup for %s: got reply code %d", name.c_str(), code); @@ -272,8 +272,8 @@ void NameDirectory::registerName(const std::string& addr, const std::string& n, RING_WARN("registerName: sending request %s %s", addr.c_str(), name.c_str()); auto ret = restbed::Http::async(request, - [this,cb,addr,name](const std::shared_ptr, - const std::shared_ptr reply) + [this,cb,addr,name](const std::shared_ptr&, + const std::shared_ptr& reply) { auto code = reply->get_status_code(); RING_DBG("Got reply for registration of %s -> %s: code %d", name.c_str(), addr.c_str(), code); diff --git a/src/ringdht/ringaccount.cpp b/src/ringdht/ringaccount.cpp index ffc9e0fb5..5c64f10a8 100644 --- a/src/ringdht/ringaccount.cpp +++ b/src/ringdht/ringaccount.cpp @@ -1172,7 +1172,7 @@ RingAccount::loadAccountFromDHT(const std::string& archive_password, const std:: std::tie(key, loc) = computeKeys(archive_password, archive_pin, previous); RING_DBG("Trying to load account from DHT with %s at %s", archive_pin.c_str(), loc.toString().c_str()); if (auto this_ = w.lock()) { - this_->dht_.get(loc, [w,key,found,archive_password,archiveFound](std::shared_ptr val) { + this_->dht_.get(loc, [w,key,found,archive_password,archiveFound](const std::shared_ptr& val) { std::vector decrypted; try { decrypted = dht::crypto::aesDecrypt(val->data, key); @@ -1961,7 +1961,7 @@ RingAccount::doRegister_() for (const auto& crl : identity_.second->issuer->getRevocationLists()) dht_.put(h, crl, dht::DoneCallback{}, {}, true); dht_.listen(h, [shared](DeviceAnnouncement&& dev) { - shared->findCertificate(dev.dev, [shared](const std::shared_ptr crt) { + shared->findCertificate(dev.dev, [shared](const std::shared_ptr& crt) { shared->foundAccountDevice(crt); }); return true; @@ -2014,7 +2014,7 @@ RingAccount::doRegister_() if (v.service != DHT_TYPE_NS) return true; - shared->findCertificate(v.from, [shared, v](const std::shared_ptr cert) mutable { + shared->findCertificate(v.from, [shared, v](const std::shared_ptr& cert) mutable { auto& this_ = *shared.get(); // check peer certificate @@ -2140,7 +2140,7 @@ RingAccount::onPeerMessage(const dht::InfoHash& peer_device, std::function from_cert) +RingAccount::incomingCall(dht::IceCandidates&& msg, const std::shared_ptr& from_cert) { RING_WARN("ICE incoming from DHT peer %s", msg.from.toString().c_str()); auto call = Manager::instance().callFactory.newCall(*this, Manager::instance().getNewCallID(), Call::CallType::INCOMING); @@ -2240,10 +2240,10 @@ RingAccount::foundPeerDevice(const std::shared_ptr& cr } void -RingAccount::replyToIncomingIceMsg(std::shared_ptr call, - std::shared_ptr ice, - const dht::IceCandidates& peer_ice_msg, - std::shared_ptr peer_cert) +RingAccount::replyToIncomingIceMsg(const std::shared_ptr& call, + const std::shared_ptr& ice, + const dht::IceCandidates& peer_ice_msg, + const std::shared_ptr& peer_cert) { registerDhtAddress(*ice); @@ -2819,7 +2819,7 @@ RingAccount::onReceiveDeviceSync(DeviceSync&& sync) RING_WARN("Received device sync data %zu", sync.devices_known.size()); for (const auto& d : sync.devices_known) { auto shared = std::static_pointer_cast(shared_from_this()); - findCertificate(d.first, [shared,d](const std::shared_ptr crt) { + findCertificate(d.first, [shared,d](const std::shared_ptr& crt) { if (not crt) return; shared->foundAccountDevice(crt, d.second); diff --git a/src/ringdht/ringaccount.h b/src/ringdht/ringaccount.h index 0316799a7..768faea65 100644 --- a/src/ringdht/ringaccount.h +++ b/src/ringdht/ringaccount.h @@ -356,7 +356,7 @@ class RingAccount : public SIPAccountBase { void onTrackedBuddyOnline(std::map::iterator& buddy_info_it, const dht::InfoHash& device_id); void doRegister_(); - void incomingCall(dht::IceCandidates&& msg, std::shared_ptr from); + void incomingCall(dht::IceCandidates&& msg, const std::shared_ptr& from); const dht::ValueType USER_PROFILE_TYPE = {9, "User profile", std::chrono::hours(24 * 7)}; @@ -497,10 +497,10 @@ class RingAccount : public SIPAccountBase { void loadKnownDevices(); void saveKnownDevices() const; - void replyToIncomingIceMsg(std::shared_ptr, - std::shared_ptr, + void replyToIncomingIceMsg(const std::shared_ptr&, + const std::shared_ptr&, const dht::IceCandidates&, - std::shared_ptr); + const std::shared_ptr&); static tls::DhParams loadDhParams(const std::string path); diff --git a/src/security/certstore.cpp b/src/security/certstore.cpp index dc22ff45d..8dd1fb88b 100644 --- a/src/security/certstore.cpp +++ b/src/security/certstore.cpp @@ -142,7 +142,7 @@ CertificateStore::findCertificateByUID(const std::string& uid) const } std::shared_ptr -CertificateStore::findIssuer(std::shared_ptr crt) const +CertificateStore::findIssuer(const std::shared_ptr& crt) const { std::shared_ptr ret {}; auto n = crt->getIssuerUID(); @@ -260,13 +260,13 @@ CertificateStore::pinCertificate(crypto::Certificate&& cert, bool local) } std::vector -CertificateStore::pinCertificate(std::shared_ptr cert, bool local) +CertificateStore::pinCertificate(const std::shared_ptr& cert, bool local) { bool sig {false}; std::vector ids {}; { - std::lock_guard l(lock_); auto c = cert; + std::lock_guard l(lock_); while (c) { bool inserted; auto id = c->getId().toString(); diff --git a/src/security/certstore.h b/src/security/certstore.h index 1ded45ea3..f8591916e 100644 --- a/src/security/certstore.h +++ b/src/security/certstore.h @@ -61,11 +61,11 @@ public: std::shared_ptr findCertificateByName(const std::string& name, crypto::Certificate::NameType type = crypto::Certificate::NameType::UNKNOWN) const; std::shared_ptr findCertificateByUID(const std::string& uid) const; - std::shared_ptr findIssuer(std::shared_ptr crt) const; + std::shared_ptr findIssuer(const std::shared_ptr& crt) const; std::vector pinCertificate(const std::vector& crt, bool local = true) noexcept; std::vector pinCertificate(crypto::Certificate&& crt, bool local = true); - std::vector pinCertificate(std::shared_ptr crt, bool local = true); + std::vector pinCertificate(const std::shared_ptr& crt, bool local = true); bool unpinCertificate(const std::string&); void pinCertificatePath(const std::string& path, std::function&)> cb = {}); diff --git a/src/security/tls_session.cpp b/src/security/tls_session.cpp index 1bff93402..b4f1dcf3c 100644 --- a/src/security/tls_session.cpp +++ b/src/security/tls_session.cpp @@ -152,7 +152,7 @@ private: T creds_; }; -TlsSession::TlsSession(std::shared_ptr ice, int ice_comp_id, +TlsSession::TlsSession(const std::shared_ptr& ice, int ice_comp_id, const TlsParams& params, const TlsSessionCallbacks& cbs, bool anonymous) : socket_(new IceSocket(ice, ice_comp_id)) , isServer_(not ice->isInitiator()) diff --git a/src/security/tls_session.h b/src/security/tls_session.h index 3f1794eeb..28e905d92 100644 --- a/src/security/tls_session.h +++ b/src/security/tls_session.h @@ -136,7 +136,7 @@ public: VerifyCertificate verifyCertificate; }; - TlsSession(std::shared_ptr ice, int ice_comp_id, const TlsParams& params, + TlsSession(const std::shared_ptr& ice, int ice_comp_id, const TlsParams& params, const TlsSessionCallbacks& cbs, bool anonymous=true); ~TlsSession(); diff --git a/src/sip/sipcall.cpp b/src/sip/sipcall.cpp index 19ae3e81f..e1b7c94f2 100644 --- a/src/sip/sipcall.cpp +++ b/src/sip/sipcall.cpp @@ -177,7 +177,7 @@ void SIPCall::setContactHeader(pj_str_t *contact) } void -SIPCall::setTransport(std::shared_ptr t) +SIPCall::setTransport(const std::shared_ptr& t) { if (isSecure() and t and not t->isSecure()) { RING_ERR("Can't set unsecure transport to secure call."); @@ -1118,7 +1118,7 @@ SIPCall::initIceTransport(bool master, unsigned channel_num) } void -SIPCall::merge(std::shared_ptr scall) +SIPCall::merge(const std::shared_ptr& scall) { RING_WARN("SIPCall::merge %s -> %s", scall->getCallId().c_str(), getCallId().c_str()); inv = std::move(scall->inv); diff --git a/src/sip/sipcall.h b/src/sip/sipcall.h index 733fc783c..c7542f6ff 100644 --- a/src/sip/sipcall.h +++ b/src/sip/sipcall.h @@ -128,7 +128,7 @@ class SIPCall : public Call void setContactHeader(pj_str_t *contact); - void setTransport(std::shared_ptr t); + void setTransport(const std::shared_ptr& t); inline SipTransport* getTransport() { return transport_.get(); @@ -215,10 +215,10 @@ class SIPCall : public Call void terminateSipSession(int status); - virtual void merge(std::shared_ptr scall) { + virtual void merge(const std::shared_ptr& scall) { merge(std::dynamic_pointer_cast(scall)); } - virtual void merge(std::shared_ptr scall); + virtual void merge(const std::shared_ptr& scall); void setPeerRegistredName(const std::string& name) { peerRegistredName_ = name; diff --git a/src/sip/siptransport.cpp b/src/sip/siptransport.cpp index 47463c708..c0860ae1b 100644 --- a/src/sip/siptransport.cpp +++ b/src/sip/siptransport.cpp @@ -418,7 +418,7 @@ SipTransportBroker::getTlsTransport(const std::shared_ptr& l, const } std::shared_ptr -SipTransportBroker::getIceTransport(const std::shared_ptr ice, +SipTransportBroker::getIceTransport(const std::shared_ptr& ice, unsigned comp_id) { auto sip_ice_tr = std::unique_ptr( @@ -437,7 +437,7 @@ SipTransportBroker::getIceTransport(const std::shared_ptr ice, } std::shared_ptr -SipTransportBroker::getTlsIceTransport(const std::shared_ptr ice, +SipTransportBroker::getTlsIceTransport(const std::shared_ptr& ice, unsigned comp_id, const tls::TlsParams& params) { diff --git a/src/sip/siptransport.h b/src/sip/siptransport.h index 91960372f..0605719a2 100644 --- a/src/sip/siptransport.h +++ b/src/sip/siptransport.h @@ -176,10 +176,10 @@ public: getTlsTransport(const std::shared_ptr&, const IpAddr& remote, const std::string& remote_name = {}); std::shared_ptr - getIceTransport(const std::shared_ptr, unsigned comp_id); + getIceTransport(const std::shared_ptr&, unsigned comp_id); std::shared_ptr - getTlsIceTransport(const std::shared_ptr, unsigned comp_id, + getTlsIceTransport(const std::shared_ptr&, unsigned comp_id, const tls::TlsParams&); std::shared_ptr addTransport(pjsip_transport*); -- 2.22.0