Maintenance: use SetSocketOption() template

Author: yadij

src/Makefile.am

@@ -917,10 +917,13 @@ nodist_tests_testURL_SOURCES = \
 	tests/stub_access_log.cc \
 	anyp/Uri.h \
 	anyp/UriScheme.h \
+	tests/stub_cache_manager.cc \
 	tests/stub_cbdata.cc \
 	tests/stub_debug.cc \
+	tests/stub_libcomm.cc \
 	tests/stub_libhttp.cc \
-	tests/stub_libmem.cc
+	tests/stub_libmem.cc \
+	tests/stub_MemBuf.cc
 tests_testURL_LDADD = \
 	libsquid.la \
 	anyp/libanyp.la \
@@ -2033,6 +2036,7 @@ tests_testHttp1Parser_SOURCES = \
 	wordlist.h
 nodist_tests_testHttp1Parser_SOURCES = \
 	$(TESTSOURCES) \
+	tests/stub_libcomm.cc \
 	tests/stub_libtime.cc
 tests_testHttp1Parser_LDADD= \
 	http/libhttp.la \
@@ -2363,8 +2367,8 @@ tests_testHttpRequest_LDADD = \
 	anyp/libanyp.la \
 	$(SNMP_LIBS) \
 	icmp/libicmp.la \
-	comm/libcomm.la \
 	ip/libip.la \
+	comm/libcomm.la \
 	log/liblog.la \
 	format/libformat.la \
 	store/libstore.la \
@@ -2397,8 +2401,12 @@ check_PROGRAMS += tests/testIpAddress
 tests_testIpAddress_SOURCES = \
 	tests/testIpAddress.cc
 nodist_tests_testIpAddress_SOURCES = \
+	tests/stub_MemBuf.cc \
 	tests/stub_SBuf.cc \
+	tests/stub_cache_manager.cc \
+	tests/stub_cbdata.cc \
 	tests/stub_debug.cc \
+	tests/stub_libcomm.cc \
 	tests/stub_libmem.cc \
 	tests/stub_tools.cc
 tests_testIpAddress_LDADD = \
@@ -2416,9 +2424,13 @@ check_PROGRAMS += tests/testIcmp
 tests_testIcmp_SOURCES = \
 	tests/testIcmp.cc
 nodist_tests_testIcmp_SOURCES = \
+	tests/stub_MemBuf.cc \
 	tests/stub_SBuf.cc \
+	tests/stub_cache_manager.cc \
+	tests/stub_cbdata.cc \
 	tests/stub_debug.cc \
 	icmp/Icmp.h \
+	tests/stub_libcomm.cc \
 	tests/stub_libmem.cc \
 	tests/stub_libtime.cc
 tests_testIcmp_LDADD=\

src/client_side.cc

