[#805,!6-p] Addressed review comments

src/lib/dhcpsrv/csv_lease_file4.*
src/lib/dhcpsrv/tests/csv_lease_file4_unittest.cc
    - minor cleanup

src/lib/dhcpsrv/csv_lease_file6.*
    CSVLeaseFile6::append() - now throws if DUID is empty and
    state is not STATE_DECLINED

src/lib/dhcpsrv/tests/csv_lease_file6_unittest.cc
    Updated tests to verify duid/state logic

6 files changed, 74 insertions, 15 deletions

diff --git a/src/lib/dhcpsrv/csv_lease_file4.cc b/src/lib/dhcpsrv/csv_lease_file4.cc
index 6649506579..acf672362f 100644
--- a/src/lib/dhcpsrv/csv_lease_file4.cc
+++ b/src/lib/dhcpsrv/csv_lease_file4.cc
@@ -44,15 +44,21 @@ CSVLeaseFile4::append(const Lease4& lease) {
         ++write_errs_;
 
         isc_throw(BadValue, "Lease4: " << lease.addr_.toText() << ", state: "
-                  << lease.state_ << " has neither hardware address or client id");
+                  << Lease::basicStatesToText(lease.state_)
+                  << " has neither hardware address or client id");
+
+    }
+
+    // Hardware addr may be unset (NULL).
+    if (lease.hwaddr_) {
+        row.writeAt(getColumnIndex("hwaddr"), lease.hwaddr_->toText(false));
     }
 
-    row.writeAt(getColumnIndex("hwaddr"),
-                (!lease.hwaddr_  ? "" : lease.hwaddr_->toText(false)));
     // Client id may be unset (NULL).
     if (lease.client_id_) {
         row.writeAt(getColumnIndex("client_id"), lease.client_id_->toText());
     }
+
     row.writeAt(getColumnIndex("valid_lifetime"), lease.valid_lft_);
     row.writeAt(getColumnIndex("expire"), static_cast<uint64_t>(lease.cltt_ + lease.valid_lft_));
     row.writeAt(getColumnIndex("subnet_id"), lease.subnet_id_);
@@ -117,7 +123,8 @@ CSVLeaseFile4::next(Lease4Ptr& lease) {
         if ((hwaddr.hwaddr_.empty()) && (client_id_vec.empty()) &&
             (state != Lease::STATE_DECLINED)) {
             isc_throw(BadValue, "Lease4: " << addr.toText() << ", state: "
-                      << state << "has neither hardware address or client id");
+                      << Lease::basicStatesToText(state)
+                      << "has neither hardware address or client id");
         }
 
         // Get the user context (can be NULL).
diff --git a/src/lib/dhcpsrv/csv_lease_file4.h b/src/lib/dhcpsrv/csv_lease_file4.h
index 950417c9e1..7f6b17f88f 100644
--- a/src/lib/dhcpsrv/csv_lease_file4.h
+++ b/src/lib/dhcpsrv/csv_lease_file4.h
@@ -68,8 +68,8 @@ public:
     /// message using @c CSVFile::setReadMsg and returns false. The error
     /// string may be read using @c CSVFile::getReadMsg.
     ///
-    /// Treats rows that no hardware address, no client id and state is not
-    /// STATE_DECLINED as an error.
+    /// Treats rows without a hardware address or a client id when their
+    /// state is not STATE_DECLINED as an error.
     ///
     /// This function is exception safe.
     ///
