Browse Source

[3499] Changes after review:

 - HandlesTest.CheckModifiedArgument test covers DROP status
 - Hooks Component Developer's Guide updated
 - Nest -> Next
Tomek Mrugalski 9 years ago
parent
commit
e1aa57e32c

+ 3 - 3
src/bin/dhcp4/dhcp4_hooks.dox

@@ -92,7 +92,7 @@ packet processing. Hook points that are not specific to packet processing
    field has been already parsed and stored in other fields. Therefore,
    field has been already parsed and stored in other fields. Therefore,
    the modification in the data_ field has no effect.
    the modification in the data_ field has no effect.
 
 
- - <b>Nest step status</b>: If any callout sets the status to SKIP, the server will
+ - <b>Next step status</b>: If any callout sets the status to SKIP, the server will
    drop the packet and start processing the next one.  The reason for the drop
    drop the packet and start processing the next one.  The reason for the drop
    will be logged if logging is set to the appropriate debug level.
    will be logged if logging is set to the appropriate debug level.
 
 
@@ -172,7 +172,7 @@ packet processing. Hook points that are not specific to packet processing
    The "lease4" argument points to @c Lease4 object that contains the lease to
    The "lease4" argument points to @c Lease4 object that contains the lease to
    be released. It doesn't make sense to modify it at this time.
    be released. It doesn't make sense to modify it at this time.
 
 
- - <b>Nest step status</b>: If any callout installed on the "lease4_release" hook
+ - <b>Next step status</b>: If any callout installed on the "lease4_release" hook
    sets the next step action to SKIP, the server will not delete the lease.
    sets the next step action to SKIP, the server will not delete the lease.
    It will be kept in the database and will go through the regular expiration/reuse
    It will be kept in the database and will go through the regular expiration/reuse
    process.
    process.
@@ -192,7 +192,7 @@ packet processing. Hook points that are not specific to packet processing
    pkt4_send callouts are complete, so any changes to that field will
    pkt4_send callouts are complete, so any changes to that field will
    be overwritten.)
    be overwritten.)
 
 
- - <b>Nest step action</b>: if any callout installed on the "pkt4_send" hook
+ - <b>Next step action</b>: if any callout installed on the "pkt4_send" hook
    sets the next step action to SKIP, the server will not construct the raw
    sets the next step action to SKIP, the server will not construct the raw
    buffer. The expectation is that if the callout set skip flag, it is
    buffer. The expectation is that if the callout set skip flag, it is
    responsible for constructing raw form on its own. Otherwise the output
    responsible for constructing raw form on its own. Otherwise the output

+ 29 - 23
src/lib/hooks/hooks_component_developer.dox

@@ -99,13 +99,14 @@ information can be passed to a callout and/or the callout can affect
 the processing of the component. The latter is achieved in either or both
 the processing of the component. The latter is achieved in either or both
 of the following ways:
 of the following ways:
 
 
-- Setting the "skip" flag.  This is a boolean flag that the callout can set
+- Setting the nest step status.  This is an enum that the callout can set
   and is a quick way of passing information back to the component.  It is used
   and is a quick way of passing information back to the component.  It is used
