From a10534d74d6d3caf85c180eedfdbb51698da0277 Mon Sep 17 00:00:00 2001 From: Michael Davidsaver Date: Tue, 28 Nov 2017 15:28:59 -0600 Subject: [PATCH 1/7] eliminate Connector interface no code generically creates UDP or TCP connections, so this abstraction results only in unnecessary virtual calls and casts. --- src/remote/blockingTCPConnector.cpp | 3 --- src/remote/blockingUDPConnector.cpp | 19 +++++++++---------- src/remote/blockingUDPTransport.cpp | 18 +++++++++--------- src/remote/pv/blockingTCP.h | 6 ++---- src/remote/pv/blockingUDP.h | 21 ++------------------- src/remote/pv/remote.h | 22 ---------------------- 6 files changed, 22 insertions(+), 67 deletions(-) diff --git a/src/remote/blockingTCPConnector.cpp b/src/remote/blockingTCPConnector.cpp index cb82484..d7c2530 100644 --- a/src/remote/blockingTCPConnector.cpp +++ b/src/remote/blockingTCPConnector.cpp @@ -33,9 +33,6 @@ BlockingTCPConnector::BlockingTCPConnector( { } -BlockingTCPConnector::~BlockingTCPConnector() { -} - SOCKET BlockingTCPConnector::tryConnect(osiSockAddr& address, int tries) { char strBuffer[64]; diff --git a/src/remote/blockingUDPConnector.cpp b/src/remote/blockingUDPConnector.cpp index 5e916d2..3b83faa 100644 --- a/src/remote/blockingUDPConnector.cpp +++ b/src/remote/blockingUDPConnector.cpp @@ -18,9 +18,9 @@ using namespace epics::pvData; namespace { struct closer { - epics::pvAccess::Transport::shared_pointer P; - closer(const epics::pvAccess::Transport::shared_pointer& P) :P(P) {} - void operator()(epics::pvAccess::Transport*) { + epics::pvAccess::BlockingUDPTransport::shared_pointer P; + closer(const epics::pvAccess::BlockingUDPTransport::shared_pointer& P) :P(P) {} + void operator()(epics::pvAccess::BlockingUDPTransport*) { try{ P->close(); }catch(...){ @@ -35,7 +35,7 @@ struct closer { namespace epics { namespace pvAccess { -Transport::shared_pointer BlockingUDPConnector::connect(std::tr1::shared_ptr const & /*client*/, +BlockingUDPTransport::shared_pointer BlockingUDPConnector::connect(std::tr1::shared_ptr const & /*client*/, ResponseHandler::shared_pointer const & responseHandler, osiSockAddr& bindAddress, int8 transportRevision, int16 /*priority*/) { @@ -47,7 +47,7 @@ Transport::shared_pointer BlockingUDPConnector::connect(std::tr1::shared_ptrinternal_this = transport; // the worker thread holds a strong ref, which is released by transport->close() - // note: casting to Transport* to prevent iOS version of shared_ptr from trying (and failing) - // to setup shared_from_this() using the wrapped pointer - Transport::shared_pointer ret(static_cast(transport.get()), closer(transport)); + BlockingUDPTransport::shared_pointer ret(transport.get(), closer(transport)); return ret; } diff --git a/src/remote/blockingUDPTransport.cpp b/src/remote/blockingUDPTransport.cpp index 2437d54..bcf49d0 100644 --- a/src/remote/blockingUDPTransport.cpp +++ b/src/remote/blockingUDPTransport.cpp @@ -207,7 +207,7 @@ void BlockingUDPTransport::run() { osiSockAddr fromAddress; osiSocklen_t addrStructSize = sizeof(sockaddr); - Transport::shared_pointer thisTransport = shared_from_this(); + Transport::shared_pointer thisTransport(internal_this); try { @@ -573,10 +573,10 @@ void initializeUDPTransports(bool serverFlag, anyAddress.ia.sin_port = htons(0); anyAddress.ia.sin_addr.s_addr = htonl(INADDR_ANY); - sendTransport = static_pointer_cast(connector->connect( + sendTransport = connector->connect( nullTransportClient, responseHandler, anyAddress, PVA_PROTOCOL_REVISION, - PVA_DEFAULT_PRIORITY)); + PVA_DEFAULT_PRIORITY); if (!sendTransport) { THROW_BASE_EXCEPTION("Failed to initialize UDP transport."); @@ -693,10 +693,10 @@ void initializeUDPTransports(bool serverFlag, listenLocalAddress.ia.sin_port = htons(listenPort); listenLocalAddress.ia.sin_addr.s_addr = node.ifaceAddr.ia.sin_addr.s_addr; - BlockingUDPTransport::shared_pointer transport = static_pointer_cast(connector->connect( + BlockingUDPTransport::shared_pointer transport = connector->connect( nullTransportClient, responseHandler, listenLocalAddress, PVA_PROTOCOL_REVISION, - PVA_DEFAULT_PRIORITY)); + PVA_DEFAULT_PRIORITY); if (!transport) continue; listenLocalAddress = *transport->getRemoteAddress(); @@ -730,10 +730,10 @@ void initializeUDPTransports(bool serverFlag, bcastAddress.ia.sin_port = htons(listenPort); bcastAddress.ia.sin_addr.s_addr = node.ifaceBCast.ia.sin_addr.s_addr; - transport2 = static_pointer_cast(connector->connect( + transport2 = connector->connect( nullTransportClient, responseHandler, bcastAddress, PVA_PROTOCOL_REVISION, - PVA_DEFAULT_PRIORITY)); + PVA_DEFAULT_PRIORITY); if (transport2) { /* The other wrinkle is that nothing should be sent from this second @@ -786,7 +786,7 @@ void initializeUDPTransports(bool serverFlag, try { // NOTE: multicast receiver socket must be "bound" to INADDR_ANY or multicast address - localMulticastTransport = static_pointer_cast(connector->connect( + localMulticastTransport = connector->connect( nullTransportClient, responseHandler, #if !defined(_WIN32) group, @@ -794,7 +794,7 @@ void initializeUDPTransports(bool serverFlag, anyAddress, #endif PVA_PROTOCOL_REVISION, - PVA_DEFAULT_PRIORITY)); + PVA_DEFAULT_PRIORITY); if (!localMulticastTransport) throw std::runtime_error("Failed to bind UDP socket."); diff --git a/src/remote/pv/blockingTCP.h b/src/remote/pv/blockingTCP.h index 084e702..ceb361a 100644 --- a/src/remote/pv/blockingTCP.h +++ b/src/remote/pv/blockingTCP.h @@ -49,16 +49,14 @@ class ClientChannelImpl; * @author Matej Sekoranja * @version $Id: BlockingTCPConnector.java,v 1.1 2010/05/03 14:45:47 mrkraimer Exp $ */ -class BlockingTCPConnector : public Connector { +class BlockingTCPConnector { public: POINTER_DEFINITIONS(BlockingTCPConnector); BlockingTCPConnector(Context::shared_pointer const & context, int receiveBufferSize, float beaconInterval); - virtual ~BlockingTCPConnector(); - - virtual Transport::shared_pointer connect(std::tr1::shared_ptr const & client, + Transport::shared_pointer connect(std::tr1::shared_ptr const & client, ResponseHandler::shared_pointer const & responseHandler, osiSockAddr& address, epics::pvData::int8 transportRevision, epics::pvData::int16 priority); private: diff --git a/src/remote/pv/blockingUDP.h b/src/remote/pv/blockingUDP.h index fe836f6..2d0afc8 100644 --- a/src/remote/pv/blockingUDP.h +++ b/src/remote/pv/blockingUDP.h @@ -44,13 +44,13 @@ enum InetAddressType { inetAddressType_all, inetAddressType_unicast, inetAddress class BlockingUDPTransport : public epics::pvData::NoDefaultMethods, public Transport, public TransportSendControl, - public std::tr1::enable_shared_from_this, public epicsThreadRunable { public: POINTER_DEFINITIONS(BlockingUDPTransport); private: + std::tr1::weak_ptr internal_this; friend class BlockingUDPConnector; BlockingUDPTransport(bool serverFlag, ResponseHandler::shared_pointer const & responseHandler, @@ -58,19 +58,6 @@ private: short remoteTransportRevision); public: - static shared_pointer create(bool serverFlag, - ResponseHandler::shared_pointer const & responseHandler, - SOCKET channel, osiSockAddr& bindAddress, - short remoteTransportRevision) EPICS_DEPRECATED - { - shared_pointer thisPointer( - new BlockingUDPTransport(serverFlag, responseHandler, - channel, bindAddress, - remoteTransportRevision) - ); - return thisPointer; - } - virtual ~BlockingUDPTransport(); virtual bool isClosed() { @@ -428,7 +415,6 @@ private: }; class BlockingUDPConnector : - public Connector, private epics::pvData::NoDefaultMethods { public: POINTER_DEFINITIONS(BlockingUDPConnector); @@ -442,13 +428,10 @@ public: _broadcast(broadcast) { } - virtual ~BlockingUDPConnector() { - } - /** * NOTE: transport client is ignored for broadcast (UDP). */ - virtual Transport::shared_pointer connect(std::tr1::shared_ptr const & client, + BlockingUDPTransport::shared_pointer connect(std::tr1::shared_ptr const & client, ResponseHandler::shared_pointer const & responseHandler, osiSockAddr& bindAddress, epics::pvData::int8 transportRevision, epics::pvData::int16 priority); diff --git a/src/remote/pv/remote.h b/src/remote/pv/remote.h index 8d987c7..26f78a4 100644 --- a/src/remote/pv/remote.h +++ b/src/remote/pv/remote.h @@ -377,28 +377,6 @@ protected: epics::pvData::int32 _debugLevel; }; -/** - * Interface defining socket connector (Connector-Transport pattern). - */ -class Connector { -public: - virtual ~Connector() {} - - /** - * Connect. - * @param[in] client client requesting connection (transport). - * @param[in] address address of the server. - * @param[in] responseHandler reponse handler. - * @param[in] transportRevision transport revision to be used. - * @param[in] priority process priority. - * @return transport instance. - */ - virtual Transport::shared_pointer connect(std::tr1::shared_ptr const & client, - ResponseHandler::shared_pointer const & responseHandler, osiSockAddr& address, - epics::pvData::int8 transportRevision, epics::pvData::int16 priority) = 0; - -}; - /** * A request that expects an response. * Responses identified by its I/O ID. From c30d4347d0d1724e3ef94a01c95ae877f8ea8e7f Mon Sep 17 00:00:00 2001 From: Michael Davidsaver Date: Tue, 28 Nov 2017 15:50:23 -0600 Subject: [PATCH 2/7] notate BlockingUDPTransport w/ OVERRIDE FINAL and de-virtualize methods never actual overridden --- src/remote/pv/blockingUDP.h | 85 +++++++++++++++++-------------------- 1 file changed, 39 insertions(+), 46 deletions(-) diff --git a/src/remote/pv/blockingUDP.h b/src/remote/pv/blockingUDP.h index 2d0afc8..6e33268 100644 --- a/src/remote/pv/blockingUDP.h +++ b/src/remote/pv/blockingUDP.h @@ -60,84 +60,77 @@ public: virtual ~BlockingUDPTransport(); - virtual bool isClosed() { + virtual bool isClosed() OVERRIDE FINAL { return _closed.get(); } - /* - void setReplyTransport(const Transport::shared_pointer& T) - { - _replyTransport = T; - } - */ - - virtual const osiSockAddr* getRemoteAddress() const { + virtual const osiSockAddr* getRemoteAddress() const OVERRIDE FINAL { return &_remoteAddress; } - virtual const std::string& getRemoteName() const { + virtual const std::string& getRemoteName() const OVERRIDE FINAL { return _remoteName; } - virtual std::string getType() const { + virtual std::string getType() const OVERRIDE FINAL { return std::string("udp"); } - virtual std::size_t getReceiveBufferSize() const { + virtual std::size_t getReceiveBufferSize() const OVERRIDE FINAL { return _receiveBuffer.getSize(); } - virtual std::size_t getSocketReceiveBufferSize() const; + virtual std::size_t getSocketReceiveBufferSize() const OVERRIDE FINAL; - virtual epics::pvData::int16 getPriority() const { + virtual epics::pvData::int16 getPriority() const OVERRIDE FINAL { return PVA_DEFAULT_PRIORITY; } - virtual epics::pvData::int8 getRevision() const { + virtual epics::pvData::int8 getRevision() const OVERRIDE FINAL { return PVA_PROTOCOL_REVISION; } - virtual void setRemoteRevision(epics::pvData::int8 /*revision*/) { + virtual void setRemoteRevision(epics::pvData::int8 /*revision*/) OVERRIDE FINAL { // noop } virtual void setRemoteTransportReceiveBufferSize( - std::size_t /*receiveBufferSize*/) { + std::size_t /*receiveBufferSize*/) OVERRIDE FINAL { // noop for UDP (limited by 64k; MAX_UDP_SEND for PVA) } virtual void setRemoteTransportSocketReceiveBufferSize( - std::size_t /*socketReceiveBufferSize*/) { + std::size_t /*socketReceiveBufferSize*/) OVERRIDE FINAL { // noop for UDP (limited by 64k; MAX_UDP_SEND for PVA) } - virtual void aliveNotification() { + virtual void aliveNotification() OVERRIDE FINAL { // noop } - virtual void changedTransport() { + virtual void changedTransport() OVERRIDE FINAL { // noop } - virtual bool verify(epics::pvData::int32 /*timeoutMs*/) { + virtual bool verify(epics::pvData::int32 /*timeoutMs*/) OVERRIDE FINAL { // noop return true; } - virtual void verified(epics::pvData::Status const & /*status*/) { + virtual void verified(epics::pvData::Status const & /*status*/) OVERRIDE FINAL { // noop } - virtual void authNZMessage(epics::pvData::PVField::shared_pointer const & data) { + virtual void authNZMessage(epics::pvData::PVField::shared_pointer const & data) OVERRIDE FINAL { // noop } - virtual std::tr1::shared_ptr getSecuritySession() const { + virtual std::tr1::shared_ptr getSecuritySession() const OVERRIDE FINAL { return std::tr1::shared_ptr(); } // NOTE: this is not yet used for UDP - virtual void setByteOrder(int byteOrder) { + virtual void setByteOrder(int byteOrder) OVERRIDE FINAL { // called from receive thread... or before processing _receiveBuffer.setEndianess(byteOrder); @@ -145,93 +138,93 @@ public: _sendBuffer.setEndianess(byteOrder); } - virtual void enqueueSendRequest(TransportSender::shared_pointer const & sender); + virtual void enqueueSendRequest(TransportSender::shared_pointer const & sender) OVERRIDE FINAL; - virtual void flushSendQueue(); + virtual void flushSendQueue() OVERRIDE FINAL; void start(); - virtual void close(); + virtual void close() OVERRIDE FINAL; - virtual void ensureData(std::size_t size) { + virtual void ensureData(std::size_t size) OVERRIDE FINAL { if (_receiveBuffer.getRemaining() < size) throw std::underflow_error("no more data in UDP packet"); } - virtual void alignData(std::size_t alignment) { + virtual void alignData(std::size_t alignment) OVERRIDE FINAL { _receiveBuffer.align(alignment); } virtual bool directSerialize(epics::pvData::ByteBuffer* /*existingBuffer*/, const char* /*toSerialize*/, - std::size_t /*elementCount*/, std::size_t /*elementSize*/) + std::size_t /*elementCount*/, std::size_t /*elementSize*/) OVERRIDE FINAL { return false; } virtual bool directDeserialize(epics::pvData::ByteBuffer* /*existingBuffer*/, char* /*deserializeTo*/, - std::size_t /*elementCount*/, std::size_t /*elementSize*/) + std::size_t /*elementCount*/, std::size_t /*elementSize*/) OVERRIDE FINAL { return false; } - virtual void startMessage(epics::pvData::int8 command, std::size_t ensureCapacity, epics::pvData::int32 payloadSize = 0); - virtual void endMessage(); + virtual void startMessage(epics::pvData::int8 command, std::size_t ensureCapacity, epics::pvData::int32 payloadSize = 0) OVERRIDE FINAL; + virtual void endMessage() OVERRIDE FINAL; - virtual void flush(bool /*lastMessageCompleted*/) { + virtual void flush(bool /*lastMessageCompleted*/) OVERRIDE FINAL { // noop since all UDP requests are sent immediately } - virtual void setRecipient(const osiSockAddr& sendTo) { + virtual void setRecipient(const osiSockAddr& sendTo) OVERRIDE FINAL { _sendToEnabled = true; _sendTo = sendTo; } - virtual void setLocalMulticastAddress(const osiSockAddr& sendTo) { + void setLocalMulticastAddress(const osiSockAddr& sendTo) { _localMulticastAddressEnabled = true; _localMulticastAddress = sendTo; } - virtual bool hasLocalMulticastAddress() const { + bool hasLocalMulticastAddress() const { return _localMulticastAddressEnabled; } - virtual const osiSockAddr& getLocalMulticastAddress() const { + const osiSockAddr& getLocalMulticastAddress() const { return _localMulticastAddress; } - virtual void flushSerializeBuffer() { + virtual void flushSerializeBuffer() OVERRIDE FINAL { // noop } - virtual void ensureBuffer(std::size_t /*size*/) { + virtual void ensureBuffer(std::size_t /*size*/) OVERRIDE FINAL { // noop } - virtual void alignBuffer(std::size_t alignment) { + virtual void alignBuffer(std::size_t alignment) OVERRIDE FINAL { _sendBuffer.align(alignment); } virtual void cachedSerialize( - const std::tr1::shared_ptr& field, epics::pvData::ByteBuffer* buffer) + const std::tr1::shared_ptr& field, epics::pvData::ByteBuffer* buffer) OVERRIDE FINAL { // no cache field->serialize(buffer, this); } virtual std::tr1::shared_ptr - cachedDeserialize(epics::pvData::ByteBuffer* buffer) + cachedDeserialize(epics::pvData::ByteBuffer* buffer) OVERRIDE FINAL { // no cache // TODO return epics::pvData::getFieldCreate()->deserialize(buffer, this); } - virtual bool acquire(std::tr1::shared_ptr const & /*client*/) + virtual bool acquire(std::tr1::shared_ptr const & /*client*/) OVERRIDE FINAL { return false; } - virtual void release(pvAccessID /*clientId*/) {} + virtual void release(pvAccessID /*clientId*/) OVERRIDE FINAL {} /** * Set ignore list. From f076adc1ae27e959e73b3b14d0d286eaef749d56 Mon Sep 17 00:00:00 2001 From: Michael Davidsaver Date: Thu, 28 Dec 2017 10:38:05 -0600 Subject: [PATCH 3/7] remove unnecessary atexit The client factory is installed by global ctor. --- src/ioc/PVAClientRegister.cpp | 28 +--------------------------- 1 file changed, 1 insertion(+), 27 deletions(-) diff --git a/src/ioc/PVAClientRegister.cpp b/src/ioc/PVAClientRegister.cpp index 729e1c6..50ca380 100644 --- a/src/ioc/PVAClientRegister.cpp +++ b/src/ioc/PVAClientRegister.cpp @@ -11,35 +11,9 @@ /* Author: Marty Kraimer */ -#include -#include -#include -#include -#include -#include -#include - -#include -#include - -#include -#include - #include -using namespace epics::pvAccess; - -static void stopPVAClient(void*) -{ - ClientFactory::stop(); -} - -static void registerStartPVAClient(void) -{ - ClientFactory::start(); - epicsAtExit(stopPVAClient, 0); -} - +static void registerStartPVAClient(void) {} extern "C" { epicsExportRegistrar(registerStartPVAClient); From 903d117485c4f09593bd312040506c48c62b3e79 Mon Sep 17 00:00:00 2001 From: Michael Davidsaver Date: Thu, 28 Dec 2017 10:49:54 -0600 Subject: [PATCH 4/7] pvAccessLog() add EPICS_PRINTF_STYLE() and fixup incorrect specifiers --- pvtoolsSrc/pvlist.cpp | 2 +- src/remote/blockingUDPTransport.cpp | 10 +++++----- src/remote/codec.cpp | 4 ++-- src/remoteClient/clientContextImpl.cpp | 2 +- src/server/serverContext.cpp | 2 +- src/utils/pv/logger.h | 3 ++- 6 files changed, 12 insertions(+), 11 deletions(-) diff --git a/pvtoolsSrc/pvlist.cpp b/pvtoolsSrc/pvlist.cpp index 0eff7bb..675a152 100644 --- a/pvtoolsSrc/pvlist.cpp +++ b/pvtoolsSrc/pvlist.cpp @@ -250,7 +250,7 @@ bool discoverServers(double timeOut) for (size_t i = 0; i < broadcastAddresses.size(); i++) LOG(logLevelDebug, - "Broadcast address #%d: %s.", i, inetAddressToString(broadcastAddresses[i]).c_str()); + "Broadcast address #%zu: %s.", i, inetAddressToString(broadcastAddresses[i]).c_str()); // --- diff --git a/src/remote/blockingUDPTransport.cpp b/src/remote/blockingUDPTransport.cpp index bcf49d0..ad524ed 100644 --- a/src/remote/blockingUDPTransport.cpp +++ b/src/remote/blockingUDPTransport.cpp @@ -395,7 +395,7 @@ bool BlockingUDPTransport::send(const char* buffer, size_t length, const osiSock { if (IS_LOGGABLE(logLevelDebug)) { - LOG(logLevelDebug, "Sending %d bytes to %s.", + LOG(logLevelDebug, "Sending %zu bytes to %s.", length, inetAddressToString(address).c_str()); } @@ -419,7 +419,7 @@ bool BlockingUDPTransport::send(ByteBuffer* buffer, const osiSockAddr& address) if (IS_LOGGABLE(logLevelDebug)) { - LOG(logLevelDebug, "Sending %d bytes to %s.", + LOG(logLevelDebug, "Sending %zu bytes to %s.", buffer->getRemaining(), inetAddressToString(address).c_str()); } @@ -456,7 +456,7 @@ bool BlockingUDPTransport::send(ByteBuffer* buffer, InetAddressType target) { if (IS_LOGGABLE(logLevelDebug)) { - LOG(logLevelDebug, "Sending %d bytes to %s.", + LOG(logLevelDebug, "Sending %zu bytes to %s.", buffer->getRemaining(), inetAddressToString(_sendAddresses[i]).c_str()); } @@ -586,7 +586,7 @@ void initializeUDPTransports(bool serverFlag, if (listenPort == 0) { listenPort = ntohs(sendTransport->getRemoteAddress()->ia.sin_port); - LOG(logLevelDebug, "Dynamic listen UDP port set to %d.", listenPort); + LOG(logLevelDebug, "Dynamic listen UDP port set to %u.", (unsigned)listenPort); } // TODO current implementation shares the port (aka beacon and search port) @@ -651,7 +651,7 @@ void initializeUDPTransports(bool serverFlag, else for (size_t i = 0; i < blist.size(); i++) LOG(logLevelDebug, - "Broadcast address #%d: %s.", i, inetAddressToString(blist[i]).c_str()); + "Broadcast address #%zu: %s.", i, inetAddressToString(blist[i]).c_str()); // TODO configurable local NIF, address diff --git a/src/remote/codec.cpp b/src/remote/codec.cpp index bb1d6cd..8e89293 100644 --- a/src/remote/codec.cpp +++ b/src/remote/codec.cpp @@ -1569,7 +1569,7 @@ void BlockingServerTCPTransportCodec::destroyAllChannels() { { LOG( logLevelDebug, - "Transport to %s still has %zd channel(s) active and closing...", + "Transport to %s still has %zu channel(s) active and closing...", _socketName.c_str(), _channels.size()); } @@ -1793,7 +1793,7 @@ void BlockingClientTCPTransportCodec::closedNotifyClients() { { LOG( logLevelDebug, - "Transport to %s still has %d client(s) active and closing...", + "Transport to %s still has %zu client(s) active and closing...", _socketName.c_str(), refs); } diff --git a/src/remoteClient/clientContextImpl.cpp b/src/remoteClient/clientContextImpl.cpp index 0b2bde0..6d021f5 100644 --- a/src/remoteClient/clientContextImpl.cpp +++ b/src/remoteClient/clientContextImpl.cpp @@ -4230,7 +4230,7 @@ private: } if (transportCount) - LOG(logLevelDebug, "PVA client context destroyed with %d transport(s) active.", transportCount); + LOG(logLevelDebug, "PVA client context destroyed with %u transport(s) active.", (unsigned)transportCount); } void destroyAllChannels() { diff --git a/src/server/serverContext.cpp b/src/server/serverContext.cpp index 4f4e52e..84f8e24 100644 --- a/src/server/serverContext.cpp +++ b/src/server/serverContext.cpp @@ -369,7 +369,7 @@ void ServerContextImpl::destroyAllTransports() if (size == 0) return; - LOG(logLevelDebug, "Server context still has %d transport(s) active and closing...", size); + LOG(logLevelDebug, "Server context still has %zu transport(s) active and closing...", size); for (size_t i = 0; i < size; i++) { diff --git a/src/utils/pv/logger.h b/src/utils/pv/logger.h index cfdbdbb..54e636d 100644 --- a/src/utils/pv/logger.h +++ b/src/utils/pv/logger.h @@ -9,6 +9,7 @@ #include +#include #include namespace epics { @@ -37,7 +38,7 @@ OFF */ -epicsShareFunc void pvAccessLog(pvAccessLogLevel level, const char* format, ...); +epicsShareFunc void pvAccessLog(pvAccessLogLevel level, const char* format, ...) EPICS_PRINTF_STYLE(2, 3); epicsShareFunc void pvAccessSetLogLevel(pvAccessLogLevel level); epicsShareFunc bool pvAccessIsLoggable(pvAccessLogLevel level); From d1fca67cf297f78aba15eaa0d9b511097c416710 Mon Sep 17 00:00:00 2001 From: Michael Davidsaver Date: Thu, 28 Dec 2017 11:08:13 -0600 Subject: [PATCH 5/7] ioc: shutdown PVA server via epicsAtExit() Otherwise is shutdown in global dtor. Better to use epicsAtExit() which has some ordering --- src/ioc/PVAServerRegister.cpp | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/ioc/PVAServerRegister.cpp b/src/ioc/PVAServerRegister.cpp index dba4b96..fbee91f 100644 --- a/src/ioc/PVAServerRegister.cpp +++ b/src/ioc/PVAServerRegister.cpp @@ -103,10 +103,16 @@ void pvasr(int lvl) } } +void pva_server_cleanup(void *) +{ + stopPVAServer(); +} + void initStartPVAServer(initHookState state) { pvd::Lock G(the_server_lock); if(state==initHookAfterIocRunning && !the_server) { + epicsAtExit(&pva_server_cleanup, 0); startitup(); } else if(state==initHookAtIocPause) { @@ -114,7 +120,6 @@ void initStartPVAServer(initHookState state) } } - void registerStartPVAServer(void) { epics::iocshRegister("startPVAServer", "provider names"); From fa46935d3560f512ea5d8c732fc056f3bf8b9438 Mon Sep 17 00:00:00 2001 From: Andrew Johnson Date: Thu, 4 Jan 2018 17:52:36 -0600 Subject: [PATCH 6/7] Clean up compiler warnings --- src/remote/codec.cpp | 2 +- testApp/utils/configurationTest.cpp | 14 -------------- 2 files changed, 1 insertion(+), 15 deletions(-) diff --git a/src/remote/codec.cpp b/src/remote/codec.cpp index 8e89293..8233528 100644 --- a/src/remote/codec.cpp +++ b/src/remote/codec.cpp @@ -1600,7 +1600,7 @@ void BlockingServerTCPTransportCodec::authenticationCompleted(epics::pvData::Sta string errorMessage = "Re-authentication failed: " + status.getMessage(); if (!status.getStackDump().empty()) errorMessage += "\n" + status.getStackDump(); - LOG(logLevelInfo, errorMessage.c_str()); + LOG(logLevelInfo, "%s", errorMessage.c_str()); close(); } diff --git a/testApp/utils/configurationTest.cpp b/testApp/utils/configurationTest.cpp index f00c613..316db3a 100644 --- a/testApp/utils/configurationTest.cpp +++ b/testApp/utils/configurationTest.cpp @@ -32,20 +32,6 @@ void setenv(char * a, char * b, int c) using namespace epics::pvAccess; using namespace epics::pvData; -static const char indata[] = - "hello = world \n" - " # oops\n" - " #dd=da\n" - " empty = \n" - " this = is a test\n\n" - ; - -static const char expectdata[] = - "empty = \n" - "hello = world\n" - "this = is a test\n" - ; - static void showEnv(const char *name) { testDiag("%s = \"%s\"", name, getenv(name)); From 5c5da3b515eb1f8d1dc4a628f6e6c86c0fe1cdd2 Mon Sep 17 00:00:00 2001 From: Marty Kraimer Date: Mon, 29 Jan 2018 13:53:25 -0500 Subject: [PATCH 7/7] Revert "When caProvider is destroyed make sure all channels are cleared" --- src/ca/caChannel.cpp | 44 ++++++++++------------------------- src/ca/caChannel.h | 7 +++--- src/ca/caProvider.cpp | 52 +++++++----------------------------------- src/ca/caProviderPvt.h | 9 -------- 4 files changed, 23 insertions(+), 89 deletions(-) diff --git a/src/ca/caChannel.cpp b/src/ca/caChannel.cpp index d145a71..a1e3c90 100644 --- a/src/ca/caChannel.cpp +++ b/src/ca/caChannel.cpp @@ -374,8 +374,7 @@ CAChannel::CAChannel(std::string const & _channelName, channelRequester(_channelRequester), channelID(0), channelType(0), - elementCount(0), - channelCreated(false) + elementCount(0) { if(DEBUG_LEVEL>0) { cout<< "CAChannel::CAChannel " << channelName << endl; @@ -396,9 +395,6 @@ void CAChannel::activate(short priority) &channelID); if (result == ECA_NORMAL) { - channelCreated = true; - CAChannelProviderPtr provider(channelProvider.lock()); - if(provider) provider->addChannel(shared_from_this()); req->channelCreated(Status::Ok, shared_from_this()); } else { Status errorStatus(Status::STATUSTYPE_ERROR, string(ca_message(result))); @@ -406,33 +402,6 @@ void CAChannel::activate(short priority) } } -CAChannel::~CAChannel() -{ - if(DEBUG_LEVEL>0) { - cout << "CAChannel::~CAChannel() " << channelName << endl; - } - disconnectChannel(); -} - -void CAChannel::disconnectChannel() -{ - if(DEBUG_LEVEL>0) { - cout << "CAChannel::disconnectChannel() " - << channelName - << " channelCreated " << (channelCreated ? "true" : "false") - << endl; - } - { - Lock lock(requestsMutex); - if(!channelCreated) return; - } - /* Clear CA Channel */ - threadAttach(); - ca_clear_channel(channelID); -} - - - void CAChannel::addChannelGet(const CAChannelGetPtr & get) { if(DEBUG_LEVEL>0) { @@ -479,6 +448,17 @@ void CAChannel::addChannelMonitor(const CAChannelMonitorPtr & monitor) monitorList.push_back(monitor); } +CAChannel::~CAChannel() +{ + if(DEBUG_LEVEL>0) { + cout << "CAChannel::~CAChannel() " << channelName << endl; + } + /* Clear CA Channel */ + threadAttach(); + ca_clear_channel(channelID); +} + + chid CAChannel::getChannelID() { return channelID; diff --git a/src/ca/caChannel.h b/src/ca/caChannel.h index 72d201c..cdb716b 100644 --- a/src/ca/caChannel.h +++ b/src/ca/caChannel.h @@ -22,7 +22,8 @@ namespace epics { namespace pvAccess { namespace ca { - +class CAChannel; +typedef std::tr1::shared_ptr CAChannelPtr; class CAChannelGetField; typedef std::tr1::shared_ptr CAChannelGetFieldPtr; typedef std::tr1::weak_ptr CAChannelGetFieldWPtr; @@ -59,7 +60,7 @@ public: static size_t num_instances; - static CAChannelPtr create(CAChannelProvider::shared_pointer const & channelProvider, + static shared_pointer create(CAChannelProvider::shared_pointer const & channelProvider, std::string const & channelName, short priority, ChannelRequester::shared_pointer const & channelRequester); @@ -111,7 +112,6 @@ public: void addChannelGet(const CAChannelGetPtr & get); void addChannelPut(const CAChannelPutPtr & get); void addChannelMonitor(const CAChannelMonitorPtr & get); - void disconnectChannel(); private: @@ -129,7 +129,6 @@ private: chid channelID; chtype channelType; unsigned elementCount; - bool channelCreated; epics::pvData::Structure::const_shared_pointer structure; epics::pvData::Mutex requestsMutex; diff --git a/src/ca/caProvider.cpp b/src/ca/caProvider.cpp index e7fb52e..3da9c69 100644 --- a/src/ca/caProvider.cpp +++ b/src/ca/caProvider.cpp @@ -52,28 +52,7 @@ CAChannelProvider::CAChannelProvider(const std::tr1::shared_ptr&) CAChannelProvider::~CAChannelProvider() { - if(DEBUG_LEVEL>0) { - std::cout << "CAChannelProvider::~CAChannelProvider()" - << " caChannelList.size() " << caChannelList.size() - << std::endl; - } - std::queue channelQ; - { - Lock lock(channelListMutex); - for(size_t i=0; i< caChannelList.size(); ++i) { - CAChannelPtr caChannel(caChannelList[i].lock()); - if(caChannel) channelQ.push(caChannel); - } - } - while(!channelQ.empty()) { - if(DEBUG_LEVEL>0) { - std::cout << "disconnectAllChannels calling disconnectChannel " - << channelQ.front()->getChannelName() - << std::endl; - } - channelQ.front()->disconnectChannel(); - channelQ.pop(); - } + if(DEBUG_LEVEL>0) std::cout << "CAChannelProvider::~CAChannelProvider()\n"; } std::string CAChannelProvider::getProviderName() @@ -133,23 +112,6 @@ Channel::shared_pointer CAChannelProvider::createChannel( return CAChannel::create(shared_from_this(), channelName, priority, channelRequester); } -void CAChannelProvider::addChannel(const CAChannelPtr & channel) -{ - if(DEBUG_LEVEL>0) { - std::cout << "CAChannelProvider::addChannel " - << channel->getChannelName() - << std::endl; - } - Lock lock(channelListMutex); - for(size_t i=0; i< caChannelList.size(); ++i) { - if(!(caChannelList[i].lock())) { - caChannelList[i] = channel; - return; - } - } - caChannelList.push_back(channel); -} - void CAChannelProvider::configure(epics::pvData::PVStructure::shared_pointer /*configuration*/) { } @@ -174,11 +136,13 @@ void CAChannelProvider::initialize() /* Create Channel Access */ int result = ca_context_create(ca_enable_preemptive_callback); if (result != ECA_NORMAL) { - throw std::runtime_error( - std::string("CA error %s occurred while trying to start channel access:") - + ca_message(result)); + throw std::runtime_error(std::string("CA error %s occurred while trying " + "to start channel access:") + ca_message(result)); } + current_context = ca_current_context(); + + // TODO create a ca_poll thread, if ca_disable_preemptive_callback } @@ -190,7 +154,7 @@ void ca_factory_cleanup(void*) ChannelProviderRegistry::clients()->remove("ca"); ca_context_destroy(); } catch(std::exception& e) { - LOG(logLevelWarn, "Error on unregistering \"ca\" factory: %s", e.what()); + LOG(logLevelWarn, "Error when unregister \"ca\" factory"); } } @@ -209,7 +173,7 @@ void CAClientFactory::start() registerRefCounter("CAChannelPut", &CAChannelPut::num_instances); registerRefCounter("CAChannelMonitor", &CAChannelMonitor::num_instances); - if(ChannelProviderRegistry::clients()->add("ca", true)) + if(ChannelProviderRegistry::clients()->add("ca", false)) { epicsAtExit(&ca_factory_cleanup, NULL); } diff --git a/src/ca/caProviderPvt.h b/src/ca/caProviderPvt.h index 6d01361..3e4b3a5 100644 --- a/src/ca/caProviderPvt.h +++ b/src/ca/caProviderPvt.h @@ -19,10 +19,6 @@ namespace ca { #define DEBUG_LEVEL 0 -class CAChannel; -typedef std::tr1::shared_ptr CAChannelPtr; -typedef std::tr1::weak_ptr CAChannelWPtr; - class CAChannelProvider; typedef std::tr1::shared_ptr CAChannelProviderPtr; typedef std::tr1::weak_ptr CAChannelProviderWPtr; @@ -68,18 +64,13 @@ public: virtual void destroy() EPICS_DEPRECATED {}; - void addChannel(const CAChannelPtr & get); - /* ---------------------------------------------------------------- */ void threadAttach(); - private: void initialize(); ca_client_context* current_context; - epics::pvData::Mutex channelListMutex; - std::vector caChannelList; }; }