From: Marcin Siodelski <marcin@isc.org>
Date: Fri, 29 Jun 2018 18:44:37 +0000 (+0200)
Subject: [5664] Addressed review comments.
X-Git-Tag: trac5555_base~1^2~2
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=9627dbba770f7cac8a733fdf7081ce800feb3eb7;p=thirdparty%2Fkea.git

[5664] Addressed review comments.

Added test for preserving context upon callout handle reset. Also updated
a commentary.
---

diff --git a/src/lib/hooks/callout_handle.h b/src/lib/hooks/callout_handle.h
index c7356aa251..abb5b06f78 100644
--- a/src/lib/hooks/callout_handle.h
+++ b/src/lib/hooks/callout_handle.h
@@ -448,10 +448,11 @@ typedef boost::shared_ptr<CalloutHandle> CalloutHandlePtr;
 /// - the next step status is set to @c CalloutHandle::NEXT_STEP CONTINUE
 ///
 /// This class must never be modified to also delete the context
-/// information from the callout handle, because retaining this information
-/// is one of the primary reasons why this class is created. Otherwise,
-/// we could simply re-create the callout handle for each hook point
-/// without the need to reset the state like we do.
+/// information from the callout handle. The context is intended
+/// to be used to share stateful data across callouts and hook points
+/// and its contents must exist for the duration of the packet lifecycle.
+/// Otherwise, we could simply re-create the callout handle for
+/// each hook point and we wouldn't need this RAII class.
 class ScopedCalloutHandleState {
 public:
 
diff --git a/src/lib/hooks/tests/callout_handle_unittest.cc b/src/lib/hooks/tests/callout_handle_unittest.cc
index 80de99446a..71a04847a5 100644
--- a/src/lib/hooks/tests/callout_handle_unittest.cc
+++ b/src/lib/hooks/tests/callout_handle_unittest.cc
@@ -327,8 +327,10 @@ TEST_F(CalloutHandleTest, scopedState) {
     // Set two arguments and the non-default status.
     int one = 1;
     int two = 2;
+    int three = 3;
     handle->setArgument("one", one);
     handle->setArgument("two", two);
+    handle->setContext("three", three);
     handle->setStatus(CalloutHandle::NEXT_STEP_DROP);
 
     int value = 0;
@@ -344,6 +346,10 @@ TEST_F(CalloutHandleTest, scopedState) {
         EXPECT_THROW(handle->getArgument("two", value), NoSuchArgument);
         EXPECT_EQ(CalloutHandle::NEXT_STEP_CONTINUE, handle->getStatus());
 
+        // Context should be intact.
+        ASSERT_NO_THROW(handle->getContext("three", value));
+        EXPECT_EQ(three, value);
+
         // Set the arguments and status again prior to the destruction of
         // the wrapper.
         handle->setArgument("one", one);
@@ -356,6 +362,10 @@ TEST_F(CalloutHandleTest, scopedState) {
     EXPECT_THROW(handle->getArgument("one", value), NoSuchArgument);
     EXPECT_THROW(handle->getArgument("two", value), NoSuchArgument);
     EXPECT_EQ(CalloutHandle::NEXT_STEP_CONTINUE, handle->getStatus());
+
+    // Context should be intact.
+    ASSERT_NO_THROW(handle->getContext("three", value));
+    EXPECT_EQ(three, value);
 }
 
 // Further tests of the "skip" flag and tests of getting the name of the