From: Marcin Siodelski Date: Mon, 10 Sep 2012 18:43:51 +0000 (+0200) Subject: [1959] Implemented suggestions from code review. X-Git-Tag: trac2351_base~47^2~11^2~5 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=b5aa1dadff2c0e7c9fce8fab0aa6dee28071e29b;p=thirdparty%2Fkea.git [1959] Implemented suggestions from code review. --- diff --git a/tests/tools/perfdhcp/command_options.cc b/tests/tools/perfdhcp/command_options.cc index 8d9f02e763..24e7e790dc 100644 --- a/tests/tools/perfdhcp/command_options.cc +++ b/tests/tools/perfdhcp/command_options.cc @@ -12,6 +12,7 @@ // OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR // PERFORMANCE OF THIS SOFTWARE. +#include #include #include #include @@ -866,7 +867,7 @@ CommandOptions::usage() const { void CommandOptions::version() const { - fprintf(stdout, "version 0.01\n"); + std::cout << "VERSION: " << VERSION << std::endl; } diff --git a/tests/tools/perfdhcp/pkt_transform.h b/tests/tools/perfdhcp/pkt_transform.h index 08563a2720..1f57105720 100644 --- a/tests/tools/perfdhcp/pkt_transform.h +++ b/tests/tools/perfdhcp/pkt_transform.h @@ -114,6 +114,8 @@ public: template static void writeValueAt(dhcp::OptionBuffer& in_buffer, size_t dest_pos, T val) { + // @todo consider replacing the loop with switch statement + // checking sizeof(T). for (int i = 0; i < sizeof(T); ++i) { in_buffer[dest_pos + i] = (val >> 8 * (sizeof(T) - i - 1)) & 0xFF; } diff --git a/tests/tools/perfdhcp/test_control.cc b/tests/tools/perfdhcp/test_control.cc index ef970a7948..090fb32411 100644 --- a/tests/tools/perfdhcp/test_control.cc +++ b/tests/tools/perfdhcp/test_control.cc @@ -299,8 +299,8 @@ TestControl::factoryOptionRequestOption6(Option::Universe, uint16_t, const OptionBuffer&) { const uint8_t buf_array[] = { - D6O_NAME_SERVERS, 0, - D6O_DOMAIN_SEARCH, 0, + 0, D6O_NAME_SERVERS, + 0, D6O_DOMAIN_SEARCH, }; OptionBuffer buf_with_options(buf_array, buf_array + sizeof(buf_array)); return (OptionPtr(new Option(Option::V6, D6O_ORO, buf_with_options))); @@ -731,7 +731,7 @@ TestControl::readPacketTemplate(const std::string& file_name) { } void -TestControl::receivePacket4(const TestControlSocket& socket, +TestControl::processReceivedPacket4(const TestControlSocket& socket, const Pkt4Ptr& pkt4) { if (pkt4->getType() == DHCPOFFER) { Pkt4Ptr discover_pkt4(stats_mgr4_->passRcvdPacket(StatsMgr4::XCHG_DO, @@ -742,6 +742,8 @@ TestControl::receivePacket4(const TestControlSocket& socket, if (template_buffers_.size() < 2) { sendRequest4(socket, discover_pkt4, pkt4); } else { + // @todo add defines for packet type index that can be + // used to access template_buffers_. sendRequest4(socket, template_buffers_[1], discover_pkt4, pkt4); } } @@ -751,7 +753,7 @@ TestControl::receivePacket4(const TestControlSocket& socket, } void -TestControl::receivePacket6(const TestControlSocket& socket, +TestControl::processReceivedPacket6(const TestControlSocket& socket, const Pkt6Ptr& pkt6) { uint8_t packet_type = pkt6->getType(); if (packet_type == DHCPV6_ADVERTISE) { @@ -766,6 +768,8 @@ TestControl::receivePacket6(const TestControlSocket& socket, if (template_buffers_.size() < 2) { sendRequest6(socket, pkt6); } else { + // @todo add defines for packet type index that can be + // used to access template_buffers_. sendRequest6(socket, template_buffers_[1], pkt6); } } @@ -790,7 +794,7 @@ TestControl::receivePackets(const TestControlSocket& socket) { stats_mgr4_->incrementCounter("multircvd"); } pkt4->unpack(); - receivePacket4(socket, pkt4); + processReceivedPacket4(socket, pkt4); } } else if (CommandOptions::instance().getIpVersion() == 6) { Pkt6Ptr pkt6 = IfaceMgr::instance().receive6(timeout); @@ -802,7 +806,7 @@ TestControl::receivePackets(const TestControlSocket& socket) { stats_mgr6_->incrementCounter("multircvd"); } if (pkt6->unpack()) { - receivePacket6(socket, pkt6); + processReceivedPacket6(socket, pkt6); } } } @@ -949,8 +953,9 @@ TestControl::run() { } else { // Pick template #0 if Discover is being sent. // For Request it would be #1. - const uint8_t template_idx = 0; - sendDiscover4(socket, template_buffers_[template_idx], + // @todo add defines for packet type index that can be + // used to access template_buffers_. + sendDiscover4(socket, template_buffers_[0], do_preload); } } else if (options.getIpVersion() == 6) { @@ -961,8 +966,9 @@ TestControl::run() { } else { // Pick template #0 if Solicit is being sent. // For Request it would be #1. - const uint8_t template_idx = 0; - sendSolicit6(socket, template_buffers_[template_idx], + // @todo add defines for packet type index that can be + // used to access template_buffers_. + sendSolicit6(socket, template_buffers_[0], do_preload); } } @@ -1000,8 +1006,9 @@ TestControl::run() { if (template_buffers_.size() == 0) { sendDiscover4(socket); } else { - const uint8_t template_idx = 0; - sendDiscover4(socket, template_buffers_[template_idx]); + // @todo add defines for packet type index that can be + // used to access template_buffers_. + sendDiscover4(socket, template_buffers_[0]); } } else { // No template packets means that no -T option was specified. @@ -1009,8 +1016,9 @@ TestControl::run() { if (template_buffers_.size() == 0) { sendSolicit6(socket); } else { - const uint8_t template_idx = 0; - sendSolicit6(socket, template_buffers_[template_idx]); + // @todo add defines for packet type index that can be + // used to access template_buffers_. + sendSolicit6(socket, template_buffers_[0]); } } } diff --git a/tests/tools/perfdhcp/test_control.h b/tests/tools/perfdhcp/test_control.h index 8e492af133..1b0b83c385 100644 --- a/tests/tools/perfdhcp/test_control.h +++ b/tests/tools/perfdhcp/test_control.h @@ -46,6 +46,12 @@ namespace perfdhcp { /// \ref dhcp::Option::factory with DHCP message type specified as one of /// parameters. Some of the parameters passed to factory function /// may be ignored (e.g. option buffer). +/// Please note that naming convention for factory functions within this +/// class is as follows: +/// - factoryABC4 - factory function for DHCPv4 option, +/// - factoryDEF6 - factory function for DHCPv6 option, +/// - factoryGHI - factory function that can be used to create either +/// DHCPv4 or DHCPv6 option. class TestControl : public boost::noncopyable { public: @@ -449,11 +455,11 @@ protected: /// not initialized. void printStats() const; - /// \brief Receive DHCPv4 packet. + /// \brief Process received DHCPv4 packet. /// - /// Method performs reception of the DHCPv4 packet, updates - /// statistics and responsds to the server if required, e.g. - /// when OFFER packet arrives, this function will initiate + /// Method performs processing of the received DHCPv4 packet, + /// updates statistics and responds to the server if required, + /// e.g. when OFFER packet arrives, this function will initiate /// REQUEST message to the server. /// /// \warning this method does not check if provided socket is @@ -463,14 +469,14 @@ protected: /// \param [in] pkt4 object representing DHCPv4 packet received. /// \throw isc::BadValue if unknown message type received. /// \throw isc::Unexpected if unexpected error occured. - void receivePacket4(const TestControlSocket& socket, - const dhcp::Pkt4Ptr& pkt4); + void processReceivedPacket4(const TestControlSocket& socket, + const dhcp::Pkt4Ptr& pkt4); - /// \brief Receive DHCPv6 packet. + /// \brief Process received DHCPv6 packet. /// - /// Method performs reception of the DHCPv6 packet, updates - /// statistics and responsds to the server if required, e.g. - /// when ADVERTISE packet arrives, this function will initiate + /// Method performs processing of the received DHCPv6 packet, + /// updates statistics and responsds to the server if required, + /// e.g. when ADVERTISE packet arrives, this function will initiate /// REQUEST message to the server. /// /// \warning this method does not check if provided socket is @@ -480,8 +486,8 @@ protected: /// \param [in] pkt6 object representing DHCPv6 packet received. /// \throw isc::BadValue if unknown message type received. /// \throw isc::Unexpected if unexpected error occured. - void receivePacket6(const TestControlSocket& socket, - const dhcp::Pkt6Ptr& pkt6); + void processReceivedPacket6(const TestControlSocket& socket, + const dhcp::Pkt6Ptr& pkt6); /// \brief Receive DHCPv4 or DHCPv6 packets from the server. /// @@ -706,6 +712,8 @@ private: /// \brief Convert binary value to hex string. /// + /// \todo Consider moving this function to src/lib/util. + /// /// \param b byte to convert. /// \return hex string. std::string byte2Hex(const uint8_t b) const; @@ -760,6 +768,8 @@ private: /// \brief Convert vector in hexadecimal string. /// + /// \todo Consider moving this function to src/lib/util. + /// /// \param vec vector to be converted. /// \param separator separator. std::string vector2Hex(const std::vector& vec, diff --git a/tests/tools/perfdhcp/tests/test_control_unittest.cc b/tests/tools/perfdhcp/tests/test_control_unittest.cc index e5dbaf12a2..a4cde00f19 100644 --- a/tests/tools/perfdhcp/tests/test_control_unittest.cc +++ b/tests/tools/perfdhcp/tests/test_control_unittest.cc @@ -81,8 +81,8 @@ public: using TestControl::initPacketTemplates; using TestControl::initializeStatsMgr; using TestControl::openSocket; - using TestControl::receivePacket4; - using TestControl::receivePacket6; + using TestControl::processReceivedPacket4; + using TestControl::processReceivedPacket6; using TestControl::registerOptionFactories; using TestControl::sendDiscover4; using TestControl::sendSolicit6; @@ -418,7 +418,7 @@ public: // packet drops. if (i < receive_num) { boost::shared_ptr offer_pkt4(createOfferPkt4(transid)); - ASSERT_NO_THROW(tc.receivePacket4(sock, offer_pkt4)); + ASSERT_NO_THROW(tc.processReceivedPacket4(sock, offer_pkt4)); ++transid; } if (tc.checkExitConditions()) { @@ -483,7 +483,7 @@ public: boost::shared_ptr advertise_pkt6(createAdvertisePkt6(transid)); // Receive ADVERTISE and send REQUEST. - ASSERT_NO_THROW(tc.receivePacket6(sock, advertise_pkt6)); + ASSERT_NO_THROW(tc.processReceivedPacket6(sock, advertise_pkt6)); ++transid; } if (tc.checkExitConditions()) { @@ -770,8 +770,8 @@ TEST_F(TestControlTest, Options6) { OptionPtr opt_oro(Option::factory(Option::V6, D6O_ORO)); // Prepare the reference buffer with requested options. const uint8_t requested_options[] = { - D6O_NAME_SERVERS, 0, - D6O_DOMAIN_SEARCH, 0, + 0, D6O_NAME_SERVERS, + 0, D6O_DOMAIN_SEARCH, }; int requested_options_num = sizeof(requested_options) / sizeof(requested_options[0]);