[#691,!395] More review comments

src/hooks/dhcp/high_availability/ha_messages.mes
    Removed HA_SERVICE_CONNECT_INVALID_SOCKET message

src/hooks/dhcp/high_availability/ha_service.cc
    HAService::clientConnectHandler() - now just avoids registering
    an invalid FD with no log and return(true)

src/lib/http/client.cc
    Added commen in Connection::close()

src/lib/http/tests/server_client_unittests.cc
    Removed invalid FD failure in test

diff --git a/src/hooks/dhcp/high_availability/ha_messages.cc b/src/hooks/dhcp/high_availability/ha_messages.cc
index 52fc6d53b5ef1e838a66354d9805ca6f996f21c1..708db8faca55d033b8f4732ba0425fe1b55fd10c 100644 (file)
--- a/src/hooks/dhcp/high_availability/ha_messages.cc
+++ b/src/hooks/dhcp/high_availability/ha_messages.cc
@@ -1,4 +1,4 @@
-// File created from ../../../../src/hooks/dhcp/high_availability/ha_messages.mes on Wed Jun 26 2019 09:25
+// File created from ../../../../src/hooks/dhcp/high_availability/ha_messages.mes on Wed Jun 26 2019 15:13
 
 #include <cstddef>
 #include <log/message_types.h>
@@ -60,7 +60,6 @@ extern const isc::log::MessageID HA_LOCAL_DHCP_DISABLE = "HA_LOCAL_DHCP_DISABLE"
 extern const isc::log::MessageID HA_LOCAL_DHCP_ENABLE = "HA_LOCAL_DHCP_ENABLE";
 extern const isc::log::MessageID HA_MISSING_CONFIGURATION = "HA_MISSING_CONFIGURATION";
 extern const isc::log::MessageID HA_SCOPES_HANDLER_FAILED = "HA_SCOPES_HANDLER_FAILED";
-extern const isc::log::MessageID HA_SERVICE_CONNECT_INVALID_SOCKET = "HA_SERVICE_CONNECT_INVALID_SOCKET";
 extern const isc::log::MessageID HA_SERVICE_STARTED = "HA_SERVICE_STARTED";
 extern const isc::log::MessageID HA_STATE_MACHINE_CONTINUED = "HA_STATE_MACHINE_CONTINUED";
 extern const isc::log::MessageID HA_STATE_MACHINE_PAUSED = "HA_STATE_MACHINE_PAUSED";
@@ -130,7 +129,6 @@ const char* values[] = {
     "HA_LOCAL_DHCP_ENABLE", "local DHCP service is enabled while the %1 is in the %2 state",
     "HA_MISSING_CONFIGURATION", "high-availability parameter not specified for High Availability hooks library",
     "HA_SCOPES_HANDLER_FAILED", "ha-scopes command failed: %1",
-    "HA_SERVICE_CONNECT_INVALID_SOCKET", "Attempted to register an invalid socket, error code: %1",
     "HA_SERVICE_STARTED", "started high availability service in %1 mode as %2 server",
     "HA_STATE_MACHINE_CONTINUED", "state machine is un-paused",
     "HA_STATE_MACHINE_PAUSED", "state machine paused in state %1",

diff --git a/src/hooks/dhcp/high_availability/ha_messages.h b/src/hooks/dhcp/high_availability/ha_messages.h
index 14337ab4bbc807ff50535825b21a964121fdfd13..bbe359fe03e73272df29db66ed16d9642909bc1e 100644 (file)
--- a/src/hooks/dhcp/high_availability/ha_messages.h
+++ b/src/hooks/dhcp/high_availability/ha_messages.h
@@ -1,4 +1,4 @@
-// File created from ../../../../src/hooks/dhcp/high_availability/ha_messages.mes on Wed Jun 26 2019 09:25
+// File created from ../../../../src/hooks/dhcp/high_availability/ha_messages.mes on Wed Jun 26 2019 15:13
 
 #ifndef HA_MESSAGES_H
 #define HA_MESSAGES_H
@@ -61,7 +61,6 @@ extern const isc::log::MessageID HA_LOCAL_DHCP_DISABLE;
 extern const isc::log::MessageID HA_LOCAL_DHCP_ENABLE;
 extern const isc::log::MessageID HA_MISSING_CONFIGURATION;
 extern const isc::log::MessageID HA_SCOPES_HANDLER_FAILED;
-extern const isc::log::MessageID HA_SERVICE_CONNECT_INVALID_SOCKET;
 extern const isc::log::MessageID HA_SERVICE_STARTED;
 extern const isc::log::MessageID HA_STATE_MACHINE_CONTINUED;
 extern const isc::log::MessageID HA_STATE_MACHINE_PAUSED;

diff --git a/src/hooks/dhcp/high_availability/ha_messages.mes b/src/hooks/dhcp/high_availability/ha_messages.mes
index 83a9231378bd3dbc510fbb0af61890f6195910b8..85ba97bea46eae692f8c9655512d6bc40b092a16 100644 (file)
--- a/src/hooks/dhcp/high_availability/ha_messages.mes
+++ b/src/hooks/dhcp/high_availability/ha_messages.mes
@@ -116,11 +116,6 @@ failure.
 This informational message indicates that the High Availability hooks library
 has been unloaded successfully.
 
-% HA_SERVICE_CONNECT_INVALID_SOCKET Attempted to register an invalid socket, error code: %1
-This is an error message that indicates that the server attempted to
-register an invalid socket with the Interface Manager. This is an internal
-server error and a bug report should be created.
-
 % HA_DHCP4_START_SERVICE_FAILED failed to start DHCPv4 HA service in dhcp4_srv_configured callout: %1
 This error message is issued when an attempt to start High Availability service
 for the DHCPv4 server failed in the dhcp4_srv_configured callout. This

diff --git a/src/hooks/dhcp/high_availability/ha_service.cc b/src/hooks/dhcp/high_availability/ha_service.cc
index f3188b3019057889b685c6aa1adf53f27d9484fb..4589a8ad57a04378882d7517940ac3aa9ebe796a 100644 (file)
--- a/src/hooks/dhcp/high_availability/ha_service.cc
+++ b/src/hooks/dhcp/high_availability/ha_service.cc
@@ -1621,19 +1621,17 @@ HAService::verifyAsyncResponse(const HttpResponsePtr& response) {
 
 bool
 HAService::clientConnectHandler(const boost::system::error_code& ec, int tcp_native_fd) {
-    if (!ec || (ec.value() == boost::asio::error::in_progress)) {
 
-        if (tcp_native_fd < 0) {
-            // This really should not be possible, but just in case.
-            LOG_ERROR(ha_logger, HA_SERVICE_CONNECT_INVALID_SOCKET)
-                      .arg(ec.value());
-            return (false);
-        }
-   
-        // Register the socket with Interface Manager. 
-        // External socket callback is a NOP. Ready events handlers are run by an
-        // explicit call IOService ready in kea-dhcp<n> code. We are registering
-        // the socket only to interrupt main-thread select().
+    // If things look ok register the socket with Interface Manager. Note
+    // we don't register if the FD is < 0 to avoid an expection throw.
+    // It is unlikely that this will occur but we want to be liberal
+    // and avoid issues.
+    if ((!ec || (ec.value() == boost::asio::error::in_progress))
+        && (tcp_native_fd >= 0)) {
+        // External socket callback is a NOP. Ready events handlers are
+        // run by an explicit call IOService ready in kea-dhcp<n> code.
+        // We are registerin the socket only to interrupt main-thread
+        // select().
         IfaceMgr::instance().addExternalSocket(tcp_native_fd, 0);
     }
 

diff --git a/src/lib/http/client.cc b/src/lib/http/client.cc
index a45236fb7e1af227c9b59473a9a4504a96107912..50a581fcfbaf712ca5c00b6be428938b59cf0d90 100644 (file)
--- a/src/lib/http/client.cc
+++ b/src/lib/http/client.cc
@@ -594,6 +594,7 @@ Connection::doTransaction(const HttpRequestPtr& request,
 
 void
 Connection::close() {
+    // Pass in true to discard the callback.
     closeCallback(true);
 
     timer_.cancel();

diff --git a/src/lib/http/tests/server_client_unittests.cc b/src/lib/http/tests/server_client_unittests.cc
index ad705fa39eccf25c0b16c55d821fe6529f447cc5..dd846eb131f959b9aa61d3f2e81e9f0b27ca5f35 100644 (file)
--- a/src/lib/http/tests/server_client_unittests.cc
+++ b/src/lib/http/tests/server_client_unittests.cc
@@ -1473,21 +1473,13 @@ public:
         /// @param tcp_native_fd socket descriptor to register
         bool connectHandler(const boost::system::error_code& ec, int tcp_native_fd) {
             ++connect_cnt_;
-            if (!ec || ec.value() == boost::asio::error::in_progress) {
-                if (tcp_native_fd >= 0) {
-                    registered_fd_ = tcp_native_fd;
-                    return (true);
-                }
-
-                // Invalid fd?, this really should not be possible.  EXPECT makes
-                // sure we log it.
-                EXPECT_TRUE (tcp_native_fd >= 0) << "no ec error but invalid fd?";
+            if ((!ec || (ec.value() == boost::asio::error::in_progress))
+                && (tcp_native_fd >= 0)) {
+                registered_fd_ = tcp_native_fd;
+                return (true);
+            } else if ((ec.value() == boost::asio::error::already_connected)
+                       && (registered_fd_ != tcp_native_fd)) {
                 return (false);
-
-            } else if (ec.value() == boost::asio::error::already_connected) {
-                if (registered_fd_ != tcp_native_fd) {
-                    return (false);
-                }
             }
 
             // ec indicates an error, return true, so that error can be handled