From 49fe9fbf351ed7139e48d006ecc558e15d8f085b Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Thu, 21 Oct 2021 11:14:38 -0400 Subject: [PATCH] [#1584] Addressed minor review comments modified: src/bin/dhcp4/dhcp4_srv.cc src/bin/dhcp4/tests/dora_unittest.cc src/bin/dhcp4/tests/hooks_unittest.cc src/lib/dhcpsrv/alloc_engine.cc --- src/bin/dhcp4/dhcp4_srv.cc | 8 ++++---- src/bin/dhcp4/tests/dora_unittest.cc | 4 ++-- src/bin/dhcp4/tests/hooks_unittest.cc | 7 +++---- src/lib/dhcpsrv/alloc_engine.cc | 4 ++-- 4 files changed, 11 insertions(+), 12 deletions(-) diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc index e3ffbae559..ed3b56baa4 100644 --- a/src/bin/dhcp4/dhcp4_srv.cc +++ b/src/bin/dhcp4/dhcp4_srv.cc @@ -2578,7 +2578,7 @@ Dhcpv4Srv::assignLease(Dhcpv4Exchange& ex) { // Address might have been rejected via class guard (i.e. not // allowed for this client). We need to determine if we truly // do not know about the address or whether this client just - // isn't allowed to have that address. We should only NAK + // isn't allowed to have that address. We should only DHCPNAK // For the latter. while (s) { if (s->inPool(Lease::TYPE_V4, hint)) { @@ -2589,7 +2589,7 @@ Dhcpv4Srv::assignLease(Dhcpv4Exchange& ex) { } // If we didn't find a subnet, it's not an address we know about - // so we drop the NAK. + // so we drop the DHCPNAK. if (!s) { LOG_DEBUG(bad_packet4_logger, DBG_DHCP4_DETAIL, DHCP4_UNKNOWN_ADDRESS_REQUESTED) @@ -2886,9 +2886,9 @@ Dhcpv4Srv::adjustRemoteAddr(Dhcpv4Exchange& ex) { } else if (!query->getCiaddr().isV4Zero()) { response->setRemoteAddr(query->getCiaddr()); - // We can't unicast the response to the client when sending NAK, + // We can't unicast the response to the client when sending DHCPNAK, // because we haven't allocated address for him. Therefore, - // NAK is broadcast. + // DHCPNAK is broadcast. } else if (response->getType() == DHCPNAK) { response->setRemoteAddr(IOAddress::IPV4_BCAST_ADDRESS()); diff --git a/src/bin/dhcp4/tests/dora_unittest.cc b/src/bin/dhcp4/tests/dora_unittest.cc index 78df3176e9..6fa77c5e05 100644 --- a/src/bin/dhcp4/tests/dora_unittest.cc +++ b/src/bin/dhcp4/tests/dora_unittest.cc @@ -2001,10 +2001,10 @@ DORATest::reservationsWithConflicts() { ASSERT_NO_THROW(client.doRequest()); // The reservation has been removed. Since address that the client is - // using doesn't belong a dynamic pool and the server is not + // using doesn't belong to a dynamic pool and the server is not // authoritative it should not send a DHCPNAK. resp = client.getContext().response_; - ASSERT_FALSE(client.getContext().response_); + ASSERT_FALSE(resp); // A conforming client would go back to the server discovery. client.setState(Dhcp4Client::SELECTING); diff --git a/src/bin/dhcp4/tests/hooks_unittest.cc b/src/bin/dhcp4/tests/hooks_unittest.cc index c30b709323..6733e87c50 100644 --- a/src/bin/dhcp4/tests/hooks_unittest.cc +++ b/src/bin/dhcp4/tests/hooks_unittest.cc @@ -2040,14 +2040,13 @@ TEST_F(HooksDhcpv4SrvTest, leases4CommittedRequest) { resetCalloutBuffers(); - // Now request an address that can't be allocated. The client should receive - // the DHCPNAK. + // Now request an address that can't be allocated. client.ciaddr_ = IOAddress("10.0.0.1"); ASSERT_NO_THROW(client.doRequest()); - // Make sure that we did not receive a response. If we're - // not authoritative, there should not be a NAK. + // Make sure that we did not receive a response. Since we're + // not authoritative, there should not be a DHCPNAK. ASSERT_FALSE(client.getContext().response_); // Check that no callback was not called. diff --git a/src/lib/dhcpsrv/alloc_engine.cc b/src/lib/dhcpsrv/alloc_engine.cc index 0829c96ca5..3c0e870b1f 100644 --- a/src/lib/dhcpsrv/alloc_engine.cc +++ b/src/lib/dhcpsrv/alloc_engine.cc @@ -3600,7 +3600,7 @@ AllocEngine::findGlobalReservation(ClientContext4& ctx) { Lease4Ptr AllocEngine::discoverLease4(AllocEngine::ClientContext4& ctx) { - // Find an existing lease for this client. This function will return true + // Find an existing lease for this client. This function will return null // if there is a conflict with existing lease and the allocation should // not be continued. Lease4Ptr client_lease; @@ -3711,7 +3711,7 @@ AllocEngine::discoverLease4(AllocEngine::ClientContext4& ctx) { Lease4Ptr AllocEngine::requestLease4(AllocEngine::ClientContext4& ctx) { - // Find an existing lease for this client. This function will return true + // Find an existing lease for this client. This function will return null // if there is a conflict with existing lease and the allocation should // not be continued. Lease4Ptr client_lease;