[2524] Various fixes in response to review

diff --git a/src/lib/dhcpsrv/Makefile.am b/src/lib/dhcpsrv/Makefile.am
index 3e2891b63af15213c47c9c46a906d0d1b26a7d51..d2e604a58d28c47815375dac139f98413353fb94 100644 (file)
--- a/src/lib/dhcpsrv/Makefile.am
+++ b/src/lib/dhcpsrv/Makefile.am
@@ -12,9 +12,9 @@ AM_CXXFLAGS = $(B10_CXXFLAGS)
 dhcpsrv_messages.h dhcpsrv_messages.cc: dhcpsrv_messages.mes
        $(top_builddir)/src/lib/log/compiler/message $(top_srcdir)/src/lib/dhcpsrv/dhcpsrv_messages.mes
 
-# Tell Automake that the nsas_messages.{cc,h} source files are created in the build
-# process, so it must create these before doing anything else.  Although they
-# are a dependency of the library (so will be created from the message file
+# Tell Automake that the dhcpsrv_messages.{cc,h} source files are created in the
+# build process, so it must create these before doing anything else.  Although
+# they are a dependency of the library (so will be created from the message file
 # anyway), there is no guarantee as to exactly _when_ in the build they will be
 # created.  As the .h file is included in other sources file (so must be
 # present when they are compiled), the safest option is to create it first.

diff --git a/src/lib/dhcpsrv/dhcpsrv_log.cc b/src/lib/dhcpsrv/dhcpsrv_log.cc
index 10ac3ee84d8bea7fe8c95253f74287c761fe19d8..6cbfb0d1d81e1db2bdcfecbf6dc85ec32afcc113 100644 (file)
--- a/src/lib/dhcpsrv/dhcpsrv_log.cc
+++ b/src/lib/dhcpsrv/dhcpsrv_log.cc
@@ -1,4 +1,4 @@
-// Copyright (C) 2011  Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2012  Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above

diff --git a/src/lib/dhcpsrv/hwaddr.h b/src/lib/dhcpsrv/hwaddr.h
index a735792afacb50bace8f312e326fe1e64e2ff7fa..776f37bf0e1dff16d539951774eae8b20b19e0a8 100644 (file)
--- a/src/lib/dhcpsrv/hwaddr.h
+++ b/src/lib/dhcpsrv/hwaddr.h
@@ -12,8 +12,8 @@
 // OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
 // PERFORMANCE OF THIS SOFTWARE.
 
-#ifndef __HWADDR_H
-#define __HWADDR_H
+#ifndef HWADDR_H
+#define HWADDR_H
 
 #include <string>
 #include <vector>
@@ -30,9 +30,6 @@ typedef std::vector<uint8_t> HWAddr;
 /// Returns a string containing the hardware address. This is only used for
 /// logging.
 ///
-/// @note Six characters is an arbitrary length, chosen to provide a
-///       suitably wide string.
-///
 /// @todo Create a "hardware address" class of which this will be a member.
 ///
 /// @param hwaddr Hardware address to convert to string form
@@ -44,4 +41,4 @@ hardwareAddressString(const HWAddr& hwaddr);
 };  // namespace dhcp
 };  // namespace isc
 
-#endif // __HWADDR_H
+#endif // HWADDR_H

