diff --git a/src/bin/d2/d2_messages.mes b/src/bin/d2/d2_messages.mes index 3cb9d45c34..dc5103dfba 100644 --- a/src/bin/d2/d2_messages.mes +++ b/src/bin/d2/d2_messages.mes @@ -1,4 +1,4 @@ -# Copyright (C) 2013 Internet Systems Consortium, Inc. ("ISC") +# Copyright (C) 2013-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 @@ -438,10 +438,12 @@ This is an error message issued after DHCP_DDNS attempts to submit DNS mapping entry removals have failed. The precise reason for the failure should be documented in preceding log entries. -% DHCP_DDNS_UPDATE_REQUEST_SENT for transaction key: %1 to server : %2 +% DHCP_DDNS_STARTING_TRANSACTION Transaction Key: %1 + +% DHCP_DDNS_UPDATE_REQUEST_SENT for transaction key: %1 to server: %2 This is a debug message issued when DHCP_DDNS sends a DNS request to a DNS server. -% DHCP_DDNS_UPDATE_RESPONSE_RECEIVED for transaction key: %1 to server : %2 status: %3 +% DHCP_DDNS_UPDATE_RESPONSE_RECEIVED for transaction key: %1 to server: %2 status: %3 This is a debug message issued when DHCP_DDNS receives sends a DNS update response from a DNS server. diff --git a/src/bin/d2/d2_update_mgr.cc b/src/bin/d2/d2_update_mgr.cc index d4584f75f8..abd1aa3863 100644 --- a/src/bin/d2/d2_update_mgr.cc +++ b/src/bin/d2/d2_update_mgr.cc @@ -171,7 +171,7 @@ D2UpdateMgr::makeTransaction(dhcp_ddns::NameChangeRequestPtr& next_ncr) { forward_domain, reverse_domain)); } else { trans.reset(new NameRemoveTransaction(io_service_, next_ncr, - forward_domain, reverse_domain)); + forward_domain, reverse_domain)); } // Add the new transaction to the list. diff --git a/src/bin/d2/dns_client.cc b/src/bin/d2/dns_client.cc index 723924bb10..00b0ff989c 100644 --- a/src/bin/d2/dns_client.cc +++ b/src/bin/d2/dns_client.cc @@ -44,7 +44,14 @@ class DNSClientImpl : public asiodns::IOFetch::Callback { public: // A buffer holding response from a DNS. util::OutputBufferPtr in_buf_; - // A caller-supplied object holding a parsed response from DNS. + // A caller-supplied object which will hold the parsed response from DNS. + // The response object is (or descends from) isc::dns::Message and is + // populated using Message::fromWire(). This method may only be called + // once in the lifetime of a Message instance. Therefore, response_ is a + // pointer reference thus allowing this class to replace the object + // pointed to with a new Message instance each time a message is + // received. This allows a single DNSClientImpl instance to be used in + // for multiple, sequential IOFetch calls. D2UpdateMessagePtr& response_; // A caller-supplied external callback which is invoked when DNS message // exchange is complete or interrupted. @@ -81,6 +88,12 @@ DNSClientImpl::DNSClientImpl(D2UpdateMessagePtr& response_placeholder, : in_buf_(new OutputBuffer(DEFAULT_BUFFER_SIZE)), response_(response_placeholder), callback_(callback), proto_(proto) { + // Response should be an empty pointer. It gets populated by the + // operator() method. + if (response_) { + isc_throw(isc::BadValue, "Response buffer pointer should be null"); + } + // @todo Currently we only support UDP. The support for TCP is planned for // the future release. if (proto_ == DNSClient::TCP) { diff --git a/src/bin/d2/dns_client.h b/src/bin/d2/dns_client.h index c1c54f682b..5960f6a147 100644 --- a/src/bin/d2/dns_client.h +++ b/src/bin/d2/dns_client.h @@ -93,8 +93,8 @@ public: /// @brief Constructor. /// - /// @param response_placeholder Pointer to an object which will hold a - /// DNS server's response. Caller is responsible for allocating this object. + /// @param response_placeholder Messge object pointer which will be updated + /// with dynamically allocated object holding the DNS server's response. /// @param callback Pointer to an object implementing @c DNSClient::Callback /// class. This object will be called when DNS message exchange completes or /// if an error occurs. NULL value disables callback invocation. diff --git a/src/bin/d2/nc_trans.cc b/src/bin/d2/nc_trans.cc index 5d61078e76..08e443d372 100644 --- a/src/bin/d2/nc_trans.cc +++ b/src/bin/d2/nc_trans.cc @@ -80,6 +80,10 @@ NameChangeTransaction::~NameChangeTransaction(){ void NameChangeTransaction::startTransaction() { + LOG_DEBUG(dctl_logger, DBGLVL_TRACE_DETAIL, + DHCP_DDNS_STARTING_TRANSACTION) + .arg(getTransactionKey().toStr()); + setNcrStatus(dhcp_ddns::ST_PENDING); startModel(READY_ST); } @@ -366,8 +370,9 @@ NameChangeTransaction::selectNextServer() { if ((current_server_list_) && (next_server_pos_ < current_server_list_->size())) { current_server_ = (*current_server_list_)[next_server_pos_]; - dns_update_response_.reset(new - D2UpdateMessage(D2UpdateMessage::INBOUND)); + // Toss out any previous response. + dns_update_response_.reset(); + // @todo Protocol is set on DNSClient constructor. We need // to propagate a configuration value downward, probably starting // at global, then domain, then server diff --git a/src/bin/d2/nc_trans.h b/src/bin/d2/nc_trans.h index 04a1d6f690..5305b476ac 100644 --- a/src/bin/d2/nc_trans.h +++ b/src/bin/d2/nc_trans.h @@ -152,15 +152,10 @@ public: //@} /// @brief Defualt time to assign to a single DNS udpate. -#if 0 - /// @todo This value will be configurable in the near future, but - /// until it is there is no way to replace its value. For now - /// we will define it to be relatively short, so unit tests will - /// run within reasonable amount of time. - static const unsigned int DNS_UPDATE_DEFAULT_TIMEOUT = 5 * 1000; -#else + /// @todo This value will be made configurable in the very near future + /// under trac3268. For now we will define it to 100 milliseconds + /// so unit tests will run within a reasonable amount of time. static const unsigned int DNS_UPDATE_DEFAULT_TIMEOUT = 100; -#endif /// @brief Maximum times to attempt a single update on a given server. static const unsigned int MAX_UPDATE_TRIES_PER_SERVER = 3; @@ -296,7 +291,7 @@ protected: void setDnsUpdateRequest(D2UpdateMessagePtr& request); /// @brief Destroys the current update request packet and resets - /// udpate attempts count.. + /// udpate attempts count. void clearDnsUpdateRequest(); /// @brief Sets the update status to the given status value. diff --git a/src/bin/d2/tests/d2_update_mgr_unittests.cc b/src/bin/d2/tests/d2_update_mgr_unittests.cc index a427cb2b8d..c97b744ce1 100644 --- a/src/bin/d2/tests/d2_update_mgr_unittests.cc +++ b/src/bin/d2/tests/d2_update_mgr_unittests.cc @@ -32,7 +32,7 @@ using namespace isc::d2; namespace { -/// @brief Wrapper class for D2UpdateMgr to provide access non-public methods. +/// @brief Wrapper class for D2UpdateMgr providing access to non-public methods. /// /// This class facilitates testing by making non-public methods accessible so /// they can be invoked directly in test routines. diff --git a/src/bin/d2/tests/dns_client_unittests.cc b/src/bin/d2/tests/dns_client_unittests.cc index fc654a2a05..a24196ea97 100644 --- a/src/bin/d2/tests/dns_client_unittests.cc +++ b/src/bin/d2/tests/dns_client_unittests.cc @@ -89,7 +89,7 @@ public: test_timer_(service_), received_(0), expected_(0) { asiodns::logger.setSeverity(isc::log::INFO); - response_.reset(new D2UpdateMessage(D2UpdateMessage::INBOUND)); + response_.reset(); dns_client_.reset(new DNSClient(response_, this)); // Set the test timeout to break any running tasks if they hang. diff --git a/src/bin/d2/tests/nc_test_utils.h b/src/bin/d2/tests/nc_test_utils.h index 21c0f6696b..7476bb2693 100644 --- a/src/bin/d2/tests/nc_test_utils.h +++ b/src/bin/d2/tests/nc_test_utils.h @@ -101,7 +101,16 @@ public: } }; -class TimedIO { +/// @brief Provides a means to process IOService IO for a finite amount of time. +/// +/// This class instantiates an IOService provides a single method, runTimedIO +/// which will run the IOService for no more than a finite amount of time, +/// at least one event is executed or the IOService is stopped. +/// It provides an overridable handler for timer expiration event. It is +/// intended to be used as a base class for test fixtures that need to process +/// IO by providing them a consistent way to do so while retaining a safety valve +/// so tests do not hang. +class TimedIO { public: IOServicePtr io_service_; asiolink::IntervalTimer timer_; diff --git a/src/bin/d2/tests/nc_trans_unittests.cc b/src/bin/d2/tests/nc_trans_unittests.cc index 78582fef98..0829ff844e 100644 --- a/src/bin/d2/tests/nc_trans_unittests.cc +++ b/src/bin/d2/tests/nc_trans_unittests.cc @@ -578,7 +578,16 @@ TEST_F(NameChangeTransactionTest, serverSelectionTest) { // they are correct after each selection. DnsServerInfoPtr prev_server = name_change->getCurrentServer(); DNSClientPtr prev_client = name_change->getDNSClient(); - D2UpdateMessagePtr prev_response = name_change->getDnsUpdateResponse(); + + // Verify response pointer is empty. + EXPECT_FALSE(name_change->getDnsUpdateResponse()); + + // Create dummy response so we can verify it is cleared at each + // new server select. + D2UpdateMessagePtr dummyResp; + dummyResp.reset(new D2UpdateMessage(D2UpdateMessage::INBOUND)); + ASSERT_NO_THROW(name_change->setDnsUpdateResponse(dummyResp)); + ASSERT_TRUE(name_change->getDnsUpdateResponse()); // Iteratively select through the list of servers. int passes = 0; @@ -591,17 +600,22 @@ TEST_F(NameChangeTransactionTest, serverSelectionTest) { // Verify that the new values are not empty. EXPECT_TRUE(server); EXPECT_TRUE(client); - EXPECT_TRUE(response); + + // Verify response pointer is now empty. + EXPECT_FALSE(name_change->getDnsUpdateResponse()); // Verify that the new values are indeed new. EXPECT_NE(server, prev_server); EXPECT_NE(client, prev_client); - EXPECT_NE(response, prev_response); // Remember the selected values for the next pass. prev_server = server; prev_client = client; - prev_response = response; + + // Create new dummy response. + dummyResp.reset(new D2UpdateMessage(D2UpdateMessage::INBOUND)); + ASSERT_NO_THROW(name_change->setDnsUpdateResponse(dummyResp)); + ASSERT_TRUE(name_change->getDnsUpdateResponse()); ++passes; } @@ -626,12 +640,11 @@ TEST_F(NameChangeTransactionTest, serverSelectionTest) { ASSERT_NO_THROW(name_change->initServerSelection(domain)); // The server selection process determines the current server, - // instantiates a new DNSClient, and a DNS response message buffer. + // instantiates a new DNSClient, and resets the DNS response message buffer. // We need to save the values before each selection, so we can verify // they are correct after each selection. prev_server = name_change->getCurrentServer(); prev_client = name_change->getDNSClient(); - prev_response = name_change->getDnsUpdateResponse(); // Iteratively select through the list of servers. passes = 0; @@ -644,17 +657,22 @@ TEST_F(NameChangeTransactionTest, serverSelectionTest) { // Verify that the new values are not empty. EXPECT_TRUE(server); EXPECT_TRUE(client); - EXPECT_TRUE(response); + + // Verify response pointer is now empty. + EXPECT_FALSE(name_change->getDnsUpdateResponse()); // Verify that the new values are indeed new. EXPECT_NE(server, prev_server); EXPECT_NE(client, prev_client); - EXPECT_NE(response, prev_response); // Remember the selected values for the next pass. prev_server = server; prev_client = client; - prev_response = response; + + // Create new dummy response. + dummyResp.reset(new D2UpdateMessage(D2UpdateMessage::INBOUND)); + ASSERT_NO_THROW(name_change->setDnsUpdateResponse(dummyResp)); + ASSERT_TRUE(name_change->getDnsUpdateResponse()); ++passes; }