From 40868b7b8b01caa5d51ca6218eb2de0624baaa1e Mon Sep 17 00:00:00 2001 From: Michael Davidsaver Date: Tue, 6 Feb 2018 18:15:13 -0800 Subject: [PATCH 01/17] cleanup --- src/remote/transportRegistry.cpp | 22 ---------------------- 1 file changed, 22 deletions(-) diff --git a/src/remote/transportRegistry.cpp b/src/remote/transportRegistry.cpp index 62dfb45..74fafd1 100644 --- a/src/remote/transportRegistry.cpp +++ b/src/remote/transportRegistry.cpp @@ -74,28 +74,6 @@ TransportRegistry::~TransportRegistry() if(!transports.empty()) LOG(logLevelWarn, "TransportRegistry destroyed while not empty"); } -/* -void -TransportRegistry::get(const osiSockAddr* address, transportVector_t& output) -{ - pvd::Lock guard(_mutex); - transportsMap_t::iterator transportsIter = _transports.find(address); - if(transportsIter != _transports.end()) - { - prioritiesMapSharedPtr_t& priorities = transportsIter->second; - - size_t i = output.size(); - output.resize(output.size() + priorities->size()); - - for(prioritiesMap_t::iterator prioritiesIter = priorities->begin(); - prioritiesIter != priorities->end(); - prioritiesIter++, i++) - { - output[i] = prioritiesIter->second; - } - } -} -*/ Transport::shared_pointer TransportRegistry::get(const osiSockAddr& address, epics::pvData::int16 prio) { From 24ffccf90dc751719d722009124238267ce3411d Mon Sep 17 00:00:00 2001 From: Michael Davidsaver Date: Tue, 6 Feb 2018 18:20:33 -0800 Subject: [PATCH 02/17] better TransportRegistry::remove() now with the same complexity as the original... --- src/remote/transportRegistry.cpp | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/src/remote/transportRegistry.cpp b/src/remote/transportRegistry.cpp index 74fafd1..952a0fa 100644 --- a/src/remote/transportRegistry.cpp +++ b/src/remote/transportRegistry.cpp @@ -102,19 +102,14 @@ void TransportRegistry::install(const Transport::shared_pointer& ptr) Transport::shared_pointer TransportRegistry::remove(Transport::shared_pointer const & transport) { assert(!!transport); + const Key key(transport->getRemoteAddress(), transport->getPriority()); Transport::shared_pointer ret; pvd::Lock guard(_mutex); - for(transports_t::iterator it(transports.begin()), end(transports.end()); - it != end; ++it) - { - Transport::shared_pointer& tr = it->second; - - if(transport.get() == tr.get()) { - ret.swap(it->second); - transports.erase(it); - break; - } + transports_t::iterator it(transports.find(key)); + if(it!=transports.end()) { + ret.swap(it->second); + transports.erase(it); } return ret; } From e8347f17b4170274447cb313bcfcfcccb1b48602 Mon Sep 17 00:00:00 2001 From: Michael Davidsaver Date: Wed, 7 Feb 2018 20:16:38 -0800 Subject: [PATCH 03/17] minor --- src/remote/codec.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/remote/codec.cpp b/src/remote/codec.cpp index 3ed6b93..a8f1021 100644 --- a/src/remote/codec.cpp +++ b/src/remote/codec.cpp @@ -1586,7 +1586,7 @@ void BlockingServerTCPTransportCodec::destroyAllChannels() { _socketName.c_str(), _channels.size()); } - std::map::iterator it = _channels.begin(); + _channels_t::iterator it(_channels.begin()); for(; it!=_channels.end(); it++) it->second->destroy(); From 5c1b638db504c9e61c58db953b58ad0f49dad761 Mon Sep 17 00:00:00 2001 From: Michael Davidsaver Date: Wed, 7 Feb 2018 20:17:40 -0800 Subject: [PATCH 04/17] avoid spurious Leak warning copying an array just to see if it is empty isn't so efficient. --- src/server/serverContext.cpp | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/server/serverContext.cpp b/src/server/serverContext.cpp index 3d7a969..3ba9364 100644 --- a/src/server/serverContext.cpp +++ b/src/server/serverContext.cpp @@ -362,13 +362,6 @@ void ServerContextImpl::shutdown() void ServerContextImpl::destroyAllTransports() { - TransportRegistry::transportVector_t transports; - _transportRegistry.toArray(transports); - - size_t size = transports.size(); - if (size == 0) - return; - // now clear all (release) _transportRegistry.clear(); } From b397e3e9286f3822a78755a9cb1e2678437be31d Mon Sep 17 00:00:00 2001 From: Michael Davidsaver Date: Thu, 8 Feb 2018 11:00:40 -0800 Subject: [PATCH 05/17] Revert "convert global string "constants" to macros" This reverts commit 49b250798570bdc9c5451a4a79ab03dbcb09e233. --- src/ioc/PVAServerRegister.cpp | 2 +- src/pva/pv/pvaConstants.h | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/ioc/PVAServerRegister.cpp b/src/ioc/PVAServerRegister.cpp index 6360914..fbee91f 100644 --- a/src/ioc/PVAServerRegister.cpp +++ b/src/ioc/PVAServerRegister.cpp @@ -46,7 +46,7 @@ void startitup() { the_server = pva::ServerContext::create(pva::ServerContext::Config() .config(pva::ConfigurationBuilder() // default to all providers instead of just "local" - .add("EPICS_PVAS_PROVIDER_NAMES", PVACCESS_ALL_PROVIDERS) + .add("EPICS_PVAS_PROVIDER_NAMES", pva::PVACCESS_ALL_PROVIDERS) .push_map() // prefer to use EPICS_PVAS_PROVIDER_NAMES // from environment diff --git a/src/pva/pv/pvaConstants.h b/src/pva/pv/pvaConstants.h index 408d96f..14f2868 100644 --- a/src/pva/pv/pvaConstants.h +++ b/src/pva/pv/pvaConstants.h @@ -73,13 +73,13 @@ const epics::pvData::int16 INVALID_DATA_TYPE = static_cast const epics::pvData::int32 INVALID_IOID = 0; /** Default PVA provider name. */ -#define PVACCESS_DEFAULT_PROVIDER "local" +const std::string PVACCESS_DEFAULT_PROVIDER = "local"; /** "All-providers registered" PVA provider name. */ -#define PVACCESS_ALL_PROVIDERS "" +const std::string PVACCESS_ALL_PROVIDERS = ""; /** Name of the system env. variable to turn on debugging. */ -#define PVACCESS_DEBUG "EPICS_PVA_DEBUG" +const std::string PVACCESS_DEBUG = "EPICS_PVA_DEBUG"; } } From d12a6cad9b3efbe350b55ecb1f0f1bb7c3205cf8 Mon Sep 17 00:00:00 2001 From: Michael Davidsaver Date: Thu, 8 Feb 2018 11:32:18 -0800 Subject: [PATCH 06/17] Move PVACCESS_DEFAULT_PROVIDER def out of header --- src/pva/pv/pvaConstants.h | 8 ++++---- src/pva/pvaVersion.cpp | 4 ++++ 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/pva/pv/pvaConstants.h b/src/pva/pv/pvaConstants.h index 14f2868..9ee652d 100644 --- a/src/pva/pv/pvaConstants.h +++ b/src/pva/pv/pvaConstants.h @@ -67,19 +67,19 @@ const epics::pvData::int16 PVA_DEFAULT_PRIORITY = 0; const epics::pvData::uint32 MAX_CHANNEL_NAME_LENGTH = 500; /** Invalid data type. */ -const epics::pvData::int16 INVALID_DATA_TYPE = static_cast(0xFFFF); +const epics::pvData::int16 INVALID_DATA_TYPE = 0xFFFF; /** Invalid IOID. */ const epics::pvData::int32 INVALID_IOID = 0; /** Default PVA provider name. */ -const std::string PVACCESS_DEFAULT_PROVIDER = "local"; +const std::string PVACCESS_DEFAULT_PROVIDER; /** "All-providers registered" PVA provider name. */ -const std::string PVACCESS_ALL_PROVIDERS = ""; +const std::string PVACCESS_ALL_PROVIDERS; /** Name of the system env. variable to turn on debugging. */ -const std::string PVACCESS_DEBUG = "EPICS_PVA_DEBUG"; +const std::string PVACCESS_DEBUG; } } diff --git a/src/pva/pvaVersion.cpp b/src/pva/pvaVersion.cpp index b925442..f196242 100644 --- a/src/pva/pvaVersion.cpp +++ b/src/pva/pvaVersion.cpp @@ -15,6 +15,10 @@ using std::string; namespace epics { namespace pvAccess { +const std::string PVACCESS_DEFAULT_PROVIDER = "local"; +const std::string PVACCESS_ALL_PROVIDERS = ""; +const std::string PVACCESS_DEBUG = "EPICS_PVA_DEBUG"; + Version::Version(std::string const & productName, std::string const & implementationLangugage, int majorVersion, int minorVersion, From 8e88f534bb3bcd9e3ed10e90a0635a59889cb7f3 Mon Sep 17 00:00:00 2001 From: Michael Davidsaver Date: Tue, 13 Feb 2018 17:49:02 -0800 Subject: [PATCH 07/17] spamme advertise NTScalar --- examples/spamme.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/examples/spamme.cpp b/examples/spamme.cpp index c225224..6c37e2e 100644 --- a/examples/spamme.cpp +++ b/examples/spamme.cpp @@ -41,6 +41,7 @@ void alldone(int num) #endif pvd::Structure::const_shared_pointer spamtype(pvd::getFieldCreate()->createFieldBuilder() + ->setId("epics:nt/NTScalar:1.0") ->add("value", pvd::pvInt) ->createStructure()); From a266777d826f1b48b25557fb9bdbd43fac4e6eec Mon Sep 17 00:00:00 2001 From: Michael Davidsaver Date: Tue, 13 Feb 2018 17:51:20 -0800 Subject: [PATCH 08/17] destroyAllChannels() swap out channels list avoid possible modifications while iterating. --- src/remote/codec.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/remote/codec.cpp b/src/remote/codec.cpp index a8f1021..d252cea 100644 --- a/src/remote/codec.cpp +++ b/src/remote/codec.cpp @@ -1586,11 +1586,11 @@ void BlockingServerTCPTransportCodec::destroyAllChannels() { _socketName.c_str(), _channels.size()); } - _channels_t::iterator it(_channels.begin()); - for(; it!=_channels.end(); it++) - it->second->destroy(); + _channels_t temp; + temp.swap(_channels); - _channels.clear(); + for(_channels_t::iterator it(temp.begin()), end(temp.end()); it!=end; ++it) + it->second->destroy(); } void BlockingServerTCPTransportCodec::internalClose(bool force) { From c6b1d3b738ea330bac4886adb6fe6c7e7773547e Mon Sep 17 00:00:00 2001 From: Michael Davidsaver Date: Tue, 13 Feb 2018 17:56:02 -0800 Subject: [PATCH 09/17] fairQueue: logic error allows perpetual clear() oops, the argument isn't cleared in all cases, and can be swapped in. As in the case of clear(). --- src/utils/pv/fairQueue.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/utils/pv/fairQueue.h b/src/utils/pv/fairQueue.h index b3c30df..83eebf5 100644 --- a/src/utils/pv/fairQueue.h +++ b/src/utils/pv/fairQueue.h @@ -92,6 +92,7 @@ public: ~entry() { // nodes should be removed from the list before deletion assert(!enode.node.next && !enode.node.previous); + assert(Qcnt==0 && !holder); #ifndef NDEBUG assert(!owner); #endif @@ -144,6 +145,7 @@ public: bool pop_front_try(value_type& ret) { + ret.reset(); guard_t G(mutex); ELLNODE *cur = ellGet(&list); // pop_front @@ -165,7 +167,6 @@ public: } return true; } else { - ret.reset(); return false; } } From 18758fa4971c544c1413da0589cb075682d3bee2 Mon Sep 17 00:00:00 2001 From: Michael Davidsaver Date: Tue, 13 Feb 2018 18:07:58 -0800 Subject: [PATCH 10/17] throw new doesn't work javaism... --- src/remote/codec.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/remote/codec.cpp b/src/remote/codec.cpp index d252cea..cd45e83 100644 --- a/src/remote/codec.cpp +++ b/src/remote/codec.cpp @@ -337,7 +337,7 @@ void AbstractCodec::processReadSegmented() { " %s:%d: %s, disconnecting...", __FILE__, __LINE__, inetAddressToString(*getLastReadBufferSocketAddress()).c_str()); invalidDataStreamHandler(); - throw new invalid_data_stream_exception( + throw invalid_data_stream_exception( "not-a-first segmented message expected"); } From 685490fc1abc03e4d704037a1be698168d12bdb3 Mon Sep 17 00:00:00 2001 From: Michael Davidsaver Date: Wed, 14 Feb 2018 08:55:01 -0800 Subject: [PATCH 11/17] no NDEBUG Apparently the Base makefiles never have defined NDEBUG. So no point for this conditional. --- src/utils/pv/fairQueue.h | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/utils/pv/fairQueue.h b/src/utils/pv/fairQueue.h index 83eebf5..20447ff 100644 --- a/src/utils/pv/fairQueue.h +++ b/src/utils/pv/fairQueue.h @@ -72,9 +72,7 @@ public: } enode; unsigned Qcnt; value_type holder; -#ifndef NDEBUG fair_queue *owner; -#endif friend class fair_queue; @@ -82,9 +80,7 @@ public: entry& operator=(const entry&); public: entry() :Qcnt(0), holder() -#ifndef NDEBUG , owner(NULL) -#endif { enode.node.next = enode.node.previous = NULL; enode.self = this; @@ -93,9 +89,7 @@ public: // nodes should be removed from the list before deletion assert(!enode.node.next && !enode.node.previous); assert(Qcnt==0 && !holder); -#ifndef NDEBUG assert(!owner); -#endif } }; From 50cee1d1619b616dbca3210ba9503dfe258ac3fd Mon Sep 17 00:00:00 2001 From: Michael Davidsaver Date: Tue, 20 Feb 2018 10:18:39 -0800 Subject: [PATCH 12/17] minor --- examples/getme.cpp | 2 +- examples/monitorme.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/examples/getme.cpp b/examples/getme.cpp index 21f19cb..fdf2632 100644 --- a/examples/getme.cpp +++ b/examples/getme.cpp @@ -114,7 +114,7 @@ int main(int argc, char *argv[]) { std::cout<<"Usage: "<] [-w ] [-R] ...\n"; return 0; default: - std::cerr<<"Unknown argument: "<] [-w ] [-r ] [-R] ...\n"; return 0; default: - std::cerr<<"Unknown argument: "< Date: Tue, 20 Feb 2018 11:28:32 -0800 Subject: [PATCH 13/17] getRequester() may return NULL --- src/remoteClient/clientContextImpl.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/remoteClient/clientContextImpl.cpp b/src/remoteClient/clientContextImpl.cpp index 3800503..18ce6b1 100644 --- a/src/remoteClient/clientContextImpl.cpp +++ b/src/remoteClient/clientContextImpl.cpp @@ -3828,7 +3828,9 @@ public: for(size_t i=0, N=ops.size(); igetRequester()->channelDisconnect(connectionState==Channel::DESTROYED);) + ChannelBaseRequester::shared_pointer req(R->getRequester()); + if(!req) continue; + EXCEPTION_GUARD(req->channelDisconnect(connectionState==Channel::DESTROYED);) } } } From b5bf6a4ccdb3f79beb07cccbc79c656f06d5e19f Mon Sep 17 00:00:00 2001 From: Michael Davidsaver Date: Wed, 21 Feb 2018 11:19:21 -0800 Subject: [PATCH 14/17] client: (shallow) copy into Monitor::root Avoid exposing refs. from monitor queue which might then be used after release(). Also allows users the possibility of caching getSubField() until root.get() actually changes. --- src/client/clientMonitor.cpp | 19 ++++++++++++++++--- src/client/pva/client.h | 20 +++++++++++++++++++- 2 files changed, 35 insertions(+), 4 deletions(-) diff --git a/src/client/clientMonitor.cpp b/src/client/clientMonitor.cpp index 2a7c321..8b7debd 100644 --- a/src/client/clientMonitor.cpp +++ b/src/client/clientMonitor.cpp @@ -196,16 +196,29 @@ bool Monitor::poll() Guard G(impl->mutex); if(!impl->done && impl->last.next()) { - root = impl->last->pvStructurePtr; + const epics::pvData::PVStructurePtr& ptr = impl->last->pvStructurePtr; changed = *impl->last->changedBitSet; overrun = *impl->last->overrunBitSet; + /* copy the exposed PVStructure for two reasons. + * 1. Prevent accidental use of shared container after release() + * 2. Allows caller to cache results of getSubField() until root.get() changes. + */ + if(!root || (void*)root->getField().get()!=(void*)ptr->getField().get()) { + // initial connection, or new type + root = pvd::getPVDataCreate()->createPVStructure(ptr); // also calls copyUnchecked() + } else { + // same type + const_cast(*root).copyUnchecked(*ptr, changed); + } + + impl->seenEmpty = false; } else { - root.reset(); changed.clear(); overrun.clear(); + impl->seenEmpty = true; } - return impl->seenEmpty = !!root; + return !impl->seenEmpty; } bool Monitor::complete() const diff --git a/src/client/pva/client.h b/src/client/pva/client.h index e9b6897..21f6b3a 100644 --- a/src/client/pva/client.h +++ b/src/client/pva/client.h @@ -104,13 +104,31 @@ struct epicsShareClass Monitor void cancel(); /** updates root, changed, overrun * - * @return true if root!=NULL + * @return true if a new update was extracted from the queue. + * @note This method does not block. * @note MonitorEvent::Data will not be repeated until poll()==false. + * @post root!=NULL (after version 6.0.0) + * @post root!=NULL iff poll()==true (In version 6.0.0) */ bool poll(); //! true if all events received. //! Check after poll()==false bool complete() const; + /** Monitor update data. + * + * After version 6.0.0 + * + * Initially NULL, becomes !NULL the first time poll()==true. + * The PVStructure pointed to be root will presist until + * Monitor reconnect w/ type change. This can be detected + * by comparing `root.get()`. references to root may be cached + * subject to this test. + * + * In version 6.0.0 + * + * NULL except after poll()==true. poll()==false sets root=NULL. + * references to root should not be stored between calls to poll(). + */ epics::pvData::PVStructure::const_shared_pointer root; epics::pvData::BitSet changed, overrun; From 909cef9200278500a08309fadeb5870b4780112d Mon Sep 17 00:00:00 2001 From: Michael Davidsaver Date: Wed, 21 Feb 2018 11:19:58 -0800 Subject: [PATCH 15/17] minor --- examples/monitorme.cpp | 2 ++ src/server/responseHandlers.cpp | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/examples/monitorme.cpp b/examples/monitorme.cpp index a9f52c1..a4dc199 100644 --- a/examples/monitorme.cpp +++ b/examples/monitorme.cpp @@ -175,6 +175,8 @@ struct MonTracker : public pvac::ClientChannel::MonitorCallback, if(n==2) { // too many updates, re-queue to balance with others monwork.push(shared_from_this(), evt); + } else if(n==0) { + std::cerr<<"Spurious Data event "<unregisterRequest(_ioid); From d605eaca112b29b3165ec3a9ccb93c5dee1ad2b2 Mon Sep 17 00:00:00 2001 From: Michael Davidsaver Date: Tue, 27 Feb 2018 13:27:38 -0800 Subject: [PATCH 16/17] pvput avoid unnecessary iteration --- pvtoolsSrc/pvput.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/pvtoolsSrc/pvput.cpp b/pvtoolsSrc/pvput.cpp index 8a36926..dadf7af 100644 --- a/pvtoolsSrc/pvput.cpp +++ b/pvtoolsSrc/pvput.cpp @@ -321,6 +321,7 @@ struct Putter : public pvac::ClientChannel::PutCallback if(bare[0]==choices[i]) { idxfld->putFrom(i); found=true; + break; } } From 74fbd22d40d54a4c63dd63f210628b1ef1b8267d Mon Sep 17 00:00:00 2001 From: Michael Davidsaver Date: Tue, 27 Feb 2018 14:18:15 -0800 Subject: [PATCH 17/17] pvput: fix enum_t Need to get value.choices --- pvtoolsSrc/pvput.cpp | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/pvtoolsSrc/pvput.cpp b/pvtoolsSrc/pvput.cpp index dadf7af..db5ae9a 100644 --- a/pvtoolsSrc/pvput.cpp +++ b/pvtoolsSrc/pvput.cpp @@ -269,6 +269,8 @@ struct Putter : public pvac::ClientChannel::PutCallback shared_vector jarr; + PVStructure::const_shared_pointer current; + virtual void putBuild(const epics::pvData::StructureConstPtr& build, Args& args) { if(debug) std::cerr<<"Server defined structure\n"<(fld.get())); PVScalar* idxfld(sfld->getSubFieldT("index").get()); - PVStringArray::const_svector choices(sfld->getSubFieldT("choices")->view()); + PVStringArray::const_svector choices(current->getSubFieldT("value.choices")->view()); bool found=false; for(size_t i=0; i