Merge ongoing #1593 into #1534

To minimize future conflicts, the #1593 (sockcreator cleanup) might have
severe impact on the code.

src/bin/sockcreator/main.cc

@@ -22,5 +22,11 @@ main() {
      * TODO Maybe use some OS-specific caps interface and drop everything
      * but ability to bind ports? It would be nice.
      */
-    return run(0, 1); // Read commands from stdin, output to stdout
+    int status = 0;
+    try {
+        run(0, 1); // Read commands from stdin, output to stdout
+    } catch (const SocketCreatorError& ec) {
+        status = ec.getExitCode();
+    }
+    return (status);
 }

src/bin/sockcreator/sockcreator.cc

@@ -16,150 +16,201 @@
 
 #include <util/io/fd.h>
 
-#include <unistd.h>
 #include <cerrno>
-#include <string.h>
+#include <cstring>
+
+#include <unistd.h>
 #include <sys/types.h>
 #include <sys/socket.h>
 #include <netinet/in.h>
 
 using namespace isc::util::io;
+using namespace isc::socket_creator;
+
+namespace {
+
+// Simple wrappers for read_data/write_data that throw an exception on error.
+void
+readMessage(const int fd, void* where, const size_t length) {
+    if (read_data(fd, where, length) < length) {
+        isc_throw(ReadError, "Error reading from socket creator client");
+    }
+}
+
+void
+writeMessage(const int fd, const void* what, const size_t length) {
+    if (!write_data(fd, what, length)) {
+        isc_throw(WriteError, "Error writing to socket creator client");
+    }
+}
+
+// Exit on a protocol error after informing the client of the problem.
+void
+protocolError(const int fd, const char reason = 'I') {
+
+    // Tell client we have a problem
+    char message[2];
+    message[0] = 'F';
+    message[1] = reason;
+    writeMessage(fd, message, sizeof(message));
+
+    // ... and exit
+    isc_throw(ProtocolError, "Fatal error, reason: " << reason);
+}
+
+// Handle the request from the client.
+//
+// Reads the type and family of socket required, creates the socket and returns
+// it to the client.
+//
+// The arguments passed (and the exceptions thrown) are the same as those for
+// run().
+void
+handleRequest(const int input_fd, const int output_fd,
+              const get_sock_t get_sock, const send_fd_t send_fd_fun,
+              const close_t close_fun)
+{
+    // Read the message from the client
+    char type[2];
+    readMessage(input_fd, type, sizeof(type));
+
+    // Decide what type of socket is being asked for
+    int sock_type = 0;
+    switch (type[0]) {
+        case 'T':
+            sock_type = SOCK_STREAM;
+            break;
+
+        case 'U':
+            sock_type = SOCK_DGRAM;
+            break;
+
+        default:
+            protocolError(output_fd);
+    }
+
+    // Read the address they ask for depending on what address family was
+    // specified.
+    sockaddr* addr = NULL;
+    size_t addr_len = 0;
+    sockaddr_in addr_in;
+    sockaddr_in6 addr_in6;
+    switch (type[1]) { // The address family
+
+        // The casting to apparently incompatible types by reinterpret_cast
+        // is required by the C low-level interface.
+
+        case '4':
+            addr = reinterpret_cast<sockaddr*>(&addr_in);
+            addr_len = sizeof(addr_in);
+            memset(&addr_in, 0, sizeof(addr_in));
+            addr_in.sin_family = AF_INET;
+            readMessage(input_fd, &addr_in.sin_port, sizeof(addr_in.sin_port));
+            readMessage(input_fd, &addr_in.sin_addr.s_addr,
+                        sizeof(addr_in.sin_addr.s_addr));
+            break;
+
+        case '6':
+            addr = reinterpret_cast<sockaddr*>(&addr_in6);
+            addr_len = sizeof addr_in6;
+            memset(&addr_in6, 0, sizeof(addr_in6));
+            addr_in6.sin6_family = AF_INET6;
+            readMessage(input_fd, &addr_in6.sin6_port,
+                        sizeof(addr_in6.sin6_port));
+            readMessage(input_fd, &addr_in6.sin6_addr.s6_addr,
+                        sizeof(addr_in6.sin6_addr.s6_addr));
+            break;
+
+        default:
+            protocolError(output_fd);
+    }
+
+    // Obtain the socket
+    const int result = get_sock(sock_type, addr, addr_len);
+    if (result >= 0) {
+        // Got the socket, send it to the client.
+        writeMessage(output_fd, "S", 1);
+        if (send_fd_fun(output_fd, result) != 0) {
+            // Error.  Close the socket (ignore any error from that operation)
+            // and abort.
+            close_fun(result);
+            isc_throw(InternalError, "Error sending descriptor");
+        }
+
+        // Successfully sent the socket, so free up resources we still hold
+        // for it.
+        if (close_fun(result) == -1) {
+            isc_throw(InternalError, "Error closing socket");
+        }
+    } else {
+        // Error.  Tell the client.
+        writeMessage(output_fd, "E", 1);           // Error occurred...
+        switch (result) {
+            case -1:
+                writeMessage(output_fd, "S", 1);   // ... in the socket() call
+                break;
+
+            case -2:
+                writeMessage(output_fd, "B", 1);   // ... in the bind() call
+                break;
+
+            default:
+                isc_throw(InternalError, "Error creating socket");
+        }
+
+        // ...and append the reason code to the error message
+        const int error = errno;
+        writeMessage(output_fd, &error, sizeof(error));
+    }
+}
+
+} // Anonymous namespace
 
 namespace isc {
 namespace socket_creator {
 
+// Get the socket and bind to it.
 int
-get_sock(const int type, struct sockaddr *bind_addr, const socklen_t addr_len)
-{
-    int sock(socket(bind_addr->sa_family, type, 0));
+getSock(const int type, struct sockaddr* bind_addr, const socklen_t addr_len) {
+    const int sock = socket(bind_addr->sa_family, type, 0);
     if (sock == -1) {
-        return -1;
+        return (-1);
     }
-    const int on(1);
+    const int on = 1;
     if (setsockopt(sock, SOL_SOCKET, SO_REUSEADDR, &on, sizeof(on)) == -1) {
-        return -2; // This is part of the binding process, so it's a bind error
+        // This is part of the binding process, so it's a bind error
+        return (-2);
     }
     if (bind_addr->sa_family == AF_INET6 &&
         setsockopt(sock, IPPROTO_IPV6, IPV6_V6ONLY, &on, sizeof(on)) == -1) {
-        return -2; // This is part of the binding process, so it's a bind error
+        // This is part of the binding process, so it's a bind error
+        return (-2);
     }
     if (bind(sock, bind_addr, addr_len) == -1) {
-        return -2;
+        return (-2);
     }
-    return sock;
+    return (sock);
 }
 
-// These are macros so they can exit the function
-#define READ(WHERE, HOW_MANY) do { \
-        size_t how_many = (HOW_MANY); \
-        if (read_data(input_fd, (WHERE), how_many) < how_many) { \
-            return 1; \
-        } \
-    } while (0)
-
-#define WRITE(WHAT, HOW_MANY) do { \
-        if (!write_data(output_fd, (WHAT), (HOW_MANY))) { \
-            return 2; \
-        } \
-    } while (0)
-
-#define DEFAULT \
-    default: /* Unrecognized part of protocol */ \
-        WRITE("FI", 2); \
-        return 3;
-
-int
+// Main run loop.
+void
 run(const int input_fd, const int output_fd, const get_sock_t get_sock,
     const send_fd_t send_fd_fun, const close_t close_fun)
 {
     for (;;) {
-        // Read the command
         char command;
-        READ(&command, 1);
+        readMessage(input_fd, &command, sizeof(command));
         switch (command) {
-            case 'T': // The "terminate" command
-                return 0;
-            case 'S': { // Create a socket
-                // Read what type of socket they want
-                char type[2];
-                READ(type, 2);
-                // Read the address they ask for
-                struct sockaddr *addr(NULL);
-                size_t addr_len(0);
-                struct sockaddr_in addr_in;
-                struct sockaddr_in6 addr_in6;
-                switch (type[1]) { // The address family
-                    /*
-                     * Here are some casts. They are required by C++ and
-                     * the low-level interface (they are implicit in C).
-                     */
-                    case '4':
-                        addr = static_cast<struct sockaddr *>(
-                            static_cast<void *>(&addr_in));
-                        addr_len = sizeof addr_in;
-                        memset(&addr_in, 0, sizeof addr_in);
-                        addr_in.sin_family = AF_INET;
-                        READ(static_cast<char *>(static_cast<void *>(
-                            &addr_in.sin_port)), 2);
-                        READ(static_cast<char *>(static_cast<void *>(
-                            &addr_in.sin_addr.s_addr)), 4);
-                        break;
-                    case '6':
-                        addr = static_cast<struct sockaddr *>(
-                            static_cast<void *>(&addr_in6));
-                        addr_len = sizeof addr_in6;
-                        memset(&addr_in6, 0, sizeof addr_in6);
-                        addr_in6.sin6_family = AF_INET6;
-                        READ(static_cast<char *>(static_cast<void *>(
-                            &addr_in6.sin6_port)), 2);
-                        READ(static_cast<char *>(static_cast<void *>(
-                            &addr_in6.sin6_addr.s6_addr)), 16);
-                        break;
-                    DEFAULT
-                }
-                int sock_type;
-                switch (type[0]) { // Translate the type
-                    case 'T':
-                        sock_type = SOCK_STREAM;
-                        break;
-                    case 'U':
-                        sock_type = SOCK_DGRAM;
-                        break;
-                    DEFAULT
-                }
-                int result(get_sock(sock_type, addr, addr_len));
-                if (result >= 0) { // We got the socket
-                    WRITE("S", 1);
-                    if (send_fd_fun(output_fd, result) != 0) {
-                        // We'll soon abort ourselves, but make sure we still
-                        // close the socket; don't bother if it fails as the
-                        // higher level result (abort) is the same.
-                        close_fun(result);
-                        return 3;
-                    }
-                    // Don't leak the socket
-                    if (close_fun(result) == -1) {
-                        return 4;
-                    }
-                } else {
-                    WRITE("E", 1);
-                    switch (result) {
-                        case -1:
-                            WRITE("S", 1);
-                            break;
-                        case -2:
-                            WRITE("B", 1);
-                            break;
-                        default:
-                            return 4;
-                    }
-                    int error(errno);
-                    WRITE(static_cast<char *>(static_cast<void *>(&error)),
-                        sizeof error);
-                }
+            case 'S':   // The "get socket" command
+                handleRequest(input_fd, output_fd, get_sock,
+                              send_fd_fun, close_fun);
                 break;
-            }
-            DEFAULT
+
+            case 'T':   // The "terminate" command
+                return;
+
+            default:    // Don't recognise anything else
+                protocolError(output_fd);
         }
     }
 }

src/bin/sockcreator/sockcreator.h

@@ -1,4 +1,4 @@
-// Copyright (C) 2011  Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2011-2012  Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -12,18 +12,17 @@
 // OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
 // PERFORMANCE OF THIS SOFTWARE.
 
-/**
- * \file sockcreator.h
- * \short Socket creator functionality.
- *
- * This module holds the functionality of the socket creator. It is
- * a separate module from main to ease up the tests.
- */
+/// \file sockcreator.h
+/// \short Socket creator functionality.
+///
+/// This module holds the functionality of the socket creator. It is a separate
+/// module from main to make testing easier.
 
 #ifndef __SOCKCREATOR_H
 #define __SOCKCREATOR_H 1
 
 #include <util/io/fd_share.h>
+#include <exceptions/exceptions.h>
 
 #include <sys/types.h>
 #include <sys/socket.h>
@@ -32,78 +31,115 @@
 namespace isc {
 namespace socket_creator {
 
-/**
- * \short Create a socket and bind it.
- *
- * This is just a bundle of socket() and bind() calls. The sa_family of
- * bind_addr is used to determine the domain of the socket.
- *
- * \return The file descriptor of the newly created socket, if everything
- *     goes well. A negative number is returned if an error occurs -
- *     -1 if the socket() call fails or -2 if bind() fails. In case of error,
- *     errno is set (or better, left intact from socket() or bind()).
- * \param type The type of socket to create (SOCK_STREAM, SOCK_DGRAM, etc).
- * \param bind_addr The address to bind.
- * \param addr_len The actual length of bind_addr.
- */
-int
-get_sock(const int type, struct sockaddr *bind_addr, const socklen_t addr_len);
+// Exception classes - the base class exception SocketCreatorError is caught
+// by main() and holds an exit code returned to the environment.  The code
+// depends on the exact exception raised.
+class SocketCreatorError : public isc::Exception {
+public:
+    SocketCreatorError(const char* file, size_t line, const char* what,
+                       int exit_code) :
+        isc::Exception(file, line, what), exit_code_(exit_code) {}
 
-/**
- * Type of the get_sock function, to pass it as parameter.
- */
-typedef
-int
-(*get_sock_t)(const int, struct sockaddr *, const socklen_t);
+    int getExitCode() const {
+        return (exit_code_);
+    }
 
-/**
- * Type of the send_fd() function, so it can be passed as a parameter.
- */
-typedef
-int
-(*send_fd_t)(const int, const int);
+private:
+    const int exit_code_;   // Code returned to exit()
+};
 
-/// \brief Type of the close() function, so it can be passed as a parameter.
-typedef
-int
-(*close_t)(int);
-
-/**
- * \short Infinite loop parsing commands and returning the sockets.
- *
- * This reads commands and socket descriptions from the input_fd
- * file descriptor, creates sockets and writes the results (socket or
- * error) to output_fd.
- *
- * Current errors are:
- * - 1: Read error
- * - 2: Write error
- * - 3: Protocol error (unknown command, etc)
- * - 4: Some internal inconsistency detected
- *
- * It terminates either if a command asks it to or when unrecoverable
- * error happens.
- *
- * \return Like a return value of a main - 0 means everything OK, anything
- *     else is error.
- * \param input_fd Here is where it reads the commads.
- * \param output_fd Here is where it writes the results.
- * \param get_sock_fun The function that is used to create the sockets.
- *     This should be left on the default value, the parameter is here
- *     for testing purposes.
- * \param send_fd_fun The function that is used to send the socket over
- *     a file descriptor. This should be left on the default value, it is
- *     here for testing purposes.
- * \param close_fun The close function used to close sockets, coming from
- *     unistd.h. It can be overriden in tests.
- */
+class ReadError : public SocketCreatorError {
+public:
+    ReadError(const char* file, size_t line, const char* what) :
+        SocketCreatorError(file, line, what, 1) {}
+};
+
+class WriteError : public SocketCreatorError {
+public:
+    WriteError(const char* file, size_t line, const char* what) :
+        SocketCreatorError(file, line, what, 2) {}
+};
+
+class ProtocolError : public SocketCreatorError {
+public:
+    ProtocolError(const char* file, size_t line, const char* what) :
+        SocketCreatorError(file, line, what, 3) {}
+};
+
+class InternalError : public SocketCreatorError {
+public:
+    InternalError(const char* file, size_t line, const char* what) :
+        SocketCreatorError(file, line, what, 4) {}
+};
+
+
+
+/// \short Create a socket and bind it.
+///
+/// This is just a bundle of socket() and bind() calls. The sa_family of
+/// bind_addr is used to determine the domain of the socket.
+///
+/// \param type The type of socket to create (SOCK_STREAM, SOCK_DGRAM, etc).
+/// \param bind_addr The address to bind.
+/// \param addr_len The actual length of bind_addr.
+///
+/// \return The file descriptor of the newly created socket, if everything
+///         goes well. A negative number is returned if an error occurs -
+///         -1 if the socket() call fails or -2 if bind() fails. In case of
+///         error, errno is set (or left intact from socket() or bind()).
 int
+getSock(const int type, struct sockaddr* bind_addr, const socklen_t addr_len);
+
+// Define some types for functions used to perform socket-related operations.
+// These are typedefed so that alternatives can be passed through to the
+// main functions for testing purposes.
+
+// Type of the function to get a socket and to pass it as parameter.
+// Arguments are those described above for getSock().
+typedef int (*get_sock_t)(const int, struct sockaddr *, const socklen_t);
+
+// Type of the send_fd() function, so it can be passed as a parameter.
+// Arguments are the same as those of the send_fd() function.
+typedef int (*send_fd_t)(const int, const int);
+
+// Type of the close() function, so it can be passed as a parameter.
+// Argument is the same as that for close(2).
+typedef int (*close_t)(int);
+
+
+/// \brief Infinite loop parsing commands and returning the sockets.
+///
+/// This reads commands and socket descriptions from the input_fd file
+/// descriptor, creates sockets and writes the results (socket or error) to
+/// output_fd.
+///
+/// It terminates either if a command asks it to or when unrecoverable error
+/// happens.
+///
+/// \param input_fd File descriptor of the stream from which the input commands
+///        are read.
+/// \param output_fd File descriptor of the stream to which the output
+///        (message/socket or error message) is written.
+/// \param get_sock_fun The function that is used to create the sockets.
+///        This should be left on the default value, the parameter is here
+///        for testing purposes.
+/// \param send_fd_fun The function that is used to send the socket over
+///        a file descriptor. This should be left on the default value, it is
+///        here for testing purposes.
+/// \param close_fun The close function used to close sockets, coming from
+///        unistd.h. It can be overriden in tests.
+///
+/// \exception isc::socket_creator::ReadError Error reading from input
+/// \exception isc::socket_creator::WriteError Error writing to output
+/// \exception isc::socket_creator::ProtocolError Unrecognised command received
+/// \exception isc::socket_creator::InternalError Other error
+void
 run(const int input_fd, const int output_fd,
-    const get_sock_t get_sock_fun = get_sock,
+    const get_sock_t get_sock_fun = getSock,
     const send_fd_t send_fd_fun = isc::util::io::send_fd,
     const close_t close_fun = close);
 
-} // End of the namespaces
-}
+}   // namespace socket_creator
+}   // NAMESPACE ISC
 
 #endif // __SOCKCREATOR_H

