[2157] pass IP versions and transport protocols with enum

src/bin/auth/auth_srv.cc

@@ -494,9 +494,13 @@ AuthSrv::processMessage(const IOMessage& io_message, Message& message,
     MessageAttributes stats_attrs;
 
     stats_attrs.setRequestIPVersion(
-        io_message.getRemoteEndpoint().getFamily());
+        io_message.getRemoteEndpoint().getFamily() == AF_INET ?
+            MessageAttributes::IP_VERSION_IPV4 :
+            MessageAttributes::IP_VERSION_IPV6);
     stats_attrs.setRequestTransportProtocol(
-        io_message.getRemoteEndpoint().getProtocol());
+        io_message.getRemoteEndpoint().getProtocol() == IPPROTO_UDP ?
+            MessageAttributes::TRANSPORT_UDP :
+            MessageAttributes::TRANSPORT_TCP);
 
     // First, check the header part.  If we fail even for the base header,
     // just drop the message.

src/bin/auth/statistics.cc.pre

@@ -24,19 +24,6 @@
 
 #include <statistics/counter.h>
 
-#include <algorithm>
-#include <cctype>
-#include <cassert>
-#include <string>
-#include <sstream>
-#include <iostream>
-
-#include <unistd.h>
-#include <sys/types.h>
-#include <sys/socket.h>
-#include <netinet/in.h>
-#include <netdb.h>
-
 using namespace isc::dns;
 using namespace isc::auth;
 using namespace isc::statistics;
