[#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

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",

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;

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

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) {
+    // If things look ok register the socket with Interface Manager. Note
-            // This really should not be possible, but just in case.
+    // we don't register if the FD is < 0 to avoid an expection throw.
-            LOG_ERROR(ha_logger, HA_SERVICE_CONNECT_INVALID_SOCKET)
+    // It is unlikely that this will occur but we want to be liberal
-                      .arg(ec.value());
+    // and avoid issues.
-            return (false);
+    if ((!ec || (ec.value() == boost::asio::error::in_progress))
-        }
+        && (tcp_native_fd >= 0)) {
-   
+        // External socket callback is a NOP. Ready events handlers are
-        // Register the socket with Interface Manager. 
+        // run by an explicit call IOService ready in kea-dhcp<n> code.
-        // External socket callback is a NOP. Ready events handlers are run by an
+        // We are registerin the socket only to interrupt main-thread
-        // explicit call IOService ready in kea-dhcp<n> code. We are registering
+        // select().
-        // the socket only to interrupt main-thread select().
         IfaceMgr::instance().addExternalSocket(tcp_native_fd, 0);
     }
 

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();

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 ((!ec || (ec.value() == boost::asio::error::in_progress))
-                if (tcp_native_fd >= 0) {
+                && (tcp_native_fd >= 0)) {
-                    registered_fd_ = tcp_native_fd;
+                registered_fd_ = tcp_native_fd;
-                    return (true);
+                return (true);
-                }
+            } else if ((ec.value() == boost::asio::error::already_connected)
-
+                       && (registered_fd_ != tcp_native_fd)) {
-                // 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?";
                 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