[3694] Addressed review comments.

doc/guide/dhcp4-srv.xml

@@ -2073,13 +2073,14 @@ temporarily override a list of interface names and listen on all interfaces.
       that it is no longer allowed to use it by sending DHCPNAK message. Host A
       will then revert to server discovery and will eventually get a different
       address.  The address 192.0.2.10 is now no longer used. When host B tries
-      to renew its temporary address, the server will detect that it has a valid
-      lease, but there is a reservation for a different address. The server will
-      send DHCPNAK to inform host B that its address is no longer usable. The
-      server will also remove its temporary lease. It will revert to the server
-      discovery phase and will eventually send a DHCPREQUEST message. This time the
-      server will find out that there is a reservation for that host and the
-      reserved address 192.0.2.10 is not used, so it will be granted.</para> -->
+      to renew its temporarily assigned  address, the server will detect that
+      it has a valid lease, but there is a reservation for a different address.
+      The server will send DHCPNAK to inform host B that its address is no
+      longer usable. The server will also remove its temporary lease. It will
+      revert to the server discovery phase and will eventually send a
+      DHCPREQUEST message. This time the server will find out that there is a
+      reservation for that host and the reserved address 192.0.2.10 is not used,
+      so it will be granted.</para> -->
 
       <para>When the Host A renews its address, the server will discover that
       the address being renewed is now reserved for another host - the Host
@@ -2092,16 +2093,17 @@ temporarily override a list of interface names and listen on all interfaces.
       discovery and will eventually get a different address. Besides
       allocating a new lease, the server will also remove the old one. As
       a result, the address 192.0.2.10 will be no longer used. When Host B
-      tries to renew its temporary address, the server will detect that it has
-      a valid lease, but there is a reservation for a different address. The
-      server will send DHCPNAK to inform Host B that its address is no longer
-      usable, but will keep its lease (again, the DHCPNAK may be lost, so the
-      server will keep it, until the client returns for a new address). The
-      Host B will revert to the server discovery phase and will eventually
-      send a DHCPREQUEST message. This time the server will find out that
-      there is a reservation for that host and the reserved address
+      tries to renew its temporarily assigned address, the server will detect
+      that it has a valid lease, but there is a reservation for a different
+      address. The server will send DHCPNAK to inform Host B that its address
+      is no longer usable, but will keep its lease (again, the DHCPNAK may be
+      lost, so the server will keep it, until the client returns for a new
+      address). The Host B will revert to the server discovery phase and will
+      eventually send a DHCPREQUEST message. This time the server will find
+      out that there is a reservation for that host and the reserved address
       192.0.2.10 is not used, so it will be granted. It will also remove the
-      lease for the temporary address that the Host B previously obtained.</para>
+      lease for the temporarily assigned address that the Host B previously
+      obtained.</para>
 
       <para>This recovery will succeed, even if other hosts will attempt to get
       the reserved address. Had the Host C requested address 192.0.2.10 after

src/bin/dhcp4/tests/direct_client_unittest.cc