@@ -2134,10 +2134,7 @@ ConnStateData::start()
             (transparent() || port->disable_pmtu_discovery == DISABLE_PMTU_ALWAYS)) {
 #if defined(IP_MTU_DISCOVER) && defined(IP_PMTUDISC_DONT)
         int i = IP_PMTUDISC_DONT;
-        if (setsockopt(clientConnection->fd, SOL_IP, IP_MTU_DISCOVER, &i, sizeof(i)) < 0) {
-            int xerrno = errno;
-            debugs(33, 2, "WARNING: Path MTU discovery disabling failed on " << clientConnection << " : " << xstrerr(xerrno));
-        }
+        Comm::SetSocketOption(clientConnection->fd, SOL_IP, IP_MTU_DISCOVER, i, ToSBuf("IP_MTU_DISCOVER disabled for client ", clientConnection));
 #else
         static bool reported = false;
 

src/client_side_request.cc

@@ -1708,8 +1708,7 @@ ClientHttpRequest::doCallouts()
 
         if (!calloutContext->toClientMarkingDone) {
             calloutContext->toClientMarkingDone = true;
-            tos_t tos = aclMapTOS(Ip::Qos::TheConfig.tosToClient, &ch);
-            if (tos)
+            if (tos_t tos = aclMapTOS(Ip::Qos::TheConfig.tosToClient, &ch))
                 Ip::Qos::setSockTos(getConn()->clientConnection, tos);
 
             const auto packetMark = aclFindNfMarkConfig(Ip::Qos::TheConfig.nfmarkToClient, &ch);

src/clients/FtpGateway.cc

@@ -1774,15 +1774,7 @@ ftpOpenListenSocket(Ftp::Gateway * ftpState, int fallback)
      * REUSEADDR is needed in fallback mode, since the same port is
      * used for both control and data.
      */
-    if (fallback) {
-        int on = 1;
-        errno = 0;
-        if (setsockopt(ftpState->ctrl.conn->fd, SOL_SOCKET, SO_REUSEADDR,
-                       (char *) &on, sizeof(on)) == -1) {
-            int xerrno = errno;
-            // SO_REUSEADDR is only an optimization, no need to be verbose about error
-            debugs(9, 4, "setsockopt failed: " << xstrerr(xerrno));
-        }
+    if (fallback && Comm::SetBooleanSocketOption(ftpState->ctrl.conn->fd, SOL_SOCKET, SO_REUSEADDR, true, SBuf("SO_REUSEADDR"))) {
         ftpState->ctrl.conn->flags |= COMM_REUSEADDR;
         temp->flags |= COMM_REUSEADDR;
     } else {

src/comm.cc

@@ -210,11 +210,7 @@ static void
 commSetBindAddressNoPort(const int fd)
 {
 #if defined(IP_BIND_ADDRESS_NO_PORT)
-    int flag = 1;
-    if (setsockopt(fd, IPPROTO_IP, IP_BIND_ADDRESS_NO_PORT, reinterpret_cast<char*>(&flag), sizeof(flag)) < 0) {
-        const auto savedErrno = errno;
-        debugs(50, DBG_IMPORTANT, "ERROR: setsockopt(IP_BIND_ADDRESS_NO_PORT) failure: " << xstrerr(savedErrno));
-    }
+    Comm::SetBooleanSocketOption(fd, IPPROTO_IP, IP_BIND_ADDRESS_NO_PORT, true, SBuf("IP_BIND_ADDRESS_NO_PORT"));
 #else
     (void)fd;
 #endif
@@ -291,16 +287,13 @@ limitError(int const anErrno)
 }
 
 static void
-comm_set_v6only(int fd, int tos)
+comm_set_v6only(int fd, bool enabled)
 {
-#ifdef IPV6_V6ONLY
-    if (setsockopt(fd, IPPROTO_IPV6, IPV6_V6ONLY, (char *) &tos, sizeof(int)) < 0) {
-        int xerrno = errno;
-        debugs(50, DBG_IMPORTANT, MYNAME << "setsockopt(IPV6_V6ONLY) " << (tos?"ON":"OFF") << " for FD " << fd << ": " << xstrerr(xerrno));
-    }
+#if defined(IPV6_V6ONLY)
+    Comm::SetBooleanSocketOption(fd, IPPROTO_IPV6, IPV6_V6ONLY, enabled, SBuf("IPV6_V6ONLY"));
 #else
-    debugs(50, DBG_CRITICAL, MYNAME << "WARNING: setsockopt(IPV6_V6ONLY) not supported on this platform");
-#endif /* sockopt */
+    debugs(50, DBG_CRITICAL, "WARNING: setsockopt(IPV6_V6ONLY) not supported on this platform");
+#endif
 }
 
 /**
@@ -315,17 +308,20 @@ comm_set_transparent(int fd)
 #if _SQUID_LINUX_ && defined(IP_TRANSPARENT) // Linux
 # define soLevel SOL_IP
 # define soFlag  IP_TRANSPARENT
+    const SBuf name("IP_TRANSPARENT");
     bool doneSuid = false;
 
 #elif defined(SO_BINDANY) // OpenBSD 4.7+ and NetBSD with PF
 # define soLevel SOL_SOCKET
 # define soFlag  SO_BINDANY
+    const SBuf name("SO_BINDANY");
     enter_suid();
     bool doneSuid = true;
 
 #elif defined(IP_BINDANY) // FreeBSD with IPFW
 # define soLevel IPPROTO_IP
 # define soFlag  IP_BINDANY
+    const SBuf name("IP_BINDANY");
     enter_suid();
     bool doneSuid = true;
 
@@ -335,11 +331,7 @@ comm_set_transparent(int fd)
 #endif /* sockopt */
 
 #if defined(soLevel) && defined(soFlag)
-    int tos = 1;
-    if (setsockopt(fd, soLevel, soFlag, (char *) &tos, sizeof(int)) < 0) {
-        int xerrno = errno;
-        debugs(50, DBG_IMPORTANT, MYNAME << "setsockopt(TPROXY) on FD " << fd << ": " << xstrerr(xerrno));
-    } else {
+    if (Comm::SetBooleanSocketOption(fd, soLevel, soFlag, true, name)) {
         /* mark the socket as having transparent options */
         fd_table[fd].flags.transparent = true;
     }
@@ -423,12 +415,12 @@ comm_openex(int sock_type,
     debugs(50, 3, "comm_openex: Opened socket " << conn << " : family=" << AI->ai_family << ", type=" << AI->ai_socktype << ", protocol=" << AI->ai_protocol );
 
     if ( Ip::EnableIpv6&IPV6_SPECIAL_SPLITSTACK && addr.isIPv6() )
-        comm_set_v6only(conn->fd, 1);
+        comm_set_v6only(conn->fd, true);
 
     /* Windows Vista supports Dual-Sockets. BUT defaults them to V6ONLY. Turn it OFF. */
     /* Other OS may have this administratively disabled for general use. Same deal. */
     if ( Ip::EnableIpv6&IPV6_SPECIAL_V4MAPPING && addr.isIPv6() )
-        comm_set_v6only(conn->fd, 0);
+        comm_set_v6only(conn->fd, false);
 
     comm_init_opened(conn, note, AI);
     new_socket = comm_apply_flags(conn->fd, addr, flags, AI);
@@ -505,8 +497,7 @@ comm_apply_flags(int new_socket,
 
 #if defined(SO_REUSEPORT)
         if (flags & COMM_REUSEPORT) {
-            int on = 1;
-            if (setsockopt(new_socket, SOL_SOCKET, SO_REUSEPORT, reinterpret_cast<char*>(&on), sizeof(on)) < 0) {
+            if (!Comm::SetBooleanSocketOption(new_socket, SOL_SOCKET, SO_REUSEPORT, true, ToSBuf("SO_REUSEPORT on ", addr))) {
                 const auto savedErrno = errno;
                 const auto errorMessage = ToSBuf("cannot enable SO_REUSEPORT socket option when binding to ",
                                                  addr, ": ", xstrerr(savedErrno));
@@ -770,10 +761,7 @@ commConfigureLinger(const int fd, const OnOff enabled)
 
     fd_table[fd].flags.harshClosureRequested = (l.l_onoff && !l.l_linger); // close(2) sends TCP RST if true
 
-    if (setsockopt(fd, SOL_SOCKET, SO_LINGER, reinterpret_cast<char*>(&l), sizeof(l)) < 0) {
-        const auto xerrno = errno;
-        debugs(50, DBG_CRITICAL, "ERROR: Failed to set closure behavior (SO_LINGER) for FD " << fd << ": " << xstrerr(xerrno));
-    }
+    Comm::SetSocketOption(fd, SOL_SOCKET, SO_LINGER, l, ToSBuf("SO_LINGER (0 seconds) ", (l.l_onoff?"enabled":"disabled")));
 }
 
 /**
@@ -1011,29 +999,16 @@ comm_remove_close_handler(int fd, AsyncCall::Pointer &call)
 static void
 commSetReuseAddr(int fd)
 {
-    int on = 1;
-    if (setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, (char *) &on, sizeof(on)) < 0) {
-        int xerrno = errno;
-        debugs(50, DBG_IMPORTANT, MYNAME << "FD " << fd << ": " << xstrerr(xerrno));
-    }
+    Comm::SetBooleanSocketOption(fd, SOL_SOCKET, SO_REUSEADDR, true, SBuf("SO_REUSEADDR"));
 }
 
 static void
 commSetTcpRcvbuf(int fd, int size)
 {
-    if (setsockopt(fd, SOL_SOCKET, SO_RCVBUF, (char *) &size, sizeof(size)) < 0) {
-        int xerrno = errno;
-        debugs(50, DBG_IMPORTANT, MYNAME << "FD " << fd << ", SIZE " << size << ": " << xstrerr(xerrno));
-    }
-    if (setsockopt(fd, SOL_SOCKET, SO_SNDBUF, (char *) &size, sizeof(size)) < 0) {
-        int xerrno = errno;
-        debugs(50, DBG_IMPORTANT, MYNAME << "FD " << fd << ", SIZE " << size << ": " << xstrerr(xerrno));
-    }
-#ifdef TCP_WINDOW_CLAMP
-    if (setsockopt(fd, SOL_TCP, TCP_WINDOW_CLAMP, (char *) &size, sizeof(size)) < 0) {
-        int xerrno = errno;
-        debugs(50, DBG_IMPORTANT, MYNAME << "FD " << fd << ", SIZE " << size << ": " << xstrerr(xerrno));
-    }
+    Comm::SetSocketOption(fd, SOL_SOCKET, SO_RCVBUF, size, ToSBuf("SO_RCVBUF to ", size, " bytes"));
+    Comm::SetSocketOption(fd, SOL_SOCKET, SO_SNDBUF, size, ToSBuf("SO_SNDBUF to ", size, " bytes"));
+#if defined(TCP_WINDOW_CLAMP)
+    Comm::SetSocketOption(fd, SOL_TCP, TCP_WINDOW_CLAMP, size, ToSBuf("TCP_WINDOW_CLAMP to ", size, " bytes"));
 #endif
 }
 
@@ -1118,20 +1093,13 @@ commSetCloseOnExec(int fd)
 #endif
 }
 
-#ifdef TCP_NODELAY
+#if defined(TCP_NODELAY)
 static void
 commSetTcpNoDelay(int fd)
 {
-    int on = 1;
-
-    if (setsockopt(fd, IPPROTO_TCP, TCP_NODELAY, (char *) &on, sizeof(on)) < 0) {
-        int xerrno = errno;
-        debugs(50, DBG_IMPORTANT, MYNAME << "FD " << fd << ": " << xstrerr(xerrno));
-    }
-
+    Comm::SetBooleanSocketOption(fd, IPPROTO_TCP, TCP_NODELAY, true, SBuf("TCP_NODELAY"));
     fd_table[fd].flags.nodelay = true;
 }
-
 #endif
 
 void

src/comm/ConnOpener.cc

@@ -291,12 +291,10 @@ Comm::ConnOpener::createFd()
     }
 
     // Set TOS if needed.
-    if (conn_->tos &&
-            Ip::Qos::setSockTos(temporaryFd_, conn_->tos, conn_->remote.isIPv4() ? AF_INET : AF_INET6) < 0)
+    if (conn_->tos && !Ip::Qos::setSockTos(temporaryFd_, conn_->tos, conn_->remote.isIPv4() ? AF_INET : AF_INET6))
         conn_->tos = 0;
 #if SO_MARK
-    if (conn_->nfmark &&
-            Ip::Qos::setSockNfmark(temporaryFd_, conn_->nfmark) < 0)
+    if (conn_->nfmark && !Ip::Qos::setSockNfmark(temporaryFd_, conn_->nfmark))
         conn_->nfmark = 0;
 #endif
 

src/comm/Makefile.am

@@ -43,4 +43,6 @@ libcomm_la_SOURCES = \
 
 # a bare-bones implementation of few Comm APIs sufficient for helpers use
 libminimal_la_SOURCES = \
+	Tcp.cc \
+	Tcp.h \
 	minimal.cc

src/comm/Tcp.cc

@@ -11,6 +11,7 @@
 #include "squid.h"
 #include "comm/Tcp.h"
 #include "debug/Stream.h"
+#include "sbuf/Stream.h"
 
 #if HAVE_NETINET_TCP_H
 #include <netinet/tcp.h>
@@ -23,28 +24,11 @@
 #endif
 #include <type_traits>
 
-/// setsockopt(2) wrapper
-template <typename Option>
-static bool
-SetSocketOption(const int fd, const int level, const int optName, const Option &optValue)
-{
-    static_assert(std::is_trivially_copyable<Option>::value, "setsockopt() expects POD-like options");
-    static_assert(!std::is_same<Option, bool>::value, "setsockopt() uses int to represent boolean options");
-    if (setsockopt(fd, level, optName, reinterpret_cast<const char *>(&optValue), sizeof(optValue)) < 0) {
-        const auto xerrno = errno;
-        debugs(5, DBG_IMPORTANT, "ERROR: setsockopt(2) failure: " << xstrerr(xerrno));
-        // TODO: Generalize to throw on errors when some callers need that.
-        return false;
-    }
-    return true;
-}
-
-/// setsockopt(2) wrapper for setting typical on/off options
-static bool
-SetBooleanSocketOption(const int fd, const int level, const int optName, const bool enable)
+bool
+Comm::SetBooleanSocketOption(const int fd, const int level, const int optName, const bool enable, const SBuf &description)
 {
     const int optValue = enable ? 1 :0;
-    return SetSocketOption(fd, level, optName, optValue);
+    return SetSocketOption(fd, level, optName, optValue, ToSBuf((enable ? "enable ":"disable "), description));
 }
 
 void
@@ -56,20 +40,20 @@ Comm::ApplyTcpKeepAlive(int fd, const TcpKeepAlive &cfg)
 #if defined(TCP_KEEPCNT)
     if (cfg.timeout && cfg.interval) {
         const int count = (cfg.timeout + cfg.interval - 1) / cfg.interval; // XXX: unsigned-to-signed conversion
-        (void)SetSocketOption(fd, IPPROTO_TCP, TCP_KEEPCNT, count);
+        SetSocketOption(fd, IPPROTO_TCP, TCP_KEEPCNT, count, ToSBuf("TCP_KEEPCNT to ", count));
     }
 #endif
 #if defined(TCP_KEEPIDLE)
     if (cfg.idle) {
         // XXX: TCP_KEEPIDLE expects an int; cfg.idle is unsigned
-        (void)SetSocketOption(fd, IPPROTO_TCP, TCP_KEEPIDLE, cfg.idle);
+        SetSocketOption(fd, IPPROTO_TCP, TCP_KEEPIDLE, cfg.idle, ToSBuf("TCP_KEEPIDLE to ", cfg.idle));
     }
 #endif
 #if defined(TCP_KEEPINTVL)
     if (cfg.interval) {
         // XXX: TCP_KEEPINTVL expects an int; cfg.interval is unsigned
-        (void)SetSocketOption(fd, IPPROTO_TCP, TCP_KEEPINTVL, cfg.interval);
+        SetSocketOption(fd, IPPROTO_TCP, TCP_KEEPINTVL, cfg.interval, ToSBuf("TCP_KEEPINTVL to ", cfg.interval));
     }
 #endif
-    (void)SetBooleanSocketOption(fd, SOL_SOCKET, SO_KEEPALIVE, true);
+    SetBooleanSocketOption(fd, SOL_SOCKET, SO_KEEPALIVE, true, SBuf("SO_KEEPALIVE"));
 }

src/comm/Tcp.h

@@ -9,6 +9,9 @@
 #ifndef SQUID_SRC_COMM_TCP_H
 #define SQUID_SRC_COMM_TCP_H
 
+#include "debug/Stream.h"
+#include "sbuf/SBuf.h"
+
 namespace Comm
 {
 
@@ -25,6 +28,26 @@ class TcpKeepAlive
 /// apply configured TCP keep-alive settings to the given FD socket
 void ApplyTcpKeepAlive(int fd, const TcpKeepAlive &);
 
+/// setsockopt(2) wrapper
+template <typename Option>
+bool
+SetSocketOption(const int fd, const int level, const int optName, const Option &optValue, const SBuf &description)
+{
+    static_assert(std::is_trivially_copyable<Option>::value, "setsockopt() expects POD-like options");
+    static_assert(!std::is_same<Option, bool>::value, "setsockopt() uses int to represent boolean options");
+    if (setsockopt(fd, level, optName, reinterpret_cast<const char *>(&optValue), sizeof(optValue)) < 0) {
+        const auto xerrno = errno;
+        debugs(5, DBG_IMPORTANT, "ERROR: setsockopt(2) failure on FD " << fd << " : " << xstrerr(xerrno)
+               << Debug::Extra << "setting " << description);
+        // TODO: Generalize to throw on errors when some callers need that.
+        return false;
+    }
+    return true;
+}
+
+/// setsockopt(2) wrapper for setting typical on/off options
+bool SetBooleanSocketOption(const int fd, const int level, const int optName, const bool enable, const SBuf &description);
+
 } // namespace Comm
 
 #endif /* SQUID_SRC_COMM_TCP_H */