From: Razvan Becheriu <razvan@isc.org>
Date: Mon, 24 Feb 2020 16:38:59 +0000 (+0200)
Subject: [#1088] addressed review
X-Git-Tag: Kea-1.7.6~101
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=be1241b0563fc559e3ec01942a417933020dcf18;p=thirdparty%2Fkea.git

[#1088] addressed review
---

diff --git a/src/hooks/dhcp/bootp/bootp_callouts.cc b/src/hooks/dhcp/bootp/bootp_callouts.cc
index f97692d31d..baaa378da6 100644
--- a/src/hooks/dhcp/bootp/bootp_callouts.cc
+++ b/src/hooks/dhcp/bootp/bootp_callouts.cc
@@ -64,8 +64,7 @@ extern "C" {
 /// @return 0 upon success, non-zero otherwise.
 int buffer4_receive(CalloutHandle& handle) {
     CalloutHandle::CalloutNextStep status = handle.getStatus();
-    if (status == CalloutHandle::NEXT_STEP_DROP ||
-        status == CalloutHandle::NEXT_STEP_SKIP) {
+    if (status == CalloutHandle::NEXT_STEP_DROP) {
         return (0);
     }
 
@@ -74,8 +73,9 @@ int buffer4_receive(CalloutHandle& handle) {
     handle.getArgument("query4", query);
 
     try {
-        // Unpack it (TODO check if it was already unpacked).
-        query->unpack();
+        if (handle.getStatus() != CalloutHandle::NEXT_STEP_SKIP) {
+            query->unpack();
+        }
 
         // Not DHCP query nor BOOTP response?
         if ((query->getType() == DHCP_NOTYPE) &&
diff --git a/src/hooks/dhcp/flex_option/flex_option_callouts.cc b/src/hooks/dhcp/flex_option/flex_option_callouts.cc
index e9cf1f5c41..930d0bba19 100644
--- a/src/hooks/dhcp/flex_option/flex_option_callouts.cc
+++ b/src/hooks/dhcp/flex_option/flex_option_callouts.cc
@@ -42,8 +42,7 @@ extern "C" {
 /// @return 0 upon success, non-zero otherwise
 int pkt4_send(CalloutHandle& handle) {
     CalloutHandle::CalloutNextStep status = handle.getStatus();
-    if (status == CalloutHandle::NEXT_STEP_DROP ||
-        status == CalloutHandle::NEXT_STEP_SKIP) {
+    if (status == CalloutHandle::NEXT_STEP_DROP) {
         return (0);
     }
 
@@ -58,6 +57,10 @@ int pkt4_send(CalloutHandle& handle) {
     handle.getArgument("query4", query);
     handle.getArgument("response4", response);
 
+    if (status == CalloutHandle::NEXT_STEP_SKIP) {
+        isc_throw(InvalidOperation, "packet pack already handled");
+    }
+
     try {
         impl->process<Pkt4Ptr>(Option::V4, query, response);
     } catch (const std::exception& ex) {
@@ -80,8 +83,7 @@ int pkt4_send(CalloutHandle& handle) {
 /// @return 0 upon success, non-zero otherwise
 int pkt6_send(CalloutHandle& handle) {
     CalloutHandle::CalloutNextStep status = handle.getStatus();
-    if (status == CalloutHandle::NEXT_STEP_DROP ||
-        status == CalloutHandle::NEXT_STEP_SKIP) {
+    if (status == CalloutHandle::NEXT_STEP_DROP) {
         return (0);
     }
 
@@ -90,6 +92,10 @@ int pkt6_send(CalloutHandle& handle) {
         return (0);
     }
 
+    if (status == CalloutHandle::NEXT_STEP_SKIP) {
+        isc_throw(InvalidOperation, "packet pack already handled");
+    }
+
     // Get the parameters.
     Pkt6Ptr query;
     Pkt6Ptr response;
diff --git a/src/hooks/dhcp/high_availability/ha_impl.cc b/src/hooks/dhcp/high_availability/ha_impl.cc
index db055abe4c..06788b0ebb 100644
--- a/src/hooks/dhcp/high_availability/ha_impl.cc
+++ b/src/hooks/dhcp/high_availability/ha_impl.cc
@@ -51,18 +51,12 @@ HAImpl::buffer4Receive(hooks::CalloutHandle& callout_handle) {
     Pkt4Ptr query4;
     callout_handle.getArgument("query4", query4);
 
-    bool unpack = true;
-    CalloutHandle::CalloutNextStep status = callout_handle.getStatus();
-    if (status == CalloutHandle::NEXT_STEP_SKIP) {
-        unpack = false;
-    }
-
     /// @todo Add unit tests to verify the behavior for different
     /// malformed packets.
     try {
         // We have to unpack the query to get access into HW address which is
         // used to load balance the packet.
-        if (unpack) {
+        if (callout_handle.getStatus() != CalloutHandle::NEXT_STEP_SKIP) {
             query4->unpack();
         }
 
@@ -164,18 +158,12 @@ HAImpl::buffer6Receive(hooks::CalloutHandle& callout_handle) {
     Pkt6Ptr query6;
     callout_handle.getArgument("query6", query6);
 
-    bool unpack = true;
-    CalloutHandle::CalloutNextStep status = callout_handle.getStatus();
-    if (status == CalloutHandle::NEXT_STEP_SKIP) {
-        unpack = false;
-    }
-
     /// @todo Add unit tests to verify the behavior for different
     /// malformed packets.
     try {
         // We have to unpack the query to get access into DUID which is
         // used to load balance the packet.
-        if (unpack) {
+        if (callout_handle.getStatus() != CalloutHandle::NEXT_STEP_SKIP) {
             query6->unpack();
         }
 
diff --git a/src/hooks/dhcp/user_chk/pkt_send_co.cc b/src/hooks/dhcp/user_chk/pkt_send_co.cc
index 65b57b5e43..e03bc9b479 100644
--- a/src/hooks/dhcp/user_chk/pkt_send_co.cc
+++ b/src/hooks/dhcp/user_chk/pkt_send_co.cc
@@ -68,11 +68,14 @@ extern "C" {
 /// @return 0 upon success, non-zero otherwise.
 int pkt4_send(CalloutHandle& handle) {
     CalloutHandle::CalloutNextStep status = handle.getStatus();
-    if (status == CalloutHandle::NEXT_STEP_DROP ||
-        status == CalloutHandle::NEXT_STEP_SKIP) {
+    if (status == CalloutHandle::NEXT_STEP_DROP) {
         return (0);
     }
 
+    if (status == CalloutHandle::NEXT_STEP_SKIP) {
+        isc_throw(InvalidOperation, "packet pack already handled");
+    }
+
     try {
         Pkt4Ptr response;
         handle.getArgument("response4", response);
@@ -145,11 +148,14 @@ int pkt4_send(CalloutHandle& handle) {
 /// @return 0 upon success, non-zero otherwise.
 int pkt6_send(CalloutHandle& handle) {
     CalloutHandle::CalloutNextStep status = handle.getStatus();
-    if (status == CalloutHandle::NEXT_STEP_DROP ||
-        status == CalloutHandle::NEXT_STEP_SKIP) {
+    if (status == CalloutHandle::NEXT_STEP_DROP) {
         return (0);
     }
 
+    if (status == CalloutHandle::NEXT_STEP_SKIP) {
+        isc_throw(InvalidOperation, "packet pack already handled");
+    }
+
     try {
         Pkt6Ptr response;
         handle.getArgument("response6", response);
diff --git a/src/lib/hooks/hooks_user.dox b/src/lib/hooks/hooks_user.dox
index e1a0da88b3..70388a34b0 100644
--- a/src/lib/hooks/hooks_user.dox
+++ b/src/lib/hooks/hooks_user.dox
@@ -480,7 +480,7 @@ setStatus. Their usage is intuitive:
 
 @code
     // Get the current setting of the next step status.
-    CalloutHandle::CalloutNextStep status = handle.getStatus();
+    auto status = handle.getStatus();
 
     if (status == CalloutHandle::NEXT_STEP_DROP)
        // Do something...
@@ -523,8 +523,8 @@ also perform a check for the SKIP flag before anything else.  If it is already
 set, do not pack/unpack the packet (other library, or even the same library, if
 loaded multiple times, has done it already).  Some libraries might also need to
 throw exceptions in such cases because they need to perform specific actions before
-pack/unpack, which have no effect if pack/unpack is done previously by some other
-library.
+pack/unpack (eg. addOption/delOption before pack action), which have no effect if
+pack/unpack action is done previously by some other library.
 
 As stated before, the order of loading libraries is crucial in achieving the
 desired behavior, so please read @ref hooksdgMultipleLibraries when configuring
@@ -567,7 +567,7 @@ handle.setStatus(CalloutHandle::NEXT_STEP_CONTINUE);
 handle.setStatus(CalloutHandle::NEXT_STEP_SKIP);
 
 // Check the status state.
-CalloutHandle::CalloutNextStep status = handle.getStatus();
+auto status = handle.getStatus();
 if (status == CalloutHandle::NEXT_STEP_SKIP) {
     ...
 }