[5058] - Addressed review comments

    Fixed typos and added decline lease checks for DHCPv6

src/lib/dhcp/duid.h
src/lib/dhcp/duid.cc
src/lib/dhcp/tests/duid_unittest.cc
    - Replaced DUID::generateEmpty() with DUID::EMPTY() which returns
    a constant reference to a static DUID instance.  This facilitates
    comparisions.
    - Updated TEST(DuidTest,empty) accordingly

src/lib/dhcpsrv/csv_lease_file6.cc
    CSVLeaseFile6::next() - added test to permit the Empty DUID
    only if state is STATE_DECLINED.

src/lib/dhcpsrv/lease.cc
    Lease6::decline() - modified to use DUID::EMPTY().

src/lib/dhcpsrv/tests/csv_lease_file4_unittest.cc
    Fixed Typos

src/lib/dhcpsrv/tests/csv_lease_file6_unittest.cc
    TEST_F(CSVLeaseFile6Test, declinedLeaseTest) - new test to
    verify proper handling of declined leases

diff --git a/src/lib/dhcp/duid.cc b/src/lib/dhcp/duid.cc
index 067d4e15ce1daa04031b79df8056f7a8fc6cc4e5..f500ad155e28b96411701fba20e1ebced9118922 100644 (file)
--- a/src/lib/dhcp/duid.cc
+++ b/src/lib/dhcp/duid.cc
@@ -62,16 +62,11 @@ DUID::fromText(const std::string& text) {
     return (DUID(binary));
 }
 