@@ -132,16 +119,16 @@ Counters::Counters() :
 void
 Counters::incRequest(const MessageAttributes& msgattrs) {
     // protocols carrying request
-    if (msgattrs.getRequestIPVersion() == AF_INET) {
-        server_msg_counter_.inc(MSG_REQUEST_IPV4);
-    } else if (msgattrs.getRequestIPVersion() == AF_INET6) {
-        server_msg_counter_.inc(MSG_REQUEST_IPV6);
-    }
-    if (msgattrs.getRequestTransportProtocol() == IPPROTO_UDP) {
-        server_msg_counter_.inc(MSG_REQUEST_UDP);
-    } else if (msgattrs.getRequestTransportProtocol() == IPPROTO_TCP) {
-        server_msg_counter_.inc(MSG_REQUEST_TCP);
-    }
+    server_msg_counter_.inc(
+        msgattrs.getRequestIPVersion() ==
+            MessageAttributes::IP_VERSION_IPV4 ?
+            MSG_REQUEST_IPV4 :
+            MSG_REQUEST_IPV6);
+    server_msg_counter_.inc(
+        msgattrs.getRequestTransportProtocol() ==
+            MessageAttributes::TRANSPORT_UDP ?
+            MSG_REQUEST_UDP :
+            MSG_REQUEST_TCP);
 
     // request TSIG
     if (msgattrs.getRequestSigTSIG()) {

src/bin/auth/statistics.h

@@ -39,11 +39,25 @@ using isc::dns::Opcode;
 /// This class holds some attributes related to a DNS message
 /// for statistics data collection.
 class MessageAttributes {
+public:
+    /// \brief IP version of DNS message.
+    enum IPVersion {
+        IP_VERSION_UNSPEC,          // (initial value; internal use only)
+        IP_VERSION_IPV4,            ///< IPv4 message
+        IP_VERSION_IPV6             ///< IPv6 message
+    };
+
+    /// \brief Transport protocol of DNS message.
+    enum TransportProtocol {
+        TRANSPORT_UNSPEC,           // (initial value; internal use only)
+        TRANSPORT_UDP,              ///< UDP message
+        TRANSPORT_TCP               ///< TCP message
+    };
 private:
     // request attributes
-    int req_ip_version_;            // IP version
-    int req_transport_protocol_;    // Transport layer protocol
-    Opcode req_opcode_;   // OpCode
+    IPVersion req_ip_version_;                 // IP version
+    TransportProtocol req_transport_protocol_; // Transport layer protocol
+    Opcode req_opcode_;                        // OpCode
     enum BitAttributes {
         REQ_IS_EDNS_0,              // EDNS ver.0
         REQ_IS_DNSSEC_OK,           // DNSSEC OK (DO) bit is set
@@ -57,7 +71,8 @@ public:
     /// \brief The constructor.
     ///
     /// \throw None
-    MessageAttributes() : req_ip_version_(0), req_transport_protocol_(0),
+    MessageAttributes() : req_ip_version_(IP_VERSION_UNSPEC),
+                          req_transport_protocol_(TRANSPORT_UNSPEC),
                           req_opcode_(Opcode::RESERVED15_CODE),
                           bit_attributes_()
     {}
@@ -76,31 +91,34 @@ public:
     }
 
     /// \brief Get IP version carrying a request.
-    /// \return IP version carrying a request (AF_INET or AF_INET6)
+    /// \return IP version carrying a request
     /// \throw None
-    int getRequestIPVersion() const {
+    IPVersion getRequestIPVersion() const {
         return (req_ip_version_);
     }
 
     /// \brief Set IP version carrying a request.
-    /// \param ip_version AF_INET or AF_INET6
+    /// \param ip_version IP_VERSION_IPV4 or IP_VERSION_IPV6
     /// \throw None
-    void setRequestIPVersion(const int ip_version) {
+    void setRequestIPVersion(const IPVersion ip_version) {
+        assert(ip_version != IP_VERSION_UNSPEC);
         req_ip_version_ = ip_version;
     }
 
     /// \brief Get transport protocol carrying a request.
     /// \return Transport protocol carrying a request
-    ///         (IPPROTO_UDP or IPPROTO_TCP)
     /// \throw None
-    int getRequestTransportProtocol() const {
+    TransportProtocol getRequestTransportProtocol() const {
         return (req_transport_protocol_);
     }
 
     /// \brief Set transport protocol carrying a request.
-    /// \param transport_protocol IPPROTO_UDP or IPPROTO_TCP
+    /// \param transport_protocol TRANSPORT_UDP or TRANSPORT_TCP
     /// \throw None
-    void setRequestTransportProtocol(const int transport_protocol) {
+    void setRequestTransportProtocol(
+        const TransportProtocol transport_protocol)
+    {
+        assert(transport_protocol != TRANSPORT_UNSPEC);
         req_transport_protocol_ = transport_protocol;
     }
 
@@ -193,6 +211,7 @@ class Counters : boost::noncopyable {
 private:
     // counter for DNS message attributes
     isc::statistics::Counter server_msg_counter_;
+
     void incRequest(const MessageAttributes& msgattrs);
     void incResponse(const MessageAttributes& msgattrs,
                      const isc::dns::Message& response);

src/bin/auth/tests/statistics_unittest.cc.pre

@@ -100,6 +100,25 @@ checkCounters(const isc::data::ConstElementPtr counters,
     }
 }
 
+TEST_F(CountersTest, invalidParameter) {
+    MessageAttributes msgattrs;
+
+    // Note: Not all systems have EXPECT_DEATH.  As it is a macro we can just
+    // test for its presence and bypass the test if not available.
+#ifdef EXPECT_DEATH
+    // Passing *_UNSPEC should trigger an assertion failure.
+    // Note that we just check that it dies - we don't check what message is
+    // output.
+    EXPECT_DEATH(
+        msgattrs.setRequestIPVersion(MessageAttributes::IP_VERSION_UNSPEC),
+        ".*");
+    EXPECT_DEATH(
+        msgattrs.setRequestTransportProtocol(
+            MessageAttributes::TRANSPORT_UNSPEC),
+        ".*");
+#endif
+}
+
 TEST_F(CountersTest, incrementResponse) {
     Message response(Message::RENDER);
     MessageAttributes msgattrs;
@@ -109,8 +128,8 @@ TEST_F(CountersTest, incrementResponse) {
     for (int i = 0; i < 2; ++i) {
         const bool responded = i & 1;
 
-        msgattrs.setRequestIPVersion(AF_INET);
-        msgattrs.setRequestTransportProtocol(IPPROTO_UDP);
+        msgattrs.setRequestIPVersion(MessageAttributes::IP_VERSION_IPV4);
+        msgattrs.setRequestTransportProtocol(MessageAttributes::TRANSPORT_UDP);
         msgattrs.setRequestOpCode(Opcode::QUERY());
         msgattrs.setRequestEDNS0(true);
         msgattrs.setRequestDO(true);
@@ -151,8 +170,12 @@ TEST_F(CountersTest, incrementProtocolType) {
     //      ipv6   tcp
     int count_v4 = 0, count_v6 = 0, count_udp = 0, count_tcp = 0;
     for (int i = 0; i < 4; ++i) {
-        const int af = i & 1 ? AF_INET : AF_INET6;
-        const int proto = i & 2 ? IPPROTO_UDP : IPPROTO_TCP;
+        const enum MessageAttributes::IPVersion af = i & 1 ?
+            MessageAttributes::IP_VERSION_IPV4 :
+            MessageAttributes::IP_VERSION_IPV6;
+        const enum MessageAttributes::TransportProtocol proto = i & 2 ?
+            MessageAttributes::TRANSPORT_UDP :
+            MessageAttributes::TRANSPORT_TCP;
 
         msgattrs.setRequestIPVersion(af);
         msgattrs.setRequestTransportProtocol(proto);
@@ -167,12 +190,12 @@ TEST_F(CountersTest, incrementProtocolType) {
 
         counters.inc(msgattrs, response, true);
 
-        if (af == AF_INET) {
+        if (af == MessageAttributes::IP_VERSION_IPV4) {
             ++count_v4;
         } else {
             ++count_v6;
         }
-        if (proto == IPPROTO_UDP) {
+        if (proto == MessageAttributes::TRANSPORT_UDP) {
             ++count_udp;
         } else {
             ++count_tcp;
@@ -207,8 +230,8 @@ TEST_F(CountersTest, incrementDO) {
     //     true
     for (int i = 0; i < 2; ++i) {
         const bool is_dnssec_ok = i & 1;
-        msgattrs.setRequestIPVersion(AF_INET);
-        msgattrs.setRequestTransportProtocol(IPPROTO_UDP);
+        msgattrs.setRequestIPVersion(MessageAttributes::IP_VERSION_IPV4);
+        msgattrs.setRequestTransportProtocol(MessageAttributes::TRANSPORT_UDP);
         msgattrs.setRequestOpCode(Opcode::QUERY());
         msgattrs.setRequestEDNS0(true);
         msgattrs.setRequestDO(is_dnssec_ok);
@@ -250,8 +273,8 @@ TEST_F(CountersTest, incrementEDNS) {
     int count_req_edns0 = 0, count_res_edns0 = 0;
     for (int i = 0; i < 2; ++i) {
         const bool is_edns0 = i & 1;
-        msgattrs.setRequestIPVersion(AF_INET);
-        msgattrs.setRequestTransportProtocol(IPPROTO_UDP);
+        msgattrs.setRequestIPVersion(MessageAttributes::IP_VERSION_IPV4);
+        msgattrs.setRequestTransportProtocol(MessageAttributes::TRANSPORT_UDP);
         msgattrs.setRequestOpCode(Opcode::QUERY());
         msgattrs.setRequestEDNS0(is_edns0);
         msgattrs.setRequestDO(true);
@@ -308,8 +331,8 @@ TEST_F(CountersTest, incrementTSIG) {
     for (int i = 0; i < 3; ++i) {
         const bool is_tsig = (i == 2) ? true : i & 1;
         const bool is_badsig = i & 2;
-        msgattrs.setRequestIPVersion(AF_INET);
-        msgattrs.setRequestTransportProtocol(IPPROTO_UDP);
+        msgattrs.setRequestIPVersion(MessageAttributes::IP_VERSION_IPV4);
+        msgattrs.setRequestTransportProtocol(MessageAttributes::TRANSPORT_UDP);
         msgattrs.setRequestOpCode(Opcode::QUERY());
         msgattrs.setRequestEDNS0(true);
         msgattrs.setRequestDO(true);
@@ -364,8 +387,8 @@ TEST_F(CountersTest, incrementOpcode) {
          i <= e;
          ++i)
     {
-        msgattrs.setRequestIPVersion(AF_INET);
-        msgattrs.setRequestTransportProtocol(IPPROTO_UDP);
+        msgattrs.setRequestIPVersion(MessageAttributes::IP_VERSION_IPV4);
+        msgattrs.setRequestTransportProtocol(MessageAttributes::TRANSPORT_UDP);
         msgattrs.setRequestOpCode(Opcode(i));
         msgattrs.setRequestEDNS0(true);
         msgattrs.setRequestDO(true);
@@ -422,8 +445,8 @@ TEST_F(CountersTest, incrementRcode) {
          i <= e;
          ++i)
     {
-        msgattrs.setRequestIPVersion(AF_INET);
-        msgattrs.setRequestTransportProtocol(IPPROTO_UDP);
+        msgattrs.setRequestIPVersion(MessageAttributes::IP_VERSION_IPV4);
+        msgattrs.setRequestTransportProtocol(MessageAttributes::TRANSPORT_UDP);
         msgattrs.setRequestOpCode(Opcode::IQUERY());
         msgattrs.setRequestEDNS0(true);
         msgattrs.setRequestDO(true);
@@ -485,8 +508,8 @@ TEST_F(CountersTest, incrementTruncated) {
     int count_truncated = 0;
     for (int i = 0; i < 2; ++i) {
         const bool is_truncated = i & 1;
-        msgattrs.setRequestIPVersion(AF_INET);
-        msgattrs.setRequestTransportProtocol(IPPROTO_UDP);
+        msgattrs.setRequestIPVersion(MessageAttributes::IP_VERSION_IPV4);
+        msgattrs.setRequestTransportProtocol(MessageAttributes::TRANSPORT_UDP);
         msgattrs.setRequestOpCode(Opcode::IQUERY());
         msgattrs.setRequestEDNS0(true);
         msgattrs.setRequestDO(true);
@@ -531,8 +554,8 @@ TEST_F(CountersTest, incrementQryAuthAnsAndNoAuthAns) {
     int count_authans = 0, count_noauthans = 0;
     for (int i = 0; i < 2; ++i) {
         const bool is_aa_set = i & 1;
-        msgattrs.setRequestIPVersion(AF_INET);
-        msgattrs.setRequestTransportProtocol(IPPROTO_UDP);
+        msgattrs.setRequestIPVersion(MessageAttributes::IP_VERSION_IPV4);
+        msgattrs.setRequestTransportProtocol(MessageAttributes::TRANSPORT_UDP);
         msgattrs.setRequestOpCode(Opcode::QUERY());
         msgattrs.setRequestEDNS0(true);
         msgattrs.setRequestDO(true);
@@ -571,8 +594,8 @@ TEST_F(CountersTest, incrementQrySuccess) {
     std::map<std::string, int> expect;
 
     // Opcode = QUERY, Rcode = NOERROR, ANCOUNT > 0
-    msgattrs.setRequestIPVersion(AF_INET);
-    msgattrs.setRequestTransportProtocol(IPPROTO_UDP);
+    msgattrs.setRequestIPVersion(MessageAttributes::IP_VERSION_IPV4);
+    msgattrs.setRequestTransportProtocol(MessageAttributes::TRANSPORT_UDP);
     msgattrs.setRequestOpCode(Opcode::QUERY());
     msgattrs.setRequestEDNS0(true);
     msgattrs.setRequestDO(true);
@@ -620,8 +643,8 @@ TEST_F(CountersTest, incrementQryReferralAndNxrrset) {
     int count_referral = 0, count_nxrrset = 0;
     for (int i = 0; i < 2; ++i) {
         const bool is_aa_set = i & 1;
-        msgattrs.setRequestIPVersion(AF_INET);
-        msgattrs.setRequestTransportProtocol(IPPROTO_UDP);
+        msgattrs.setRequestIPVersion(MessageAttributes::IP_VERSION_IPV4);
+        msgattrs.setRequestTransportProtocol(MessageAttributes::TRANSPORT_UDP);
         msgattrs.setRequestOpCode(Opcode::QUERY());
         msgattrs.setRequestEDNS0(true);
         msgattrs.setRequestDO(true);
@@ -663,8 +686,8 @@ TEST_F(CountersTest, incrementAuthQryRej) {
     std::map<std::string, int> expect;
 
     // Opcode = QUERY, Rcode = REFUSED, ANCOUNT = 0 (don't care)
-    msgattrs.setRequestIPVersion(AF_INET);
-    msgattrs.setRequestTransportProtocol(IPPROTO_UDP);
+    msgattrs.setRequestIPVersion(MessageAttributes::IP_VERSION_IPV4);
+    msgattrs.setRequestTransportProtocol(MessageAttributes::TRANSPORT_UDP);
     msgattrs.setRequestOpCode(Opcode::QUERY());
     msgattrs.setRequestEDNS0(true);
     msgattrs.setRequestDO(true);