src/bin/sockcreator/tests/sockcreator_tests.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2011  Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2011-2012  Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -17,11 +17,14 @@
 #include <util/unittests/fork.h>
 #include <util/io/fd.h>
 
+#include <boost/lexical_cast.hpp>
 #include <gtest/gtest.h>
 #include <sys/types.h>
 #include <sys/socket.h>
 #include <netinet/in.h>
 #include <unistd.h>
+
+#include <iostream>
 #include <cstring>
 #include <cerrno>
 
@@ -29,174 +32,225 @@ using namespace isc::socket_creator;
 using namespace isc::util::unittests;
 using namespace isc::util::io;
 
+// The tests check both TCP and UDP sockets on IPv4 and IPv6.
+//
+// Essentially we need to check all four combinations of TCP/UDP and IPv4/IPv6.
+// The different address families (IPv4/IPv6) require different structures to
+// hold the address information, and so some common code is in the form of
+// templates (or overloads), parameterised on the structure type.
+//
+// The protocol is determined by an integer (SOCK_STREAM or SOCK_DGRAM) so
+// cannot be templated in the same way.  Relevant check functions are
+// selected manually.
+
 namespace {
 
-/*
- * Generic version of the creation of socket test. It just tries to
- * create the socket and checks the result is not negative (eg.
- * it is valid descriptor) and that it can listen.
- *
- * This is a macro so ASSERT_* does abort the TEST, not just the
- * function inside.
- */
-#define TEST_ANY_CREATE(SOCK_TYPE, ADDR_TYPE, ADDR_FAMILY, FAMILY_FIELD, \
-    ADDR_SET, CHECK_SOCK) \
-    do { \
-        /*
-         * This should create an address that binds on all interfaces
-         * and lets the OS choose a free port.
-         */ \
-        struct ADDR_TYPE addr; \
-        memset(&addr, 0, sizeof addr); \
-        ADDR_SET(addr); \
-        addr.FAMILY_FIELD = ADDR_FAMILY; \
-        struct sockaddr *addr_ptr = static_cast<struct sockaddr *>( \
-            static_cast<void *>(&addr)); \
-        \
-        int socket = get_sock(SOCK_TYPE, addr_ptr, sizeof addr); \
-        /* Provide even nice error message. */ \
-        ASSERT_GE(socket, 0) << "Couldn't create a socket of type " \
-            #SOCK_TYPE " and family " #ADDR_FAMILY ", failed with " \
-            << socket << " and error " << strerror(errno); \
-        CHECK_SOCK(ADDR_TYPE, socket); \
-        int on; \
-        socklen_t len(sizeof(on)); \
-        EXPECT_EQ(0, getsockopt(socket, SOL_SOCKET, SO_REUSEADDR, &on, &len));\
-        EXPECT_NE(0, on); \
-        if (ADDR_FAMILY == AF_INET6) { \
-            EXPECT_EQ(0, getsockopt(socket, IPPROTO_IPV6, IPV6_V6ONLY, &on, \
-                                    &len)); \
-            EXPECT_NE(0, on); \
-        } \
-        EXPECT_EQ(0, close(socket)); \
-    } while (0)
-
-// Just helper macros
-#define INADDR_SET(WHAT) do { WHAT.sin_addr.s_addr = INADDR_ANY; } while (0)
-#define IN6ADDR_SET(WHAT) do { WHAT.sin6_addr = in6addr_loopback; } while (0)
-// If the get_sock returned something useful, listen must work
-#define TCP_CHECK(UNUSED, SOCKET) do { \
-        EXPECT_EQ(0, listen(SOCKET, 1)); \
-    } while (0)
-// More complicated with UDP, so we send a packet to ourselfs and se if it
-// arrives
-#define UDP_CHECK(ADDR_TYPE, SOCKET) do { \
-        struct ADDR_TYPE addr; \
-        memset(&addr, 0, sizeof addr); \
-        struct sockaddr *addr_ptr = static_cast<struct sockaddr *>( \
-            static_cast<void *>(&addr)); \
-        \
-        socklen_t len = sizeof addr; \
-        ASSERT_EQ(0, getsockname(SOCKET, addr_ptr, &len)); \
-        ASSERT_EQ(5, sendto(SOCKET, "test", 5, 0, addr_ptr, sizeof addr)) << \
-            "Send failed with error " << strerror(errno) << " on socket " << \
-            SOCKET; \
-        char buffer[5]; \
-        ASSERT_EQ(5, recv(SOCKET, buffer, 5, 0)) << \
-            "Recv failed with error " << strerror(errno) << " on socket " << \
-            SOCKET; \
-        EXPECT_STREQ("test", buffer); \
-    } while (0)
-
-/*
- * Several tests to ensure we can create the sockets.
- */
+// Set IP-version-specific fields.
+
+void
+setAddressFamilyFields(sockaddr_in* address) {
+    address->sin_family = AF_INET;
+    address->sin_addr.s_addr = INADDR_ANY;
+}
+
+void
+setAddressFamilyFields(sockaddr_in6* address) {
+    address->sin6_family = AF_INET6;
+    address->sin6_addr = in6addr_loopback;
+}
+
+// Socket has been opened, peform a check on it.  The sole argument is the
+// socket descriptor.  The TCP check is the same regardless of the address
+// family.  The UDP check requires that the socket address be obtained so
+// is parameterised on the type of structure required to hold the address.
+
+void
+tcpCheck(const int socknum) {
+    // Sufficient to be able to listen on the socket.
+    EXPECT_EQ(0, listen(socknum, 1));
+}
+
+template <typename ADDRTYPE>
+void
+udpCheck(const int socknum) {
+    // UDP testing is more complicated than TCP: send a packet to ourselves and
+    // see if it arrives.
+
+    // Get details of the socket so that we can use it as the target of the
+    // sendto().
+    ADDRTYPE addr;
+    memset(&addr, 0, sizeof(addr));
+    sockaddr* addr_ptr = reinterpret_cast<sockaddr*>(&addr);
+    socklen_t len = sizeof(addr);
+    ASSERT_EQ(0, getsockname(socknum, addr_ptr, &len));
+
+    // Send the packet to ourselves and check we receive it.
+    ASSERT_EQ(5, sendto(socknum, "test", 5, 0, addr_ptr, sizeof(addr))) <<
+        "Send failed with error " << strerror(errno) << " on socket " <<
+        socknum;
+    char buffer[5];
+    ASSERT_EQ(5, recv(socknum, buffer, 5, 0)) <<
+        "Recv failed with error " << strerror(errno) << " on socket " <<
+        socknum;
+    EXPECT_STREQ("test", buffer);
+}
+
+// The check function (tcpCheck/udpCheck) is passed as a parameter to the test
+// code, so provide a conveniet typedef.
+typedef void (*socket_check_t)(const int);
+
+// Address-family-specific scoket checks.
+//
+// The first argument is used to select the overloaded check function.
+// The other argument is the socket descriptor number.
+
+// IPv4 check
+void addressFamilySpecificCheck(const sockaddr_in*, const int) {
+}
+
+// IPv6 check
+void addressFamilySpecificCheck(const sockaddr_in6*, const int socknum) {
+    int options;
+    socklen_t len = sizeof(options);
+    EXPECT_EQ(0, getsockopt(socknum, IPPROTO_IPV6, IPV6_V6ONLY, &options, &len));
+    EXPECT_NE(0, options);
+}
+
+
+// Generic version of the socket test.  It creates the socket and checks that
+// it is a valid descriptor.  The family-specific check functions are called
+// to check that the socket is valid.  The function is parameterised according
+// to the structure used to hold the address.
+//
+// Arguments:
+// socket_type Type of socket to create (SOCK_DGRAM or SOCK_STREAM)
+// socket_check Associated check function - udpCheck() or tcpCheck()
+template <typename ADDRTYPE>
+void testAnyCreate(int socket_type, socket_check_t socket_check) {
+
+    // Create the socket.
+    ADDRTYPE addr;
+    memset(&addr, 0, sizeof(addr));
+    setAddressFamilyFields(&addr);
+    sockaddr* addr_ptr = reinterpret_cast<sockaddr*>(&addr);
+    const int socket = getSock(socket_type, addr_ptr, sizeof(addr));
+    ASSERT_GE(socket, 0) << "Couldn't create socket: failed with " <<
+        "return code " << socket << " and error " << strerror(errno);
+
+    // Perform socket-type-specific testing.
+    socket_check(socket);
+
+    // Do address-family-independent
+    int options;
+    socklen_t len = sizeof(options);
+    EXPECT_EQ(0, getsockopt(socket, SOL_SOCKET, SO_REUSEADDR, &options, &len));
+    EXPECT_NE(0, options);
+
+    // ...and the address-family specific tests.
+    addressFamilySpecificCheck(&addr, socket);
+
+    // Tidy up and exit.
+    EXPECT_EQ(0, close(socket));
+}
+
+
+// Several tests to ensure we can create the sockets.
 TEST(get_sock, udp4_create) {
-    TEST_ANY_CREATE(SOCK_DGRAM, sockaddr_in, AF_INET, sin_family, INADDR_SET,
-        UDP_CHECK);
+    testAnyCreate<sockaddr_in>(SOCK_DGRAM, udpCheck<sockaddr_in>);
 }
 
 TEST(get_sock, tcp4_create) {
-    TEST_ANY_CREATE(SOCK_STREAM, sockaddr_in, AF_INET, sin_family, INADDR_SET,
-        TCP_CHECK);
+    testAnyCreate<sockaddr_in>(SOCK_STREAM, tcpCheck);
 }
 
 TEST(get_sock, udp6_create) {
-    TEST_ANY_CREATE(SOCK_DGRAM, sockaddr_in6, AF_INET6, sin6_family,
-        IN6ADDR_SET, UDP_CHECK);
+    testAnyCreate<sockaddr_in6>(SOCK_DGRAM, udpCheck<sockaddr_in6>);
 }
 
 TEST(get_sock, tcp6_create) {
-    TEST_ANY_CREATE(SOCK_STREAM, sockaddr_in6, AF_INET6, sin6_family,
-        IN6ADDR_SET, TCP_CHECK);
+    testAnyCreate<sockaddr_in6>(SOCK_STREAM, tcpCheck);
 }
 
