[2157] use boost::optional for getRequestOpcode()

diff --git a/src/bin/auth/statistics.cc.pre b/src/bin/auth/statistics.cc.pre
index 63151659a825eb1970c7614c2c7efc3f81c72e23..d540729f34282e6cb19f4c582eb99b486a93ca1a 100644 (file)
--- a/src/bin/auth/statistics.cc.pre
+++ b/src/bin/auth/statistics.cc.pre
@@ -24,6 +24,8 @@
 
 #include <statistics/counter.h>
 
+#include <boost/optional.hpp>
+
 using namespace isc::dns;
 using namespace isc::auth;
 using namespace isc::statistics;
@@ -151,14 +153,11 @@ Counters::incRequest(const MessageAttributes& msgattrs) {
     }
 
     // OPCODE
-    try {
-        server_msg_counter_.inc(
-            opcode_to_msgcounter[msgattrs.getRequestOpCode().getCode()]);
-    } catch (const isc::InvalidOperation& ex) {
-        // The exception will be thrown if OpCode is not set
-        //  (e.g. message is short).
-        // Increment MSG_OPCODE_OTHER.
-        server_msg_counter_.inc(MSG_OPCODE_OTHER);
+    const boost::optional<const isc::dns::Opcode&> opcode =
+        msgattrs.getRequestOpCode();
+    // Increment opcode counter only if the opcode exists.
+    if (opcode) {
+        server_msg_counter_.inc(opcode_to_msgcounter[opcode.get().getCode()]);
     }
 }
 
@@ -198,47 +197,42 @@ Counters::incResponse(const MessageAttributes& msgattrs,
         server_msg_counter_.inc(MSG_REQUEST_BADEDNSVER);
     }
 
