[3588] Addressed review comments.

diff --git a/src/lib/dhcp/option_definition.cc b/src/lib/dhcp/option_definition.cc
index ba9644acfaade600a9172ffb96ce7e1a360a2e7f..214d01fff650a8519aca7904a270bfe54312cfe2 100644 (file)
--- a/src/lib/dhcp/option_definition.cc
+++ b/src/lib/dhcp/option_definition.cc
@@ -92,15 +92,6 @@ OptionDefinition::OptionDefinition(const std::string& name,
       encapsulated_space_(encapsulated_space) {
 }
 
-OptionDefinition::OptionDefinition(const OptionDefinition& def)
-    : name_(def.getName()),
-      code_(def.getCode()),
-      type_(def.getType()),
-      array_type_(def.getArrayType()),
-      encapsulated_space_(def.getEncapsulatedSpace()),
-      record_fields_(def.getRecordFields()) {
-}
-
 bool
 OptionDefinition::equals(const OptionDefinition& other) const {
     return (name_ == other.name_ &&

diff --git a/src/lib/dhcp/option_definition.h b/src/lib/dhcp/option_definition.h
index a8d2f768bcec2f142857770e4499eb8da7414b63..1e012e74d1bdb965c709460e3b891295d7702aaf 100644 (file)
--- a/src/lib/dhcp/option_definition.h
+++ b/src/lib/dhcp/option_definition.h
@@ -183,10 +183,25 @@ public:
                               const std::string& type,
                               const char* encapsulated_space);
 
-    /// @brief Copy constructor.
+    /// @brief Constructor.
+    ///
+    /// This constructor sets the name of the option space that is
+    /// encapsulated by this option. The encapsulated option space
+    /// identifies sub-options that are carried within this option.
+    /// This constructor does not allow to set array indicator
+    /// because options comprising an array of data fields must
+    /// not be used with sub-options.
     ///
-    /// @param def Option definition to be used to create a new instance.
-    explicit OptionDefinition(const OptionDefinition& def);
+    /// @param name option name.
+    /// @param code option code.
+    /// @param type option data type.
+    /// @param encapsulated_space name of the option space being
+    /// encapsulated by this option.
+    explicit OptionDefinition(const std::string& name,
+                              const uint16_t code,
+                              const OptionDataType type,
+                              const char* encapsulated_space);
+
 
     /// @name Comparison functions and operators.
     ///
@@ -217,26 +232,6 @@ public:
     }
     //@}
 
-    /// @brief Constructor.
-    ///
-    /// This constructor sets the name of the option space that is
-    /// encapsulated by this option. The encapsulated option space
-    /// identifies sub-options that are carried within this option.
-    /// This constructor does not allow to set array indicator
-    /// because options comprising an array of data fields must
-    /// not be used with sub-options.
-    ///
-    /// @param name option name.
-    /// @param code option code.
-    /// @param type option data type.
-    /// @param encapsulated_space name of the option space being
-    /// encapsulated by this option.
-    explicit OptionDefinition(const std::string& name,
-                              const uint16_t code,
-                              const OptionDataType type,
-                              const char* encapsulated_space);
-
-
     /// @brief Adds data field to the record.
     ///
     /// @param data_type_name name of the data type for the field.

diff --git a/src/lib/dhcp/tests/option_definition_unittest.cc b/src/lib/dhcp/tests/option_definition_unittest.cc
index 6c7a418e1b7e46ed1b48386c34251883de8495c6..7608a6bb294d4851856aa75b86dc4585b42a48e6 100644 (file)
--- a/src/lib/dhcp/tests/option_definition_unittest.cc
+++ b/src/lib/dhcp/tests/option_definition_unittest.cc
@@ -127,11 +127,22 @@ TEST_F(OptionDefinitionTest, copyConstructor) {
     EXPECT_EQ(27, opt_def_copy.getCode());
     EXPECT_TRUE(opt_def_copy.getArrayType());
     EXPECT_TRUE(opt_def_copy.getEncapsulatedSpace().empty());
+    ASSERT_EQ(OPT_RECORD_TYPE, opt_def_copy.getType());
     const OptionDefinition::RecordFieldsCollection fields =
         opt_def_copy.getRecordFields();
     ASSERT_EQ(2, fields.size());
     EXPECT_EQ(OPT_UINT16_TYPE, fields[0]);
     EXPECT_EQ(OPT_STRING_TYPE, fields[1]);
+
+    // Let's make another test to check if encapsulated option space is
+    // copied properly.
+    OptionDefinition opt_def2("option-bar", 30, "uint32", "isc");
+    OptionDefinition opt_def_copy2(opt_def2);
+    EXPECT_EQ("option-bar", opt_def_copy2.getName());
+    EXPECT_EQ(30, opt_def_copy2.getCode());
+    EXPECT_FALSE(opt_def_copy2.getArrayType());
+    EXPECT_EQ(OPT_UINT32_TYPE, opt_def_copy2.getType());
+    EXPECT_EQ("isc", opt_def_copy2.getEncapsulatedSpace());
 }
 
 // This test checks that two option definitions may be compared fot equality.
@@ -143,6 +154,8 @@ TEST_F(OptionDefinitionTest, equality) {
                  != OptionDefinition("option-foo", 5, "uint16", false));
 
     // Differ by name.
+    EXPECT_FALSE(OptionDefinition("option-foo", 5, "uint16", false)
+                 == OptionDefinition("option-foobar", 5, "uint16", false));
     EXPECT_FALSE(OptionDefinition("option-bar", 5, "uint16", false)
                 == OptionDefinition("option-foo", 5, "uint16", false));
     EXPECT_TRUE(OptionDefinition("option-bar", 5, "uint16", false)

diff --git a/src/lib/dhcpsrv/cfg_option_def.cc b/src/lib/dhcpsrv/cfg_option_def.cc
index d51d7403cf1669229cf23efbf501703f418ef3fd..00e73ff11d30ee105a881e1c45eb8df195d45bbb 100644 (file)
--- a/src/lib/dhcpsrv/cfg_option_def.cc
+++ b/src/lib/dhcpsrv/cfg_option_def.cc
@@ -21,9 +21,9 @@ namespace isc {
 namespace dhcp {
 
 void
-CfgOptionDef::copy(CfgOptionDef& new_config) const {
-    CfgOptionDef cfg;
-    std::list<std::string> names = option_definitions_.getOptionSpaceNames();
+CfgOptionDef::copyTo(CfgOptionDef& new_config) const {
+    const std::list<std::string>& names =
+        option_definitions_.getOptionSpaceNames();
     for (std::list<std::string>::const_iterator name = names.begin();
          name != names.end(); ++name) {
         OptionDefContainerPtr defs = getAll(*name);
@@ -31,18 +31,17 @@ CfgOptionDef::copy(CfgOptionDef& new_config) const {
              def != defs->end(); ++def) {
             OptionDefinitionPtr new_def =
                 OptionDefinitionPtr(new OptionDefinition(**def));
-            cfg.add(new_def, *name);
+            new_config.add(new_def, *name);
         }
     }
-    new_config = cfg;
 }
 
 bool
 CfgOptionDef::equals(const CfgOptionDef& other) const {
     // Get our option space names.
-    std::list<std::string> names = option_definitions_.getOptionSpaceNames();
+    const std::list<std::string>& names = option_definitions_.getOptionSpaceNames();
     // Get option space names held by the other object.
-    std::list<std::string>
+    const std::list<std::string>&
         other_names = other.option_definitions_.getOptionSpaceNames();
     // Compareg that sizes are the same. If they hold different number of
     // option space names the objects are not equal.
@@ -86,11 +85,13 @@ CfgOptionDef::add(const OptionDefinitionPtr& def,
 
     // Option definition being added must be a valid pointer.
     } else if (!def) {
-        isc_throw(MalformedOptionDefinition, "option definition must not be NULL");
+        isc_throw(MalformedOptionDefinition,
+                  "option definition must not be NULL");
     // Must not duplicate an option definition.
     } else if (get(option_space, def->getCode())) {
         isc_throw(DuplicateOptionDefinition, "option definition with code '"
-                  << def->getCode() << "' already exists");
+                  << def->getCode() << "' already exists in option"
+                  " space '" << option_space << "'");
 
     // Must not override standard option definition.
     } else if (((option_space == DHCP4_OPTION_SPACE) &&

diff --git a/src/lib/dhcpsrv/cfg_option_def.h b/src/lib/dhcpsrv/cfg_option_def.h
index 0035ed1f00b5c4f720e0af30ac2608da6fceed70..fadc8b6d4434025196b758319f5adaa808501670 100644 (file)
--- a/src/lib/dhcpsrv/cfg_option_def.h
+++ b/src/lib/dhcpsrv/cfg_option_def.h
@@ -43,9 +43,14 @@ public:
     /// to an object passed as parameter. There are no shared objects or
     /// pointers between the original object and a copy.
     ///
+    /// @warning This function doesn't perform a cleanup of the @c new_config
+    /// before copying the new configuration into it. It is caller's
+    /// responsibility to make sure that this object doesn't contain any
+    /// garbage data.
+    ///
     /// @param [out] new_config An object to which the configuration will be
     /// copied.
-    void copy(CfgOptionDef& new_config) const;
+    void copyTo(CfgOptionDef& new_config) const;
 
     /// @name Methods and operators used for comparing objects.
     ///

diff --git a/src/lib/dhcpsrv/srv_config.cc b/src/lib/dhcpsrv/srv_config.cc
index 210c40a1cf9a46cdbb1ac10a8158fead4e474db4..d7b4cc3ba2d80cccc2f1c37637c974673c351cb4 100644 (file)
--- a/src/lib/dhcpsrv/srv_config.cc
+++ b/src/lib/dhcpsrv/srv_config.cc
@@ -89,7 +89,7 @@ SrvConfig::copy(SrvConfig& new_config) const {
     // Replace interface configuration.
     new_config.setCfgIface(cfg_iface_);
     // Replace option definitions.
-    cfg_option_def_->copy(*new_config.cfg_option_def_);
+    cfg_option_def_->copyTo(*new_config.cfg_option_def_);
 }
 
 void