diff --git a/src/lib/dhcpsrv/lease_mgr_factory.cc b/src/lib/dhcpsrv/lease_mgr_factory.cc
index 06e2f798bbdef58a6e233641127c2eda6a30bd82..23ba12f5a88055f585d396b06f4916ff75ee1bbd 100644 (file)
--- a/src/lib/dhcpsrv/lease_mgr_factory.cc
+++ b/src/lib/dhcpsrv/lease_mgr_factory.cc
@@ -54,7 +54,7 @@ LeaseMgrFactory::parse(const std::string& dbaccess) {
         // there are cryptic warnings on Debian6 running g++ 4.4 in
         // /usr/include/c++/4.4/bits/stl_algo.h:2178 "array subscript is above
         // array bounds"
-        boost::split(tokens, dbaccess, boost::is_any_of( string("\t ") ));
+        boost::split(tokens, dbaccess, boost::is_any_of(string("\t ")));
         BOOST_FOREACH(std::string token, tokens) {
             size_t pos = token.find("=");
             if (pos != string::npos) {

diff --git a/src/lib/dhcpsrv/tests/cfgmgr_unittest.cc b/src/lib/dhcpsrv/tests/cfgmgr_unittest.cc
index bd4eba6b52136dc65d8c0db2836a59e3ae7b3fc9..353d833d3ca9307fd37176e8c0f995e8365a54e4 100644 (file)
--- a/src/lib/dhcpsrv/tests/cfgmgr_unittest.cc
+++ b/src/lib/dhcpsrv/tests/cfgmgr_unittest.cc
@@ -58,7 +58,7 @@ TEST_F(CfgMgrTest, subnet4) {
     Subnet4Ptr subnet3(new Subnet4(IOAddress("192.0.2.128"), 26, 1, 2, 3));
 
     // There shouldn't be any subnet configured at this stage
-    EXPECT_EQ(Subnet4Ptr(), cfg_mgr.getSubnet4(IOAddress("192.0.2.0")));
+    EXPECT_FALSE(cfg_mgr.getSubnet4(IOAddress("192.0.2.0")));
 
     cfg_mgr.addSubnet4(subnet1);
 
@@ -75,13 +75,13 @@ TEST_F(CfgMgrTest, subnet4) {
     EXPECT_EQ(subnet2, cfg_mgr.getSubnet4(IOAddress("192.0.2.85")));
 
     // Try to find an address that does not belong to any subnet
-    EXPECT_EQ(Subnet4Ptr(), cfg_mgr.getSubnet4(IOAddress("192.0.2.192")));
+    EXPECT_FALSE(cfg_mgr.getSubnet4(IOAddress("192.0.2.192")));
 
     // Check that deletion of the subnets works.
     cfg_mgr.deleteSubnets4();
-    EXPECT_EQ(Subnet4Ptr(), cfg_mgr.getSubnet4(IOAddress("192.0.2.191")));
-    EXPECT_EQ(Subnet4Ptr(), cfg_mgr.getSubnet4(IOAddress("192.0.2.15")));
-    EXPECT_EQ(Subnet4Ptr(), cfg_mgr.getSubnet4(IOAddress("192.0.2.85")));
+    EXPECT_FALSE(cfg_mgr.getSubnet4(IOAddress("192.0.2.191")));
+    EXPECT_FALSE(cfg_mgr.getSubnet4(IOAddress("192.0.2.15")));
+    EXPECT_FALSE(cfg_mgr.getSubnet4(IOAddress("192.0.2.85")));
 }
 
 // This test verifies if the configuration manager is able to hold and return
@@ -95,7 +95,7 @@ TEST_F(CfgMgrTest, subnet6) {
     Subnet6Ptr subnet3(new Subnet6(IOAddress("4000::"), 48, 1, 2, 3, 4));
 
     // There shouldn't be any subnet configured at this stage
-    EXPECT_EQ(Subnet6Ptr(), cfg_mgr.getSubnet6(IOAddress("2000::1")));
+    EXPECT_FALSE(cfg_mgr.getSubnet6(IOAddress("2000::1")));
 
     cfg_mgr.addSubnet6(subnet1);
 
@@ -111,13 +111,13 @@ TEST_F(CfgMgrTest, subnet6) {
 
     EXPECT_EQ(subnet3, cfg_mgr.getSubnet6(IOAddress("4000::123")));
     EXPECT_EQ(subnet2, cfg_mgr.getSubnet6(IOAddress("3000::dead:beef")));
-    EXPECT_EQ(Subnet6Ptr(), cfg_mgr.getSubnet6(IOAddress("5000::1")));
+    EXPECT_FALSE(cfg_mgr.getSubnet6(IOAddress("5000::1")));
 
     // Check that deletion of the subnets works.
     cfg_mgr.deleteSubnets6();
-    EXPECT_EQ(Subnet6Ptr(), cfg_mgr.getSubnet6(IOAddress("200::123")));
-    EXPECT_EQ(Subnet6Ptr(), cfg_mgr.getSubnet6(IOAddress("3000::123")));
-    EXPECT_EQ(Subnet6Ptr(), cfg_mgr.getSubnet6(IOAddress("4000::123")));
+    EXPECT_FALSE(cfg_mgr.getSubnet6(IOAddress("200::123")));
+    EXPECT_FALSE(cfg_mgr.getSubnet6(IOAddress("3000::123")));
+    EXPECT_FALSE(cfg_mgr.getSubnet6(IOAddress("4000::123")));
 }
 
 } // end of anonymous namespace

diff --git a/src/lib/dhcpsrv/tests/lease_mgr_factory_unittest.cc b/src/lib/dhcpsrv/tests/lease_mgr_factory_unittest.cc
index e83541da522533703d845a3b02f0f9e71f56d1bf..99244768da6bc931d60c8cd840f945c86c6b7551 100644 (file)
--- a/src/lib/dhcpsrv/tests/lease_mgr_factory_unittest.cc
+++ b/src/lib/dhcpsrv/tests/lease_mgr_factory_unittest.cc
@@ -80,15 +80,45 @@ TEST_F(LeaseMgrFactoryTest, parseInvalid) {
 ///
 /// Checks that the redacted configuration string includes the password only
 /// as a set of asterisks.
-TEST(LeaseMgr, redactAccessString) {
+TEST_F(LeaseMgrFactoryTest, redactAccessString) {
 
-    // To check the redacted string, break it down into its component
-    // parameters and check the results.
-    LeaseMgr::ParameterMap parameters = LeaseMgrFactory::parse(
-        "user=me password=forbidden name=kea type=mysql");
+    LeaseMgr::ParameterMap parameters =
+        LeaseMgrFactory::parse("user=me password=forbidden name=kea type=mysql");
+    EXPECT_EQ(4, parameters.size());
+    EXPECT_EQ("me", parameters["user"]);
+    EXPECT_EQ("forbidden", parameters["password"]);
+    EXPECT_EQ("kea", parameters["name"]);
+    EXPECT_EQ("mysql", parameters["type"]);
+
+    // Redact the result.  To check, break the redacted string down into its
+    // components.
     std::string redacted = LeaseMgrFactory::redactedAccessString(parameters);
+    parameters = LeaseMgrFactory::parse(redacted);
+
+    EXPECT_EQ(4, parameters.size());
+    EXPECT_EQ("me", parameters["user"]);
+    EXPECT_EQ("*****", parameters["password"]);
+    EXPECT_EQ("kea", parameters["name"]);
+    EXPECT_EQ("mysql", parameters["type"]);
+}
 
-    // ... and break the redacted string down into its components.
+/// @brief redactConfigString test - empty password
+///
+/// Checks that the redacted configuration string includes the password only
+/// as a set of asterisks, even if the password is null.
+TEST_F(LeaseMgrFactoryTest, redactAccessStringEmptyPassword) {
+
+    LeaseMgr::ParameterMap parameters =
+        LeaseMgrFactory::parse("user=me name=kea type=mysql password=");
+    EXPECT_EQ(4, parameters.size());
+    EXPECT_EQ("me", parameters["user"]);
+    EXPECT_EQ("", parameters["password"]);
+    EXPECT_EQ("kea", parameters["name"]);
+    EXPECT_EQ("mysql", parameters["type"]);
+
+    // Redact the result.  To check, break the redacted string down into its
+    // components.
+    std::string redacted = LeaseMgrFactory::redactedAccessString(parameters);
     parameters = LeaseMgrFactory::parse(redacted);
 
     EXPECT_EQ(4, parameters.size());
@@ -97,9 +127,15 @@ TEST(LeaseMgr, redactAccessString) {
     EXPECT_EQ("kea", parameters["name"]);
     EXPECT_EQ("mysql", parameters["type"]);
 
-    // Do the same, but check it works for an empty password.
-    parameters = LeaseMgrFactory::parse(
-        "user=me password=forbidden name=kea type=mysql");
+    // ... and again to check that the position of the empty password in the
+    // string does not matter.
+    parameters = LeaseMgrFactory::parse("user=me password= name=kea type=mysql");
+    EXPECT_EQ(4, parameters.size());
+    EXPECT_EQ("me", parameters["user"]);
+    EXPECT_EQ("", parameters["password"]);
+    EXPECT_EQ("kea", parameters["name"]);
+    EXPECT_EQ("mysql", parameters["type"]);
+
     redacted = LeaseMgrFactory::redactedAccessString(parameters);
     parameters = LeaseMgrFactory::parse(redacted);
 
@@ -108,10 +144,24 @@ TEST(LeaseMgr, redactAccessString) {
     EXPECT_EQ("*****", parameters["password"]);
     EXPECT_EQ("kea", parameters["name"]);
     EXPECT_EQ("mysql", parameters["type"]);
+}
 
-    // Do the same, but check it works in the absence of a password token
-    parameters = LeaseMgrFactory::parse("user=me name=kea type=mysql");
-    redacted = LeaseMgrFactory::redactedAccessString(parameters);
+/// @brief redactConfigString test - no password
+///
+/// Checks that the redacted configuration string excludes the password if there
+/// was no password to begion with.
+TEST_F(LeaseMgrFactoryTest, redactAccessStringNoPassword) {
+
+    LeaseMgr::ParameterMap parameters =
+        LeaseMgrFactory::parse("user=me name=kea type=mysql");
+    EXPECT_EQ(3, parameters.size());
+    EXPECT_EQ("me", parameters["user"]);
+    EXPECT_EQ("kea", parameters["name"]);
+    EXPECT_EQ("mysql", parameters["type"]);
+
+    // Redact the result.  To check, break the redacted string down into its
+    // components.
+    std::string redacted = LeaseMgrFactory::redactedAccessString(parameters);
     parameters = LeaseMgrFactory::parse(redacted);
 
     EXPECT_EQ(3, parameters.size());