[2984] Changes after review:

 - unknown message log added
 - comments fixed

src/bin/dhcp6/dhcp6_messages.mes

@@ -372,6 +372,12 @@ to a misconfiguration of the server. The packet processing will continue, but
 the response will only contain generic configuration parameters and no
 addresses or prefixes.
 
+% DHCP6_UNKNOWN_MSG_RECEIVED received unknown message (type %d) on interface %2
+This debug message is printed when server receives a message of unknown type.
+That could either mean missing functionality or invalid or broken relay or client.
+The list of formally defined message types is available here:
+www.iana.org/assignments/dhcpv6-parameters.
+
 % DHCP6_UNKNOWN_RELEASE received RELEASE from unknown client (duid=%1, iaid=%2)
 This warning message is printed when client attempts to release a lease,
 but no such lease is known by the server. See DHCP6_UNKNOWN_RENEW for

src/bin/dhcp6/dhcp6_srv.cc

@@ -199,7 +199,8 @@ bool Dhcpv6Srv::run() {
 
         bool skip_unpack = false;
 
-        // Let's execute all callouts registered for buffer6_receive
+        // The packet has just been received so contains the uninterpreted wire
+        // data; execute callouts registered for buffer6_receive.
         if (HooksManager::getHooksManager().calloutsPresent(Hooks.hook_index_buffer6_receive_)) {
             CalloutHandlePtr callout_handle = getCalloutHandle(query);
 
@@ -214,7 +215,8 @@ bool Dhcpv6Srv::run() {
 
             // Callouts decided to skip the next processing step. The next
             // processing step would to parse the packet, so skip at this
-            // stage means drop.
+            // stage means that callouts did the parsing already, so server
+            // should skip parsing.
             if (callout_handle->getSkip()) {
                 LOG_DEBUG(dhcp6_logger, DBG_DHCP6_HOOKS, DHCP6_HOOK_BUFFER_RCVD_SKIP);
                 skip_unpack = true;
@@ -223,6 +225,8 @@ bool Dhcpv6Srv::run() {
             callout_handle->getArgument("query6", query);
         }
 
+        // Unpack the packet information unless the buffer6_receive callouts
+        // indicated they did it
         if (!skip_unpack) {
             if (!query->unpack()) {
                 LOG_DEBUG(dhcp6_logger, DBG_DHCP6_DETAIL,
@@ -237,7 +241,9 @@ bool Dhcpv6Srv::run() {
             .arg(query->getBuffer().getLength())
             .arg(query->toText());
 
-        // Let's execute all callouts registered for packet6_receive
+        // At this point the information in the packet has been unpacked into
+        // the various packet fields and option objects has been cretated.
+        // Execute callouts registered for packet6_receive.
         if (HooksManager::getHooksManager().calloutsPresent(Hooks.hook_index_pkt6_receive_)) {
             CalloutHandlePtr callout_handle = getCalloutHandle(query);
 
@@ -296,6 +302,10 @@ bool Dhcpv6Srv::run() {
                 break;
 
             default:
+                // We received a packet type that we do not recognize.
+                LOG_DEBUG(dhcp6_logger, DBG_DHCP6_BASIC, DHCP6_UNKNOWN_MSG_RECEIVED)
+                    .arg(static_cast<int>(query->getType()))
+                    .arg(query->getIface());
                 // Only action is to output a message if debug is enabled,
                 // and that will be covered by the debug statement before
                 // the "switch" statement.
@@ -331,9 +341,12 @@ bool Dhcpv6Srv::run() {
             rsp->setIndex(query->getIndex());
             rsp->setIface(query->getIface());
 
-            // specifies if server should do the packing
+            // Specifies if server should do the packing
             bool skip_pack = false;
 
+            // Server's reply packet now has all options and fields set.
+            // Options are represented by individual objects, but the
+            // output wire data has not been prepared yet.
             // Execute all callouts registered for packet6_send
             if (HooksManager::getHooksManager().calloutsPresent(Hooks.hook_index_pkt6_send_)) {
                 CalloutHandlePtr callout_handle = getCalloutHandle(query);
@@ -348,8 +361,10 @@ bool Dhcpv6Srv::run() {
                 HooksManager::callCallouts(Hooks.hook_index_pkt6_send_, *callout_handle);
 
                 // Callouts decided to skip the next processing step. The next
-                // processing step would to send the packet, so skip at this
-                // stage means "drop response".
+                // processing step would to pack the packet (create wire data).
+                // That step will be skipped if any callout sets skip flag.
+                // It essentially means that the callout already did packing,
+                // so the server does not have to do it again.
                 if (callout_handle->getSkip()) {
                     LOG_DEBUG(dhcp6_logger, DBG_DHCP6_HOOKS, DHCP6_HOOK_PACKET_SEND_SKIP);
                     skip_pack = true;
@@ -369,6 +384,9 @@ bool Dhcpv6Srv::run() {
 
             try {
 
+                // Now all fields and options are constructed into output wire buffer.
+                // Option objects modification does not make sense anymore. Hooks
+                // can only manipulate wire buffer at this stage.
                 // Let's execute all callouts registered for buffer6_send
                 if (HooksManager::getHooksManager().calloutsPresent(Hooks.hook_index_buffer6_send_)) {
                     CalloutHandlePtr callout_handle = getCalloutHandle(query);
@@ -995,8 +1013,8 @@ Dhcpv6Srv::renewIA_NA(const Subnet6Ptr& subnet, const DuidPtr& duid,
         HooksManager::callCallouts(Hooks.hook_index_lease6_renew_, *callout_handle);
 
         // Callouts decided to skip the next processing step. The next
-        // processing step would to send the packet, so skip at this
-        // stage means "drop response".
+        // processing step would to actually renew the lease, so skip at this
+        // stage means "keep the old lease as it is".
         if (callout_handle->getSkip()) {
             skip = true;
             LOG_DEBUG(dhcp6_logger, DBG_DHCP6_HOOKS, DHCP6_HOOK_LEASE6_RENEW_SKIP);
@@ -1242,8 +1260,7 @@ Dhcpv6Srv::releaseIA_NA(const DuidPtr& duid, const Pkt6Ptr& query,
     }
 
     // Ok, we've passed all checks. Let's release this address.
-
-    bool success = false; // did the removal was successful
+    bool success = false; // was the removal operation succeessful?
 
     if (!skip) {
         success = LeaseMgrFactory::instance().deleteLease(lease->addr_);

src/bin/dhcp6/tests/hooks_unittest.cc

@@ -56,9 +56,9 @@ TEST_F(Dhcpv6SrvTest, Hooks) {
     int hook_index_buffer6_send    = -1;
     int hook_index_lease6_renew    = -1;
     int hook_index_lease6_release  = -1;
-    int hook_index_pkt6_received = -1;
-    int hook_index_select_subnet = -1;
-    int hook_index_pkt6_send     = -1;
+    int hook_index_pkt6_received   = -1;
+    int hook_index_select_subnet   = -1;
+    int hook_index_pkt6_send       = -1;
 
     // check if appropriate indexes are set
     EXPECT_NO_THROW(hook_index_buffer6_receive = ServerHooks::getServerHooks()
@@ -76,13 +76,13 @@ TEST_F(Dhcpv6SrvTest, Hooks) {
     EXPECT_NO_THROW(hook_index_pkt6_send     = ServerHooks::getServerHooks()
                     .getIndex("pkt6_send"));
 
-    EXPECT_TRUE(hook_index_pkt6_received > 0);
-    EXPECT_TRUE(hook_index_select_subnet > 0);
-    EXPECT_TRUE(hook_index_pkt6_send > 0);
+    EXPECT_TRUE(hook_index_pkt6_received   > 0);
+    EXPECT_TRUE(hook_index_select_subnet   > 0);
+    EXPECT_TRUE(hook_index_pkt6_send       > 0);
     EXPECT_TRUE(hook_index_buffer6_receive > 0);
-    EXPECT_TRUE(hook_index_buffer6_send > 0);
-    EXPECT_TRUE(hook_index_lease6_renew > 0);
-    EXPECT_TRUE(hook_index_lease6_release > 0);
+    EXPECT_TRUE(hook_index_buffer6_send    > 0);
+    EXPECT_TRUE(hook_index_lease6_renew    > 0);
+    EXPECT_TRUE(hook_index_lease6_release  > 0);
 }
 
 // This function returns buffer for very simple Solicit
@@ -130,7 +130,7 @@ public:
         // Allocate new DHCPv6 Server
         srv_ = new NakedDhcpv6Srv(0);
 
-        // clear static buffers
+        // Clear static buffers
         resetCalloutBuffers();
     }
 
@@ -149,7 +149,7 @@ public:
     /// @return pointer to create option object
     static OptionPtr createOption(uint16_t option_code) {
 
-        char payload[] = {
+        uint8_t payload[] = {
             0xa, 0xb, 0xc, 0xe, 0xf, 0x10, 0x11, 0x12, 0x13, 0x14
         };
 
@@ -179,17 +179,17 @@ public:
         Pkt6Ptr pkt;
         callout_handle.getArgument("query6", pkt);
 
-        // get rid of the old client-id
+        // Get rid of the old client-id
         pkt->delOption(D6O_CLIENTID);
 
-        // add a new option
+        // Add a new option
         pkt->addOption(createOption(D6O_CLIENTID));
 
-        // carry on as usual
+        // Carry on as usual
         return pkt6_receive_callout(callout_handle);
     }
 
-    /// test callback that deletes client-id
+    /// Test callback that deletes client-id
     /// @param callout_handle handle passed by the hooks framework
     /// @return always 0
     static int
@@ -198,14 +198,14 @@ public:
         Pkt6Ptr pkt;
         callout_handle.getArgument("query6", pkt);
 
-        // get rid of the old client-id
+        // Get rid of the old client-id
         pkt->delOption(D6O_CLIENTID);
 
-        // carry on as usual
+        // Carry on as usual
         return pkt6_receive_callout(callout_handle);
     }
 
-    /// test callback that sets skip flag
+    /// Test callback that sets skip flag
     /// @param callout_handle handle passed by the hooks framework
     /// @return always 0
     static int
@@ -216,11 +216,11 @@ public:
 
         callout_handle.setSkip(true);
 
-        // carry on as usual
+        // Carry on as usual
         return pkt6_receive_callout(callout_handle);
     }
 
-    /// test callback that stores received callout name and pkt6 value
+    /// Test callback that stores received callout name and pkt6 value
     /// @param callout_handle handle passed by the hooks framework
     /// @return always 0
     static int
@@ -233,7 +233,7 @@ public:
         return (0);
     }
 
-    /// test callback that changes first byte of client-id value
+    /// Test callback that changes first byte of client-id value
     /// @param callout_handle handle passed by the hooks framework
     /// @return always 0
     static int
@@ -243,15 +243,17 @@ public:
         callout_handle.getArgument("query6", pkt);
 
         // If there is at least one option with data
-        if (pkt->data_.size()>Pkt6::DHCPV6_PKT_HDR_LEN + Option::OPTION6_HDR_LEN) {
-            pkt->data_[8] = 0xff;
+        if (pkt->data_.size() > Pkt6::DHCPV6_PKT_HDR_LEN + Option::OPTION6_HDR_LEN) {
+            // Offset of the first byte of the first option. Let's set this byte
+            // to some new value that we could later check
+            pkt->data_[Pkt6::DHCPV6_PKT_HDR_LEN + Option::OPTION6_HDR_LEN] = 0xff;
         }
 
-        // carry on as usual
+        // Carry on as usual
         return buffer6_receive_callout(callout_handle);
     }
 
-    /// test callback that deletes client-id
+    /// Test callback that deletes client-id
     /// @param callout_handle handle passed by the hooks framework
     /// @return always 0
     static int
@@ -279,7 +281,7 @@ public:
         return buffer6_receive_callout(callout_handle);
     }
 
-    /// test callback that sets skip flag
+    /// Test callback that sets skip flag
     /// @param callout_handle handle passed by the hooks framework
     /// @return always 0
     static int
@@ -290,7 +292,7 @@ public:
 
         callout_handle.setSkip(true);
 
-        // carry on as usual
+        // Carry on as usual
         return buffer6_receive_callout(callout_handle);
     }
 
@@ -316,17 +318,17 @@ public:
         Pkt6Ptr pkt;
         callout_handle.getArgument("response6", pkt);
 
-        // get rid of the old server-id
+        // Get rid of the old server-id
         pkt->delOption(D6O_SERVERID);
 
-        // add a new option
+        // Add a new option
         pkt->addOption(createOption(D6O_SERVERID));
 
-        // carry on as usual
+        // Carry on as usual
         return pkt6_send_callout(callout_handle);
     }
 
-    /// test callback that deletes server-id
+    /// Test callback that deletes server-id
     /// @param callout_handle handle passed by the hooks framework
     /// @return always 0
     static int
@@ -335,10 +337,10 @@ public:
         Pkt6Ptr pkt;
         callout_handle.getArgument("response6", pkt);
 
-        // get rid of the old client-id
+        // Get rid of the old client-id
         pkt->delOption(D6O_SERVERID);
 
-        // carry on as usual
+        // Carry on as usual
         return pkt6_send_callout(callout_handle);
     }
 
@@ -395,7 +397,7 @@ public:
         return (0);
     }
 
-    /// test callback that stores received callout name and pkt6 value
+    /// Test callback that stores received callout name and pkt6 value
     /// @param callout_handle handle passed by the hooks framework
     /// @return always 0
     static int
@@ -418,7 +420,7 @@ public:
     static const uint32_t override_preferred_;
     static const uint32_t override_valid_;
 
-    /// test callback that overrides received lease. It updates
+    /// Test callback that overrides received lease. It updates
     /// T1, T2, preferred and valid lifetimes
     /// @param callout_handle handle passed by the hooks framework
     /// @return always 0
@@ -446,7 +448,7 @@ public:
         return (0);
     }
 
-    /// test callback that sets the skip flag
+    /// Test callback that sets the skip flag
     /// @param callout_handle handle passed by the hooks framework
     /// @return always 0
     static int
@@ -458,7 +460,7 @@ public:
         return (0);
     }
 
-    /// test callback that stores received callout name passed parameters
+    /// Test callback that stores received callout name passed parameters
     /// @param callout_handle handle passed by the hooks framework
     /// @return always 0
     static int
@@ -472,7 +474,7 @@ public:
         return (0);
     }
 
-    /// test callback that sets the skip flag
+    /// Test callback that sets the skip flag
     /// @param callout_handle handle passed by the hooks framework
     /// @return always 0
     static int
@@ -484,7 +486,7 @@ public:
         return (0);
     }
 
-    /// resets buffers used to store data received by callouts
+    /// Resets buffers used to store data received by callouts
     void resetCalloutBuffers() {
         callback_name_ = string("");
         callback_pkt6_.reset();
@@ -495,7 +497,7 @@ public:
         callback_argument_names_.clear();
     }
 
-    /// pointer to Dhcpv6Srv that is used in tests
+    /// Pointer to Dhcpv6Srv that is used in tests
     NakedDhcpv6Srv* srv_;
 
     // The following fields are used in testing pkt6_receive_callout
@@ -563,10 +565,10 @@ TEST_F(HooksDhcpv6SrvTest, simple_buffer6_receive) {
     // In particular, it should call registered pkt6_receive callback.
     srv_->run();
 
-    // check that the callback called is indeed the one we installed
+    // Check that the callback called is indeed the one we installed
     EXPECT_EQ("buffer6_receive", callback_name_);
 
-    // check that pkt6 argument passing was successful and returned proper value
+    // Check that pkt6 argument passing was successful and returned proper value
     EXPECT_TRUE(callback_pkt6_.get() == sol.get());
 
     // Check that all expected parameters are there
@@ -596,7 +598,7 @@ TEST_F(HooksDhcpv6SrvTest, valueChange_buffer6_receive) {
     // In particular, it should call registered pkt6_receive callback.
     srv_->run();
 
-    // check that the server did send a reposonce
+    // Check that the server did send a reposonce
     ASSERT_EQ(1, srv_->fake_sent_.size());
 
     // Make sure that we received a response
@@ -657,7 +659,7 @@ TEST_F(HooksDhcpv6SrvTest, skip_buffer6_receive) {
     // In particular, it should call registered pkt6_receive callback.
     srv_->run();
 
-    // check that the server dropped the packet and did not produce any response
+    // Check that the server dropped the packet and did not produce any response
     ASSERT_EQ(0, srv_->fake_sent_.size());
 }
 
@@ -684,10 +686,10 @@ TEST_F(HooksDhcpv6SrvTest, simple_pkt6_receive) {
     // In particular, it should call registered pkt6_receive callback.
     srv_->run();
 
-    // check that the callback called is indeed the one we installed
+    // Check that the callback called is indeed the one we installed
     EXPECT_EQ("pkt6_receive", callback_name_);
 
-    // check that pkt6 argument passing was successful and returned proper value
+    // Check that pkt6 argument passing was successful and returned proper value
     EXPECT_TRUE(callback_pkt6_.get() == sol.get());
 
     // Check that all expected parameters are there
@@ -717,7 +719,7 @@ TEST_F(HooksDhcpv6SrvTest, valueChange_pkt6_receive) {
     // In particular, it should call registered pkt6_receive callback.
     srv_->run();
 
-    // check that the server did send a reposonce
+    // Check that the server did send a reposonce
     ASSERT_EQ(1, srv_->fake_sent_.size());
 
     // Make sure that we received a response
@@ -777,7 +779,7 @@ TEST_F(HooksDhcpv6SrvTest, skip_pkt6_receive) {
     // In particular, it should call registered pkt6_receive callback.
     srv_->run();
 
-    // check that the server dropped the packet and did not produce any response
+    // Check that the server dropped the packet and did not produce any response
     ASSERT_EQ(0, srv_->fake_sent_.size());
 }
 
@@ -838,7 +840,7 @@ TEST_F(HooksDhcpv6SrvTest, valueChange_pkt6_send) {
     // In particular, it should call registered pkt6_receive callback.
     srv_->run();
 
-    // check that the server did send a reposonce
+    // Check that the server did send a response
     ASSERT_EQ(1, srv_->fake_sent_.size());
 
     // Make sure that we received a response
@@ -906,10 +908,10 @@ TEST_F(HooksDhcpv6SrvTest, skip_pkt6_send) {
     // In particular, it should call registered pkt6_receive callback.
     srv_->run();
 
-    // check that the server send the packet
+    // Check that the server send the packet
     ASSERT_EQ(1, srv_->fake_sent_.size());
 
-    // but the sent packet should have 0 length (we told the server to
+    // But the sent packet should have 0 length (we told the server to
     // skip pack(), but did not do packing outselves)
     Pkt6Ptr sent = srv_->fake_sent_.front();
 
@@ -961,7 +963,7 @@ TEST_F(HooksDhcpv6SrvTest, subnet6_select) {
     // Pass it to the server and get an advertise
     Pkt6Ptr adv = srv_->processSolicit(sol);
 
-    // check if we get response at all
+    // Check if we get response at all
     ASSERT_TRUE(adv);
 
     // Check that the callback called is indeed the one we installed
@@ -1029,7 +1031,7 @@ TEST_F(HooksDhcpv6SrvTest, subnet_select_change) {
     // Pass it to the server and get an advertise
     Pkt6Ptr adv = srv_->processSolicit(sol);
 
-    // check if we get response at all
+    // Check if we get response at all
     ASSERT_TRUE(adv);
 
     // The response should have an address from second pool, so let's check it
@@ -1232,7 +1234,7 @@ TEST_F(HooksDhcpv6SrvTest, leaseUpdate_lease6_renew) {
     // Checking for CLTT is a bit tricky if we want to avoid off by 1 errors
     int32_t cltt = static_cast<int32_t>(l->cltt_);
     int32_t expected = static_cast<int32_t>(time(NULL));
-    // equality or difference by 1 between cltt and expected is ok.
+    // Equality or difference by 1 between cltt and expected is ok.
     EXPECT_GE(1, abs(cltt - expected));
 
     EXPECT_TRUE(LeaseMgrFactory::instance().deleteLease(addr_opt->getAddress()));
@@ -1381,7 +1383,7 @@ TEST_F(HooksDhcpv6SrvTest, basic_lease6_release) {
     l = LeaseMgrFactory::instance().getLease6(addr);
     ASSERT_FALSE(l);
 
-    // get lease by subnetid/duid/iaid combination
+    // Get lease by subnetid/duid/iaid combination
     l = LeaseMgrFactory::instance().getLease6(*duid_, iaid, subnet_->getID());
     ASSERT_FALSE(l);
 }
@@ -1448,7 +1450,7 @@ TEST_F(HooksDhcpv6SrvTest, skip_lease6_release) {
     l = LeaseMgrFactory::instance().getLease6(addr);
     ASSERT_TRUE(l);
 
-    // get lease by subnetid/duid/iaid combination
+    // Get lease by subnetid/duid/iaid combination
     l = LeaseMgrFactory::instance().getLease6(*duid_, iaid, subnet_->getID());
     ASSERT_TRUE(l);
 }