[4204] Runtime option definitions created using set/commit process.

11 files changed, 337 insertions, 7 deletions

diff --git a/src/bin/dhcp4/json_config_parser.cc b/src/bin/dhcp4/json_config_parser.cc
index 8b6a719254..1483f6288c 100644
--- a/src/bin/dhcp4/json_config_parser.cc
+++ b/src/bin/dhcp4/json_config_parser.cc
@@ -513,6 +513,9 @@ configureDhcp4Server(Dhcpv4Srv&, isc::data::ConstElementPtr config_set) {
     // Remove any existing timers.
     TimerMgr::instance()->unregisterTimers();
 
+    // Revert any runtime option definitions configured so far and not committed.
+    LibDHCP::revertRuntimeOptionDefs();
+
     // Some of the values specified in the configuration depend on
     // other values. Typically, the values in the subnet4 structure
     // depend on the global values. Also, option values configuration
@@ -699,6 +702,9 @@ configureDhcp4Server(Dhcpv4Srv&, isc::data::ConstElementPtr config_set) {
     // Rollback changes as the configuration parsing failed.
     if (rollback) {
         globalContext().reset(new ParserContext(original_context));
+        // Revert to original configuration of runtime option definitions
+        // in the libdhcp++.
+        LibDHCP::revertRuntimeOptionDefs();
         return (answer);
     }
 
diff --git a/src/bin/dhcp4/tests/config_parser_unittest.cc b/src/bin/dhcp4/tests/config_parser_unittest.cc
index 2f5938516c..86c2dee095 100644
--- a/src/bin/dhcp4/tests/config_parser_unittest.cc
+++ b/src/bin/dhcp4/tests/config_parser_unittest.cc
@@ -1356,6 +1356,15 @@ TEST_F(Dhcp4ParserTest, optionDefIpv4Address) {
     EXPECT_FALSE(def->getArrayType());
     EXPECT_EQ(OPT_IPV4_ADDRESS_TYPE, def->getType());
     EXPECT_TRUE(def->getEncapsulatedSpace().empty());
+
+    // The copy of the option definition should be available in the libdhcp++.
+    OptionDefinitionPtr def_libdhcp = LibDHCP::getRuntimeOptionDef("isc", 100);
+    ASSERT_TRUE(def_libdhcp);
+
+    // Both definitions should be held in distinct pointers but they should
+    // be equal.
+    EXPECT_TRUE(def_libdhcp != def);
+    EXPECT_TRUE(*def_libdhcp == *def);
 }
 
 // The goal of this test is to check whether an option definition
@@ -1468,6 +1477,14 @@ TEST_F(Dhcp4ParserTest, optionDefMultiple) {
 // The goal of this test is to verify that the duplicated option
 // definition is not accepted.
 TEST_F(Dhcp4ParserTest, optionDefDuplicate) {
+    // Preconfigure libdhcp++ with option definitions. The new configuration
+    // should override it, but when the new configuration fails, it should
+    // revert to this original configuration.
+    OptionDefSpaceContainer defs;
+    OptionDefinitionPtr def(new OptionDefinition("bar", 233, "string"));
+    defs.addItem(def, "isc");
+    LibDHCP::setRuntimeOptionDefs(defs);
+    LibDHCP::commitRuntimeOptionDefs();
 
     // Configuration string. Both option definitions have
     // the same code and belong to the same option space.
@@ -1498,6 +1515,15 @@ TEST_F(Dhcp4ParserTest, optionDefDuplicate) {
     ASSERT_TRUE(status);
     checkResult(status, 1);
     EXPECT_TRUE(errorContainsPosition(status, "<string>"));
+
+    // The new configuration should have inserted option 100, but
+    // once configuration failed (on the duplicate option definition)
+    // the original configuration in libdhcp++ should be reverted.
+    EXPECT_FALSE(LibDHCP::getRuntimeOptionDef("isc", 100));
+    def = LibDHCP::getRuntimeOptionDef("isc", 233);
+    ASSERT_TRUE(def);
+    EXPECT_EQ("bar", def->getName());
+    EXPECT_EQ(233, def->getCode());
 }
 
 // The goal of this test is to verify that the option definition
diff --git a/src/bin/dhcp6/ctrl_dhcp6_srv.cc b/src/bin/dhcp6/ctrl_dhcp6_srv.cc
index e02f7be2cb..8ac0aa1e62 100644
--- a/src/bin/dhcp6/ctrl_dhcp6_srv.cc
+++ b/src/bin/dhcp6/ctrl_dhcp6_srv.cc
@@ -15,6 +15,7 @@
 #include <config.h>
 #include <cc/data.h>
 #include <config/command_mgr.h>
+#include <dhcp/libdhcp++.h>
 #include <dhcpsrv/cfgmgr.h>
 #include <dhcp6/ctrl_dhcp6_srv.h>
 #include <dhcp6/dhcp6_log.h>
@@ -219,6 +220,10 @@ ControlledDhcpv6Srv::processConfig(isc::data::ConstElementPtr config) {
         }
     }
 
+    // Finally, we can commit runtime option definitions in libdhcp++. This is
+    // exception free.
+    LibDHCP::commitRuntimeOptionDefs();
+
     return (answer);
 }
 
diff --git a/src/bin/dhcp6/json_config_parser.cc b/src/bin/dhcp6/json_config_parser.cc
index fe0af18311..692d38db64 100644
--- a/src/bin/dhcp6/json_config_parser.cc
+++ b/src/bin/dhcp6/json_config_parser.cc
@@ -752,6 +752,9 @@ configureDhcp6Server(Dhcpv6Srv&, isc::data::ConstElementPtr config_set) {
     // Remove any existing timers.
     TimerMgr::instance()->unregisterTimers();
 
+    // Revert any runtime option definitions configured so far and not committed.
+    LibDHCP::revertRuntimeOptionDefs();
+
     // Some of the values specified in the configuration depend on
     // other values. Typically, the values in the subnet6 structure
     // depend on the global values. Also, option values configuration
@@ -945,6 +948,9 @@ configureDhcp6Server(Dhcpv6Srv&, isc::data::ConstElementPtr config_set) {
     // Rollback changes as the configuration parsing failed.
     if (rollback) {
         globalContext().reset(new ParserContext(original_context));
+        // Revert to original configuration of runtime option definitions
+        // in the libdhcp++.
+        LibDHCP::revertRuntimeOptionDefs();
         return (answer);
     }
 
diff --git a/src/bin/dhcp6/tests/config_parser_unittest.cc b/src/bin/dhcp6/tests/config_parser_unittest.cc
index d08ee4a0c0..4ab9e9a4f1 100644
--- a/src/bin/dhcp6/tests/config_parser_unittest.cc
+++ b/src/bin/dhcp6/tests/config_parser_unittest.cc
@@ -1593,6 +1593,15 @@ TEST_F(Dhcp6ParserTest, optionDefIpv6Address) {
     EXPECT_EQ(100, def->getCode());
     EXPECT_FALSE(def->getArrayType());
     EXPECT_EQ(OPT_IPV6_ADDRESS_TYPE, def->getType());
+
+    // The copy of the option definition should be available in the libdhcp++.
+    OptionDefinitionPtr def_libdhcp = LibDHCP::getRuntimeOptionDef("isc", 100);
+    ASSERT_TRUE(def_libdhcp);
+
+    // Both definitions should be held in distinct pointers but they should
+    // be equal.
+    EXPECT_TRUE(def_libdhcp != def);
+    EXPECT_TRUE(*def_libdhcp == *def);
 }
 
 // The goal of this test is to check whether an option definition
@@ -1702,6 +1711,14 @@ TEST_F(Dhcp6ParserTest, optionDefMultiple) {
 // The goal of this test is to verify that the duplicated option
 // definition is not accepted.
 TEST_F(Dhcp6ParserTest, optionDefDuplicate) {
+    // Preconfigure libdhcp++ with option definitions. The new configuration
+    // should override it, but when the new configuration fails, it should
+    // revert to this original configuration.
+    OptionDefSpaceContainer defs;
+    OptionDefinitionPtr def(new OptionDefinition("bar", 233, "string"));
+    defs.addItem(def, "isc");
+    LibDHCP::setRuntimeOptionDefs(defs);
+    LibDHCP::commitRuntimeOptionDefs();
 
     // Configuration string. Both option definitions have
     // the same code and belong to the same option space.
@@ -1732,6 +1749,15 @@ TEST_F(Dhcp6ParserTest, optionDefDuplicate) {
     ASSERT_TRUE(status);
     checkResult(status, 1);
     EXPECT_TRUE(errorContainsPosition(status, "<string>"));
+
+    // The new configuration should have inserted option 100, but
+    // once configuration failed (on the duplicate option definition)
+    // the original configuration in libdhcp++ should be reverted.
+    EXPECT_FALSE(LibDHCP::getRuntimeOptionDef("isc", 100));
+    def = LibDHCP::getRuntimeOptionDef("isc", 233);
+    ASSERT_TRUE(def);
+    EXPECT_EQ("bar", def->getName());
+    EXPECT_EQ(233, def->getCode());
 }
 
 // The goal of this test is to verify that the option definition
diff --git a/src/lib/dhcp/libdhcp++.cc b/src/lib/dhcp/libdhcp++.cc
index e10acfd0f6..b450a6307a 100644
--- a/src/lib/dhcp/libdhcp++.cc
+++ b/src/lib/dhcp/libdhcp++.cc
@@ -55,7 +55,7 @@ VendorOptionDefContainers LibDHCP::vendor4_defs_;
 VendorOptionDefContainers LibDHCP::vendor6_defs_;
 
 // Static container with option definitions created in runtime.
-OptionDefSpaceContainer LibDHCP::runtime_option_defs_;
+StagedValue<OptionDefSpaceContainer> LibDHCP::runtime_option_defs_;
 
 
 // Those two vendor classes are used for cable modems:
@@ -202,7 +202,7 @@ LibDHCP::getVendorOptionDef(const Option::Universe u, const uint32_t vendor_id,
 
 OptionDefinitionPtr
 LibDHCP::getRuntimeOptionDef(const std::string& space, const uint16_t code) {
-    OptionDefContainerPtr container = runtime_option_defs_.getItems(space);
+    OptionDefContainerPtr container = runtime_option_defs_.getValue().getItems(space);
     const OptionDefContainerTypeIndex& index = container->get<1>();
     const OptionDefContainerTypeRange& range = index.equal_range(code);
     if (range.first != range.second) {
@@ -214,7 +214,7 @@ LibDHCP::getRuntimeOptionDef(const std::string& space, const uint16_t code) {
 
 OptionDefinitionPtr
 LibDHCP::getRuntimeOptionDef(const std::string& space, const std::string& name) {
-    OptionDefContainerPtr container = runtime_option_defs_.getItems(space);
+    OptionDefContainerPtr container = runtime_option_defs_.getValue().getItems(space);
     const OptionDefContainerNameIndex& index = container->get<2>();
     const OptionDefContainerNameRange& range = index.equal_range(name);
     if (range.first != range.second) {
@@ -226,11 +226,12 @@ LibDHCP::getRuntimeOptionDef(const std::string& space, const std::string& name)
 
 OptionDefContainerPtr
 LibDHCP::getRuntimeOptionDefs(const std::string& space) {
-    return (runtime_option_defs_.getItems(space));
+    return (runtime_option_defs_.getValue().getItems(space));
 }
 
 void
 LibDHCP::setRuntimeOptionDefs(const OptionDefSpaceContainer& defs) {
+    OptionDefSpaceContainer defs_copy;
     std::list<std::string> option_space_names = defs.getOptionSpaceNames();
     for (std::list<std::string>::const_iterator name = option_space_names.begin();
          name != option_space_names.end(); ++name) {
@@ -238,14 +239,25 @@ LibDHCP::setRuntimeOptionDefs(const OptionDefSpaceContainer& defs) {
         for (OptionDefContainer::const_iterator def = container->begin();
              def != container->end(); ++def) {
             OptionDefinitionPtr def_copy(new OptionDefinition(**def));
-            runtime_option_defs_.addItem(def_copy, *name);
+            defs_copy.addItem(def_copy, *name);
         }
     }
+    runtime_option_defs_ = defs_copy;
 }
 
 void
 LibDHCP::clearRuntimeOptionDefs() {
-    runtime_option_defs_.clearItems();
+    runtime_option_defs_.reset();
+}
+
+void
+LibDHCP::revertRuntimeOptionDefs() {
+    runtime_option_defs_.revert();
+}
+
+void
+LibDHCP::commitRuntimeOptionDefs() {
+    runtime_option_defs_.commit();
 }
 
 bool
diff --git a/src/lib/dhcp/libdhcp++.h b/src/lib/dhcp/libdhcp++.h
index 9bc344aa16..767a200384 100644
--- a/src/lib/dhcp/libdhcp++.h
+++ b/src/lib/dhcp/libdhcp++.h
@@ -19,6 +19,7 @@
 #include <dhcp/option_space_container.h>
 #include <dhcp/pkt6.h>
 #include <util/buffer.h>
+#include <util/staged_value.h>
 
 #include <iostream>
 #include <string>
@@ -302,6 +303,12 @@ public:
     /// @brief Removes runtime option definitions.
     static void clearRuntimeOptionDefs();
 
+    /// @brief Reverts uncommited changes to runtime option definitions.
+    static void revertRuntimeOptionDefs();
+
+    /// @brief Commits runtime option definitions.
+    static void commitRuntimeOptionDefs();
+
 private:
 
     /// Initialize standard DHCPv4 option definitions.
@@ -349,7 +356,7 @@ private:
     static VendorOptionDefContainers vendor6_defs_;
 
     /// Container for additional option defnitions created in runtime.
-    static OptionDefSpaceContainer runtime_option_defs_;
+    static util::StagedValue<OptionDefSpaceContainer> runtime_option_defs_;
 };
 
 }
diff --git a/src/lib/util/Makefile.am b/src/lib/util/Makefile.am
index 6806ca2ffa..b6ba4a095a 100644
--- a/src/lib/util/Makefile.am
+++ b/src/lib/util/Makefile.am
@@ -21,6 +21,7 @@ libkea_util_la_SOURCES += pointer_util.h
 libkea_util_la_SOURCES += process_spawn.h process_spawn.cc
 libkea_util_la_SOURCES += range_utilities.h
 libkea_util_la_SOURCES += signal_set.cc signal_set.h
+libkea_util_la_SOURCES += staged_value.h
 libkea_util_la_SOURCES += stopwatch.cc stopwatch.h
 libkea_util_la_SOURCES += stopwatch_impl.cc stopwatch_impl.h
 libkea_util_la_SOURCES += versioned_csv_file.h versioned_csv_file.cc
diff --git a/src/lib/util/staged_value.h b/src/lib/util/staged_value.h
new file mode 100644
index 0000000000..bb41399af5
--- /dev/null
+++ b/src/lib/util/staged_value.h
@@ -0,0 +1,126 @@
+// Copyright (C) 2015 Internet Systems Consortium, Inc. ("ISC")
+//
+// Permission to use, copy, modify, and/or distribute this software for any
+// purpose with or without fee is hereby granted, provided that the above
+// copyright notice and this permission notice appear in all copies.
+//
+// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH
+// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY
+// AND FITNESS.  IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT,
+// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM
+// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE
+// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
+// PERFORMANCE OF THIS SOFTWARE.
+
+#ifndef STAGED_VALUE_H
+#define STAGED_VALUE_H
+
+#include <boost/noncopyable.hpp>
+#include <boost/shared_ptr.hpp>
+
+namespace isc {
+namespace util {
+
+/// @brief This class implements set/commit mechanism for a single object.
+///
+/// In some cases it is desired to set value for an object while keeping
+/// ability to revert to an original value under certain conditions.
+/// This is often desired for objects holding some part of application's
+/// configuration. Configuration is usually a multi-step process and
+/// may fail on almost any stage. If this happens, the last good
+/// configuration should be used. This implies that some of the state
+/// of some of the objects needs to be reverted.
+///
+/// This class implements a mechanism for setting and committing a value.
+/// Until the new value has been committed it is possible to revert to
+/// an original value.
+///
+/// @tparam ValueType Type of the value represented by this class.
+template<typename ValueType>
+class StagedValue : public boost::noncopyable {
+public:
+
+    /// @brief Constructor.
+    ///
+    /// Initializes the default value.
+    StagedValue()
+        : staging_(new ValueType()), current_(new ValueType()),
+          modified_(false) {
+    }
+
+    /// @brief Retrieves current value.
+    ///
+    /// If the value hasn't been modified since last commit, reset or
+    /// revert operation, a committed value is returned. If the value
+    /// has been modified, the modified value is returned.
+    const ValueType& getValue() const {
+        return (modified_ ? *staging_ : *current_);
+    }
+
+    /// @brief Sets new value.
+    ///
+    /// @param new_value New value to be assigned.
+    void setValue(const ValueType& new_value) {
+        *staging_ = new_value;
+        modified_ = true;
+    }
+
+    /// @brief Commits a value.
+    void commit() {
+        // Only apply changes if any modifications made.
+        if (modified_) {
+            current_ = staging_;
+        }
+        revert();
+    }
+
+    /// @brief Resets value to defaults.
+    void reset() {
+        revert();
+        current_.reset(new ValueType());
+    }
+
+    /// @brief Reverts any modifications since last commit.
+    void revert() {
+        staging_.reset(new ValueType());
+        modified_ = false;
+    }
+
+    /// @brief Assignment operator.
+    ///
+    /// @param value New value to be assigned.
+    /// @return Reference to this.
+    StagedValue& operator=(const ValueType& value) {
+        setValue(value);
+        return (*this);
+    }
+
+    /// @brief Conversion operator to value type.
+    ///
+    /// @return Reference to value represented by this object.
+    operator const ValueType&() const {
+        return (getValue());
+    }
+
+private:
+
+    /// @brief Pointer to staging value.
+    ///
+    /// This value holds any modifications made.
+    boost::shared_ptr<ValueType> staging_;
+
+    /// @brief Pointer to committed value.
+    ///
+    /// This value holds last committed changes.
+    boost::shared_ptr<ValueType> current_;
+
+    /// @brief Boolean flag which indicates if any modifications have been
+    /// applied since last commit.
+    bool modified_;
+
+};
+
+} // namespace isc::util
+} // namespace isc
+
+#endif // STAGED_VALUE_H
diff --git a/src/lib/util/tests/Makefile.am b/src/lib/util/tests/Makefile.am
index 00122b62f3..96cd389fd6 100644
--- a/src/lib/util/tests/Makefile.am
+++ b/src/lib/util/tests/Makefile.am
@@ -45,6 +45,7 @@ run_unittests_SOURCES += process_spawn_unittest.cc
 run_unittests_SOURCES += qid_gen_unittest.cc
 run_unittests_SOURCES += random_number_generator_unittest.cc
 run_unittests_SOURCES += socketsession_unittest.cc
+run_unittests_SOURCES += staged_value_unittest.cc
 run_unittests_SOURCES += strutil_unittest.cc
 run_unittests_SOURCES += time_utilities_unittest.cc
 run_unittests_SOURCES += range_utilities_unittest.cc
diff --git a/src/lib/util/tests/staged_value_unittest.cc b/src/lib/util/tests/staged_value_unittest.cc
new file mode 100644
index 0000000000..a108d5def9
--- /dev/null
+++ b/src/lib/util/tests/staged_value_unittest.cc
@@ -0,0 +1,114 @@
+// Copyright (C) 2015 Internet Systems Consortium, Inc. ("ISC")
+//
+// Permission to use, copy, modify, and/or distribute this software for any
+// purpose with or without fee is hereby granted, provided that the above
+// copyright notice and this permission notice appear in all copies.
+//
+// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH
+// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY
+// AND FITNESS.  IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT,
+// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM
+// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE
+// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
+// PERFORMANCE OF THIS SOFTWARE.
+
+#include <config.h>
+#include <util/staged_value.h>
+#include <boost/shared_ptr.hpp>
+#include <gtest/gtest.h>
+
+namespace {
+
+using namespace isc::util;
+
+// This test verifies that the value can be assigned and committed.
+TEST(StagedValueTest, assignAndCommit) {
+    // Initally the value should be set to a default
+    StagedValue<int> value;
+    ASSERT_EQ(0, value.getValue());
+
+    // Set the new value without committing it and make sure it
+    // can be retrieved.
+    value.setValue(4);
+    ASSERT_EQ(4, value.getValue());
+
+    // Commit the value and make sure it still can be retrieved.
+    value.commit();
+    ASSERT_EQ(4, value.getValue());
+
+    // Set new value and retrieve it.
+    value.setValue(10);
+    ASSERT_EQ(10, value.getValue());
+
+    // Do it again and commit it.
+    value.setValue(20);
+    ASSERT_EQ(20, value.getValue());
+    value.commit();
+    EXPECT_EQ(20, value.getValue());
+}
+
+// This test verifies that the value can be reverted if it hasn't been
+// committed.
+TEST(StagedValueTest, revert) {
+    // Set the value and commit.
+    StagedValue<int> value;
+    value.setValue(123);
+    value.commit();
+
+    // Set new value and do not commit.
+    value.setValue(500);
+    // The new value should be the one returned.
+    ASSERT_EQ(500, value.getValue());
+    // But, reverting gets us back to original value.
+    value.revert();
+    EXPECT_EQ(123, value.getValue());
+    // Reverting again doesn't have any effect.
+    value.revert();
+    EXPECT_EQ(123, value.getValue());
+}
+
+// This test verifies that the value can be restored to an original one.
+TEST(StagedValueTest, reset) {
+    // Set the new value and commit.
+    StagedValue<int> value;
+    value.setValue(123);
+    value.commit();
+
+    // Override the value but do not commit.
+    value.setValue(500);
+
+    // Resetting should take us back to default value.
+    value.reset();
+    EXPECT_EQ(0, value.getValue());
+    value.revert();
+    EXPECT_EQ(0, value.getValue());
+}
+
+// This test verifies that second commit doesn't modify a value.
+TEST(StagedValueTest, commit) {
+    // Set the value and commit.
+    StagedValue<int> value;
+    value.setValue(123);
+    value.commit();
+
+    // Second commit should have no effect.
+    value.commit();
+    EXPECT_EQ(123, value.getValue());
+}
+
+// This test checks that type conversion operator works correctly.
+TEST(StagedValueTest, conversionOperator) {
+    StagedValue<int> value;
+    value.setValue(244);
+    EXPECT_EQ(244, value);
+}
+
+// This test checks that the assignment operator works correctly.
+TEST(StagedValueTest, assignmentOperator) {
+    StagedValue<int> value;
+    value = 111;
+    EXPECT_EQ(111, value);
+}
+
+
+} // end of anonymous namespace