[3880] Changes after review:

 - UnixCommandSocket does not use ElementPtr anymore
 - strerror() is now used correctly
 - Doxygen comments corrected and updated

src/lib/config/command-socket.dox

@@ -23,7 +23,7 @@ Control Channel allows an external entity (e.g. a tool run by a sysadmin
 or a script) to issue commands to the server which can influence the its
 behavior or retreive information from it. Several notable examples
 envisioned are: reconfiguration, statistics retrival and manipulation,
-and shutdown.  
+and shutdown.
 @note Currently, only the DHCPv4 component supports Control Channel.
 @todo: Update this text once Control Channel support in DHCPv6 is added.
 
@@ -46,7 +46,7 @@ commands structured as follows:
 }
 @endcode
 
-- command - is the name of command to execute and is mandatory. 
+- command - is the name of command to execute and is mandatory.
 - arguments - it may be absent, contain a single parameter or a map or parameters
 required to carry out the given command.  The exact content and format is command specific.
 
@@ -69,7 +69,7 @@ any non-zero value designates an error. Currently 1 is used as a generic error,
 error codes may be added in the future.
 - text field - typically appears when result is non-zero and contains description of the error
 encountered.
-- arguments - is a map of additional data values returned by the server, specific to the 
+- arguments - is a map of additional data values returned by the server, specific to the
 command issue. The map is always present, even if it contains no data values.
 
 @section ctrlSocketClient Using Control Channel
@@ -173,24 +173,29 @@ or HTTPS connection):
   in the configuration file. Currently only two parameters are supported: socket-type
   (which must contain value 'unix') and socket-name (which contains unix path for
   the named socket to be created). This method calls @ref
-  isc::config::CommandSocketFactory::create method, which in turn calls type specific
-  creation method. Again, currently only UNIX type is supported, but the factory
+  isc::config::CommandSocketFactory::create method, which parses the parameters
+  and instantiates one object from a class derived from @ref isc::config::CommandSocket.
+  Again, currently only UNIX type is supported, but the factory
   class is expected to be extended to cover additional types.
 - @ref isc::config::CommandMgr::closeCommandSocket() - it is used to close the
-  socket. It calls @ref isc::config::CommandSocketFactory::close method that may
-  do type specific closure operations. In particular, for UNIX socket, it also
-  deletes the file after socket was closed.
+  socket. It calls close method on the @ref isc::config::CommandSocket object, if
+  one exists. In particular, for UNIX socket, it also deletes the file after socket
+  was closed.
 
 @section ctrlSocketConnections Accepting connections
 
 Control Channel is connection oriented communication. In that sense it is
 different than all other communications supported so far in Kea. To facilitate
-connections, several mechanisms were implemented. Once the control socket is opened,
-a special callback (@ref isc::config::CommandMgr::connectionAcceptor) is
-installed to process incoming connections. When the select call in
-@ref isc::dhcp::IfaceMgr::receive4 indicates that there is some data to be
+connections, several mechanisms were implemented. Intially a single UNIX socket
+it opened (see isc::config::UnixCommandSocket). Its @ref
+isc::config::UnixCommandSocket::receiveHandler callback method is
+installed in @ref isc::dhcp::IfaceMgr to process incoming connections. When the
+select call in @ref isc::dhcp::IfaceMgr::receive4 indicates that there is some data to be
 processed, this callback calls accept, which creates a new socket for handling