-  to indicate that the component should skip the processing step associated with
-  the hook.  The exact action is up to the component, but is likely to be one
-  of skipping the processing step (probably because the callout has
-  done its own processing for the action) or dropping the current packet
-  and starting on a new request.
+  to indicate that the component should perform certain actions. Currently
+  there are three statuses defined: CONTINUE (this is the default, the server
+  is expected to continue as usual), SKIP (the server is expected to skip the
+  next processing step, but otherwise continue as usual) and DROP (the server
+  is expected to drop the packet or request completely. The exact action is up
+  to the component.
 
 
 - Modifying data passed to it.  The component should be prepared to continue
 - Modifying data passed to it.  The component should be prepared to continue
   processing with the data returned by the callout.  It is up to the component
   processing with the data returned by the callout.  It is up to the component
@@ -282,7 +283,11 @@ underlying object is altered through that pointer, the change will be
 reflected in the component even if the callout makes no call to setArgument.
 reflected in the component even if the callout makes no call to setArgument.
 This can be avoided by passing a pointer to a "const" object.
 This can be avoided by passing a pointer to a "const" object.
 
 
-@subsection hooksComponentSkipFlag The Skip Flag
+@subsection hooksComponentSkipFlag The Skip Flag (obsolete)
+
+
+
+@subsection hooksComponentNextStep The next step status
 
 
 Although information is passed back to the component from callouts through
 Although information is passed back to the component from callouts through
 @c CalloutHandle arguments, a common action for callouts is to inform the component
 @c CalloutHandle arguments, a common action for callouts is to inform the component
@@ -290,24 +295,25 @@ that its flow of control should be altered.  For example:
 
 
 - In the DHCP servers, there is a hook at the point at which a lease is
 - In the DHCP servers, there is a hook at the point at which a lease is
   about to be assigned.  Callouts attached to this hooks may handle the
   about to be assigned.  Callouts attached to this hooks may handle the
-  lease assignment in special cases, in which case they set the skip flag
-  to indicate that the server should not perform lease assignment in this
-  case.
+  lease assignment in special cases, in which case they set the next step
+  status to SKIP to indicate that the server should not perform lease assignment
+  in this case.
 - A server may define a hook just after a packet is received.  A callout
 - A server may define a hook just after a packet is received.  A callout
   attached to the hook might inspect the source address and compare it
   attached to the hook might inspect the source address and compare it
   against a blacklist.  If the address is on the list, the callout could set
   against a blacklist.  If the address is on the list, the callout could set
-  the skip flag to indicate to the server that the packet should be dropped.
+  the DROP flag to indicate to the server that the packet should be dropped.
 
 
 For ease of processing, the @c CalloutHandle contains
 For ease of processing, the @c CalloutHandle contains
-two methods, @c isc::hooks::CalloutHandle::getSkip() and
-@c isc::hooks::CalloutHandle::setSkip().  It is only meaningful for the
-component to use the "get" method.  The skip flag is cleared by the hooks
-framework when the component requests that callouts be executed, so any
-value set by the component is lost.  Callouts can both inspect the flag (it
+two methods, @c isc::hooks::CalloutHandle::getStatus() and
+@c isc::hooks::CalloutHandle::setStatus().  It is only meaningful for the
+component to use the "get" method.  The next step status is cleared (set to
+the default value of CONTINUE) by the hooks framework when the component
+requests that callouts be executed, so any
+value set by the component is lost.  Callouts can both inspect the status (it
 might have been set by callouts earlier in the callout list for the hook)
 might have been set by callouts earlier in the callout list for the hook)
-and set it.  Note that the setting of the flag by a callout does not
-prevent callouts later in the list from being called: the skip flag is
-just a boolean flag - the only significance comes from its interpretation
+and set it.  Note that the setting of the status by a callout does not
+prevent callouts later in the list from being called: the next step status is
+just an enum value - the only significance comes from its interpretation
 by the component.
 by the component.
 
 
 An example of use could be:
 An example of use could be:
@@ -316,7 +322,7 @@ An example of use could be:
 handle->setArgument("query", query);
 handle->setArgument("query", query);
 handle->setArgument("response", response);
 handle->setArgument("response", response);
 HooksManager::callCallouts(lease_hook_index, *handle_ptr);
 HooksManager::callCallouts(lease_hook_index, *handle_ptr);
-if (! handle_ptr->getSkip()) {
+if (! handle_ptr->getStatus() != CalloutHandle::NEXT_STEP_SKIP) {
     // Skip flag not set, do the address allocation
     // Skip flag not set, do the address allocation
     :
     :
 }
 }
@@ -359,7 +365,7 @@ the hook can be executed by:
 ... where "*handle_ptr" is a reference (note: not a pointer) to the
 ... where "*handle_ptr" is a reference (note: not a pointer) to the
 @c isc::hooks::CalloutHandle object holding the arguments.  No status code
 @c isc::hooks::CalloutHandle object holding the arguments.  No status code
 is returned.  If a component needs to get data returned (other than that
 is returned.  If a component needs to get data returned (other than that
-provided by the "skip" flag), it should define an argument through which
+provided by the next step status), it should define an argument through which
 the callout can do so.
 the callout can do so.
 
 
 @subsubsection hooksComponentConditionalCallout Conditionally Calling Hook Callouts
 @subsubsection hooksComponentConditionalCallout Conditionally Calling Hook Callouts
@@ -379,8 +385,8 @@ if (HooksManager::calloutsPresent(lease_hook_index)) {
     handle->setArgument("query", query);
     handle->setArgument("query", query);
     handle->setArgument("response", response);
     handle->setArgument("response", response);
     HooksManager::callCallouts(lease_hook_index, *handle);
     HooksManager::callCallouts(lease_hook_index, *handle);
-    if (! handle->getSkip()) {
-        // Skip flag not set, do the address allocation
+    if ( handle->getStatus() != CalloutHandle::NEXT_STEP_DROP ) {
+        // Next step allows us to continue, do the address allocation
         :
         :
     }
     }
 }
 }

+ 23 - 16
src/lib/hooks/tests/handles_unittest.cc

@@ -885,13 +885,18 @@ calloutSetArgumentCommon(CalloutHandle& handle, const char* what) {
 }
 }
 
 
 int
 int
