[3555] Changes after review:

 - Comments updated
 - getRawHWAddr renamed to getHWAddrVector and moved to Lease class
 - HWAddr object is now copied in copy ctor and = operator

src/lib/dhcpsrv/csv_lease_file6.h

@@ -106,6 +106,7 @@ private:
     /// - fqdn_fwd
     /// - fqdn_rev
     /// - hostname
+    /// - hwaddr
     void initColumns();
 
     ///

src/lib/dhcpsrv/dhcpsrv_messages.mes

@@ -313,7 +313,7 @@ testing but should not be enabled in normal circumstances. Non-persistence
 mode is enabled when 'persist4=no persist6=no' parameters are specified
 in the database access string.
 
-% DHCPSRV_MEMFILE_READ_HWADDR_FAIL failed to read hardware address from disk: %1
+% DHCPSRV_MEMFILE_READ_HWADDR_FAIL failed to read hardware address from lease file: %1
 A warning message issued when read attempt of the hardware address stored in
 a disk file failed. The parameter should provide the exact nature of the failure.
 The database read will continue, but that particular lease will no longer

src/lib/dhcpsrv/lease.cc

@@ -72,6 +72,11 @@ Lease4::Lease4(const Lease4& other)
     fixed_ = other.fixed_;
     comments_ = other.comments_;
 
