[#1088] addressed review

diff --git a/src/hooks/dhcp/bootp/bootp_callouts.cc b/src/hooks/dhcp/bootp/bootp_callouts.cc
index f97692d31d49f81e0d01009b519fb0d9e1aec9a2..baaa378da676045cf65ec8e6d257788f66a4ab51 100644 (file)
--- 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 e9cf1f5c41ed61790dac089e5ea2e6e8623e79a7..930d0bba19fc3942a6b214261be9b49caf6531ca 100644 (file)
--- 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 db055abe4cc838df101be7b64f8a1e795ba7cb3a..06788b0ebb0a8cc488bee689d22ef8e4ac3dc583 100644 (file)
--- 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 65b57b5e43eec4e5e70ac7f8c4d646ce39f19b1c..e03bc9b479366485ade093f9b3fa9be88849e28b 100644 (file)
--- 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 e1a0da88b3a0bf007cadf40c44fc75a7289acded..70388a34b05fbd50b54d51f9f0f45809591e213a 100644 (file)
--- 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) {
     ...
 }