-calloutSetArgumentYes(CalloutHandle& handle) {
-    return (calloutSetArgumentCommon(handle, "Y"));
+calloutSetArgumentSkip(CalloutHandle& handle) {
+    return (calloutSetArgumentCommon(handle, "S"));
 }
 }
 
 
 int
 int
-calloutSetArgumentNo(CalloutHandle& handle) {
-    return (calloutSetArgumentCommon(handle, "N"));
+calloutSetArgumentContinue(CalloutHandle& handle) {
+    return (calloutSetArgumentCommon(handle, "C"));
+}
+
+int
+calloutSetArgumentDrop(CalloutHandle& handle) {
+    return (calloutSetArgumentCommon(handle, "D"));
 }
 }
 
 
 // ... and a callout to just copy the argument to the "common_string_" variable
 // ... and a callout to just copy the argument to the "common_string_" variable
@@ -903,23 +908,25 @@ calloutPrintArgument(CalloutHandle& handle) {
     return (0);
     return (0);
 }
 }
 
 
+// This test verifies that the next step status is processed appropriately.
+// The test checks the following next step statuses: CONTINUE, SKIP, DROP.
 TEST_F(HandlesTest, CheckModifiedArgument) {
 TEST_F(HandlesTest, CheckModifiedArgument) {
     getCalloutManager()->setLibraryIndex(0);
     getCalloutManager()->setLibraryIndex(0);
-    getCalloutManager()->registerCallout("alpha", calloutSetArgumentYes);
-    getCalloutManager()->registerCallout("alpha", calloutSetArgumentNo);
-    getCalloutManager()->registerCallout("alpha", calloutSetArgumentNo);
+    getCalloutManager()->registerCallout("alpha", calloutSetArgumentSkip);
+    getCalloutManager()->registerCallout("alpha", calloutSetArgumentContinue);
+    getCalloutManager()->registerCallout("alpha", calloutSetArgumentContinue);
 
 
     getCalloutManager()->setLibraryIndex(1);
     getCalloutManager()->setLibraryIndex(1);
-    getCalloutManager()->registerCallout("alpha", calloutSetArgumentYes);
-    getCalloutManager()->registerCallout("alpha", calloutSetArgumentYes);
+    getCalloutManager()->registerCallout("alpha", calloutSetArgumentSkip);
+    getCalloutManager()->registerCallout("alpha", calloutSetArgumentDrop);
     getCalloutManager()->registerCallout("alpha", calloutPrintArgument);
     getCalloutManager()->registerCallout("alpha", calloutPrintArgument);
-    getCalloutManager()->registerCallout("alpha", calloutSetArgumentNo);
-    getCalloutManager()->registerCallout("alpha", calloutSetArgumentNo);
+    getCalloutManager()->registerCallout("alpha", calloutSetArgumentDrop);
+    getCalloutManager()->registerCallout("alpha", calloutSetArgumentContinue);
 
 
     getCalloutManager()->setLibraryIndex(2);
     getCalloutManager()->setLibraryIndex(2);
-    getCalloutManager()->registerCallout("alpha", calloutSetArgumentYes);
-    getCalloutManager()->registerCallout("alpha", calloutSetArgumentNo);
-    getCalloutManager()->registerCallout("alpha", calloutSetArgumentYes);
+    getCalloutManager()->registerCallout("alpha", calloutSetArgumentSkip);
+    getCalloutManager()->registerCallout("alpha", calloutSetArgumentContinue);
+    getCalloutManager()->registerCallout("alpha", calloutSetArgumentSkip);
 
 
     // Create the argument with an initial empty string value.  Then call the
     // Create the argument with an initial empty string value.  Then call the
     // sequence of callouts above.
     // sequence of callouts above.
@@ -931,10 +938,10 @@ TEST_F(HandlesTest, CheckModifiedArgument) {
     // Check the intermediate and results.  For visual checking, the expected
     // Check the intermediate and results.  For visual checking, the expected
     // string is divided into sections corresponding to the blocks of callouts
     // string is divided into sections corresponding to the blocks of callouts
     // above.
     // above.
-    EXPECT_EQ(std::string("YNN" "YY"), common_string_);
+    EXPECT_EQ(std::string("SCC" "SD"), common_string_);
 
 
     callout_handle.getArgument(MODIFIED_ARG, modified_arg);
     callout_handle.getArgument(MODIFIED_ARG, modified_arg);
-    EXPECT_EQ(std::string("YNN" "YYNN" "YNY"), modified_arg);
+    EXPECT_EQ(std::string("SCC" "SDDC" "SCS"), modified_arg);
 }
 }
 
 
 // Test that the CalloutHandle provides the name of the hook to which the
 // Test that the CalloutHandle provides the name of the hook to which the