diff --git a/src/lib/dhcpsrv/csv_lease_file6.cc b/src/lib/dhcpsrv/csv_lease_file6.cc
index 3e70c7c6ec..3774113032 100644
--- a/src/lib/dhcpsrv/csv_lease_file6.cc
+++ b/src/lib/dhcpsrv/csv_lease_file6.cc
@@ -35,6 +35,12 @@ CSVLeaseFile6::append(const Lease6& lease) {
     // Bump the number of write attempts
     ++writes_;
 
+    if ((*(lease.duid_) == DUID::EMPTY()) && (lease.state_ != Lease::STATE_DECLINED)) {
+        ++write_errs_;
+        isc_throw(BadValue, "Lease6: " << lease.addr_.toText() << ", state: "
+                  << Lease::basicStatesToText(lease.state_) << ", has no DUID");
+    }
+
     CSVRow row(getColumnCount());
     row.writeAt(getColumnIndex("address"), lease.addr_.toText());
     row.writeAt(getColumnIndex("duid"), lease.duid_->toText());
diff --git a/src/lib/dhcpsrv/csv_lease_file6.h b/src/lib/dhcpsrv/csv_lease_file6.h
index 6ee380a233..84acbd0a85 100644
--- a/src/lib/dhcpsrv/csv_lease_file6.h
+++ b/src/lib/dhcpsrv/csv_lease_file6.h
@@ -1,4 +1,4 @@
-// Copyright (C) 2014-2018 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2014-2019 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
@@ -57,6 +57,8 @@ public:
     /// error.
     ///
     /// @param lease Structure representing a DHCPv6 lease.
+    /// @throw BadValue if the lease to be written has an empty DUID and is
+    /// whose state is not STATE_DECLINED.
     void append(const Lease6& lease);
 
     /// @brief Reads next lease from the CSV file.
diff --git a/src/lib/dhcpsrv/tests/csv_lease_file4_unittest.cc b/src/lib/dhcpsrv/tests/csv_lease_file4_unittest.cc
index ad65277a71..fe5b668a1b 100644
--- a/src/lib/dhcpsrv/tests/csv_lease_file4_unittest.cc
+++ b/src/lib/dhcpsrv/tests/csv_lease_file4_unittest.cc
@@ -514,7 +514,7 @@ TEST_F(CSVLeaseFile4Test, emptyHWAddrDefaultStateOnly) {
 
     hwaddr.reset(new HWAddr());
 
-    //Create lease with empty hdaddr and default state
+    // Create lease with empty hwaddr and default state
     Lease4Ptr lease_empty_hwaddr(new Lease4(IOAddress("192.0.3.2"),
                                  hwaddr,
                                  NULL, 0,
diff --git a/src/lib/dhcpsrv/tests/csv_lease_file6_unittest.cc b/src/lib/dhcpsrv/tests/csv_lease_file6_unittest.cc
index 49ae14bf9a..bfc79791c0 100644
--- a/src/lib/dhcpsrv/tests/csv_lease_file6_unittest.cc
+++ b/src/lib/dhcpsrv/tests/csv_lease_file6_unittest.cc
@@ -109,7 +109,9 @@ CSVLeaseFile6Test::writeSampleFile() const {
                   "2001:db8:2::10,01:01:01:01:0a:01:02:03:04:05,300,300,6,150,"
                   "0,8,0,0,0,,,1,\n"
                   "3000:1::,00:01:02:03:04:05:06:0a:0b:0c:0d:0e:0f,0,200,8,0,2,"
-                  "16,64,0,0,,,1,{ \"foobar\": true }\n");
+                  "16,64,0,0,,,1,{ \"foobar\": true }\n"
+                  "2001:db8:1::2,00,200,200,8,100,0,7,0,1,1,host.example.com,,0,\n"
+                  "2001:db8:1::3,00,200,200,8,100,0,7,0,1,1,host.example.com,,1,\n");
 }
 
 // This test checks the capability to read and parse leases from the file.
@@ -153,7 +155,7 @@ TEST_F(CSVLeaseFile6Test, parse) {
     EXPECT_FALSE(lease->getContext());
     }
 
-    // Second lease is malformed - DUID is empty.
+    // Second lease is malformed - DUID is blank (i.e. ",,")
     {
     SCOPED_TRACE("Second lease malformed");
     EXPECT_FALSE(lf.next(lease));
@@ -212,21 +214,43 @@ TEST_F(CSVLeaseFile6Test, parse) {
     EXPECT_EQ("{ \"foobar\": true }", lease->getContext()->str());
     }
 
+
+    // Fifth lease is invalid - DUID is empty, state is not DECLINED
+    {
+    SCOPED_TRACE("Fifth lease invalid");
+    EXPECT_FALSE(lf.next(lease));
+    checkStats(lf, 5, 3, 2, 0, 0, 0);
+    }
+
+    // Reading the sixth lease should be successful.
+    {
+    SCOPED_TRACE("sixth lease valid");
+    EXPECT_TRUE(lf.next(lease));
+    ASSERT_TRUE(lease);
+    checkStats(lf, 6, 4, 2, 0, 0, 0);
+
+    // Verify that the lease is correct.
+    EXPECT_EQ("2001:db8:1::3", lease->addr_.toText());
+    ASSERT_TRUE(lease->duid_);
+    EXPECT_EQ("00", lease->duid_->toText());
+    EXPECT_EQ(Lease::STATE_DECLINED, lease->state_);
+    }
+
     // There are no more leases. Reading should cause no error, but the returned
     // lease pointer should be NULL.
     {
-    SCOPED_TRACE("Fifth read empty");
+    SCOPED_TRACE("Sixth read empty");
     EXPECT_TRUE(lf.next(lease));
     EXPECT_FALSE(lease);
-    checkStats(lf, 5, 3, 1, 0, 0, 0);
+    checkStats(lf, 7, 4, 2, 0, 0, 0);
     }
 
     // We should be able to do it again.
     {
-    SCOPED_TRACE("Sixth read empty");
+    SCOPED_TRACE("Seventh read empty");
     EXPECT_TRUE(lf.next(lease));
     EXPECT_FALSE(lease);
-    checkStats(lf, 6, 3, 1, 0, 0, 0);
+    checkStats(lf, 8, 4, 2, 0, 0, 0);
     }
 }
 
@@ -276,6 +300,25 @@ TEST_F(CSVLeaseFile6Test, recreate) {
     checkStats(lf, 0, 0, 0, 3, 3, 0);
     }
 
+    DuidPtr empty(new DUID(DUID::EMPTY()));
+    lease.reset(new Lease6(Lease::TYPE_NA, IOAddress("2001:db8:2::10"),
+                           empty, 8, 150, 300, 6, false, false,
+                           "", HWAddrPtr(), 128));
+    lease->cltt_ = 0;
+    {
+    SCOPED_TRACE("Fourth write - invalid, no DUID, not declined");
+    ASSERT_THROW(lf.append(*lease), BadValue);
+    checkStats(lf, 0, 0, 0, 4, 3, 1);
+    }
+
+    {
+    SCOPED_TRACE("Fifth write - valid, no DUID, declined");
+    lease->state_ = Lease::STATE_DECLINED;
+    ASSERT_NO_THROW(lf.append(*lease));
+    checkStats(lf, 0, 0, 0, 5, 4, 1);
+    }
+
+
     EXPECT_EQ("address,duid,valid_lifetime,expire,subnet_id,pref_lifetime,"
               "lease_type,iaid,prefix_len,fqdn_fwd,fqdn_rev,hostname,hwaddr,"
               "state,user_context\n"
@@ -284,7 +327,8 @@ TEST_F(CSVLeaseFile6Test, recreate) {
               "2001:db8:2::10,01:01:01:01:0a:01:02:03:04:05"
               ",300,300,6,150,0,8,128,0,0,,,0,\n"
               "3000:1:1::,00:01:02:03:04:05:06:0a:0b:0c:0d:0e:0f,"
-              "300,300,10,150,2,7,64,0,0,,,0,{ \"foobar\": true }\n",
+              "300,300,10,150,2,7,64,0,0,,,0,{ \"foobar\": true }\n"
+              "2001:db8:2::10,00,300,300,6,150,0,8,128,0,0,,,1,\n",
               io_.readFile());
 }