From: Thomas Markwalder Date: Tue, 8 Nov 2022 12:35:39 +0000 (-0500) Subject: [#2583] Addressed minor review comments X-Git-Tag: Kea-2.3.3~93 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=9217ca69f9db8d0f8684f80b2f7aad487a4dbd46;p=thirdparty%2Fkea.git [#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 17463fc9d3..8c23280665 100644 --- 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(io_service)); } else { tls_socket_.reset(new asiolink::TLSSocket(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(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(getInputBufData()), getInputBufSize(), 0, &endpoint, cb); return; } if (tls_socket_) { - HERE("tls_socket read max bytes:" << getInputBufSize()); tls_socket_->asyncReceive(static_cast(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 2ba0661e99..811e1d0503 100644 --- 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 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 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 364b24ec82..fa04c2812a 100644 --- 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(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 996d6f2248..9cf127677a 100644 --- 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 70b03766b1..55589a6632 100644 --- 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 +#include #include -#include -#include 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 d16bb4557a..c69f051d9e 100644 --- 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 ac9e77f76e..e3ea57c946 100644 --- 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 b0025f06ac..50409612ec 100644 --- 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 0cb967e3f7..e5d2496a80 100644 --- a/src/lib/util/strutil.h +++ b/src/lib/util/strutil.h @@ -394,7 +394,7 @@ isPrintable(const std::vector& 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