[2524] Various fixes in response to review

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.

src/lib/dhcpsrv/dhcpsrv_log.cc

-// 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

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

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) {

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

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());