[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 c7356aa2510a049623c745baa68a66c08229ed59..abb5b06f781161a53361cb9e3cb31e66a6a62012 100644 (file)
--- 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 80de99446a452086d46923ab64500686a0cd8a8d..71a04847a54e5d4474d4763b3a8c21089e25a76c 100644 (file)
--- 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