-DuidPtr
-DUID::generateEmpty() {
-
-    // Technically this is a one byte DUID with value of 0. However, we do have
-    // a number of safety checks against invalid duids (too long or empty) and
-    // we should keep them. Therefore "empty" means a single byte with value of 0.
-    std::vector<uint8_t> empty_duid(1,0);
-
-    DuidPtr duid(new DUID(empty_duid));
-    return (duid);
+const DUID&
+DUID::EMPTY() {
+    static std::vector<uint8_t> empty_duid(1,0);
+    static DUID empty(empty_duid);
+        return (empty);
 }
 
 std::string DUID::toText() const {

diff --git a/src/lib/dhcp/duid.h b/src/lib/dhcp/duid.h
index 150a79e5f1af9748368fb3b13636af3195e72b63..1b4e42bd6c50db61b78b2f21001fad90a412be4d 100644 (file)
--- a/src/lib/dhcp/duid.h
+++ b/src/lib/dhcp/duid.h
@@ -61,17 +61,16 @@ class DUID {
     /// @return A reference to a vector holding a DUID.
     const std::vector<uint8_t>& getDuid() const;
 
-
-    /// @brief Returns an instance of empty DUID
+    /// @brief Defines the constant "empty" DUID
     ///
     /// In general, empty DUID is not allowed. The only case where it is really
     /// valid is to designate declined IPv6 Leases. We have a broad assumption
     /// that the Lease->duid_ must always be set. However, declined lease
     /// doesn't have any DUID associated with it. Hence we need a way to
     /// indicate that fact.
-    ///
-    /// @return a smart pointer to an empty DUID
-    static DuidPtr generateEmpty();
+    //
+    /// @return reference to the static constant empty DUID
+    static const DUID& EMPTY();
 
     /// @brief Returns the DUID type
     DUIDType getType() const;

diff --git a/src/lib/dhcp/tests/duid_unittest.cc b/src/lib/dhcp/tests/duid_unittest.cc
index 8c8a4aa7d15f7cd3e52a91d95dbb7118ce3faaf6..8a78db1c8397c636665547f7938082e15e2b8878 100644 (file)
--- a/src/lib/dhcp/tests/duid_unittest.cc
+++ b/src/lib/dhcp/tests/duid_unittest.cc
@@ -160,7 +160,7 @@ TEST(DuidTest, toText) {
 // This test verifies that empty DUID returns proper value
 TEST(DuidTest, empty) {
     DuidPtr empty;
-    EXPECT_NO_THROW(empty = DUID::generateEmpty());
+    EXPECT_NO_THROW(empty.reset(new DUID(DUID::EMPTY())));
 
     // This method must return something
     ASSERT_TRUE(empty);
@@ -168,6 +168,13 @@ TEST(DuidTest, empty) {
     // Ok, technically empty is not really empty, it's just a single
     // byte with value of 0.
     EXPECT_EQ("00", empty->toText());
+
+    EXPECT_TRUE(*empty == DUID::EMPTY());
+
+    uint8_t data1[] = {0, 1, 2, 3, 4, 0xff, 0xfe};
+    DUID duid(data1, sizeof(data1));
+
+    EXPECT_FALSE(duid == DUID::EMPTY());
 }
 
 // This test checks if the comparison operators are sane.

diff --git a/src/lib/dhcpsrv/csv_lease_file6.cc b/src/lib/dhcpsrv/csv_lease_file6.cc
index 5a041b813cde9c95a9b82386d8c6e8505d45c197..c1f8cb474a233ab1382be372e512ee4cdbe81931 100644 (file)
--- a/src/lib/dhcpsrv/csv_lease_file6.cc
+++ b/src/lib/dhcpsrv/csv_lease_file6.cc
@@ -1,4 +1,4 @@
-// Copyright (C) 2014-2015 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2014-2016 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -94,7 +94,11 @@ CSVLeaseFile6::next(Lease6Ptr& lease) {
         lease->fqdn_rev_ = readFqdnRev(row);
         lease->hostname_ = readHostname(row);
         lease->state_ = readState(row);
-
+        if ((*lease->duid_ == DUID::EMPTY())
+            && lease->state_ != Lease::STATE_DECLINED) {
+            isc_throw(isc::BadValue, "The Empty DUID is"
+                      "only valid for declined leases");
+        }
     } catch (std::exception& ex) {
         // bump the read error count
         ++read_errs_;

diff --git a/src/lib/dhcpsrv/lease.cc b/src/lib/dhcpsrv/lease.cc
index 5854f06900a8785fc1552c431c91129c742c251a..7dceb9db6997ff734ac7d7efc8a9adb8b40884a0 100644 (file)
--- a/src/lib/dhcpsrv/lease.cc
+++ b/src/lib/dhcpsrv/lease.cc
@@ -275,7 +275,7 @@ Lease6::getDuidVector() const {
 void
 Lease6::decline(uint32_t probation_period) {
     hwaddr_.reset();
-    duid_ = DUID::generateEmpty();
+    duid_.reset(new DUID(DUID::EMPTY()));
     t1_ = 0;
     t2_ = 0;
     preferred_lft_ = 0;

diff --git a/src/lib/dhcpsrv/tests/csv_lease_file4_unittest.cc b/src/lib/dhcpsrv/tests/csv_lease_file4_unittest.cc
index 9bdd3f9297c12b69450fb63b6ff58ef72e2df6ba..a9ccfe189ee728d27dfa26b0d4ee5f5ad5da86e7 100644 (file)
--- a/src/lib/dhcpsrv/tests/csv_lease_file4_unittest.cc
+++ b/src/lib/dhcpsrv/tests/csv_lease_file4_unittest.cc
@@ -146,7 +146,7 @@ TEST_F(CSVLeaseFile4Test, parse) {
     }
 
     // Second lease is malformed - HW address is empty when state 
-    // is not delcined.
+    // is not declined.
     {
     SCOPED_TRACE("Second lease malformed");
     EXPECT_FALSE(lf.next(lease));
@@ -386,7 +386,7 @@ TEST_F(CSVLeaseFile4Test, downGrade) {
     }
 }
 
-// Verifies that leases with no hardware address or only permitted
+// Verifies that leases with no hardware address are only permitted
 // if they are in the declined state.
 TEST_F(CSVLeaseFile4Test, declinedLeaseTest) {
     io_.writeFile("address,hwaddr,client_id,valid_lifetime,expire,subnet_id,"
@@ -394,12 +394,10 @@ TEST_F(CSVLeaseFile4Test, declinedLeaseTest) {
                   "192.0.2.1,,,200,200,8,1,1,host.example.com,0\n"
                   "192.0.2.1,,,200,200,8,1,1,host.example.com,1\n");
 
-    // Lease file should open and report as needing downgrade.
     CSVLeaseFile4 lf(filename_);
     ASSERT_NO_THROW(lf.open());
     EXPECT_FALSE(lf.needsConversion());
-    EXPECT_EQ(util::VersionedCSVFile::CURRENT,
-              lf.getInputSchemaState());
+    EXPECT_EQ(util::VersionedCSVFile::CURRENT, lf.getInputSchemaState());
     Lease4Ptr lease;
 
     {

diff --git a/src/lib/dhcpsrv/tests/csv_lease_file6_unittest.cc b/src/lib/dhcpsrv/tests/csv_lease_file6_unittest.cc
index a2003cb9d2ba39889b6c07e2a6cbd63419ce8450..a2533fd972f01a6f06c6741ad85d0e644e44d6f7 100644 (file)
--- a/src/lib/dhcpsrv/tests/csv_lease_file6_unittest.cc
+++ b/src/lib/dhcpsrv/tests/csv_lease_file6_unittest.cc
@@ -451,6 +451,48 @@ TEST_F(CSVLeaseFile6Test, downGrade) {
     }
 }
 
+// Verifies that leases with no DUID are invalid, and that leases
+// with the "Empty" DUID (1 byte duid = 0x0) are valid only when 
+// in the declined state.
+TEST_F(CSVLeaseFile6Test, declinedLeaseTest) {
+    io_.writeFile("address,duid,valid_lifetime,expire,subnet_id,"
+                  "pref_lifetime,lease_type,iaid,prefix_len,fqdn_fwd,"
+                  "fqdn_rev,hostname,hwaddr,state\n"
+                  "2001:db8:1::1,00,"
+                  "200,200,8,100,0,7,0,1,1,host.example.com,,0\n"
+                  "2001:db8:1::1,,"
+                  "200,200,8,100,0,7,0,1,1,host.example.com,,0\n"
+                  "2001:db8:1::1,00,"
+                  "200,200,8,100,0,7,0,1,1,host.example.com,,1\n");
+
+    CSVLeaseFile6 lf(filename_);
+    ASSERT_NO_THROW(lf.open());
+    EXPECT_FALSE(lf.needsConversion());
+    EXPECT_EQ(util::VersionedCSVFile::CURRENT, lf.getInputSchemaState());
+    Lease6Ptr lease;
+
+    {
+    SCOPED_TRACE("\"Empty\" DUID and not declined, invalid");
+    EXPECT_FALSE(lf.next(lease));
+    ASSERT_FALSE(lease);
+    EXPECT_EQ(lf.getReadErrs(),1);
+    }
+
+    {
+    SCOPED_TRACE("Missing (blank) DUID and not declined, invalid");
+    EXPECT_FALSE(lf.next(lease));
+    ASSERT_FALSE(lease);
+    EXPECT_EQ(lf.getReadErrs(),2);
+    }
+
+    {
+    SCOPED_TRACE("\"Empty\" DUID and declined, valid");
+    EXPECT_TRUE(lf.next(lease));
+    ASSERT_TRUE(lease);
+    EXPECT_EQ(lf.getReadErrs(),2);
+    }
+}
+
 
 /// @todo Currently we don't check invalid lease attributes, such as invalid
 /// lease type, invalid preferred lifetime vs valid lifetime etc. The Lease6