@@ -307,10 +307,6 @@ TEST_F(DirectClientTest, renew) {
     ASSERT_NO_THROW(IfaceMgr::instance().openSockets4());
     // Add a subnet.
     ASSERT_NO_FATAL_FAILURE(configureSubnet("10.0.0.0"));
-    // Make sure that the subnet has been really added. Also, the subnet
-    // will be needed to create a lease for a client.
-    Subnet4Ptr subnet = CfgMgr::instance().getCurrentCfg()->
-        getCfgSubnets4()->selectSubnet(IOAddress("10.0.0.10"));
 
     // Create the DHCPv4 client.
     Dhcp4Client client;
@@ -343,10 +339,6 @@ TEST_F(DirectClientTest, rebind) {
     ASSERT_NO_THROW(IfaceMgr::instance().openSockets4());
     // Add a subnet.
     ASSERT_NO_FATAL_FAILURE(configureSubnet("10.0.0.0"));
-    // Make sure that the subnet has been really added. Also, the subnet
-    // will be needed to create a lease for a client.
-    Subnet4Ptr subnet = CfgMgr::instance().getCurrentCfg()->
-        getCfgSubnets4()->selectSubnet(IOAddress("10.0.0.10"));
 
     // Create the DHCPv4 client.
     Dhcp4Client client;

src/bin/dhcp4/tests/dora_unittest.cc

@@ -634,7 +634,8 @@ TEST_F(DORATest, reservationsWithConflicts) {
                                    IOAddress>(new IOAddress("0.0.0.0"))));
     ASSERT_TRUE(clientB.getContext().response_);
     ASSERT_EQ(DHCPACK, static_cast<int>(clientB.getContext().response_->getType()));
-    ASSERT_NE(clientB.config_.lease_.addr_, in_pool_addr);
+    IOAddress client_b_addr = clientB.config_.lease_.addr_;
+    ASSERT_NE(client_b_addr, in_pool_addr);
 
     // Client A renews the lease.
     client.setState(Dhcp4Client::RENEWING);
@@ -657,6 +658,7 @@ TEST_F(DORATest, reservationsWithConflicts) {
     ASSERT_TRUE(clientB.getContext().response_);
     ASSERT_EQ(DHCPACK, static_cast<int>(clientB.getContext().response_->getType()));
     ASSERT_NE(clientB.config_.lease_.addr_, in_pool_addr);
+    ASSERT_EQ(client_b_addr, clientB.config_.lease_.addr_);
 
     // Client B renews its lease.
     clientB.setState(Dhcp4Client::RENEWING);
@@ -667,6 +669,7 @@ TEST_F(DORATest, reservationsWithConflicts) {
     ASSERT_TRUE(clientB.getContext().response_);
     EXPECT_EQ(DHCPACK, static_cast<int>(clientB.getContext().response_->getType()));
     ASSERT_NE(clientB.config_.lease_.addr_, in_pool_addr);
+    ASSERT_EQ(client_b_addr, clientB.config_.lease_.addr_);
 
     // Client A performs 4-way exchange.
     client.setState(Dhcp4Client::SELECTING);
@@ -681,7 +684,7 @@ TEST_F(DORATest, reservationsWithConflicts) {
     ASSERT_EQ(DHCPACK, static_cast<int>(resp->getType()));
     // The server should have assigned a different address than the one
     // reserved for the Client B.
-    ASSERT_NE(client.config_.lease_.addr_.toText(), in_pool_addr.toText());
+    ASSERT_NE(client.config_.lease_.addr_, in_pool_addr);
     subnet = CfgMgr::instance().getCurrentCfg()->getCfgSubnets4()->
         selectSubnet(client.config_.lease_.addr_);
     ASSERT_TRUE(subnet);

src/lib/asiolink/tests/io_address_unittest.cc

@@ -120,19 +120,34 @@ TEST(IOAddressTest, isV4) {
 }
 
 TEST(IOAddressTest, isV4Zero) {
+    // 0.0.0.0
     const IOAddress address_zero("0.0.0.0");
-    const IOAddress address_non_zero("192.0.2.3");
-
     EXPECT_TRUE(address_zero.isV4Zero());
+    // 192.0.2.3
+    const IOAddress address_non_zero("192.0.2.3");
     EXPECT_FALSE(address_non_zero.isV4Zero());
+    // 0.0.0.100
+    const IOAddress address_non_zero1("0.0.0.100");
+    EXPECT_FALSE(address_non_zero1.isV4Zero());
+    // 64.0.0.0
+    const IOAddress address_non_zero2("64.0.0.0");
+    EXPECT_FALSE(address_non_zero2.isV4Zero());
 }
 
 TEST(IOAddressTest, isV4Bcast) {
+    // 255.255.255.255
     const IOAddress address_bcast("255.255.255.255");
-    const IOAddress address_non_bcast("10.2.3.4");
-
     EXPECT_TRUE(address_bcast.isV4Bcast());
+    // 10.2.3.4
+    const IOAddress address_non_bcast("10.2.3.4");
     EXPECT_FALSE(address_non_bcast.isV4Bcast());
+    // 255.255.255.23
+    const IOAddress address_non_bcast1("255.255.255.23");
+    EXPECT_FALSE(address_non_bcast1.isV4Bcast());
+    // 123.255.255.255
+    const IOAddress address_non_bcast2("123.255.255.255");
+    EXPECT_FALSE(address_non_bcast2.isV4Bcast());
+
 }
 
 TEST(IOAddressTest, isV6) {
@@ -144,11 +159,18 @@ TEST(IOAddressTest, isV6) {
 }
 
 TEST(IOAddressTest, isV6Zero) {
+    // ::
     const IOAddress address_zero("::");
-    const IOAddress address_non_zero("0.0.0.0");
-
     EXPECT_TRUE(address_zero.isV6Zero());
+    // 0.0.0.0
+    const IOAddress address_non_zero("0.0.0.0");
     EXPECT_FALSE(address_non_zero.isV6Zero());
+    // ::ff
+    const IOAddress address_non_zero1("::ff");
+    EXPECT_FALSE(address_non_zero1.isV6Zero());
+    // ff::
+    const IOAddress address_non_zero2("::ff");
+    EXPECT_FALSE(address_non_zero2.isV6Zero());
 }
 
 TEST(IOAddressTest, uint32) {

src/lib/dhcpsrv/alloc_engine.cc

@@ -288,6 +288,37 @@ AllocEngine::AllocatorPtr AllocEngine::getAllocator(Lease::Type type) {
 // #    DHCPv6 lease allocation code starts here.
 // ##########################################################################
 
+AllocEngine::ClientContext6::ClientContext6()
+    : subnet_(), duid_(), iaid_(0), type_(Lease::TYPE_NA), hwaddr_(),
+      hints_(), fwd_dns_update_(false), rev_dns_update_(false), hostname_(""),
+      callout_handle_(), fake_allocation_(false), old_leases_(), host_(),
+      query_(), ia_rsp_(), allow_new_leases_in_renewals_(false) {
+}
+
+AllocEngine::ClientContext6::ClientContext6(const Subnet6Ptr& subnet, const DuidPtr& duid,
+                                            const uint32_t iaid,
+                                            const isc::asiolink::IOAddress& hint,
+                                            const Lease::Type type, const bool fwd_dns,
+                                            const bool rev_dns,
+                                            const std::string& hostname,
+                                            const bool fake_allocation):
+    subnet_(subnet), duid_(duid), iaid_(iaid), type_(type), hwaddr_(),
+    hints_(), fwd_dns_update_(fwd_dns), rev_dns_update_(rev_dns),
+    hostname_(hostname), fake_allocation_(fake_allocation),
+    old_leases_(), host_(), query_(), ia_rsp_(),
+    allow_new_leases_in_renewals_(false) {
+
+    static asiolink::IOAddress any("::");
+
+    if (hint != any) {
+        hints_.push_back(std::make_pair(hint, 128));
+    }
+    // callout_handle, host pointers initiated to NULL by their
+    // respective constructors.
+}
+
+
+
 Lease6Collection
 AllocEngine::allocateLeases6(ClientContext6& ctx) {
 
@@ -1177,7 +1208,7 @@ addressReserved(const IOAddress& address, const AllocEngine::ClientContext4& ctx
             return (host_hwaddr->hwaddr_ != ctx.hwaddr_->hwaddr_);
 
         } else {
-            return (true);
+            return (false);
 
         }
     }
@@ -1776,16 +1807,6 @@ AllocEngine::allocateUnreservedLease4(ClientContext4& ctx) {
 void
 AllocEngine::updateLease4Information(const Lease4Ptr& lease,
                                      AllocEngine::ClientContext4& ctx) const {
-    // This should not happen in theory.
-    if (!lease) {
-        isc_throw(BadValue, "null lease specified for updateLease4Information");
-    }
-
-    if (!ctx.subnet_) {
-        isc_throw(BadValue, "null subnet specified for"
-                  " updateLease4Information");
-    }
-
     lease->subnet_id_ = ctx.subnet_->getID();
     lease->hwaddr_ = ctx.hwaddr_;
     lease->client_id_ = ctx.clientid_;

src/lib/dhcpsrv/alloc_engine.h

@@ -202,7 +202,7 @@ protected:
                     const isc::asiolink::IOAddress& hint);
     };
 
-    public:
+public:
 
     /// @brief specifies allocation type
     typedef enum {
@@ -391,12 +391,7 @@ public:
         bool allow_new_leases_in_renewals_;
 
         /// @brief Default constructor.
-        ClientContext6()
-           : subnet_(), duid_(), iaid_(0), type_(Lease::TYPE_NA), hwaddr_(),
-             hints_(), fwd_dns_update_(false), rev_dns_update_(false), hostname_(""),
-             callout_handle_(), fake_allocation_(false), old_leases_(), host_(),
-             query_(), ia_rsp_(), allow_new_leases_in_renewals_(false) {
-        }
+        ClientContext6();
 
         /// @brief Constructor with parameters.
         ///
@@ -422,21 +417,7 @@ public:
                        const uint32_t iaid, const isc::asiolink::IOAddress& hint,
                        const Lease::Type type, const bool fwd_dns, const bool
                        rev_dns, const std::string& hostname, const bool
-                       fake_allocation):
-            subnet_(subnet), duid_(duid), iaid_(iaid), type_(type), hwaddr_(),
-            hints_(), fwd_dns_update_(fwd_dns), rev_dns_update_(rev_dns),
-            hostname_(hostname), fake_allocation_(fake_allocation),
-            old_leases_(), host_(), query_(), ia_rsp_(),
-            allow_new_leases_in_renewals_(false){
-
-            static asiolink::IOAddress any("::");
-
-            if (hint != any) {
-                hints_.push_back(std::make_pair(hint, 128));
-            }
-            // callout_handle, host pointers initiated to NULL by their
-            // respective constructors.
-        }
+                       fake_allocation);
     };
 
     /// @brief Allocates IPv6 leases for a given IA container
@@ -853,15 +834,19 @@ public:
     /// dynamic pool or even an address currently allocated for this client.
     ///
     /// It is possible that the address reserved for the particular client
-    /// is in use by another client, e.g. as a result of pools reconfigruation.
+    /// is in use by another client, e.g. as a result of pools reconfiguration.
     /// In this case, when the client requests allocation of the reserved
     /// address and the server determines that it is leased to someone else,
-    /// the allocation engine doesn't allocate a lease for the client having
-    /// a reservation. When the client having a lease returns to renew, the
-    /// allocation engine doesn't extend the lease for it and returns a NULL
-    /// pointer. The client falls back to the 4-way exchange and a different
-    /// lease is allocated. At this point, the reserved address is freed and
-    /// can be allocated to the client which holds this reservation.
+    /// the allocation engine allocates a different address for this client.
+    ///
+    /// When the client having a lease returns to renew, the allocation engine
+    /// doesn't extend the lease for it and returns a NULL pointer. The client
+    /// falls back to the 4-way exchange and a different lease is allocated.
+    /// At this point, the reserved address is freed and can be allocated to
+    /// the client which holds this reservation. However, this client has a
+    /// lease for a different address at this time. When the client renews its
+    /// lease it receives the DHCPNAK and falls back to the DHCP server
+    /// discovery and obtains the lease for the reserved address.
     ///
     /// When a server should do DNS updates, it is required that allocation
     /// returns the information about how the lease was obtained by the allocation
@@ -1096,6 +1081,10 @@ private:
     ///
     /// Note that this doesn't update the lease address.
     ///
+    /// @warning This method doesn't check if the pointer to the lease is
+    /// valid nor if the subnet to the pointer in the @c ctx is valid.
+    /// The caller is responsible for making sure that they are valid.
+    ///
     /// @param [out] lease A pointer to the lease to be updated.
     /// @param ctx A context containing information from the server about the
     /// client and its message.

src/lib/dhcpsrv/dhcpsrv_messages.mes

@@ -183,8 +183,7 @@ the new parameters.
 This warning message is issued when the DHCP server finds that the
 address reserved for the client can't be offered because this address
 is currently allocated to another client. The server will try to allocate
-a different (temporary) address to the client to use until the conflict
-is resolved.
+a different address to the client to use until the conflict is resolved.
 
 % DHCPSRV_DHCP_DDNS_ERROR_EXCEPTION error handler for DHCP_DDNS IO generated an expected exception: %1
 This is an error message that occurs when an attempt to send a request to

src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc

@@ -546,37 +546,44 @@ TEST_F(AllocEngine4Test, requestReuseExpiredLease4) {
     EXPECT_TRUE(*old_lease_ == original_lease);
 }
 
-/// @todo write renewLease6
-
+// This test checks that when the client requests the address which belongs
+// to another client, the allocation engine returns NULL (for the
+// DHCPREQUEST case) or a lease for the address which belongs to this
+// client (DHCPDISCOVER case).
 TEST_F(AllocEngine4Test, requestOtherClientLease) {
+    // Create the first lease.
     Lease4Ptr lease(new Lease4(IOAddress("192.0.2.101"), hwaddr_, 0, 0,
                                100, 30, 60, time(NULL), subnet_->getID(),
                                false, false, ""));
-
+    // Create the second lease.
     Lease4Ptr lease2(new Lease4(IOAddress("192.0.2.102"), hwaddr2_, 0, 0,
                                100, 30, 60, time(NULL), subnet_->getID(),
                                false, false, ""));
-
+    // Add leases for both clients to the Lease Manager.
     LeaseMgrFactory::instance().addLease(lease);
     LeaseMgrFactory::instance().addLease(lease2);
 
     AllocEngine engine(AllocEngine::ALLOC_ITERATIVE, 100, false);
 
+    // First client requests the lease which belongs to the second client.
     Lease4Ptr new_lease = engine.allocateLease4(subnet_, clientid_, hwaddr_,
                                                 IOAddress("192.0.2.102"),
                                                 false, false, "",
                                                 false, CalloutHandlePtr(),
                                                 old_lease_);
-
+    // Allocation engine should return NULL.
     ASSERT_FALSE(new_lease);
 
+    // Now simulate the DHCPDISCOVER case when the provided address is
+    // treated as a hint. The engine should return a lease for a
+    // different address than requested.
     new_lease = engine.allocateLease4(subnet_, clientid_, hwaddr_,
                                       IOAddress("192.0.2.102"),
                                       false, false, "",
                                       true, CalloutHandlePtr(),
                                       old_lease_);
     ASSERT_TRUE(new_lease);
-
+    EXPECT_EQ("192.0.2.101", new_lease->addr_.toText());
 }
 
 // This test checks the behavior of the allocation engine in the following
@@ -880,7 +887,8 @@ TEST_F(AllocEngine4Test, reservedAddressHijackedFakeAllocation) {
                                                       false, false, "",
                                                       true, CalloutHandlePtr(),
                                                       old_lease_);
-    // The allocation engine should return no lease.
+    // The allocation engine should return a lease but for a different address
+    // than requested because this address is in use.
     ASSERT_TRUE(allocated_lease);
     EXPECT_NE(allocated_lease->addr_.toText(), "192.0.2.123");
     EXPECT_TRUE(subnet_->inPool(Lease::TYPE_V4, allocated_lease->addr_));
@@ -1074,8 +1082,8 @@ TEST_F(AllocEngine4Test, reservedAddressExistingLeaseNoHint) {
 // - Client has a lease for a different address than reserved.
 // - Client sends a DHCPDISCOVER with no hint.
 // - Server determines that there is a reservation for the client and that
-// the current lease should be removed and the reserved address should be
-// allocated.
+//   the reserved address should be offered when the client sends a
+//   DHCPDISCOVER.
 TEST_F(AllocEngine4Test, reservedAddressExistingLeaseNoHintFakeAllocation) {
     // Create a reservation.
     HostPtr host(new Host(&hwaddr_->hwaddr_[0], hwaddr_->hwaddr_.size(),
@@ -1113,6 +1121,8 @@ TEST_F(AllocEngine4Test, reservedAddressExistingLeaseNoHintFakeAllocation) {
     Lease4Ptr from_mgr = LeaseMgrFactory::instance().getLease4(lease->addr_);
     ASSERT_TRUE(from_mgr);
     detailCompareLease(lease, from_mgr);
+
+
 }
 
 // This test checks that the behavior of the allocation engine in the following
@@ -1242,6 +1252,10 @@ TEST_F(AllocEngine4Test, reservedAddressVsDynamicPool) {
                                                       old_lease_);
     ASSERT_TRUE(allocated_lease);
     EXPECT_NE(allocated_lease->addr_.toText(), "192.0.2.100");
+
+    Lease4Ptr from_mgr = LeaseMgrFactory::instance().getLease4(allocated_lease->addr_);
+    ASSERT_TRUE(from_mgr);
+    detailCompareLease(allocated_lease, from_mgr);
 }
 
 // This test checks that the client requesting an address which is