-/*
- * Try to ask the get_sock function some nonsense and test if it
- * is able to report error.
- */
+// Ask the get_sock function for some nonsense and test if it is able to report
+// an error.
 TEST(get_sock, fail_with_nonsense) {
-    struct sockaddr addr;
-    memset(&addr, 0, sizeof addr);
-    ASSERT_LT(get_sock(0, &addr, sizeof addr), 0);
+    sockaddr addr;
+    memset(&addr, 0, sizeof(addr));
+    ASSERT_LT(getSock(0, &addr, sizeof addr), 0);
 }
 
-/*
- * Helper functions to pass to run during testing.
- */
+// The main run() function in the socket creator takes three functions to
+// get the socket, send information to it, and close it.  These allow for
+// alternatives to the system functions to be used for testing.
+
+// Replacement getSock() function.
+// The return value indicates the result of checks and is encoded.  Using LSB
+// bit numbering (least-significant bit is bit 0) then:
+//
+// bit 0: 1 if "type" is known, 0 otherwise
+// bit 1: 1 for UDP, 0 for TCP
+// bit 2: 1 if address family is known, 0 otherwise
+// bit 3: 1 for IPv6, 0 for IPv4
+// bit 4: 1 if port passed was valid
+//
+// Other possible return values are:
+//
+// -1: The simulated bind() call has failed
+// -2: The simulated socket() call has failed
 int
