From: Marcin Siodelski Date: Tue, 30 Sep 2014 16:55:27 +0000 (+0200) Subject: [3588] Addressed review comments. X-Git-Tag: rt3470_base^2 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=2f666cd1e984ba271e3821b47869ddbecc513e7c;p=thirdparty%2Fkea.git [3588] Addressed review comments. --- diff --git a/src/lib/dhcp/option_definition.cc b/src/lib/dhcp/option_definition.cc index ba9644acfa..214d01fff6 100644 --- 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 a8d2f768bc..1e012e74d1 100644 --- 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 6c7a418e1b..7608a6bb29 100644 --- 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 d51d7403cf..00e73ff11d 100644 --- 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 names = option_definitions_.getOptionSpaceNames(); +CfgOptionDef::copyTo(CfgOptionDef& new_config) const { + const std::list& names = + option_definitions_.getOptionSpaceNames(); for (std::list::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 names = option_definitions_.getOptionSpaceNames(); + const std::list& names = option_definitions_.getOptionSpaceNames(); // Get option space names held by the other object. - std::list + const std::list& 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 0035ed1f00..fadc8b6d44 100644 --- 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 210c40a1cf..d7b4cc3ba2 100644 --- 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