-this particular incoming connection. It installs another callback
+this particular incoming connection. Once the socket descriptor is known, a new
+instance of @ref isc::config::ConnectionSocket is created to represent that
+socket (and the whole ongoing connection). It installs another callback
+(@ref isc::config::ConnectionSocket::receiveHandler that calls
 (@ref isc::config::CommandMgr::commandReader) that will process incoming
 data or will close the socket when necessary. CommandReader reads data from
 incoming socket and attempts to parse it as JSON structures. If successful,

src/lib/config/command_mgr.h

@@ -129,9 +129,10 @@ public:
     /// @brief Reads data from a socket, parses as JSON command and processes it
     ///
     /// This method is used to handle traffic on connected socket. This callback
-    /// is installed by the @ref connectionAcceptor once the incoming connection
-    /// is accepted. If end-of-file is detected, this method will close the socket
-    /// and will uninstall itself from @ref isc::dhcp::IfaceMgr.
+    /// is installed by the @ref isc::config::UnixCommandSocket::receiveHandler
+    /// once the incoming connection is accepted. If end-of-file is detected, this
+    /// method will close the socket and will uninstall itself from
+    /// @ref isc::dhcp::IfaceMgr.
     ///
     /// @param sockfd socket descriptor of a connected socket
     static void commandReader(int sockfd);

src/lib/config/command_socket.cc

@@ -22,15 +22,14 @@
 namespace isc {
 namespace config {
 
-ConnectionSocket::ConnectionSocket(int sockfd)
-    :CommandSocket(isc::data::ConstElementPtr()) {
-        sockfd_ = sockfd;
-
-        // Install commandReader callback. When there's any data incoming on this
-        // socket, commandReader will be called and process it. It may also
-        // eventually close this socket.
-        isc::dhcp::IfaceMgr::instance().addExternalSocket(sockfd,
-            boost::bind(&ConnectionSocket::receiveHandler, this));
+ConnectionSocket::ConnectionSocket(int sockfd) {
+    sockfd_ = sockfd;
+
+    // Install commandReader callback. When there's any data incoming on this
+    // socket, commandReader will be called and process it. It may also
+    // eventually close this socket.
+    isc::dhcp::IfaceMgr::instance().addExternalSocket(sockfd,
+        boost::bind(&ConnectionSocket::receiveHandler, this));
     }
 
 void ConnectionSocket::close() {

src/lib/config/command_socket.h

@@ -37,8 +37,8 @@ public:
 
 /// @brief Abstract base class that represents an open command socket
 ///
-/// This class is not expected to be instantiated directly. Derived classes
-/// are expected to handle specific socket types (e.g. UNIX or https).
+/// Derived classes are expected to handle specific socket types (e.g. UNIX
+/// or https).
 ///
 /// For derived classes, see @ref UnixCommandSocket for a socket that
 /// accepts connections over UNIX socket and @ref ConnectionSocket that
@@ -46,13 +46,6 @@ public:
 /// should be generic).
 class CommandSocket {
 public:
-    /// @brief Default constructor
-    ///
-    /// @param socket_info socket information from the config
-    CommandSocket(const isc::data::ConstElementPtr& socket_info)
-        :socket_info_(socket_info) {
-    }
-
     /// @brief Method used to handle incoming data
     ///
     /// This may be registered in @ref isc::dhcp::IfaceMgr
@@ -80,15 +73,12 @@ public:
 protected:
     /// Stores socket descriptor.
     int sockfd_;
-
-    /// Stores socket information.
-    isc::data::ConstElementPtr socket_info_;
 };
 
 /// Pointer to a command socket object
 typedef boost::shared_ptr<CommandSocket> CommandSocketPtr;
 
-/// @brief This class represents a straming socket for handling connections
+/// @brief This class represents a streaming socket for handling connections
 ///
 /// Initially a socket (e.g. UNIX) is opened (represented by other classes, e.g.
 /// @ref UnixCommandSocket). Once incoming connection is detected, that class

src/lib/config/command_socket_factory.cc

@@ -41,20 +41,13 @@ class UnixCommandSocket : public CommandSocket {
 public:
     /// @brief Default constructor
     ///
-    /// This socket_info map is expected to have at least one parameter:
-    /// 'socket-name' that specifies UNIX socket path. It's typically
-    /// a file somewhere in /tmp or /var/run/kea directory.
+    /// Opens specified UNIX socket.
     ///
-    /// @param socket_info socket description from the configuration
-    UnixCommandSocket(const isc::data::ConstElementPtr& socket_info)
-        : CommandSocket(socket_info) {
-
-        ConstElementPtr name = socket_info->get("socket-name");
-        if (!name) {
-            isc_throw(BadSocketInfo, "Mandatory 'socket-name' parameter missing");
-        }
-        filename_ = name->stringValue();
+    /// @param filename socket filename
+    UnixCommandSocket(const std::string& filename)
+        : filename_(filename) {
 
+        // Create the socket and set it up.
         sockfd_ = createUnixSocket(filename_);
 
         // Install this socket in Interface Manager.
@@ -72,7 +65,8 @@ private:
 
         int fd = socket(AF_UNIX, SOCK_STREAM, 0);
         if (fd == -1) {
-            isc_throw(isc::config::SocketError, "Failed to create AF_UNIX socket.");
+            isc_throw(isc::config::SocketError, "Failed to create AF_UNIX socket:"
+                      << strerror(errno));
         }
 
         // Let's remove the old file. We don't care about any possible
@@ -82,8 +76,10 @@ private:
 
         // Set this socket to be non-blocking one.
         if (fcntl(fd, F_SETFL, O_NONBLOCK) !=0 ) {
+            const char* errmsg = strerror(errno);
+            ::close(fd);
             isc_throw(SocketError, "Failed to set non-block mode on unix socket "
-                      << fd << ": " << strerror(errno));
+                      << fd << ": " << errmsg);
         }
 
         // Now bind the socket to the specified path.
@@ -92,10 +88,11 @@ private:
         addr.sun_family = AF_UNIX;
         strncpy(addr.sun_path, file_name.c_str(), sizeof(addr.sun_path)-1);
         if (bind(fd, (struct sockaddr*)&addr, sizeof(addr))) {
+            const char* errmsg = strerror(errno);
             ::close(fd);
             remove(file_name.c_str());
             isc_throw(isc::config::SocketError, "Failed to bind socket " << fd
-                      << " to " << file_name << ": " << strerror(errno));
+                      << " to " << file_name << ": " << errmsg);
         }
 
         // One means that we allow at most 1 awaiting connections.
@@ -105,11 +102,11 @@ private:
         /// @todo: Make the number of parallel connections configurable.
         int status = listen(fd, 1);
         if (status < 0) {
+            const char* errmsg = strerror(errno);
             ::close(fd);
             remove(file_name.c_str());
             isc_throw(isc::config::SocketError, "Failed to listen on socket fd="
-                      << fd << ", filename=" << file_name << ": "
-                      << strerror(errno));
+                      << fd << ", filename=" << file_name << ": " << errmsg);
         }
 
         // Woohoo! Socket opened, let's log it!
@@ -118,12 +115,12 @@ private:
         return (fd);
     }
 
-    /// CommandMgr::connectionAcceptor(int sockfd) {
-    /// @brief Callback used to accept incoming connections.
+    /// @brief Connection acceptor, a callback used to accept incoming connections.
     ///
     /// This callback is used on a control socket. Once called, it will accept
-    /// incoming connection, create new socket for it and install
-    /// @ref commandReader for that new socket in @ref isc::dhcp::IfaceMgr.
+    /// incoming connection, create a new socket for it and create an instance
+    /// of ConnectionSocket, which will take care of the rest (i.e. install
+    /// appropriate callback for that new socket in @ref isc::dhcp::IfaceMgr).
     void receiveHandler() {
 
         // This method is specific to receiving data over UNIX socket, so using
@@ -193,7 +190,17 @@ CommandSocketFactory::create(const isc::data::ConstElementPtr& socket_info) {
     }
 
     if (type->stringValue() == "unix") {
-        return (CommandSocketPtr(new UnixCommandSocket(socket_info)));
+        // UNIX socket is requested. It takes one parameter: socket-name that
+        // specifies UNIX path of the socket.
+        ConstElementPtr name = socket_info->get("socket-name");
+        if (!name) {
+            isc_throw(BadSocketInfo, "Mandatory 'socket-name' parameter missing");
+        }
+        if (name->getType() != Element::string) {
+            isc_throw(BadSocketInfo, "'socket-name' parameter expected to be a string");
+        }
+
+        return (CommandSocketPtr(new UnixCommandSocket(name->stringValue())));
     } else {
         isc_throw(BadSocketInfo, "Specified socket type ('" + type->stringValue()
                   + "') is not supported.");