From: Thomas Markwalder Date: Fri, 8 Mar 2019 21:39:19 +0000 (-0500) Subject: [#401,!24] Addressed most review comments X-Git-Tag: Kea-1.6.0-beta~362^2~8 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=65f8e39cd91dda54dbabef857b06f66f43e28c70;p=thirdparty%2Fkea.git [#401,!24] Addressed most review comments This commit addresse the more trivial review comments, it does not include option creation for networks,subnets, or pools. Will rebase and address remaining comments. --- diff --git a/src/lib/dhcpsrv/cfg_option.cc b/src/lib/dhcpsrv/cfg_option.cc index dd28e37c77..feb385f35f 100644 --- a/src/lib/dhcpsrv/cfg_option.cc +++ b/src/lib/dhcpsrv/cfg_option.cc @@ -86,7 +86,7 @@ CfgOption::getVendorIdsSpaceNames() const { void CfgOption::merge(CfgOptionDefPtr cfg_def, CfgOption& other) { // First we merge our options into other. - // This adds my opitions that are not + // This adds my options that are not // in other, to other (i.e we skip over // duplicates). mergeTo(other); @@ -148,6 +148,9 @@ CfgOption::createDescriptorOption(CfgOptionDefPtr cfg_def, const std::string& sp try { + std::cout << "def:" << def->getName() << ", code:" << def->getCode() + << ", type: " << def->getType() << std::endl; + // Definition found. Let's replace the generic option in // the descriptor with one created based on definition's factory. if (formatted_value.empty()) { diff --git a/src/lib/dhcpsrv/srv_config.cc b/src/lib/dhcpsrv/srv_config.cc index c8c2024e5b..cda2708d84 100644 --- a/src/lib/dhcpsrv/srv_config.cc +++ b/src/lib/dhcpsrv/srv_config.cc @@ -195,7 +195,7 @@ SrvConfig::merge4(SrvConfig& other) { // Merge subnets. cfg_subnets4_->merge(getCfgSharedNetworks4(), *(other.getCfgSubnets4())); - // @todo merge other parts of the configuration here. + /// @todo merge other parts of the configuration here. } void diff --git a/src/lib/dhcpsrv/tests/cfg_option_unittest.cc b/src/lib/dhcpsrv/tests/cfg_option_unittest.cc index a9bc396fe3..0127894dfa 100644 --- a/src/lib/dhcpsrv/tests/cfg_option_unittest.cc +++ b/src/lib/dhcpsrv/tests/cfg_option_unittest.cc @@ -8,7 +8,10 @@ #include #include #include +#include #include +#include +#include #include #include #include @@ -327,12 +330,12 @@ TEST_F(CfgOptionTest, copy) { EXPECT_EQ(10, container->size()); } -// This test verifies option definitions from one configuration -// can be used to update definitions in another configuration. -// In other words, definitions from an "other" configuration +// This test verifies that DHCP options from one configuration +// can be used to update options in another configuration. +// In other words, options from an "other" configuration // can be merged into an existing configuration, with any // duplicates in other overwriting those in the existing -// configuration. +// configuration. TEST_F(CfgOptionTest, validMerge) { CfgOption this_cfg; CfgOption other_cfg; @@ -358,7 +361,7 @@ TEST_F(CfgOptionTest, validMerge) { ASSERT_NO_THROW(this_cfg.add(option, false, "fluff")); // Create our other config that will be merged from. - // Add Option 1 to "isc", this should "overwrite" the original. + // Add Option 1 to "isc", this should "overwrite" the original. option.reset(new Option(Option::V4, 1, OptionBuffer(1, 0x10))); ASSERT_NO_THROW(other_cfg.add(option, false, "isc")); @@ -366,20 +369,14 @@ TEST_F(CfgOptionTest, validMerge) { option.reset(new Option(Option::V4, 2, OptionBuffer(1, 0x20))); ASSERT_NO_THROW(other_cfg.add(option, false, "isc")); - // Add option 4 to "isc". + // Add option 4 to "isc". option.reset(new Option(Option::V4, 4, OptionBuffer(1, 0x40))); ASSERT_NO_THROW(other_cfg.add(option, false, "isc")); // Merge source configuration to the destination configuration. The options // in the destination should be preserved. The options from the source // configuration should be added. - try { - this_cfg.merge(defs, other_cfg); - } catch(const std::exception& ex) { - GTEST_FAIL () << "Unexpected exception:" << ex.what(); - } - -// ASSERT_NO_THROW(this_cfg.merge(defs, other_cfg)); + ASSERT_NO_THROW(this_cfg.merge(defs, other_cfg)); // isc:1 should come from "other" config. OptionDescriptor desc = this_cfg.get("isc", 1); @@ -412,7 +409,10 @@ TEST_F(CfgOptionTest, validMerge) { EXPECT_EQ(0x4, desc.option_->getData()[0]); } -TEST_F(CfgOptionTest, invalidMerge) { +// This test verifies that attempting to merge options +// which are by incompatible with "known" option definitions +// are detected. +TEST_F(CfgOptionTest, mergeInvalid) { CfgOption this_cfg; CfgOption other_cfg; @@ -430,7 +430,7 @@ TEST_F(CfgOptionTest, invalidMerge) { ASSERT_NO_THROW(other_cfg.add(desc, "isc")); // When we attempt to merge, it should fail, recognizing that - // option two has no definition. + // option 2, which has a formatted value, has no definition. try { this_cfg.merge(defs, other_cfg); GTEST_FAIL() << "merge should have thrown"; @@ -443,10 +443,10 @@ TEST_F(CfgOptionTest, invalidMerge) { } // Now let's add an option definition that will force data truncated - // error for option one. + // error for option 1. defs->add((OptionDefinitionPtr(new OptionDefinition("one", 1, "uint16"))), "isc"); - // When we attempt to merge, it should fail because option one's data + // When we attempt to merge, it should fail because option 1's data // is not valid per its definition. try { this_cfg.merge(defs, other_cfg); @@ -461,6 +461,70 @@ TEST_F(CfgOptionTest, invalidMerge) { } } +// This test verifies the all of the valid option cases +// in createDescriptorOption(): +// 1. standard option +// 2. vendor option +// 3. user-defined, unformatted option +// 4. user-defined, formatted option +// 5. undefined, unformatted option +TEST_F(CfgOptionTest, createDescriptorOptionValid) { + // First we'll create our "known" user definitions + CfgOptionDefPtr defs(new CfgOptionDef()); + defs->add((OptionDefinitionPtr(new OptionDefinition("one", 1, "uint8"))), "isc"); + defs->add((OptionDefinitionPtr(new OptionDefinition("two", 2, "uint8", true))), "isc"); + + // We'll try a standard option first. + std::string space = "dhcp4"; + std::string value("example.org"); + OptionPtr option(new Option(Option::V4, DHO_HOST_NAME)); + option->setData(value.begin(), value.end()); + OptionDescriptorPtr desc(new OptionDescriptor(option, false)); + + ASSERT_NO_THROW(CfgOption::createDescriptorOption(defs, space, *desc)); + OptionStringPtr opstr = boost::dynamic_pointer_cast(desc->option_); + ASSERT_TRUE(opstr); + EXPECT_EQ("type=012, len=011: \"example.org\" (string)", opstr->toText()); + + // Next we'll try a vendor option with a formatted value + space = "vendor-4491"; + value = "192.0.2.1, 192.0.2.2"; + option.reset(new Option(Option::V4, 2)); + desc.reset(new OptionDescriptor(option, false, value)); + + ASSERT_NO_THROW(CfgOption::createDescriptorOption(defs, space, *desc)); + std::cout << "option:" << desc->option_->toText() << std::endl; + Option4AddrLstPtr opaddrs = boost::dynamic_pointer_cast(desc->option_); + ASSERT_TRUE(opaddrs); + EXPECT_EQ("type=002, len=008: 192.0.2.1 192.0.2.2", opaddrs->toText()); + + // Now, a user defined uint8 option + space = "isc"; + option.reset(new Option(Option::V4, 1, OptionBuffer(1, 0x77))); + desc.reset(new OptionDescriptor(option, false)); + + ASSERT_NO_THROW(CfgOption::createDescriptorOption(defs, space, *desc)); + OptionUint8Ptr opint = boost::dynamic_pointer_cast(desc->option_); + ASSERT_TRUE(opint); + EXPECT_EQ("type=001, len=001: 119 (uint8)", opint->toText()); + + // Now, a user defined array of strings + option.reset(new Option(Option::V4, 2)); + desc.reset(new OptionDescriptor(option, false, "1,2,3")); + + ASSERT_NO_THROW(CfgOption::createDescriptorOption(defs, space, *desc)); + OptionUint8ArrayPtr oparray = boost::dynamic_pointer_cast(desc->option_); + ASSERT_TRUE(oparray); + EXPECT_EQ("type=002, len=003: 1(uint8) 2(uint8) 3(uint8)", oparray->toText()); + + // Finally, a generic, undefined option + option.reset(new Option(Option::V4, 2, OptionBuffer(1, 0x77))); + desc.reset(new OptionDescriptor(option, false)); + + ASSERT_NO_THROW(CfgOption::createDescriptorOption(defs, space, *desc)); + EXPECT_EQ("type=002, len=001: 119(uint8)", desc->option_->toText()); +} + // This test verifies that encapsulated options are added as sub-options // to the top level options on request. TEST_F(CfgOptionTest, encapsulate) {