[219-allow-an-option-value-to-be-set-from-an-expression] Addressed comments

doc/sphinx/arm/hooks.rst

@@ -1170,31 +1170,23 @@ In the DHCPv6 case, the corresponding query will look similar to this:
 flex_option Flexible Option for Option value settings
 =====================================================
 
-This sectiom describes a hook application dedicated to generate flexible
-option values in response packets. The Kea software provides a way to
-configure  option values of response packets based on global configuration,
-client classes, shared networks, subnets, pools and host reservations but
-in all cases values are static. However, there are sometimes scenarios
-where the value should be computed from elements from the query packet.
-These scenarios are addressed by the Flexible Option hook application.
-
-This hook is available since Kea 1.7.1.
-
-.. note::
-
-   This library may only be loaded by the ``kea-dhp4`` or ``kea-dhp6``
-   process.
-
-The library allows the definition of an action per option using an
-expression on the query packer as for the Flex Indentifier hook library
-(See :ref:`flex-id`) or for client classification (See
-:ref:`classification-using-expressions` for a detailed description of
-the syntax available.) The ``add`` and ``supersede`` actions use an
-expression returning a string, doing nothing when it evaluates to
-the empty string. The ``remove`` application uses an expression returning
-true or false, doing nothing on false. When it is necessary to set
-an option to the empty value this mechanism does not work but a client
-class can be used instead.
+This library allows you to define an action to take, for a given option,
+based upon on the result of an expression.  These actions are carried
+out during the final stages of constructing a query response packet,
+just before it is sent to the client. The three actions currently
+supported are  ``add``, ``supersede``, and ``remove``.
+
+The syntax used for the action expressions is the same syntax used
+for client classification and the Flex Identifier hook library
+(See either :ref:`classification-using-expressions` or :ref:`flex-id`
+for detailed description of the syntax).
+
+The ``add`` and ``supersede`` actions use an expression returning a 
+string, doing nothing when it evaluates to the empty string. The 
+``remove`` application uses an expression returning true or false, 
+doing nothing on false. When it is necessary to set an option to the 
+empty value this mechanism does not work but a client class can be 
+used instead.
 
 The ``add`` action adds an option only when the option does not already
 exist and the expression does not evaluate to the empty string.
@@ -1203,16 +1195,18 @@ if it already exists. The ``remove`` action removes the option from
 the response packet if it already exists and the expression evaluates to
 true.
 
-The option where an action applies is specified by its code point or
-by its name. At least the code or the name must be specified. The option
-space is the DHCPv4 or DHCPv6 spaces depending of the server where the
-hook library is loaded. Other spaces as vendor spaces could be supported
-in a further version.
-
-The library can be loaded in a similar way as other hook libraries. It takes
-a mandatory ``options`` parameter holding a list of per option parameter
-maps with code, name, add, supersede and remove actions. Action entries
-take a string value representing an expression.
+The option to which an action applies may be specified by either its
+numeric code or its name.. At least the code or the name must be
+specified. The option space is the DHCPv4 or DHCPv6 spaces depending
+of the server where the hook library is loaded. Other spaces as vendor
+spaces could be supported in a further version.
+
+The library is available since Kea 1.7.1 and can be loaded in a
+similar way as other hook libraries by the ``kea-dhp4`` or `kea-dhp6``
+process.. It takes a mandatory ``options`` parameter holding a list of
+per option parameter maps with code, name, add, supersede and remove
+actions. Action entries take a string value representing an
+expression.
 
 ::
 

src/hooks/dhcp/flex_option/flex_option.cc

@@ -20,9 +20,55 @@ using namespace isc;
 using namespace isc::data;
 using namespace isc::dhcp;
 using namespace isc::eval;
+using namespace isc::flex_option;
 using namespace isc::log;
 using namespace std;
 
+namespace {
+
+/// @brief Parse an action.
+///
+/// @param option The option element.
+/// @param opt_cfg The option configuration.
+/// @param name The action name.
+/// @param action The action.
+/// @param parser_type The type of the parser of the expression.
+void
+parseAction(ConstElementPtr option,
+            FlexOptionImpl::OptionConfigPtr opt_cfg,
+            Option::Universe universe,
+            const string& name,
+            FlexOptionImpl::Action action,
+            EvalContext::ParserType parser_type) {
+    ConstElementPtr elem = option->get(name);
+    if (elem) {
+        if (elem->getType() != Element::string) {
+            isc_throw(BadValue, "'" << name << "' must be a string: "
+                      << elem->str());
+        }
+        string expr_text = elem->stringValue();
+        if (expr_text.empty()) {
+            isc_throw(BadValue, "'" << name << "' must not be empty");
+        }
+        if (opt_cfg->getAction() != FlexOptionImpl::NONE) {
+            isc_throw(BadValue, "multiple actions: " << option->str());
+        }
+        opt_cfg->setAction(action);
+        opt_cfg->setText(expr_text);
+        try {
+            EvalContext eval_ctx(universe);
+            eval_ctx.parseString(expr_text, parser_type);
+            ExpressionPtr expr(new Expression(eval_ctx.expression));
+            opt_cfg->setExpr(expr);
+        } catch (const std::exception& ex) {
+            isc_throw(BadValue, "can't parse " << name << " expression ["
+                      << expr_text << "] error: " << ex.what());
+        }
+    }
+}
+
+} // end of anonymous namespace
+
 namespace isc {
 namespace flex_option {
 
@@ -130,8 +176,9 @@ FlexOptionImpl::parseOptionConfig(ConstElementPtr option) {
                       << space << "' space");
         }
         if (code_elem && (def->getCode() != code)) {
-            isc_throw(BadValue, "option '" << name << "' has code "
-                      << def->getCode() << " but 'code' is " << code);
+            isc_throw(BadValue, "option '" << name << "' is defined as code: "
+                      << def->getCode() << ", not the specified code: "
+                      << code);
         }
         code = def->getCode();
     }
@@ -148,78 +195,13 @@ FlexOptionImpl::parseOptionConfig(ConstElementPtr option) {
     }
 
     OptionConfigPtr opt_cfg(new OptionConfig(code));
-    ConstElementPtr add_elem = option->get("add");
-    if (add_elem) {
-        if (add_elem->getType() != Element::string) {
-            isc_throw(BadValue, "'add' must be a string: "
-                      << add_elem->str());
-        }
-        string add = add_elem->stringValue();
-        if (add.empty()) {
-            isc_throw(BadValue, "'add' must not be empty");
-        }
-        opt_cfg->setAction(ADD);
-        opt_cfg->setText(add);
-        try {
-            EvalContext eval_ctx(universe);
-            eval_ctx.parseString(add, EvalContext::PARSER_STRING);
-            ExpressionPtr expr(new Expression(eval_ctx.expression));
-            opt_cfg->setExpr(expr);
-        } catch (const std::exception& ex) {
-            isc_throw(BadValue, "can't parse add expression ["
-                      << add << "] error: " << ex.what());
-        }
-    }
-    ConstElementPtr supersede_elem = option->get("supersede");
-    if (supersede_elem) {
-        if (supersede_elem->getType() != Element::string) {
-            isc_throw(BadValue, "'supersede' must be a string: "
-                      << supersede_elem->str());
-        }
-        string supersede = supersede_elem->stringValue();
-        if (supersede.empty()) {
-            isc_throw(BadValue, "'supersede' must not be empty");
-        }
-        if (opt_cfg->getAction() != NONE) {
-            isc_throw(BadValue, "multiple actions: " << option->str());
-        }
-        opt_cfg->setAction(SUPERSEDE);
-        opt_cfg->setText(supersede);
-        try {
-            EvalContext eval_ctx(universe);
-            eval_ctx.parseString(supersede, EvalContext::PARSER_STRING);
-            ExpressionPtr expr(new Expression(eval_ctx.expression));
-            opt_cfg->setExpr(expr);
-        } catch (const std::exception& ex) {
-            isc_throw(BadValue, "can't parse supersede expression ["
-                      << supersede << "] error: " << ex.what());
-        }
-    }
-    ConstElementPtr remove_elem = option->get("remove");
-    if (remove_elem) {
-        if (remove_elem->getType() != Element::string) {
-            isc_throw(BadValue, "'remove' must be a string: "
-                      << remove_elem->str());
-        }
-        string remove = remove_elem->stringValue();
-        if (remove.empty()) {
-            isc_throw(BadValue, "'remove' must not be empty");
-        }
-        if (opt_cfg->getAction() != NONE) {
-            isc_throw(BadValue, "multiple actions: " << option->str());
-        }
-        opt_cfg->setAction(REMOVE);
-        opt_cfg->setText(remove);
-        try {
-            EvalContext eval_ctx(universe);
-            eval_ctx.parseString(remove, EvalContext::PARSER_BOOL);
-            ExpressionPtr expr(new Expression(eval_ctx.expression));
-            opt_cfg->setExpr(expr);
-        } catch (const std::exception& ex) {
-            isc_throw(BadValue, "can't parse remove expression ["
-                      << remove << "] error: " << ex.what());
-        }
-    }
+    // opt_cfg initial action is NONE.
+    parseAction(option, opt_cfg, universe,
+                "add", ADD, EvalContext::PARSER_STRING);
+    parseAction(option, opt_cfg, universe,
+                "supersede", SUPERSEDE, EvalContext::PARSER_STRING);
+    parseAction(option, opt_cfg, universe,
+                "remove", REMOVE, EvalContext::PARSER_BOOL);
 
     if (opt_cfg->getAction() == NONE) {
         isc_throw(BadValue, "no action: " << option->str());

src/hooks/dhcp/flex_option/flex_option.dox

@@ -75,7 +75,7 @@ To configure it for kea-dhcp6, the commands are simply as shown below:
 }
 @endcode
 
-The parameter is rge options list of option configuration maps with:
+The sole parameter is a options list of options with:
  - @b code - Specifies the option code.
  - @b name - Speficies the option name, at least the option code or name
    must be given.
@@ -93,7 +93,8 @@ The parameter is rge options list of option configuration maps with:
    response. Only one action can be specified.
 
 Note for the rare options which can be empty this mechanism does not work.
-The proposed solution in this case is to use a client class.
+The proposed solution in this case is to use a client class to set the
+empty value to the option in a option-data clause.
 
 ## Internal operation
 

src/hooks/dhcp/flex_option/flex_option.h

@@ -17,6 +17,11 @@ namespace isc {
 namespace flex_option {
 
 /// @brief Flex Option implementation.
+///
+/// The implementation can be divided into two parts:
+///  - the configuration parsed and stored by load()
+///  - the response packet processing performed by the process method
+///
 class FlexOptionImpl {
 public:
 
@@ -152,13 +157,16 @@ public:
             case NONE:
                 break;
             case ADD:
+                // Don't add if option is already there.
                 if (opt) {
                     break;
                 }
                 value = isc::dhcp::evaluateString(*opt_cfg->getExpr(), *query);
+                // Do nothing is the expression evaluates to empty.
                 if (value.empty()) {
                     break;
                 }
+                // Add the option.
                 buffer.assign(value.begin(), value.end());
                 opt.reset(new isc::dhcp::Option(universe, opt_cfg->getCode(),
                                                 buffer));
@@ -166,14 +174,17 @@ public:
                 logAction(ADD, opt_cfg->getCode(), value);
                 break;
             case SUPERSEDE:
+                // Do nothing is the expression evaluates to empty.
                 value = isc::dhcp::evaluateString(*opt_cfg->getExpr(), *query);
                 if (value.empty()) {
                     break;
                 }
+                // Remove the option if already there.
                 while (opt) {
                     response->delOption(opt_cfg->getCode());
                     opt = response->getOption(opt_cfg->getCode());
                 }
+                // Add the option.
                 buffer.assign(value.begin(), value.end());
                 opt.reset(new isc::dhcp::Option(universe, opt_cfg->getCode(),
                                                 buffer));
@@ -181,12 +192,15 @@ public:
                 logAction(SUPERSEDE, opt_cfg->getCode(), value);
                 break;
             case REMOVE:
+                // Nothing to remove if option is not present.
                 if (!opt) {
                     break;
                 }
+                // Do nothing is the expression evaluates to false.
                 if (!isc::dhcp::evaluateBool(*opt_cfg->getExpr(), *query)) {
                     break;
                 }
+                // Remove the option.
                 while (opt) {
                     response->delOption(opt_cfg->getCode());
                     opt = response->getOption(opt_cfg->getCode());

src/hooks/dhcp/flex_option/tests/flex_option_unittests.cc

@@ -305,7 +305,9 @@ TEST_F(FlexOptionTest, optionConfigCodeNameMismatch) {
     ElementPtr name = Element::create(string("host-name"));
     option->set("name", name);
     EXPECT_THROW(impl_->testConfigure(options), BadValue);
-    EXPECT_EQ("option 'host-name' has code 12 but 'code' is 13", impl_->getErrMsg());
+    string expected = "option 'host-name' is defined as code: 12, ";
+    expected += "not the specified code: 13";
+    EXPECT_EQ(expected, impl_->getErrMsg());
 }
 
 // Verify that an option can be configured only once.