Commit 2f666cd1 authored by Marcin Siodelski's avatar Marcin Siodelski
Browse files

[3588] Addressed review comments.

parent f6ea7fac
...@@ -92,15 +92,6 @@ OptionDefinition::OptionDefinition(const std::string& name, ...@@ -92,15 +92,6 @@ OptionDefinition::OptionDefinition(const std::string& name,
encapsulated_space_(encapsulated_space) { 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 bool
OptionDefinition::equals(const OptionDefinition& other) const { OptionDefinition::equals(const OptionDefinition& other) const {
return (name_ == other.name_ && return (name_ == other.name_ &&
......
...@@ -183,10 +183,25 @@ public: ...@@ -183,10 +183,25 @@ public:
const std::string& type, const std::string& type,
const char* encapsulated_space); 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. /// @param name option name.
explicit OptionDefinition(const OptionDefinition& def); /// @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. /// @name Comparison functions and operators.
/// ///
...@@ -217,26 +232,6 @@ public: ...@@ -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. /// @brief Adds data field to the record.
/// ///
/// @param data_type_name name of the data type for the field. /// @param data_type_name name of the data type for the field.
......
...@@ -127,11 +127,22 @@ TEST_F(OptionDefinitionTest, copyConstructor) { ...@@ -127,11 +127,22 @@ TEST_F(OptionDefinitionTest, copyConstructor) {
EXPECT_EQ(27, opt_def_copy.getCode()); EXPECT_EQ(27, opt_def_copy.getCode());
EXPECT_TRUE(opt_def_copy.getArrayType()); EXPECT_TRUE(opt_def_copy.getArrayType());
EXPECT_TRUE(opt_def_copy.getEncapsulatedSpace().empty()); EXPECT_TRUE(opt_def_copy.getEncapsulatedSpace().empty());
ASSERT_EQ(OPT_RECORD_TYPE, opt_def_copy.getType());
const OptionDefinition::RecordFieldsCollection fields = const OptionDefinition::RecordFieldsCollection fields =
opt_def_copy.getRecordFields(); opt_def_copy.getRecordFields();
ASSERT_EQ(2, fields.size()); ASSERT_EQ(2, fields.size());
EXPECT_EQ(OPT_UINT16_TYPE, fields[0]); EXPECT_EQ(OPT_UINT16_TYPE, fields[0]);
EXPECT_EQ(OPT_STRING_TYPE, fields[1]); 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. // This test checks that two option definitions may be compared fot equality.
...@@ -143,6 +154,8 @@ TEST_F(OptionDefinitionTest, equality) { ...@@ -143,6 +154,8 @@ TEST_F(OptionDefinitionTest, equality) {
!= OptionDefinition("option-foo", 5, "uint16", false)); != OptionDefinition("option-foo", 5, "uint16", false));
// Differ by name. // 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) EXPECT_FALSE(OptionDefinition("option-bar", 5, "uint16", false)
== OptionDefinition("option-foo", 5, "uint16", false)); == OptionDefinition("option-foo", 5, "uint16", false));
EXPECT_TRUE(OptionDefinition("option-bar", 5, "uint16", false) EXPECT_TRUE(OptionDefinition("option-bar", 5, "uint16", false)
......
...@@ -21,9 +21,9 @@ namespace isc { ...@@ -21,9 +21,9 @@ namespace isc {
namespace dhcp { namespace dhcp {
void void
CfgOptionDef::copy(CfgOptionDef& new_config) const { CfgOptionDef::copyTo(CfgOptionDef& new_config) const {
CfgOptionDef cfg; const std::list<std::string>& names =
std::list<std::string> names = option_definitions_.getOptionSpaceNames(); option_definitions_.getOptionSpaceNames();
for (std::list<std::string>::const_iterator name = names.begin(); for (std::list<std::string>::const_iterator name = names.begin();
name != names.end(); ++name) { name != names.end(); ++name) {
OptionDefContainerPtr defs = getAll(*name); OptionDefContainerPtr defs = getAll(*name);
...@@ -31,18 +31,17 @@ CfgOptionDef::copy(CfgOptionDef& new_config) const { ...@@ -31,18 +31,17 @@ CfgOptionDef::copy(CfgOptionDef& new_config) const {
def != defs->end(); ++def) { def != defs->end(); ++def) {
OptionDefinitionPtr new_def = OptionDefinitionPtr new_def =
OptionDefinitionPtr(new OptionDefinition(**def)); OptionDefinitionPtr(new OptionDefinition(**def));
cfg.add(new_def, *name); new_config.add(new_def, *name);
} }
} }
new_config = cfg;
} }
bool bool
CfgOptionDef::equals(const CfgOptionDef& other) const { CfgOptionDef::equals(const CfgOptionDef& other) const {
// Get our option space names. // 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. // Get option space names held by the other object.
std::list<std::string> const std::list<std::string>&
other_names = other.option_definitions_.getOptionSpaceNames(); other_names = other.option_definitions_.getOptionSpaceNames();
// Compareg that sizes are the same. If they hold different number of // Compareg that sizes are the same. If they hold different number of
// option space names the objects are not equal. // option space names the objects are not equal.
...@@ -86,11 +85,13 @@ CfgOptionDef::add(const OptionDefinitionPtr& def, ...@@ -86,11 +85,13 @@ CfgOptionDef::add(const OptionDefinitionPtr& def,
// Option definition being added must be a valid pointer. // Option definition being added must be a valid pointer.
} else if (!def) { } 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. // Must not duplicate an option definition.
} else if (get(option_space, def->getCode())) { } else if (get(option_space, def->getCode())) {
isc_throw(DuplicateOptionDefinition, "option definition with code '" 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. // Must not override standard option definition.
} else if (((option_space == DHCP4_OPTION_SPACE) && } else if (((option_space == DHCP4_OPTION_SPACE) &&
......
...@@ -43,9 +43,14 @@ public: ...@@ -43,9 +43,14 @@ public:
/// to an object passed as parameter. There are no shared objects or /// to an object passed as parameter. There are no shared objects or
/// pointers between the original object and a copy. /// 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 /// @param [out] new_config An object to which the configuration will be
/// copied. /// copied.
void copy(CfgOptionDef& new_config) const; void copyTo(CfgOptionDef& new_config) const;
/// @name Methods and operators used for comparing objects. /// @name Methods and operators used for comparing objects.
/// ///
......
...@@ -89,7 +89,7 @@ SrvConfig::copy(SrvConfig& new_config) const { ...@@ -89,7 +89,7 @@ SrvConfig::copy(SrvConfig& new_config) const {
// Replace interface configuration. // Replace interface configuration.
new_config.setCfgIface(cfg_iface_); new_config.setCfgIface(cfg_iface_);
// Replace option definitions. // Replace option definitions.
cfg_option_def_->copy(*new_config.cfg_option_def_); cfg_option_def_->copyTo(*new_config.cfg_option_def_);
} }
void void
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment