[5046] CommandMgr dups the connection socket prior to executing command

src/lib/config/command_mgr.cc
    CommandMgr::commandReader(int sockfd) - duplicates the connection
    socket to use for repsonding in case the command closes the channel.

src/lib/testutils/io_utils.cc
    fileExists() - now uses stat() function so one can use it on any
    type of file, like a unix socket

updated unit tests accordingly

diff --git a/src/bin/dhcp4/tests/ctrl_dhcp4_srv_unittest.cc b/src/bin/dhcp4/tests/ctrl_dhcp4_srv_unittest.cc
index 7e0b6acb54f2fc7d3e5069bd65e782ce871e0fd1..47de13d40ead33b1f4b641e1ddcd3f274e7385c7 100644 (file)
--- a/src/bin/dhcp4/tests/ctrl_dhcp4_srv_unittest.cc
+++ b/src/bin/dhcp4/tests/ctrl_dhcp4_srv_unittest.cc
@@ -16,6 +16,7 @@
 #include <hooks/hooks_manager.h>
 #include <log/logger_support.h>
 #include <stats/stats_mgr.h>
+#include <testutils/io_utils.h>
 #include <testutils/unix_control_client.h>
 
 #include "marker_file.h"
@@ -592,7 +593,7 @@ TEST_F(CtrlChannelDhcpv4SrvTest, set_config) {
     EXPECT_EQ(1, subnets->size());
 
     // Create a valid config with two subnets and no command channel.
-    // It should succeed but client will not receive a the response
+    // It should succeed, client should still receive the response
     os.str("");
     os << set_config_txt << ","
         << args_txt
@@ -604,11 +605,18 @@ TEST_F(CtrlChannelDhcpv4SrvTest, set_config) {
         << "}\n"                      // close dhcp4
         << "}}";
 
+    /* Verify the control channel socket exists */
+    ASSERT_TRUE(fileExists(socket_path_));
+
     // Send the set-config command
     sendUnixCommand(os.str(), response);
 
-    // With no command channel, no response
-    EXPECT_EQ("", response);
+    /* Verify the control channel socket no longer exists */
+    EXPECT_FALSE(fileExists(socket_path_));
+
+    // With no command channel, should still receive the response.
+    EXPECT_EQ("{ \"result\": 0, \"text\": \"Configuration successful.\" }",
+              response);
 
     // Check that the config was not lost
     subnets = CfgMgr::instance().getCurrentCfg()->getCfgSubnets4()->getAll();

diff --git a/src/bin/dhcp6/tests/ctrl_dhcp6_srv_unittest.cc b/src/bin/dhcp6/tests/ctrl_dhcp6_srv_unittest.cc
index 7c9fbe97d45f3407a0187bab6245fa644aba4fa4..3f1331374c53f470e9be8b0af83a9947c7daf86e 100644 (file)
--- a/src/bin/dhcp6/tests/ctrl_dhcp6_srv_unittest.cc
+++ b/src/bin/dhcp6/tests/ctrl_dhcp6_srv_unittest.cc
@@ -18,6 +18,7 @@
 #include <log/logger_support.h>
 #include <stats/stats_mgr.h>
 #include <testutils/unix_control_client.h>
+#include <testutils/io_utils.h>
 
 #include "marker_file.h"
 #include "test_libraries.h"
@@ -26,6 +27,7 @@
 #include <gtest/gtest.h>
 
 #include <sys/select.h>
+#include <sys/stat.h>
 #include <sys/ioctl.h>
 #include <cstdlib>
 
@@ -468,11 +470,18 @@ TEST_F(CtrlChannelDhcpv6SrvTest, set_config) {
         << "}\n"                      // close dhcp6
         << "}}";
 
+    /* Verify the control channel socket exists */
+    ASSERT_TRUE(fileExists(socket_path_));
+
     // Send the set-config command
     sendUnixCommand(os.str(), response);
 
-    // With no command channel, no response
-    EXPECT_EQ("", response);
+    /* Verify the control channel socket no longer exists */
+    EXPECT_FALSE(fileExists(socket_path_));
+
+    // With no command channel, should still receive the response.
+    EXPECT_EQ("{ \"result\": 0, \"text\": \"Configuration successful.\" }",
+              response);
 
     // Check that the config was not lost
     subnets = CfgMgr::instance().getCurrentCfg()->getCfgSubnets6()->getAll();

diff --git a/src/lib/config/command_mgr.cc b/src/lib/config/command_mgr.cc
index ae33188014de8ec205db30e04518642d5e067c09..c3e3b43fe6676bf43c5af7bface3eb54ffe87bc2 100644 (file)
--- a/src/lib/config/command_mgr.cc
+++ b/src/lib/config/command_mgr.cc
@@ -11,6 +11,7 @@
 #include <dhcp/iface_mgr.h>
 #include <config/config_log.h>
 #include <boost/bind.hpp>
+#include <unistd.h>
 
 using namespace isc::data;
 
@@ -145,6 +146,19 @@ CommandMgr::commandReader(int sockfd) {
         return;
     }
 
+    // Duplicate the connection's socket in the event, the command causes the
+    // channel to close (like a reconfig).  This permits us to always have
+    // a socket on which to respond. If for some reason  we can't fall back
+    // to the connection socket.
+    int rsp_fd = dup(sockfd);
+    if (rsp_fd < 0 ) {
+        // Highly unlikely
+        const char* errmsg = strerror(errno);
+        LOG_DEBUG(command_logger, DBG_COMMAND, COMMAND_SOCKET_DUP_WARN)
+                  .arg(errmsg);
+        rsp_fd = sockfd;
+    }
+
     LOG_DEBUG(command_logger, DBG_COMMAND, COMMAND_SOCKET_READ).arg(rval).arg(sockfd);
 
     // Ok, we received something. Let's see if we can make any sense of it.
@@ -163,6 +177,11 @@ CommandMgr::commandReader(int sockfd) {
 
     if (!rsp) {
         LOG_WARN(command_logger, COMMAND_RESPONSE_ERROR);
+        // Only close the dupped socket if it's different (should be)
+        if (rsp_fd != sockfd) {
+            close(rsp_fd);
+        }
+
         return;
     }
 
@@ -179,7 +198,8 @@ CommandMgr::commandReader(int sockfd) {
     }
 
     // Send the data back over socket.
-    rval = write(sockfd, txt.c_str(), len);
+    rval = write(rsp_fd, txt.c_str(), len);
+    int saverr = errno;
 
     LOG_DEBUG(command_logger, DBG_COMMAND, COMMAND_SOCKET_WRITE).arg(len).arg(sockfd);
 
@@ -187,7 +207,13 @@ CommandMgr::commandReader(int sockfd) {
         // Response transmission failed. Since the response failed, it doesn't
         // make sense to send any status codes. Let's log it and be done with
         // it.
-        LOG_ERROR(command_logger, COMMAND_SOCKET_WRITE_FAIL).arg(len).arg(sockfd);
+        LOG_ERROR(command_logger, COMMAND_SOCKET_WRITE_FAIL)
+                  .arg(len).arg(sockfd).arg(strerror(saverr));
+    }
+
+    // Only close the dupped socket if it's different (should be)
+    if (rsp_fd != sockfd) {
+        close(rsp_fd);
     }
 }
 

diff --git a/src/lib/config/config_messages.mes b/src/lib/config/config_messages.mes
index 2a4558fd853c0b3566fe19910f03906ae5e4c9bb..ef7c0928819ef6cb90860d01d2f870af44881181 100644 (file)
--- a/src/lib/config/config_messages.mes
+++ b/src/lib/config/config_messages.mes
@@ -57,6 +57,13 @@ This error message indicates that the server failed to set non-blocking mode
 on just created socket. That socket was created for accepting specific
 incoming connection. Additional information may be provided as third parameter.
 
+% COMMAND_SOCKET_DUP_WARN Failed to duplicate socket for response: %1
+This debug message indicates that the commandReader was unable to duplicate
+the connection socket prior to executing the command. This is most likely a
+system resource issue.  The command should still be processed and the response
+sent, unless the command caused the command channel to be closed (e.g. a
+reconfiguration command).
+
 % COMMAND_SOCKET_READ Received %1 bytes over command socket %2
 This debug message indicates that specified number of bytes was received
 over command socket identified by specified file descriptor.
@@ -86,6 +93,6 @@ descriptor and path specified.
 This debug message indicates that the specified number of bytes was sent
 over command socket identifier by the specified file descriptor.
 
-% COMMAND_SOCKET_WRITE_FAIL Error while writing %1 bytes to command socket %2
+% COMMAND_SOCKET_WRITE_FAIL Error while writing %1 bytes to command socket %2 : %3
 This error message indicates that an error was encountered while
 attempting to send a response to the command socket.

diff --git a/src/lib/testutils/io_utils.cc b/src/lib/testutils/io_utils.cc
index 54aff4cb6fef4da447b23eed3d59804b1ab7f02d..caf9437071535c9a48d79a220a9b70e5f091cd03 100644 (file)
--- a/src/lib/testutils/io_utils.cc
+++ b/src/lib/testutils/io_utils.cc
@@ -15,10 +15,8 @@ namespace dhcp {
 namespace test {
 
 bool fileExists(const std::string& file_path) {
-    std::ifstream fs(file_path.c_str());
-    const bool file_exists = fs.good();
-    fs.close();
-    return (file_exists);
+    struct stat statbuf;
+    return(stat(file_path.c_str(), &statbuf) == 0);
 }
 
 std::string readFile(const std::string& file_path) {

diff --git a/src/lib/testutils/io_utils.h b/src/lib/testutils/io_utils.h
index 8cf1ef6e1431e9ca100b2f183e5a14f28e171259..61a18f8c79f5197a840e8229de14ceff0a487b12 100644 (file)
--- a/src/lib/testutils/io_utils.h
+++ b/src/lib/testutils/io_utils.h
@@ -8,6 +8,7 @@
 #define TEST_IO_UTILS_H
 
 #include <string>
+#include <sys/stat.h>
 
 namespace isc {
 namespace dhcp {