-get_sock_dummy(const int type, struct sockaddr *addr, const socklen_t)
-{
-    int result(0);
-    int port(0);
-    /*
-     * We encode the type and address family into the int and return it.
-     * Lets ignore the port and address for now
-     * First bit is 1 if it is known type. Second tells if TCP or UDP.
-     * The familly is similar - third bit is known address family,
-     * the fourth is the family.
-     */
+getSockDummy(const int type, struct sockaddr* addr, const socklen_t) {
+    int result = 0;
+    int port = 0;
+
+    // Validate type field
     switch (type) {
         case SOCK_STREAM:
-            result += 1;
+            result |= 0x01;
             break;
+
         case SOCK_DGRAM:
-            result += 3;
+            result |= 0x03;
             break;
     }
+
+    // Validate address family
     switch (addr->sa_family) {
         case AF_INET:
-            result += 4;
-            port = static_cast<struct sockaddr_in *>(
-                static_cast<void *>(addr))->sin_port;
+            result |= 0x04;
+            port = reinterpret_cast<sockaddr_in*>(addr)->sin_port;
             break;
+
         case AF_INET6:
-            result += 12;
-            port = static_cast<struct sockaddr_in6 *>(
-                static_cast<void *>(addr))->sin6_port;
+            result |= 0x0C;
+            port = reinterpret_cast<sockaddr_in6*>(addr)->sin6_port;
             break;
     }
-    /*
-     * The port should be 0xffff. If it's not, we change the result.
-     * The port of 0xbbbb means bind should fail and 0xcccc means
-     * socket should fail.
-     */
+
+    // The port should be 0xffff. If it's not, we change the result.
+    // The port of 0xbbbb means bind should fail and 0xcccc means
+    // socket should fail.
     if (port != 0xffff) {
         errno = 0;
         if (port == 0xbbbb) {
-            return -2;
+            return (-2);
         } else if (port == 0xcccc) {
-            return -1;
+            return (-1);
         } else {
-            result += 16;
+            result |= 0x10;
         }
     }
-    return result;
+    return (result);
 }
 
+// Dummy send function - return data (the result of getSock()) to the destination.
 int
-send_fd_dummy(const int destination, const int what)
-{
-    /*
-     * Make sure it is 1 byte so we know the length. We do not use more during
-     * the test anyway.
-     */
-    char fd_data(what);
-    if (!write_data(destination, &fd_data, 1)) {
-        return -1;
-    } else {
-        return 0;
-    }
+send_FdDummy(const int destination, const int what) {
+    // Make sure it is 1 byte so we know the length. We do not use more during
+    // the test anyway.  And even with the LS bute, we can distinguish between
+    // the different results.
+    const char fd_data = what & 0xff;
+    const bool status = write_data(destination, &fd_data, sizeof(fd_data));
+    return (status ? 0 : -1);
 }
 
 // Just ignore the fd and pretend success. We close invalid fds in the tests.
@@ -205,106 +259,119 @@ closeIgnore(int) {
     return (0);
 }
 
-/*
- * Generic test that it works, with various inputs and outputs.
- * It uses different functions to create the socket and send it and pass
- * data to it and check it returns correct data back, to see if the run()
- * parses the commands correctly.
- */
-void run_test(const char *input_data, const size_t input_size,
-    const char *output_data, const size_t output_size,
-    bool should_succeed = true, const close_t test_close = closeIgnore,
-    const send_fd_t send_fd = send_fd_dummy)
+// Generic test that it works, with various inputs and outputs.
+// It uses different functions to create the socket and send it and pass
+// data to it and check it returns correct data back, to see if the run()
+// parses the commands correctly.
+void runTest(const char* input_data, const size_t input_size,
+             const char* output_data, const size_t output_size,
+             bool should_succeed = true,
+             const close_t test_close = closeIgnore,
+             const send_fd_t send_fd = send_FdDummy)
 {
-    // Prepare the input feeder and output checker processes
-    int input_fd(0), output_fd(0);
-    pid_t input(provide_input(&input_fd, input_data, input_size)),
-        output(check_output(&output_fd, output_data, output_size));
+    // Prepare the input feeder and output checker processes.  The feeder
+    // process sends data from the client to run() and the checker process
+    // reads the response and checks it against the expected response.
+    int input_fd = 0;
+    const pid_t input = provide_input(&input_fd, input_data, input_size);
     ASSERT_NE(-1, input) << "Couldn't start input feeder";
+
+    int output_fd = 0;
+    const pid_t output = check_output(&output_fd, output_data, output_size);
     ASSERT_NE(-1, output) << "Couldn't start output checker";
+
     // Run the body
-    int result(run(input_fd, output_fd, get_sock_dummy, send_fd, test_close));
-    // Close the pipes
-    close(input_fd);
-    close(output_fd);
-    // Did it run well?
     if (should_succeed) {
-        EXPECT_EQ(0, result);
+        EXPECT_NO_THROW(run(input_fd, output_fd, getSockDummy, send_fd,
+                            test_close));
     } else {
-        EXPECT_NE(0, result);
+        EXPECT_THROW(run(input_fd, output_fd, getSockDummy, send_fd,
+                         test_close), isc::socket_creator::SocketCreatorError);
     }
+
+    // Close the pipes
+    close(input_fd);
+    close(output_fd);
+
     // Check the subprocesses say everything is OK too
     EXPECT_TRUE(process_ok(input));
     EXPECT_TRUE(process_ok(output));
 }
 
-/*
- * Check it terminates successfully when asked to.
- */
+
+// Check it terminates successfully when asked to.
 TEST(run, terminate) {
-    run_test("T", 1, NULL, 0);
+    runTest("T", 1, NULL, 0);
 }
 
-/*
- * Check it rejects incorrect input.
- */
+// Check it rejects incorrect input.
 TEST(run, bad_input) {
-    run_test("XXX", 3, "FI", 2, false);
+    runTest("XXX", 3, "FI", 2, false);
 }
 
-/*
- * Check it correctly parses queries to create sockets.
- */
+// Check it correctly parses query stream to create sockets.
 TEST(run, sockets) {
-    run_test(
-        "SU4\xff\xff\0\0\0\0" // This has 9 bytes
-        "ST4\xff\xff\0\0\0\0" // This has 9 bytes
-        "ST6\xff\xff\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0" // This has 21 bytes
-        "SU6\xff\xff\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0" // This has 21 bytes
-        "T", 61,
-        "S\x07S\x05S\x0dS\x0f", 8);
+    runTest(
+        // Commands:
+        "SU4\xff\xff\0\0\0\0"   // IPv4 UDP socket, port 0xffffff, address 0.0.0.0
+        "ST4\xff\xff\0\0\0\0"   // IPv4 TCP socket, port 0xffffff, address 0.0.0.0
+        "ST6\xff\xff\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"
+                                // IPv6 UDP socket, port 0xffffff, address ::
+        "SU6\xff\xff\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"
+                                // IPv6 TCP socket, port 0xffffff, address ::
+        "T",                    // ... and terminate
+        9 + 9 + 21 + 21 + 1,    // Length of command string
+        "S\x07S\x05S\x0dS\x0f", // Response ("S" + LS byte of getSock() return)
+        8);                     // Length of response
 }
 
-/*
- * Check if failures of get_socket are handled correctly.
- */
+
+// Check if failures of get_socket are handled correctly.
 TEST(run, bad_sockets) {
-    // We need to construct the answer, but it depends on int length.
-    size_t int_len(sizeof(int));
-    size_t result_len(4 + 2 * int_len);
-    char result[4 + sizeof(int) * 2];
-    // Both errno parts should be 0
-    memset(result, 0, result_len);
-    // Fill the 2 control parts
+    // We need to construct the answer, but it depends on int length.  We expect
+    // two failure answers in this test, each answer comprising two characters
+    // followed by the (int) errno value.
+    char result[2 * (2 + sizeof(int))];
+
+    // We expect the errno parts to be zero but the characters to depend on the
+    // exact failure.
+    memset(result, 0, sizeof(result));
     strcpy(result, "EB");
-    strcpy(result + 2 + int_len, "ES");
+    strcpy(result + 2 + sizeof(int), "ES");
+
     // Run the test
-    run_test(
-        "SU4\xbb\xbb\0\0\0\0"
-        "SU4\xcc\xcc\0\0\0\0"
-        "T", 19,
-        result, result_len);
+    runTest(
+        "SU4\xbb\xbb\0\0\0\0"   // Port number will trigger simulated bind() fail
+        "SU4\xcc\xcc\0\0\0\0"   // Port number will trigger simulated socket() fail
+        "T",                    // Terminate
+        19,                     // Length of command string
+        result, sizeof(result));
 }
 
-// A close that fails
+// A close that fails.  (This causes an abort.)
 int
 closeFail(int) {
     return (-1);
 }
 
 TEST(run, cant_close) {
-    run_test("SU4\xff\xff\0\0\0\0", // This has 9 bytes
-             9, "S\x07", 2, false, closeFail);
+    runTest("SU4\xff\xff\0\0\0\0", 9,
+            "S\x07", 2,
+            false, closeFail);
 }
 
+// A send of the file descriptor that fails.  In this case we expect the client
+// to receive the "S" indicating that the descriptor is being sent and nothing
+// else.  This causes an abort.
 int
 sendFDFail(const int, const int) {
     return (FD_SYSTEM_ERROR);
 }
 
 TEST(run, cant_send_fd) {
-    run_test("SU4\xff\xff\0\0\0\0", // This has 9 bytes
-             9, "S", 1, false, closeIgnore, sendFDFail);
+    runTest("SU4\xff\xff\0\0\0\0", 9,
+            "S", 1,
+            false, closeIgnore, sendFDFail);
 }
 
-}
+}   // Anonymous namespace