[3295] Addressed review comments.

src/bin/dhcp6/dhcp6_messages.mes

@@ -100,13 +100,7 @@ in its response to the client.
 This debug message is logged when server has found the DHCPv6 Client FQDN Option
 sent by a client and started processing it.
 
-% DHCP6_DDNS_REMOVE_EMPTY_HOSTNAME FQDN for the lease being deleted is empty: %1
-This error message is issued when a lease being deleted contains an indication
-that the DNS Update has been performed for it, but the FQDN is missing for this
-lease. This is an indication that someone may have messed up in the lease
-database.
-
-% DHCP6_DDNS_REMOVE_INVALID_HOSTNAME FQDN for the lease being deleted has invalid format: %1
+% DHCP6_DDNS_REMOVE_INVALID_HOSTNAME invalid FQDN: %1 for the lease: %2 when removing DNS bindings
 This error message is issued when a lease being deleted contains an indication
 that the DNS Update has been performed for it, but the FQDN held in the lease
 database has invalid format and can't be transformed to the canonical on-wire

src/bin/dhcp6/dhcp6_srv.cc

@@ -1132,20 +1132,11 @@ Dhcpv6Srv::createRemovalNameChangeRequest(const Lease6Ptr& lease) {
         return;
     }
 
-    // When lease was added into a database the host name should have
-    // been added. The hostname can be empty if someone messed up in the
-    // lease data base and removed the hostname.
-    if (lease->hostname_.empty()) {
-        LOG_ERROR(dhcp6_logger, DHCP6_DDNS_REMOVE_EMPTY_HOSTNAME)
-            .arg(lease->addr_.toText());
-        return;
-    }
-
     // If hostname is non-empty, try to convert it to wire format so as
     // DHCID can be computed from it. This may throw an exception if hostname
-    // has invalid format. Again, this should be only possible in case of
-    // manual intervention in the database. Note that the last parameter
-    // passed to the writeFqdn function forces conversion of the FQDN
+    // has invalid format or is empty. Again, this should be only possible
+    // in case of manual intervention in the database. Note that the last
+    // parameter passed to the writeFqdn function forces conversion of the FQDN
     // to lower case. This is required by the RFC4701, section 3.5.
     // The DHCID computation is further in this function.
     std::vector<uint8_t> hostname_wire;
@@ -1153,7 +1144,8 @@ Dhcpv6Srv::createRemovalNameChangeRequest(const Lease6Ptr& lease) {
         OptionDataTypeUtil::writeFqdn(lease->hostname_, hostname_wire, true);
     } catch (const Exception& ex) {
         LOG_ERROR(dhcp6_logger, DHCP6_DDNS_REMOVE_INVALID_HOSTNAME)
-            .arg(lease->hostname_);
+            .arg(lease->hostname_.empty() ? "(empty)" : lease->hostname_)
+            .arg(lease->addr_.toText());
         return;
     }
 
@@ -2445,8 +2437,15 @@ Dhcpv6Srv::generateFqdn(const Pkt6Ptr& answer) {
                 LeaseMgrFactory::instance().getLease6(Lease::TYPE_NA, addr);
             if (lease) {
                 lease->hostname_ = stream.str();
+                LeaseMgrFactory::instance().updateLease6(lease);
+
+            } else {
+                isc_throw(isc::Unexpected, "there is no lease in the database "
+                          " for address " << addr << ", so as it is impossible"
+                          " to update FQDN data. This is a programmatic error"
+                          " as the given address is now being handed to the"
+                          " client");
             }
-            LeaseMgrFactory::instance().updateLease6(lease);
         }
 
         // Set the generated FQDN in the Client FQDN option.

src/bin/dhcp6/tests/fqdn_unittest.cc

@@ -302,8 +302,9 @@ public:
         OptionPtr srvid = srv.getServerID();
         // Set the appropriate FQDN type. It must be partial if hostname is
         // empty.
-        Option6ClientFqdn::DomainNameType fqdn_type = hostname.empty() ?
-            Option6ClientFqdn::PARTIAL : Option6ClientFqdn::FULL;
+        Option6ClientFqdn::DomainNameType fqdn_type = (hostname.empty() ?
+            Option6ClientFqdn::PARTIAL : Option6ClientFqdn::FULL);
+
         Pkt6Ptr req = generateMessage(msg_type, Option6ClientFqdn::FLAG_S,
                                       hostname, fqdn_type, include_oro, srvid);
 

src/lib/dhcpsrv/alloc_engine.h

@@ -1,4 +1,4 @@
-// Copyright (C) 2012-2013 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2012-2014 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
@@ -496,6 +496,22 @@ private:
                                 const isc::hooks::CalloutHandlePtr& callout_handle,
                                 bool fake_allocation = false);
 
+    /// @brief Updates FQDN data for a collection of leases.
+    ///
+    /// @param leases Collection of leases for which FQDN data should be
+    /// updated.
+    /// @param fwd_dns_update Boolean value which indicates whether forward FQDN
+    /// update was performed for each lease (true) or not (false).
+    /// @param rev_dns_update Boolean value which indicates whether reverse FQDN
+    /// update was performed for each lease (true) or not (false).
+    /// @param hostname Client hostname associated with a lease.
+    /// @param fake_allocation Boolean value which indicates that it is a real
+    /// lease allocation, e.g. Request message is processed (false), or address
+    /// is just being picked as a result of processing Solicit (true). In the
+    /// latter case, the FQDN data should not be updated in the lease database.
+    ///
+    /// @return Collection of leases with updated FQDN data. Note that returned
+    /// collection holds updated FQDN data even for fake allocation.
     Lease6Collection updateFqdnData(const Lease6Collection& leases,
                                     const bool fwd_dns_update,
                                     const bool rev_dns_update,

src/lib/dhcpsrv/tests/lease_unittest.cc

@@ -736,17 +736,17 @@ TEST(Lease6, getDuidVector) {
 // Verify the behavior of the function which checks FQDN data for equality.
 TEST(Lease6, hasIdenticalFqdn) {
     Lease6 lease = createLease6("myhost.example.com.", true, true);
-    EXPECT_TRUE(lease.hasIdenticalFqdn(createLease4("myhost.example.com.",
+    EXPECT_TRUE(lease.hasIdenticalFqdn(createLease6("myhost.example.com.",
                                                     true, true)));
-    EXPECT_FALSE(lease.hasIdenticalFqdn(createLease4("other.example.com.",
+    EXPECT_FALSE(lease.hasIdenticalFqdn(createLease6("other.example.com.",
                                                      true, true)));
-    EXPECT_FALSE(lease.hasIdenticalFqdn(createLease4("myhost.example.com.",
+    EXPECT_FALSE(lease.hasIdenticalFqdn(createLease6("myhost.example.com.",
                                                      false, true)));
-    EXPECT_FALSE(lease.hasIdenticalFqdn(createLease4("myhost.example.com.",
+    EXPECT_FALSE(lease.hasIdenticalFqdn(createLease6("myhost.example.com.",
                                                      true, false)));
-    EXPECT_FALSE(lease.hasIdenticalFqdn(createLease4("myhost.example.com.",
+    EXPECT_FALSE(lease.hasIdenticalFqdn(createLease6("myhost.example.com.",
                                                      false, false)));
-    EXPECT_FALSE(lease.hasIdenticalFqdn(createLease4("other.example.com.",
+    EXPECT_FALSE(lease.hasIdenticalFqdn(createLease6("other.example.com.",
                                                      false, false)));
 }