[#2583] Addressed minor review comments

    Fixed the small stuff.

diff --git a/src/lib/tcp/tcp_connection.cc b/src/lib/tcp/tcp_connection.cc
index 17463fc9d368f18cc0e16bd4ed90e3fc7b261748..8c23280665912a4c9092bc8273589b48140ecfe6 100644 (file)
--- a/src/lib/tcp/tcp_connection.cc
+++ b/src/lib/tcp/tcp_connection.cc
@@ -65,7 +65,6 @@ TcpConnection::TcpConnection(asiolink::IOService& io_service,
       acceptor_callback_(callback),
       input_buf_(read_max) {
     if (!tls_context) {
-        HERE("");
         tcp_socket_.reset(new asiolink::TCPSocket<SocketCallback>(io_service));
     } else {
         tls_socket_.reset(new asiolink::TLSSocket<SocketCallback>(io_service,
@@ -74,7 +73,6 @@ TcpConnection::TcpConnection(asiolink::IOService& io_service,
 }
 
 TcpConnection::~TcpConnection() {
-    HERE("");
     close();
 }
 
@@ -151,7 +149,8 @@ TcpConnection::asyncAccept() {
     // of the callback, because the underlying boost functions make copies
     // as needed.
     TcpConnectionAcceptorCallback cb = std::bind(&TcpConnection::acceptorCallback,
-                                                 shared_from_this(), ph::_1);
+                                                 shared_from_this(),
+                                                 ph::_1);
     try {
         TlsConnectionAcceptorPtr tls_acceptor =
             boost::dynamic_pointer_cast<TlsConnectionAcceptor>(acceptor_);
@@ -159,7 +158,6 @@ TcpConnection::asyncAccept() {
             if (!tcp_socket_) {
                 isc_throw(Unexpected, "internal error: TCP socket is null");
             }
-            HERE("");
             acceptor_->asyncAccept(*tcp_socket_, cb);
         } else {
             if (!tls_socket_) {
@@ -177,12 +175,10 @@ void
 TcpConnection::doHandshake() {
     // Skip the handshake if the socket is not a TLS one.
     if (!tls_socket_) {
-        HERE("");
         doRead();
         return;
     }
 
-    HERE("setupIdleTimer");
     setupIdleTimer();
 
     // Create instance of the callback. It is safe to pass the local instance
@@ -203,10 +199,8 @@ TcpConnection::doHandshake() {
 void
 TcpConnection::doRead(TcpRequestPtr request) {
     try {
-        HERE("");
         TCPEndpoint endpoint;
 
-        HERE("setup Idle timer");
         setupIdleTimer();
 
         // Request hasn't been created if we are starting to read the
@@ -224,14 +218,12 @@ TcpConnection::doRead(TcpRequestPtr request) {
                                     ph::_1,   // error
                                     ph::_2)); // bytes_transferred
         if (tcp_socket_) {
-            HERE("tcp_socket read max bytes:" << getInputBufSize());
             tcp_socket_->asyncReceive(static_cast<void*>(getInputBufData()),
                                       getInputBufSize(), 0, &endpoint, cb);
             return;
         }
 
         if (tls_socket_) {
-            HERE("tls_socket read max bytes:" << getInputBufSize());
             tls_socket_->asyncReceive(static_cast<void*>(getInputBufData()),
                                       getInputBufSize(), 0, &endpoint, cb);
             return;
@@ -245,7 +237,6 @@ void
 TcpConnection::doWrite(TcpResponsePtr response) {
     try {
         if (response->wireDataAvail()) {
-            HERE("send:" << isc::util::str::dumpAsHex(response->getWireData(), response->getWireDataSize()));
             // Create instance of the callback. It is safe to pass the local instance
             // of the callback, because the underlying std functions make copies
             // as needed.
@@ -268,7 +259,6 @@ TcpConnection::doWrite(TcpResponsePtr response) {
             }
         } else {
             // The connection remains open and we are done sending the response.
-            HERE("setupIdleTimer - write finsihed");
             setupIdleTimer();
         }
     } catch (...) {
@@ -278,7 +268,6 @@ TcpConnection::doWrite(TcpResponsePtr response) {
 
 void
 TcpConnection::asyncSendResponse(TcpResponsePtr response) {
-    HERE("");
     doWrite(response);
 }
 
@@ -324,7 +313,6 @@ TcpConnection::handshakeCallback(const boost::system::error_code& ec) {
                   TCP_REQUEST_RECEIVE_START)
             .arg(getRemoteEndpointAddressAsText());
 
-        HERE("");
         doRead();
     }
 }
@@ -354,7 +342,6 @@ TcpConnection::socketReadCallback(TcpRequestPtr request,
     }
 
     // Data received, Restart the request timer.
-    HERE("setupIdleTimer - data recevied, post it")
     setupIdleTimer();
 
     TcpRequestPtr next_request = request;
@@ -394,7 +381,6 @@ TcpConnection::postData(TcpRequestPtr request, WireData& input_data) {
                 .arg(getRemoteEndpointAddressAsText());
 
         // Request complete, stop the timer.
-        HERE("Cancel idleTimer while we handle complete request");
         idle_timer_.cancel();
 
         // Process the completed request.
@@ -466,7 +452,6 @@ TcpConnection::idleTimeoutCallback() {
     LOG_DEBUG(tcp_logger, isc::log::DBGLVL_TRACE_DETAIL,
               TCP_IDLE_CONNECTION_TIMEOUT_OCCURRED)
         .arg(getRemoteEndpointAddressAsText());
-    HERE("idle timeout! shutting down");
     // In theory we should shutdown first and stop/close after but
     // it is better to put the connection management responsibility
     // on the client... so simply drop idle connections.

diff --git a/src/lib/tcp/tcp_connection.h b/src/lib/tcp/tcp_connection.h
index 2ba0661e9964b359cb28d29f9cfd6959c6e6231b..811e1d050344f99b2feb39b9e12923997969c0c8 100644 (file)
--- a/src/lib/tcp/tcp_connection.h
+++ b/src/lib/tcp/tcp_connection.h
@@ -25,11 +25,7 @@ namespace isc {
 namespace tcp {
 
 /// @todo Take this out, it's just for dev coding
-#if 0
 #define HERE(a) std::cout << __FILE__ << ":" << __FUNCTION__ << ":" << __LINE__ << " " << a << std::endl << std::flush;
-#else
-#define HERE(a)
-#endif
 
 /// @brief Defines a data structure for storing raw bytes of data on the wire.
 typedef std::vector<uint8_t> WireData;
@@ -51,7 +47,7 @@ public:
     /// @return Constant raw pointer to the data.
     const uint8_t* getWireData() const {
         if (wire_data_.empty()) {
-            isc_throw(InvalidOperation, "TcpMessage::geWireData() - cannot access empty wire data");
+            isc_throw(InvalidOperation, "TcpMessage::getWireData() - cannot access empty wire data");
         }
 
         return (wire_data_.data());
@@ -68,7 +64,7 @@ protected:
 };
 
 /// @brief Abstract class used to receive an inbound message.
-class TcpRequest : public TcpMessage{
+class TcpRequest : public TcpMessage {
 public:
     /// @brief Constructor.
     TcpRequest(){};
@@ -112,7 +108,7 @@ private:
 typedef boost::shared_ptr<TcpRequest> TcpRequestPtr;
 
 /// @brief Abstract class used to create and send an outbound response.
-class TcpResponse : public TcpMessage{
+class TcpResponse : public TcpMessage {
 public:
     /// @brief Constructor
     TcpResponse()

diff --git a/src/lib/tcp/tcp_stream.cc b/src/lib/tcp/tcp_stream.cc
index 364b24ec820b7fe6ecb0fdd7a159d3b31731afc5..fa04c2812a136f8b12346236169a827040af2075 100644 (file)
--- a/src/lib/tcp/tcp_stream.cc
+++ b/src/lib/tcp/tcp_stream.cc
@@ -18,13 +18,11 @@ namespace tcp {
 
 bool
 TcpStreamRequest::needData() const {
-    HERE("");
     return (!expected_size_ || (wire_data_.size() < expected_size_));
 }
 
 size_t
 TcpStreamRequest::postBuffer(const void* buf,  const size_t nbytes) {
-    HERE("");
     if (nbytes) {
         const char* bufptr = static_cast<const char*>(buf);
         wire_data_.insert(wire_data_.end(), bufptr, bufptr + nbytes);

diff --git a/src/lib/tcp/tests/Makefile.am b/src/lib/tcp/tests/Makefile.am
index 996d6f2248d882842d79ece77cc9d389327c3a2a..9cf127677a2475ee194e5272a2dca3804583d37b 100644 (file)
--- a/src/lib/tcp/tests/Makefile.am
+++ b/src/lib/tcp/tests/Makefile.am
@@ -24,6 +24,7 @@ run_unittests_SOURCES  = run_unittests.cc
 run_unittests_SOURCES += tcp_listener_unittests.cc
 run_unittests_SOURCES += tcp_test_client.h
 
+# @todo These tests will be needed. These are here as a reminder.
 #if HAVE_OPENSSL
 #run_unittests_SOURCES += tls_unittest.cc
 #run_unittests_SOURCES += tls_acceptor_unittest.cc

diff --git a/src/lib/tcp/tests/run_unittests.cc b/src/lib/tcp/tests/run_unittests.cc
index 70b03766b1f627978d010fa241ba8bf56621ff7d..55589a6632d0b0b81c042144ff02019e5c10cc5a 100644 (file)
--- a/src/lib/tcp/tests/run_unittests.cc
+++ b/src/lib/tcp/tests/run_unittests.cc
@@ -5,15 +5,16 @@
 // file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 #include <config.h>
+#include <log/logger_support.h>
 
 #include <gtest/gtest.h>
-#include <util/unittests/run_all.h>
-#include <log/logger_manager.h>
 
 int
-main(int argc, char* argv[])
-{
-    ::testing::InitGoogleTest(&argc, argv);         // Initialize Google test
-    isc::log::LoggerManager::init("unittest");      // Set a root logger name
-    return (isc::util::unittests::run_all());
+main(int argc, char* argv[]) {
+    ::testing::InitGoogleTest(&argc, argv);
+    isc::log::initLogger();
+
+    int result = RUN_ALL_TESTS();
+
+    return (result);
 }

diff --git a/src/lib/tcp/tests/tcp_listener_unittests.cc b/src/lib/tcp/tests/tcp_listener_unittests.cc
index d16bb4557aadff90d2aa8f173d025d1e7217396f..c69f051d9e716a63ad39b34e54a3971da9f2a576 100644 (file)
--- a/src/lib/tcp/tests/tcp_listener_unittests.cc
+++ b/src/lib/tcp/tests/tcp_listener_unittests.cc
@@ -321,7 +321,7 @@ TEST_F(TcpListenerTest, idleTimeoutTest) {
     TcpTestClientPtr client = *clients_.begin();
     ASSERT_TRUE(client);
 
-    // Tell the client expecting readingo to fail with an EOF.
+    // Tell the client expecting reading to fail with an EOF.
     ASSERT_NO_THROW(client->waitForEof());
 
     // Run until idle timer expires.

diff --git a/src/lib/tcp/tests/tcp_test_client.h b/src/lib/tcp/tests/tcp_test_client.h
index ac9e77f76e7791b57cc9c1143dce618b1ab5ede5..e3ea57c9465cec12b5c6dc2fdeab533a0bdb9f68 100644 (file)
--- a/src/lib/tcp/tests/tcp_test_client.h
+++ b/src/lib/tcp/tests/tcp_test_client.h
@@ -242,11 +242,13 @@ public:
                 } else if (ec.value() == boost::asio::error::eof && expect_eof)  {
                     expected_eof_ = true;
                     done_callback_();
+                    return;
                 } else {
                     // Error occurred, bail...
                     ADD_FAILURE() << "error occurred while receiving TCP"
                         " response from the server: " << ec.message();
                     done_callback_();
+                    return;
                 }
             }
 
@@ -276,6 +278,7 @@ public:
         if (response.find("good bye", 0) != std::string::npos) {
             receive_done_ = true;
             done_callback_();
+            return;
         }
 
         // Clear out for the next one.

diff --git a/src/lib/util/strutil.cc b/src/lib/util/strutil.cc
index b0025f06ac403cd9bf4208a7b891a7ca238d792c..50409612ec52f6688a2ecd436bb8672333ce0448 100644 (file)
--- a/src/lib/util/strutil.cc
+++ b/src/lib/util/strutil.cc
@@ -448,7 +448,7 @@ StringSanitizer::scrub(const std::string& original) {
     return (impl_->scrub(original));
 }
 
-std::string dumpAsHex(const uint8_t* data, size_t len) {
+std::string dumpAsHex(const uint8_t* data, size_t length) {
     std::stringstream output;
     for (unsigned int i = 0; i < len; i++) {
         if (i) {

diff --git a/src/lib/util/strutil.h b/src/lib/util/strutil.h
index 0cb967e3f79d8241ad9c80b257e7b0d53b5efccb..e5d2496a80e96437381f9e5f570cec373e0120fb 100644 (file)
--- a/src/lib/util/strutil.h
+++ b/src/lib/util/strutil.h
@@ -394,7 +394,7 @@ isPrintable(const std::vector<uint8_t>& content) {
 /// @param data pointer to the data to dump
 /// @param length number of bytes to dump. Caller should ensure the length
 /// does not exceed the buffer.
-std::string dumpAsHex(const uint8_t* data, size_t len);
+std::string dumpAsHex(const uint8_t* data, size_t length);
 
 } // namespace str
 } // namespace util