+    // Copy the hardware address if it is defined.
+    if (other.hwaddr_) {
+        hwaddr_.reset(new HWAddr(*other.hwaddr_));
+    }
+
     if (other.client_id_) {
         client_id_.reset(new ClientId(other.client_id_->getClientId()));
 
@@ -92,12 +97,10 @@ Lease4::getClientIdVector() const {
 }
 
 const std::vector<uint8_t>&
-Lease4::getRawHWAddr() const {
+Lease::getHWAddrVector() const {
     if (!hwaddr_) {
-        // something is wrong, very wrong. Hardware address must always
-        // be present for all IPv4 leases.
-        isc_throw(BadValue, "Lease4 for address " << addr_.toText()
-                  << " is missing a hardware address");
+        static std::vector<uint8_t> empty_vec;
+        return (empty_vec);
     }
     return (hwaddr_->hwaddr_);
 }
@@ -118,12 +121,20 @@ Lease4::matches(const Lease4& other) const {
 
     // Note that hwaddr_ is now a poiner to the HWAddr structure.
     // We can't simply compare smart pointers, we need to compare the
-    // actual objects they point to. It is ok to not check whether they
-    // are non-NULL, as every Lease4 must have hardware address.
+    // actual objects they point to. If one of the leases had a hardware address
+    // and the other doesn't, they clearly don't match.
+    if ( (!hwaddr_ && other.hwaddr_) ||
+         (hwaddr_ && !other.hwaddr_) ) {
+        return (false);
+    }
+
+    // The lease is equal if addresses and extensions match and there is
+    // either the same hardware address on both or neither have hardware address
+    // specified.
     return (addr_ == other.addr_ &&
             ext_ == other.ext_ &&
-            *hwaddr_ == *other.hwaddr_);
-
+            (!hwaddr_ ||
+             *hwaddr_ == *other.hwaddr_) );
 }
 
 Lease4&
@@ -141,7 +152,11 @@ Lease4::operator=(const Lease4& other) {
         fqdn_rev_ = other.fqdn_rev_;
         comments_ = other.comments_;
         ext_ = other.ext_;
-        hwaddr_ = other.hwaddr_;
+
+        // Copy the hardware address if it is defined.
+        if (other.hwaddr_) {
+            hwaddr_.reset(new HWAddr(*other.hwaddr_));
+        }
 
         if (other.client_id_) {
             client_id_.reset(new ClientId(other.client_id_->getClientId()));
@@ -211,6 +226,7 @@ Lease6::toText() const {
            << "Pref life:     " << preferred_lft_ << "\n"
            << "Valid life:    " << valid_lft_ << "\n"
            << "Cltt:          " << cltt_ << "\n"
+           << "Hardware addr: " << (hwaddr_?hwaddr_->toText(false):"(none)")
            << "Subnet ID:     " << subnet_id_ << "\n";
 
     return (stream.str());
@@ -225,6 +241,7 @@ Lease4::toText() const {
            << "T1:            " << t1_ << "\n"
            << "T2:            " << t2_ << "\n"
            << "Cltt:          " << cltt_ << "\n"
+           << "Hardware addr: " << (hwaddr_?hwaddr_->toText(false):"(none)")
            << "Subnet ID:     " << subnet_id_ << "\n";
 
     return (stream.str());

src/lib/dhcpsrv/lease.h

@@ -156,6 +156,15 @@ struct Lease {
     /// lease is equal to the FQDN data of our lease (true) or not (false).
     bool hasIdenticalFqdn(const Lease& other) const;
 
+    /// @brief Returns raw (as vector) hardware address
+    ///
+    /// This method is needed in multi-index container as key extractor.
+    /// The const reference is only valid as long as the object that returned it.
+    /// In the unlikely case when Lease4 does not have a hardware address,
+    /// the function will return an empty vector.
+    ///
+    /// @return const reference to the hardware address
+    const std::vector<uint8_t>& getHWAddrVector() const;
 };
 
 /// @brief Structure that holds a lease for IPv4 address
@@ -230,14 +239,6 @@ struct Lease4 : public Lease {
     /// or an empty vector if client identifier is NULL.
     const std::vector<uint8_t>& getClientIdVector() const;
 
-    /// @brief Returns raw (as vector) hardware address
-    ///
-    /// This method is needed in multi-index container as key extractor.
-    /// The const reference is only valid as long as the object that returned it.
-    ///
-    /// @return const reference to the hardware address
-    const std::vector<uint8_t>& getRawHWAddr() const;
-
     /// @brief Check if two objects encapsulate the lease for the same
     /// client.
     ///

src/lib/dhcpsrv/memfile_lease_mgr.h

@@ -454,8 +454,8 @@ protected:
                     // Lease4 object, which is a HWAddr object. Boost does not
                     // provide a key extractor for getting a member of a member,
                     // so we need a simple method for that.
-                    boost::multi_index::const_mem_fun<Lease4, const std::vector<uint8_t>&,
-                                               &Lease4::getRawHWAddr>,
+                    boost::multi_index::const_mem_fun<Lease, const std::vector<uint8_t>&,
+                                               &Lease::getHWAddrVector>,
                     // The subnet id is held in the subnet_id_ member of Lease4
                     // class. Note that the subnet_id_ is defined in the base
                     // class (Lease) so we have to point to this class rather
@@ -493,8 +493,8 @@ protected:
                     // access the raw data using lease->hwaddr_->hwaddr_, but Boost
                     // doesn't seem to provide a way to use member of a member for this,
                     // so we need a simple key extractor method (getRawHWAddr).
-                    boost::multi_index::const_mem_fun<Lease4, const std::vector<uint8_t>&,
-                                            &Lease4::getRawHWAddr>,
+                    boost::multi_index::const_mem_fun<Lease, const std::vector<uint8_t>&,
+                                            &Lease::getHWAddrVector>,
                     // The subnet id is accessed through the subnet_id_ member.
                     boost::multi_index::member<Lease, SubnetID, &Lease::subnet_id_>
                 >

src/lib/dhcpsrv/tests/lease_unittest.cc

@@ -141,6 +141,13 @@ TEST_F(Lease4Test, copyConstructor) {
     EXPECT_TRUE(lease == copied_lease);
     // Client IDs are equal, but they should be in two distinct pointers.
     EXPECT_FALSE(lease.client_id_ == copied_lease.client_id_);
+
+    // Hardware addresses are equal, but they should point to two objects,
+    // each holding the same data. The content should be equal...
+    EXPECT_TRUE(*lease.hwaddr_ == *copied_lease.hwaddr_);
+
+    // ... but it should point to different objects.
+    EXPECT_FALSE(lease.hwaddr_ == copied_lease.hwaddr_);
 }
 
 // This test verfies that the assignment operator copies all Lease4 fields
@@ -172,6 +179,13 @@ TEST_F(Lease4Test, operatorAssign) {
     EXPECT_TRUE(lease == copied_lease);
     // Client IDs are equal, but they should be in two distinct pointers.
     EXPECT_FALSE(lease.client_id_ == copied_lease.client_id_);
+
+    // Hardware addresses are equal, but they should point to two objects,
+    // each holding the same data. The content should be equal...
+    EXPECT_TRUE(*lease.hwaddr_ == *copied_lease.hwaddr_);
+
+    // ... but it should point to different objects.
+    EXPECT_FALSE(lease.hwaddr_ == copied_lease.hwaddr_);
 }
 
 // This test verifies that the matches() returns true if two leases differ