[#1147] Checkpoint: addressing comments

src/bin/dhcp4/client_handler.cc

@@ -180,6 +180,8 @@ ClientHandler::tryLock(Pkt4Ptr query, ContinuationPtr cont) {
 
     ClientPtr holder_id;
     ClientPtr holder_hw;
+    Pkt4Ptr next_query_id;
+    Pkt4Ptr next_query_hw;
     client_.reset(new Client(query, duid, hwaddr));
 
     // Try first duid.
@@ -191,6 +193,10 @@ ClientHandler::tryLock(Pkt4Ptr query, ContinuationPtr cont) {
         if (!holder_id) {
             locked_client_id_ = duid;
             lockById();
+        } else if (cont) {
+            next_query_id = holder_id->next_query_;
+            holder_id->next_query_ = query;
+            holder_id->cont_ = cont;
         }
     }
     if (!holder_id) {
@@ -205,65 +211,63 @@ ClientHandler::tryLock(Pkt4Ptr query, ContinuationPtr cont) {
             locked_hwaddr_ = hwaddr;
             lockByHWAddr();
             return (true);
+        } else if (cont) {
+            next_query_hw = holder_hw->next_query_;
+            holder_hw->next_query_ = query;
+            holder_hw->cont_ = cont;
         }
     }
 
     if (holder_id) {
         // This query is a by-id duplicate so put the continuation.
         if (cont) {
-            Pkt4Ptr next_query = holder_id->next_query_;
+             if (next_query_id) {
-            holder_id->next_query_ = query;
-            holder_id->cont_ = cont;
-            if (next_query) {
                 // Logging a warning as it is supposed to be a rare event
                 // with well behaving clients...
                 LOG_WARN(bad_packet4_logger, DHCP4_PACKET_DROP_0011)
-                    .arg(next_query->toText())
+                    .arg(next_query_id->toText())
                     .arg(this_thread::get_id())
-                    .arg(query->toText())
+                    .arg(holder_id->query_->toText())
-                    .arg(this_thread::get_id());
+                    .arg(holder_id->thread_);
                 stats::StatsMgr::instance().addValue("pkt4-receive-drop",
                                                      static_cast<int64_t>(1));
             }
-            return (false);
+        } else {
+            // Logging a warning as it is supposed to be a rare event
+            // with well behaving clients...
+            LOG_WARN(bad_packet4_logger, DHCP4_PACKET_DROP_0011)
+                .arg(query->toText())
+                .arg(this_thread::get_id())
+                .arg(holder_id->query_->toText())
+                .arg(holder_id->thread_);
+            stats::StatsMgr::instance().addValue("pkt4-receive-drop",
+                                                 static_cast<int64_t>(1));
         }
-        // Logging a warning as it is supposed to be a rare event
-        // with well behaving clients...
-        LOG_WARN(bad_packet4_logger, DHCP4_PACKET_DROP_0011)
-            .arg(query->toText())
-            .arg(this_thread::get_id())
-            .arg(holder_id->query_->toText())
-            .arg(holder_id->thread_);
-        stats::StatsMgr::instance().addValue("pkt4-receive-drop",
-                                             static_cast<int64_t>(1));
     } else {
         // This query is a by-hw duplicate so put the continuation.
         if (cont) {
-            Pkt4Ptr next_query = holder_hw->next_query_;
+            if (next_query_hw) {
-            holder_hw->next_query_ = query;
-            holder_hw->cont_ = cont;
-            if (next_query) {
                 // Logging a warning as it is supposed to be a rare event
                 // with well behaving clients...
                 LOG_WARN(bad_packet4_logger, DHCP4_PACKET_DROP_0012)
-                    .arg(next_query->toText())
+                    .arg(next_query_hw->toText())
                     .arg(this_thread::get_id())
-                    .arg(query->toText())
+                    .arg(holder_hw->query_->toText())
-                    .arg(this_thread::get_id());
+                    .arg(holder_hw->thread_);
                 stats::StatsMgr::instance().addValue("pkt4-receive-drop",
                                                      static_cast<int64_t>(1));
             }
-            return (false);
+        } else {
+            // Logging a warning as it is supposed to be a rare event
+            // with well behaving clients...
+            LOG_WARN(bad_packet4_logger, DHCP4_PACKET_DROP_0012)
+                .arg(query->toText())
+                .arg(this_thread::get_id())
+                .arg(holder_hw->query_->toText())
+                .arg(holder_hw->thread_);
+            stats::StatsMgr::instance().addValue("pkt4-receive-drop",
+                                                 static_cast<int64_t>(1));
         }
-        // Logging a warning as it is supposed to be a rare event
-        // with well behaving clients...
-        LOG_WARN(bad_packet4_logger, DHCP4_PACKET_DROP_0012)
-            .arg(query->toText())
-            .arg(this_thread::get_id())
-            .arg(holder_hw->query_->toText())
-            .arg(holder_hw->thread_);
-        stats::StatsMgr::instance().addValue("pkt4-receive-drop",
-                                             static_cast<int64_t>(1));
     }
     return (false);
 }

src/bin/dhcp4/client_handler.h

@@ -89,13 +89,19 @@ private:
         std::thread::id thread_;
 
         /// @brief The next query.
+        ///
+        /// @note This field can be modified from another handler
+        /// holding the mutex.
         Pkt4Ptr next_query_;
 
         /// @brief The continuation to process next query for the client.
+        ///
+        /// @note This field can be modified from another handler
+        /// holding the mutex.
         ContinuationPtr cont_;
     };
 
-    /// @brief The type of shared pointers to clients by ID.
+    /// @brief The type of shared pointers to clients.
     typedef boost::shared_ptr<Client> ClientPtr;
 
     /// @brief Local client.
@@ -197,7 +203,6 @@ private:
 
     /// @brief The client-by-hwaddr container.
     static ClientByHWAddrContainer clients_hwaddr_;
-
 };
 
 } // namespace isc

