From: Marcin Siodelski Date: Wed, 19 Dec 2012 10:28:21 +0000 (+0100) Subject: [2544] Changes as a result of a code review. X-Git-Tag: bind10-1.0.0-beta-release~2^2~3 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=fdce9eb73133bb52609ed6ad51fdb77b28447eb3;p=thirdparty%2Fkea.git [2544] Changes as a result of a code review. --- diff --git a/src/bin/dhcp4/config_parser.cc b/src/bin/dhcp4/config_parser.cc index ca0e639ccc..76181c213c 100644 --- a/src/bin/dhcp4/config_parser.cc +++ b/src/bin/dhcp4/config_parser.cc @@ -466,12 +466,12 @@ private: /// /// This parser parses configuration entries that specify value of /// a single option. These entries include option name, option code -/// and data carried by the option. If parsing is successful than an +/// and data carried by the option. If parsing is successful then an /// instance of an option is created and added to the storage provided /// by the calling class. /// /// @todo This class parses and validates the option name. However it is -/// not used anywhere util support for option spaces is implemented +/// not used anywhere until support for option spaces is implemented /// (see tickets #2319, #2314). When option spaces are implemented /// there will be a way to reference the particular option using /// its type (code) or option name. @@ -827,26 +827,21 @@ public: // a setStorage and build methods are invoked. // Try uint32 type parser. - if (buildParser(parser, uint32_values_, - param.second)) { - // Storage set, build invoked on the parser, proceed with - // next configuration element. - continue; - } - // Try string type parser. - if (buildParser(parser, string_values_, - param.second)) { - continue; - } - // Try pools parser. - if (buildParser(parser, pools_, - param.second)) { - continue; - } - // Try option data parser. - if (buildParser(parser, options_, - param.second)) { - continue; + if (!buildParser(parser, uint32_values_, + param.second) && + // Try string type parser. + !buildParser(parser, string_values_, + param.second) && + // Try pool parser. + !buildParser(parser, pools_, + param.second) && + // Try option data parser. + !buildParser(parser, options_, + param.second)) { + // Appropriate parsers are created in the createSubnet6ConfigParser + // and they should be limited to those that we check here for. Thus, + // if we fail to find a matching parser here it is a programming error. + isc_throw(Dhcp4ConfigError, "failed to find suitable parser"); } } // Ok, we now have subnet parsed diff --git a/src/bin/dhcp4/dhcp4_messages.mes b/src/bin/dhcp4/dhcp4_messages.mes index 3e46a73e33..f258430486 100644 --- a/src/bin/dhcp4/dhcp4_messages.mes +++ b/src/bin/dhcp4/dhcp4_messages.mes @@ -45,9 +45,9 @@ This is an informational message reporting that the configuration has been extended to include the specified IPv4 subnet. % DHCP4_CONFIG_OPTION_DUPLICATE multiple options with the code: %1 added to the subnet: %2 -This warning message is issued on attempt to configure multiple options with the +This warning message is issued on an attempt to configure multiple options with the same option code for the particular subnet. Adding multiple options is uncommon -for DHCPv4, yet it is not prohibited. +for DHCPv4, but it is not prohibited. % DHCP4_CONFIG_COMPLETE DHCPv4 server has completed configuration: %1 This is an informational message announcing the successful processing of a diff --git a/src/bin/dhcp4/tests/config_parser_unittest.cc b/src/bin/dhcp4/tests/config_parser_unittest.cc index 2f6df58898..b88cdcc4df 100644 --- a/src/bin/dhcp4/tests/config_parser_unittest.cc +++ b/src/bin/dhcp4/tests/config_parser_unittest.cc @@ -1,4 +1,3 @@ - // Copyright (C) 2012 Internet Systems Consortium, Inc. ("ISC") // // Permission to use, copy, modify, and/or distribute this software for any @@ -205,9 +204,10 @@ public: ASSERT_EQ(buf.getLength() - option_desc.option->getHeaderLen(), expected_data_len); } - // Verify that the data is correct. However do not verify suboptions. + // Verify that the data is correct. Do not verify suboptions and a header. const uint8_t* data = static_cast(buf.getData()); - EXPECT_TRUE(memcmp(expected_data, data, expected_data_len)); + EXPECT_EQ(0, memcmp(expected_data, data + option_desc.option->getHeaderLen(), + expected_data_len)); } /// @brief Reset configuration database. @@ -239,14 +239,15 @@ public: << ex.what() << std::endl; } - - // returned value should be 0 (configuration success) + // status object must not be NULL if (!status) { FAIL() << "Fatal error: unable to reset configuration database" << " after the test. Configuration function returned" << " NULL pointer" << std::endl; } + comment_ = parseAnswer(rcode_, status); + // returned value should be 0 (configuration success) if (rcode_ != 0) { FAIL() << "Fatal error: unable to reset configuration database" << " after the test. Configuration function returned" diff --git a/src/bin/dhcp6/config_parser.cc b/src/bin/dhcp6/config_parser.cc index 4c6fab15f2..edda3121ec 100644 --- a/src/bin/dhcp6/config_parser.cc +++ b/src/bin/dhcp6/config_parser.cc @@ -496,12 +496,12 @@ private: /// /// This parser parses configuration entries that specify value of /// a single option. These entries include option name, option code -/// and data carried by the option. If parsing is successful than an +/// and data carried by the option. If parsing is successful then an /// instance of an option is created and added to the storage provided /// by the calling class. /// /// @todo This class parses and validates the option name. However it is -/// not used anywhere util support for option spaces is implemented +/// not used anywhere until support for option spaces is implemented /// (see tickets #2319, #2314). When option spaces are implemented /// there will be a way to reference the particular option using /// its type (code) or option name. @@ -857,26 +857,21 @@ public: // a setStorage and build methods are invoked. // Try uint32 type parser. - if (buildParser(parser, uint32_values_, - param.second)) { - // Storage set, build invoked on the parser, proceed with - // next configuration element. - continue; - } - // Try string type parser. - if (buildParser(parser, string_values_, - param.second)) { - continue; - } - // Try pools parser. - if (buildParser(parser, pools_, - param.second)) { - continue; - } - // Try option data parser. - if (buildParser(parser, options_, - param.second)) { - continue; + if (!buildParser(parser, uint32_values_, + param.second) || + // Try string type parser. + !buildParser(parser, string_values_, + param.second) || + // Try pool parser. + !buildParser(parser, pools_, + param.second) || + // Try option data parser. + !buildParser(parser, options_, + param.second)) { + // Appropriate parsers are created in the createSubnet6ConfigParser + // and they should be limited to those that we check here for. Thus, + // if we fail to find a matching parser here it is a programming error. + isc_throw(Dhcp6ConfigError, "failed to find suitable parser"); } } // Ok, we now have subnet parsed diff --git a/src/bin/dhcp6/dhcp6_messages.mes b/src/bin/dhcp6/dhcp6_messages.mes index 6ab42b3c96..19e9c7ef82 100644 --- a/src/bin/dhcp6/dhcp6_messages.mes +++ b/src/bin/dhcp6/dhcp6_messages.mes @@ -47,9 +47,9 @@ This is an informational message reporting that the configuration has been extended to include the specified subnet. % DHCP6_CONFIG_OPTION_DUPLICATE multiple options with the code: %1 added to the subnet: %2 -This warning message is issued on attempt to configure multiple options with the +This warning message is issued on an attempt to configure multiple options with the same option code for the particular subnet. Adding multiple options is uncommon -for DHCPv6, yet it is not prohibited. +for DHCPv6, but it is not prohibited. % DHCP6_CONFIG_START DHCPv6 server is processing the following configuration: %1 This is a debug message that is issued every time the server receives a diff --git a/src/bin/dhcp6/tests/config_parser_unittest.cc b/src/bin/dhcp6/tests/config_parser_unittest.cc index 4fd6baa1eb..1440338069 100644 --- a/src/bin/dhcp6/tests/config_parser_unittest.cc +++ b/src/bin/dhcp6/tests/config_parser_unittest.cc @@ -144,14 +144,14 @@ public: << ex.what() << std::endl; } - - // returned value should be 0 (configuration success) + // status object must not be NULL if (!status) { FAIL() << "Fatal error: unable to reset configuration database" << " after the test. Configuration function returned" << " NULL pointer" << std::endl; } comment_ = parseAnswer(rcode_, status); + // returned value should be 0 (configuration success) if (rcode_ != 0) { FAIL() << "Fatal error: unable to reset configuration database" << " after the test. Configuration function returned" @@ -215,9 +215,10 @@ public: ASSERT_EQ(buf.getLength() - option_desc.option->getHeaderLen(), expected_data_len); } - // Verify that the data is correct. However do not verify suboptions. + // Verify that the data is correct. Do not verify suboptions and a header. const uint8_t* data = static_cast(buf.getData()); - EXPECT_TRUE(memcmp(expected_data, data, expected_data_len)); + EXPECT_EQ(0, memcmp(expected_data, data + option_desc.option->getHeaderLen(), + expected_data_len)); } Dhcpv6Srv srv_;