From: Tomek Mrugalski Date: Thu, 10 Oct 2013 16:01:41 +0000 (+0200) Subject: [3153] Changes after review: X-Git-Tag: bind10-1.2.0beta1-release~189^2~1 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=6489d09af85dddff538944f6efab14783a46fe9c;p=thirdparty%2Fkea.git [3153] Changes after review: - comments updated - whitespaces cleaned up - minor refactoring in DHCPv6 Srv --- diff --git a/src/bin/dhcp6/dhcp6_srv.cc b/src/bin/dhcp6/dhcp6_srv.cc index a07b31333b..d9155b33cf 100644 --- a/src/bin/dhcp6/dhcp6_srv.cc +++ b/src/bin/dhcp6/dhcp6_srv.cc @@ -233,7 +233,7 @@ bool Dhcpv6Srv::run() { // The packet has just been received so contains the uninterpreted wire // data; execute callouts registered for buffer6_receive. - if (HooksManager::getHooksManager().calloutsPresent(Hooks.hook_index_buffer6_receive_)) { + if (HooksManager::calloutsPresent(Hooks.hook_index_buffer6_receive_)) { CalloutHandlePtr callout_handle = getCalloutHandle(query); // Delete previously set arguments @@ -276,7 +276,7 @@ bool Dhcpv6Srv::run() { // At this point the information in the packet has been unpacked into // the various packet fields and option objects has been cretated. // Execute callouts registered for packet6_receive. - if (HooksManager::getHooksManager().calloutsPresent(Hooks.hook_index_pkt6_receive_)) { + if (HooksManager::calloutsPresent(Hooks.hook_index_pkt6_receive_)) { CalloutHandlePtr callout_handle = getCalloutHandle(query); // Delete previously set arguments @@ -381,7 +381,7 @@ bool Dhcpv6Srv::run() { // Options are represented by individual objects, but the // output wire data has not been prepared yet. // Execute all callouts registered for packet6_send - if (HooksManager::getHooksManager().calloutsPresent(Hooks.hook_index_pkt6_send_)) { + if (HooksManager::calloutsPresent(Hooks.hook_index_pkt6_send_)) { CalloutHandlePtr callout_handle = getCalloutHandle(query); // Delete all previous arguments @@ -425,7 +425,7 @@ bool Dhcpv6Srv::run() { // Option objects modification does not make sense anymore. Hooks // can only manipulate wire buffer at this stage. // Let's execute all callouts registered for buffer6_send - if (HooksManager::getHooksManager().calloutsPresent(Hooks.hook_index_buffer6_send_)) { + if (HooksManager::calloutsPresent(Hooks.hook_index_buffer6_send_)) { CalloutHandlePtr callout_handle = getCalloutHandle(query); // Delete previously set arguments @@ -777,7 +777,7 @@ Dhcpv6Srv::selectSubnet(const Pkt6Ptr& question) { } // Let's execute all callouts registered for subnet6_receive - if (HooksManager::getHooksManager().calloutsPresent(Hooks.hook_index_subnet6_select_)) { + if (HooksManager::calloutsPresent(Hooks.hook_index_subnet6_select_)) { CalloutHandlePtr callout_handle = getCalloutHandle(question); // We're reusing callout_handle from previous calls @@ -1523,7 +1523,7 @@ Dhcpv6Srv::renewIA_NA(const Subnet6Ptr& subnet, const DuidPtr& duid, bool skip = false; // Execute all callouts registered for packet6_send - if (HooksManager::getHooksManager().calloutsPresent(Hooks.hook_index_lease6_renew_)) { + if (HooksManager::calloutsPresent(Hooks.hook_index_lease6_renew_)) { CalloutHandlePtr callout_handle = getCalloutHandle(query); // Delete all previous arguments @@ -1566,11 +1566,12 @@ Dhcpv6Srv::renewIA_NA(const Subnet6Ptr& subnet, const DuidPtr& duid, OptionPtr Dhcpv6Srv::renewIA_PD(const Subnet6Ptr& subnet, const DuidPtr& duid, const Pkt6Ptr& query, boost::shared_ptr ia) { - if (!subnet) { - // There's no subnet select for this client. There's nothing to renew. - boost::shared_ptr ia_rsp(new Option6IA(D6O_IA_PD, ia->getIAID())); - // Insert status code NoAddrsAvail. + // Let's create a IA_NA response and fill it in later + boost::shared_ptr ia_rsp(new Option6IA(D6O_IA_PD, ia->getIAID())); + + if (!subnet) { + // Insert status code NoBinding ia_rsp->addOption(createStatusCode(STATUS_NoBinding, "Sorry, no known leases for this duid/iaid.")); @@ -1586,12 +1587,9 @@ Dhcpv6Srv::renewIA_PD(const Subnet6Ptr& subnet, const DuidPtr& duid, subnet->getID()); if (!lease) { - // client renewing a lease that we don't know about. + // Client is renewing a lease that we don't know about. - // Create empty IA_NA option with IAID matching the request. - boost::shared_ptr ia_rsp(new Option6IA(D6O_IA_PD, ia->getIAID())); - - // Insert status code NoAddrsAvail. + // Insert status code NoBinding ia_rsp->addOption(createStatusCode(STATUS_NoBinding, "Sorry, no known leases for this duid/iaid.")); @@ -1603,29 +1601,29 @@ Dhcpv6Srv::renewIA_PD(const Subnet6Ptr& subnet, const DuidPtr& duid, return (ia_rsp); } - // Keep the old data in case the callout tells us to skip update + // Keep the old data in case the callout tells us to skip update. Lease6 old_data = *lease; + // Do the actual lease update lease->preferred_lft_ = subnet->getPreferred(); lease->valid_lft_ = subnet->getValid(); lease->t1_ = subnet->getT1(); lease->t2_ = subnet->getT2(); lease->cltt_ = time(NULL); - // Create empty IA_NA option with IAID matching the request. - boost::shared_ptr ia_rsp(new Option6IA(D6O_IA_PD, ia->getIAID())); - + // Also update IA_PD container with proper T1, T2 values ia_rsp->setT1(subnet->getT1()); ia_rsp->setT2(subnet->getT2()); boost::shared_ptr - prefix(new Option6IAPrefix(D6O_IAPREFIX, lease->addr_, lease->prefixlen_, - lease->preferred_lft_, lease->valid_lft_)); + prefix(new Option6IAPrefix(D6O_IAPREFIX, lease->addr_, + lease->prefixlen_, lease->preferred_lft_, + lease->valid_lft_)); ia_rsp->addOption(prefix); bool skip = false; // Execute all callouts registered for packet6_send - if (HooksManager::getHooksManager().calloutsPresent(Hooks.hook_index_lease6_renew_)) { + if (HooksManager::calloutsPresent(Hooks.hook_index_lease6_renew_)) { CalloutHandlePtr callout_handle = getCalloutHandle(query); // Delete all previous arguments @@ -1641,24 +1639,25 @@ Dhcpv6Srv::renewIA_PD(const Subnet6Ptr& subnet, const DuidPtr& duid, callout_handle->setArgument("ia_pd", ia_rsp); // Call all installed callouts - HooksManager::callCallouts(Hooks.hook_index_lease6_renew_, *callout_handle); + HooksManager::callCallouts(Hooks.hook_index_lease6_renew_, + *callout_handle); - // Callouts decided to skip the next processing step. The next - // processing step would to actually renew the lease, so skip at this - // stage means "keep the old lease as it is". - if (callout_handle->getSkip()) { - skip = true; - LOG_DEBUG(dhcp6_logger, DBG_DHCP6_HOOKS, DHCP6_HOOK_LEASE6_RENEW_SKIP); - } + // Remember hook's instruction whether we want to skip update or not + skip = callout_handle->getSkip(); } if (!skip) { LeaseMgrFactory::instance().updateLease6(lease); } else { + // Callouts decided to skip the next processing step. The next + // processing step would to actually renew the lease, so skip at this + // stage means "keep the old lease as it is". + LOG_DEBUG(dhcp6_logger, DBG_DHCP6_HOOKS, DHCP6_HOOK_LEASE6_RENEW_SKIP); + // Copy back the original date to the lease. For MySQL it doesn't make // much sense, but for memfile, the Lease6Ptr points to the actual lease - // in memfile, so the actual update is performed when we manipulate fields - // of returned Lease6Ptr, the actual updateLease6() is no-op. + // in memfile, so the actual update is performed when we manipulate + // fields of returned Lease6Ptr, the actual updateLease6() is no-op. *lease = old_data; } @@ -1709,6 +1708,7 @@ Dhcpv6Srv::renewLeases(const Pkt6Ptr& renew, Pkt6Ptr& reply, for (Option::OptionCollection::iterator opt = renew->options_.begin(); opt != renew->options_.end(); ++opt) { switch (opt->second->getType()) { + case D6O_IA_NA: { OptionPtr answer_opt = renewIA_NA(subnet, duid, renew, boost::dynamic_pointer_cast< @@ -1719,6 +1719,7 @@ Dhcpv6Srv::renewLeases(const Pkt6Ptr& renew, Pkt6Ptr& reply, } break; } + case D6O_IA_PD: { OptionPtr answer_opt = renewIA_PD(subnet, duid, renew, boost::dynamic_pointer_cast< @@ -1765,6 +1766,11 @@ Dhcpv6Srv::releaseLeases(const Pkt6Ptr& release, Pkt6Ptr& reply) { } DuidPtr duid(new DUID(opt_duid->getData())); + // Let's set the status to be success by default. We can override it with + // error status if needed. The important thing to understand here is that + // the global status code may be set to success only if all IA options were + // handled properly. Therefore the releaseIA_NA and releaseIA_PD options + // may turn the status code to some error, but can't turn it back to success. int general_status = STATUS_Success; for (Option::OptionCollection::iterator opt = release->options_.begin(); opt != release->options_.end(); ++opt) { @@ -1818,7 +1824,7 @@ Dhcpv6Srv::releaseIA_NA(const DuidPtr& duid, const Pkt6Ptr& query, (ia->getOption(D6O_IAADDR)); if (!release_addr) { ia_rsp->addOption(createStatusCode(STATUS_NoBinding, - "You did not include address in your RELEASE")); + "You did not include an address in your RELEASE")); general_status = STATUS_NoBinding; return (ia_rsp); } @@ -1887,7 +1893,7 @@ Dhcpv6Srv::releaseIA_NA(const DuidPtr& duid, const Pkt6Ptr& query, bool skip = false; // Execute all callouts registered for packet6_send - if (HooksManager::getHooksManager().calloutsPresent(Hooks.hook_index_lease6_release_)) { + if (HooksManager::calloutsPresent(Hooks.hook_index_lease6_release_)) { CalloutHandlePtr callout_handle = getCalloutHandle(query); // Delete all previous arguments @@ -1962,14 +1968,15 @@ Dhcpv6Srv::releaseIA_PD(const DuidPtr& duid, const Pkt6Ptr& query, // // This method implements approach 1. - // That's our response + // That's our response. We will fill it in as we check the lease to be + // released. boost::shared_ptr ia_rsp(new Option6IA(D6O_IA_PD, ia->getIAID())); - boost::shared_ptr release_prefix = boost::dynamic_pointer_cast - (ia->getOption(D6O_IAPREFIX)); + boost::shared_ptr release_prefix = + boost::dynamic_pointer_cast(ia->getOption(D6O_IAPREFIX)); if (!release_prefix) { ia_rsp->addOption(createStatusCode(STATUS_NoBinding, - "You did not include address in your RELEASE")); + "You did not include a prefix in your RELEASE")); general_status = STATUS_NoBinding; return (ia_rsp); } @@ -1978,9 +1985,9 @@ Dhcpv6Srv::releaseIA_PD(const DuidPtr& duid, const Pkt6Ptr& query, release_prefix->getAddress()); if (!lease) { - // client releasing a lease that we don't know about. + // Client releasing a lease that we don't know about. - // Insert status code NoAddrsAvail. + // Insert status code NoBinding. ia_rsp->addOption(createStatusCode(STATUS_NoBinding, "Sorry, no known leases for this duid/iaid, can't release.")); general_status = STATUS_NoBinding; @@ -1996,7 +2003,6 @@ Dhcpv6Srv::releaseIA_PD(const DuidPtr& duid, const Pkt6Ptr& query, // Something is gravely wrong here. We do have a lease, but it does not // have mandatory DUID information attached. Someone was messing with our // database. - LOG_ERROR(dhcp6_logger, DHCP6_LEASE_PD_WITHOUT_DUID) .arg(release_prefix->getAddress().toText()); @@ -2008,7 +2014,6 @@ Dhcpv6Srv::releaseIA_PD(const DuidPtr& duid, const Pkt6Ptr& query, if (*duid != *(lease->duid_)) { // Sorry, it's not your address. You can't release it. - LOG_INFO(dhcp6_logger, DHCP6_RELEASE_PD_FAIL_WRONG_DUID) .arg(duid->toText()) .arg(release_prefix->getAddress().toText()) @@ -2038,7 +2043,7 @@ Dhcpv6Srv::releaseIA_PD(const DuidPtr& duid, const Pkt6Ptr& query, bool skip = false; // Execute all callouts registered for packet6_send - if (HooksManager::getHooksManager().calloutsPresent(Hooks.hook_index_lease6_release_)) { + if (HooksManager::calloutsPresent(Hooks.hook_index_lease6_release_)) { CalloutHandlePtr callout_handle = getCalloutHandle(query); // Delete all previous arguments @@ -2053,13 +2058,7 @@ Dhcpv6Srv::releaseIA_PD(const DuidPtr& duid, const Pkt6Ptr& query, // Call all installed callouts HooksManager::callCallouts(Hooks.hook_index_lease6_release_, *callout_handle); - // Callouts decided to skip the next processing step. The next - // processing step would to send the packet, so skip at this - // stage means "drop response". - if (callout_handle->getSkip()) { - skip = true; - LOG_DEBUG(dhcp6_logger, DBG_DHCP6_HOOKS, DHCP6_HOOK_LEASE6_RELEASE_PD_SKIP); - } + skip = callout_handle->getSkip(); } // Ok, we've passed all checks. Let's release this prefix. @@ -2067,6 +2066,11 @@ Dhcpv6Srv::releaseIA_PD(const DuidPtr& duid, const Pkt6Ptr& query, if (!skip) { success = LeaseMgrFactory::instance().deleteLease(lease->addr_); + } else { + // Callouts decided to skip the next processing step. The next + // processing step would to send the packet, so skip at this + // stage means "drop response". + LOG_DEBUG(dhcp6_logger, DBG_DHCP6_HOOKS, DHCP6_HOOK_LEASE6_RELEASE_PD_SKIP); } // Here the success should be true if we removed lease successfully @@ -2081,8 +2085,6 @@ Dhcpv6Srv::releaseIA_PD(const DuidPtr& duid, const Pkt6Ptr& query, .arg(duid->toText()) .arg(lease->iaid_); general_status = STATUS_UnspecFail; - - return (ia_rsp); } else { LOG_DEBUG(dhcp6_logger, DBG_DHCP6_DETAIL, DHCP6_RELEASE_PD) .arg(lease->addr_.toText()) @@ -2091,9 +2093,9 @@ Dhcpv6Srv::releaseIA_PD(const DuidPtr& duid, const Pkt6Ptr& query, ia_rsp->addOption(createStatusCode(STATUS_Success, "Lease released. Thank you, please come again.")); - - return (ia_rsp); } + + return (ia_rsp); } Pkt6Ptr diff --git a/src/bin/dhcp6/tests/dhcp6_test_utils.cc b/src/bin/dhcp6/tests/dhcp6_test_utils.cc index 7646ca3e6f..93b6e44605 100644 --- a/src/bin/dhcp6/tests/dhcp6_test_utils.cc +++ b/src/bin/dhcp6/tests/dhcp6_test_utils.cc @@ -295,10 +295,10 @@ Dhcpv6SrvTest::testRenewReject(Lease::Type type, const IOAddress& addr) { checkResponse(reply, DHCPV6_REPLY, transid); OptionPtr tmp = reply->getOption(code); ASSERT_TRUE(tmp); - // Check that IA_?? was returned and that there's an address included + + // Check that IA_?? was returned and that there's proper status code boost::shared_ptr ia = boost::dynamic_pointer_cast(tmp); ASSERT_TRUE(ia); - checkIA_NAStatusCode(ia, STATUS_NoBinding); // Check that there is no lease added @@ -321,7 +321,8 @@ Dhcpv6SrvTest::testRenewReject(Lease::Type type, const IOAddress& addr) { checkResponse(reply, DHCPV6_REPLY, transid); tmp = reply->getOption(code); ASSERT_TRUE(tmp); - // Check that IA_NA was returned and that there's an address included + + // Check that IA_?? was returned and that there's proper status code ia = boost::dynamic_pointer_cast(tmp); ASSERT_TRUE(ia); checkIA_NAStatusCode(ia, STATUS_NoBinding); @@ -340,7 +341,8 @@ Dhcpv6SrvTest::testRenewReject(Lease::Type type, const IOAddress& addr) { checkResponse(reply, DHCPV6_REPLY, transid); tmp = reply->getOption(code); ASSERT_TRUE(tmp); - // Check that IA_NA was returned and that there's an address included + + // Check that IA_?? was returned and that there's proper status code ia = boost::dynamic_pointer_cast(tmp); ASSERT_TRUE(ia); checkIA_NAStatusCode(ia, STATUS_NoBinding); @@ -500,7 +502,8 @@ Dhcpv6SrvTest::testReleaseReject(Lease::Type type, const IOAddress& addr) { checkResponse(reply, DHCPV6_REPLY, transid); tmp = reply->getOption(code); ASSERT_TRUE(tmp); - // Check that IA_NA was returned and that there's an address included + + // Check that IA_?? was returned and that there's proper status code ia = boost::dynamic_pointer_cast(tmp); ASSERT_TRUE(ia); checkIA_NAStatusCode(ia, STATUS_NoBinding); @@ -523,7 +526,8 @@ Dhcpv6SrvTest::testReleaseReject(Lease::Type type, const IOAddress& addr) { checkResponse(reply, DHCPV6_REPLY, transid); tmp = reply->getOption(code); ASSERT_TRUE(tmp); - // Check that IA_NA was returned and that there's an address included + + // Check that IA_?? was returned and that there's proper status code ia = boost::dynamic_pointer_cast(tmp); ASSERT_TRUE(ia); checkIA_NAStatusCode(ia, STATUS_NoBinding); diff --git a/src/bin/dhcp6/tests/dhcp6_test_utils.h b/src/bin/dhcp6/tests/dhcp6_test_utils.h index fb419f93d4..95ae9bdfbe 100644 --- a/src/bin/dhcp6/tests/dhcp6_test_utils.h +++ b/src/bin/dhcp6/tests/dhcp6_test_utils.h @@ -316,21 +316,33 @@ class Dhcpv6SrvTest : public NakedDhcpv6SrvTest { public: /// Name of the server-id file (used in server-id tests) - // these are empty for now, but let's keep them around + /// @brief Constructor that initalizes a simple default configuration + /// + /// Sets up a single subnet6 with one pool for addresses and second + /// pool for prefixes. Dhcpv6SrvTest() { subnet_ = Subnet6Ptr(new Subnet6(IOAddress("2001:db8:1::"), 48, 1000, 2000, 3000, 4000)); - pool_ = Pool6Ptr(new Pool6(Lease::TYPE_NA, IOAddress("2001:db8:1:1::"), 64)); + pool_ = Pool6Ptr(new Pool6(Lease::TYPE_NA, IOAddress("2001:db8:1:1::"), + 64)); subnet_->addPool(pool_); CfgMgr::instance().deleteSubnets6(); CfgMgr::instance().addSubnet6(subnet_); // configure PD pool - pd_pool_ = Pool6Ptr(new Pool6(Lease::TYPE_PD, IOAddress("2001:db8:1:2::"), 64, 80)); + pd_pool_ = Pool6Ptr(new Pool6(Lease::TYPE_PD, + IOAddress("2001:db8:1:2::"), 64, 80)); subnet_->addPool(pd_pool_); } + /// @brief destructor + /// + /// Removes existing configuration. + ~Dhcpv6SrvTest() { + CfgMgr::instance().deleteSubnets6(); + }; + /// @brief Checks that server response (ADVERTISE or REPLY) contains proper /// IA_NA option /// @@ -446,10 +458,6 @@ public: testReleaseBasic(Lease::Type type, const IOAddress& existing, const IOAddress& release_addr); - ~Dhcpv6SrvTest() { - CfgMgr::instance().deleteSubnets6(); - }; - /// @brief Performs negative RELEASE test /// /// See releaseReject and pdReleaseReject tests for detailed explanation.