src/bin/dhcp6/client_handler.cc

@@ -105,6 +105,7 @@ ClientHandler::tryLock(Pkt6Ptr query, ContinuationPtr cont) {
         isc_throw(Unexpected, "empty DUID in ClientHandler::tryLock");
     }
     ClientPtr holder;
+    Pkt6Ptr next_query;
     {
         // Try to acquire the lock and return the holder when it failed.
         lock_guard<mutex> lock_(mutex_);
@@ -115,35 +116,36 @@ ClientHandler::tryLock(Pkt6Ptr query, ContinuationPtr cont) {
             lock();
             return (true);
         }
+        // This query can be a duplicate so put the continuation.
+        if (cont) {
+            next_query = holder->next_query_;
+            holder->next_query_ = query;
+            holder->cont_ = cont;
+        }
     }
-    // This query can be a duplicate so put the continuation.
     if (cont) {
-        Pkt6Ptr next_query = holder->next_query_;
-        holder->next_query_ = query;
-        holder->cont_ = cont;
         if (next_query) {
             // Logging a warning as it is supposed to be a rare event
             // with well behaving clients...
             LOG_WARN(bad_packet6_logger, DHCP6_PACKET_DROP_DUPLICATE)
                 .arg(next_query->toText())
                 .arg(this_thread::get_id())
-                .arg(query->toText())
+                .arg(holder->query_->toText())
-                .arg(this_thread::get_id());
+                .arg(holder->thread_);
             stats::StatsMgr::instance().addValue("pkt6-receive-drop",
                                                  static_cast<int64_t>(1));
         }
-        return (false);
+    } else {
+        // Logging a warning as it is supposed to be a rare event
+        // with well behaving clients...
+        LOG_WARN(bad_packet6_logger, DHCP6_PACKET_DROP_DUPLICATE)
+            .arg(query->toText())
+            .arg(this_thread::get_id())
+            .arg(holder->query_->toText())
+            .arg(holder->thread_);
+        stats::StatsMgr::instance().addValue("pkt6-receive-drop",
+                                             static_cast<int64_t>(1));
     }
-
-    // Logging a warning as it is supposed to be a rare event
-    // with well behaving clients...
-    LOG_WARN(bad_packet6_logger, DHCP6_PACKET_DROP_DUPLICATE)
-        .arg(query->toText())
-        .arg(this_thread::get_id())
-        .arg(holder->query_->toText())
-        .arg(holder->thread_);
-    stats::StatsMgr::instance().addValue("pkt6-receive-drop",
-                                         static_cast<int64_t>(1));
     return (false);
 }
 

src/bin/dhcp6/client_handler.h

@@ -80,9 +80,15 @@ private:
         std::thread::id thread_;
 
         /// @brief The next query.
+        ///
+        /// @note This field can be modified from another handler
+        /// holding the mutex.
         Pkt6Ptr next_query_;
 
         /// @brief The continuation to process next query for the client.
+        ///
+        /// @note This field can be modified from another handler
+        /// holding the mutex.
         ContinuationPtr cont_;
     };