-    try {
-        if (msgattrs.getRequestOpCode() == Opcode::QUERY()) {
-            // compound attributes
-            const unsigned int answer_rrs =
-                response.getRRCount(Message::SECTION_ANSWER);
-            const bool is_aa_set =
-                response.getHeaderFlag(Message::HEADERFLAG_AA);
-
-            if (is_aa_set) {
-                // QryAuthAns
-                server_msg_counter_.inc(MSG_QRYAUTHANS);
-            } else {
-                // QryNoAuthAns
-                server_msg_counter_.inc(MSG_QRYNOAUTHANS);
-            }
+    const boost::optional<const isc::dns::Opcode&> opcode =
+        msgattrs.getRequestOpCode();
+    if (opcode && opcode.get() == Opcode::QUERY()) {
+        // compound attributes
+        const unsigned int answer_rrs =
+            response.getRRCount(Message::SECTION_ANSWER);
+        const bool is_aa_set =
+            response.getHeaderFlag(Message::HEADERFLAG_AA);
+
+        if (is_aa_set) {
+            // QryAuthAns
+            server_msg_counter_.inc(MSG_QRYAUTHANS);
+        } else {
+            // QryNoAuthAns
+            server_msg_counter_.inc(MSG_QRYNOAUTHANS);
+        }
 
-            if (rcode == Rcode::NOERROR_CODE) {
-                if (answer_rrs > 0) {
-                    // QrySuccess
-                    server_msg_counter_.inc(MSG_QRYSUCCESS);
+        if (rcode == Rcode::NOERROR_CODE) {
+            if (answer_rrs > 0) {
+                // QrySuccess
+                server_msg_counter_.inc(MSG_QRYSUCCESS);
+            } else {
+                if (is_aa_set) {
+                    // QryNxrrset
+                    server_msg_counter_.inc(MSG_QRYNXRRSET);
                 } else {
-                    if (is_aa_set) {
-                        // QryNxrrset
-                        server_msg_counter_.inc(MSG_QRYNXRRSET);
-                    } else {
-                        // QryReferral
-                        server_msg_counter_.inc(MSG_QRYREFERRAL);
-                    }
-                }
-            } else if (rcode == Rcode::REFUSED_CODE) {
-                if (!response.getHeaderFlag(Message::HEADERFLAG_RD)) {
-                    // AuthRej
-                    server_msg_counter_.inc(MSG_QRYREJECT);
+                    // QryReferral
+                    server_msg_counter_.inc(MSG_QRYREFERRAL);
                 }
             }
+        } else if (rcode == Rcode::REFUSED_CODE) {
+            if (!response.getHeaderFlag(Message::HEADERFLAG_RD)) {
+                // AuthRej
+                server_msg_counter_.inc(MSG_QRYREJECT);
+            }
         }
-    } catch (const isc::InvalidOperation& ex) {
-        // The exception will be thrown if OpCode is not set
-        //  (e.g. message is short).
-        // Do not increment the counters above since they are incremented
-        // only if Opcode is Query.
     }
 }
 

diff --git a/src/bin/auth/statistics.h b/src/bin/auth/statistics.h
index 49be6a29079ac62e9b419f0fc179673dce04bf7b..b0e3f6eb63a9a7475f798bdbf51574092302cfc0 100644 (file)
--- a/src/bin/auth/statistics.h
+++ b/src/bin/auth/statistics.h
@@ -77,13 +77,13 @@ public:
     {}
 
     /// \brief Return opcode of the request.
-    /// \return opcode of the request
+    /// \return opcode of the request wrapped with boost::optional
     /// \throw isc::InvalidOperation Opcode is not set
-    const isc::dns::Opcode& getRequestOpCode() const {
+    const boost::optional<const isc::dns::Opcode&> getRequestOpCode() const {
         if (req_opcode_) {
             return (req_opcode_.get());
         } else {
-            isc_throw(isc::InvalidOperation, "Opcode is not set");
+            return boost::none;
         }
     }
 

diff --git a/src/bin/auth/tests/auth_srv_unittest.cc b/src/bin/auth/tests/auth_srv_unittest.cc
index f6d357cb00fc35f8678b58d3bc6e1328f7e49570..9bcf7c616dace59f18c9bd8172435b2f9e33bc02 100644 (file)
--- a/src/bin/auth/tests/auth_srv_unittest.cc
+++ b/src/bin/auth/tests/auth_srv_unittest.cc
@@ -295,7 +295,6 @@ TEST_F(AuthSrvTest, shortMessage) {
     std::map<std::string, int> expect;
     expect["request.v4"] = 1;
     expect["request.udp"] = 1;
-    expect["opcode.other"] = 1;
     checkStatisticsCounters(stats_after, expect);
 }
 
@@ -309,7 +308,6 @@ TEST_F(AuthSrvTest, response) {
     std::map<std::string, int> expect;
     expect["request.v4"] = 3;
     expect["request.udp"] = 3;
-    expect["opcode.other"] = 3;
     checkStatisticsCounters(stats_after, expect);
 }
 
@@ -335,7 +333,6 @@ TEST_F(AuthSrvTest, ednsBadVers) {
     expect["request.v4"] = 1;
     expect["request.badednsver"] = 1;
     expect["request.udp"] = 1;
-    expect["opcode.other"] = 1;
     expect["responses"] = 1;
     expect["rcode.badvers"] = 1;
     checkStatisticsCounters(stats_after, expect);

diff --git a/src/bin/auth/tests/statistics_unittest.cc.pre b/src/bin/auth/tests/statistics_unittest.cc.pre
index d602ba7bfb5bd25f399dc8c8974e8dc384c62889..45b75953fba624248e701cb3c9f664522d77673e 100644 (file)
--- a/src/bin/auth/tests/statistics_unittest.cc.pre
+++ b/src/bin/auth/tests/statistics_unittest.cc.pre
@@ -80,13 +80,13 @@ TEST_F(CountersTest, invalidParameter) {
 TEST_F(CountersTest, invalidOperationForGetRequestOpCode) {
     MessageAttributes msgattrs;
 
-    // getRequestOpCode() will throw isc::InvalidOperation when called before
+    // getRequestOpCode() should return boost::none when called before
     // opcode is set with setRequestOpCode().
-    EXPECT_THROW(msgattrs.getRequestOpCode(), isc::InvalidOperation);
+    EXPECT_FALSE(msgattrs.getRequestOpCode());
 
     msgattrs.setRequestOpCode(Opcode::QUERY());
-    // getRequestOpCode() will not throw at this point.
-    EXPECT_NO_THROW(msgattrs.getRequestOpCode());
+    // getRequestOpCode() should be Opcode::QUERY.
+    EXPECT_EQ(Opcode::QUERY(), msgattrs.getRequestOpCode().get());
 }
 
 TEST_F(CountersTest, incrementResponse) {