From: Michal Nowikowski <godfryd@isc.org>
Date: Fri, 7 Dec 2018 14:33:42 +0000 (+0100)
Subject: changes after review
X-Git-Tag: 421-create-config-backend-for-dhcpv6-base_base~13
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=9b225b8619d1e4682713166ae937bf4be3ae12ae;p=thirdparty%2Fkea.git

changes after review

- added usleep to main loop in run function when nothing is to be done to not overload CPU
- changed includes to use <...>
- changed socket desctructor to be virtual
- updated docs
- changed rate type to unsigned
- other minor fixes
---

diff --git a/src/bin/perfdhcp/better_socket.cc b/src/bin/perfdhcp/better_socket.cc
index 3005340217..ace47235f9 100644
--- a/src/bin/perfdhcp/better_socket.cc
+++ b/src/bin/perfdhcp/better_socket.cc
@@ -5,7 +5,7 @@
 // file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 
-#include "better_socket.h"
+#include <perfdhcp/better_socket.h>
 
 #include <dhcp/iface_mgr.h>
 
@@ -45,8 +45,7 @@ BetterSocket::initSocketData() {
             }
         }
     }
-    isc_throw(BadValue, "interface for for specified socket "
-              "descriptor not found");
+    isc_throw(BadValue, "interface for specified socket descriptor not found");
 }
 
 }
diff --git a/src/bin/perfdhcp/better_socket.h b/src/bin/perfdhcp/better_socket.h
index 976cd83d79..e0b9f0e718 100644
--- a/src/bin/perfdhcp/better_socket.h
+++ b/src/bin/perfdhcp/better_socket.h
@@ -41,7 +41,7 @@ struct BetterSocket : public dhcp::SocketInfo {
     /// \brief Destructor of the socket wrapper class.
     ///
     /// Destructor closes wrapped socket.
-    ~BetterSocket();
+    virtual ~BetterSocket();
 
 private:
     /// \brief Initialize socket data.
diff --git a/src/bin/perfdhcp/command_options.cc b/src/bin/perfdhcp/command_options.cc
index 319152f3c0..7c5237195b 100644
--- a/src/bin/perfdhcp/command_options.cc
+++ b/src/bin/perfdhcp/command_options.cc
@@ -6,7 +6,8 @@
 
 #include <config.h>
 
-#include "command_options.h"
+#include <perfdhcp/command_options.h>
+
 #include <exceptions/exceptions.h>
 #include <dhcp/iface_mgr.h>
 #include <dhcp/duid.h>
@@ -16,7 +17,6 @@
 
 #include <boost/lexical_cast.hpp>
 #include <boost/date_time/posix_time/posix_time.hpp>
-
 #include <sstream>
 #include <stdio.h>
 #include <stdlib.h>
@@ -1105,8 +1105,8 @@ CommandOptions::usage() const {
         "    to or less than the exchange rate.\n"
         "-g  Select thread mode: 'single' or 'multi'. In multi-thread mode packets\n"
         "    are received in separate thread. This allows better utilisation of CPUs."
-        "    If more than 1 CPU is present then multi-thread mode is default otherwise"
-        "    single-thread is default."
+        "    If more than 1 CPU is present then multi-thread mode is the default,"
+        "    otherwise single-thread is the default."
         "-h: Print this help.\n"
         "-i: Do only the initial part of an exchange: DO or SA, depending on\n"
         "    whether -6 is given.\n"
diff --git a/src/bin/perfdhcp/command_options.h b/src/bin/perfdhcp/command_options.h
index 167d28ee7d..f843d2484d 100644
--- a/src/bin/perfdhcp/command_options.h
+++ b/src/bin/perfdhcp/command_options.h
@@ -7,9 +7,9 @@
 #ifndef COMMAND_OPTIONS_H
 #define COMMAND_OPTIONS_H
 
-#include <boost/noncopyable.hpp>
-
 #include <dhcp/option.h>
+
+#include <boost/noncopyable.hpp>
 #include <stdint.h>
 #include <string>
 #include <vector>
@@ -507,13 +507,13 @@ private:
     LeaseType lease_type_;
 
     /// Rate in exchange per second
-    int rate_;
+    unsigned int rate_;
 
     /// A rate at which DHCPv6 Renew messages are sent.
-    int renew_rate_;
+    unsigned int renew_rate_;
 
     /// A rate at which DHCPv6 Release messages are sent.
-    int release_rate_;
+    unsigned int release_rate_;
 
     /// Delay between generation of two consecutive performance reports
     int report_delay_;
diff --git a/src/bin/perfdhcp/main.cc b/src/bin/perfdhcp/main.cc
index bb5409a548..7548ee76cd 100644
--- a/src/bin/perfdhcp/main.cc
+++ b/src/bin/perfdhcp/main.cc
@@ -4,14 +4,15 @@
 // License, v. 2.0. If a copy of the MPL was not distributed with this
 // file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
-#include <iostream>
-#include <stdint.h>
-
 #include <config.h>
+
+#include <perfdhcp/test_control.h>
+#include <perfdhcp/command_options.h>
+
 #include <exceptions/exceptions.h>
 
-#include "test_control.h"
-#include "command_options.h"
+#include <iostream>
+#include <stdint.h>
 
 using namespace isc::perfdhcp;
 
diff --git a/src/bin/perfdhcp/perf_pkt4.cc b/src/bin/perfdhcp/perf_pkt4.cc
index d8aca95498..784f96dacc 100644
--- a/src/bin/perfdhcp/perf_pkt4.cc
+++ b/src/bin/perfdhcp/perf_pkt4.cc
@@ -6,11 +6,11 @@
 
 #include <config.h>
 
+#include <perfdhcp/perf_pkt4.h>
+
 #include <dhcp/libdhcp++.h>
 #include <dhcp/dhcp4.h>
 
-#include "perf_pkt4.h"
-
 using namespace std;
 using namespace isc;
 using namespace dhcp;
diff --git a/src/bin/perfdhcp/perf_pkt4.h b/src/bin/perfdhcp/perf_pkt4.h
index 6cc8b49374..a74ed43143 100644
--- a/src/bin/perfdhcp/perf_pkt4.h
+++ b/src/bin/perfdhcp/perf_pkt4.h
@@ -7,12 +7,14 @@
 #ifndef PERF_PKT4_H
 #define PERF_PKT4_H
 
-#include <time.h>
-#include <boost/shared_ptr.hpp>
+#include <perfdhcp/localized_option.h>
+#include <perfdhcp/pkt_transform.h>
+
 #include <dhcp/pkt4.h>
 
-#include "localized_option.h"
-#include "pkt_transform.h"
+#include <boost/shared_ptr.hpp>
+#include <time.h>
+
 
 namespace isc {
 namespace perfdhcp {
diff --git a/src/bin/perfdhcp/perf_pkt6.cc b/src/bin/perfdhcp/perf_pkt6.cc
index 4d802471df..72724148ad 100644
--- a/src/bin/perfdhcp/perf_pkt6.cc
+++ b/src/bin/perfdhcp/perf_pkt6.cc
@@ -6,13 +6,14 @@
 
 #include <config.h>
 
-#include <iostream>
+#include <perfdhcp/perf_pkt6.h>
+#include <perfdhcp/pkt_transform.h>
+
 #include <exceptions/exceptions.h>
 #include <dhcp/libdhcp++.h>
 #include <dhcp/dhcp6.h>
 
-#include "perf_pkt6.h"
-#include "pkt_transform.h"
+#include <iostream>
 
 using namespace std;
 using namespace isc;
diff --git a/src/bin/perfdhcp/perf_pkt6.h b/src/bin/perfdhcp/perf_pkt6.h
index 988dd73144..8465c396eb 100644
--- a/src/bin/perfdhcp/perf_pkt6.h
+++ b/src/bin/perfdhcp/perf_pkt6.h
@@ -7,12 +7,13 @@
 #ifndef PERF_PKT6_H
 #define PERF_PKT6_H
 
-#include <time.h>
-#include <boost/shared_ptr.hpp>
+#include <perfdhcp/localized_option.h>
+#include <perfdhcp/pkt_transform.h>
+
 #include <dhcp/pkt6.h>
 
-#include "localized_option.h"
-#include "pkt_transform.h"
+#include <time.h>
+#include <boost/shared_ptr.hpp>
 
 namespace isc {
 namespace perfdhcp {
diff --git a/src/bin/perfdhcp/perfdhcp.xml b/src/bin/perfdhcp/perfdhcp.xml
index cd8eaea691..3f85823f80 100644
--- a/src/bin/perfdhcp/perfdhcp.xml
+++ b/src/bin/perfdhcp/perfdhcp.xml
@@ -337,11 +337,11 @@
               <term><option>-g <replaceable class="parameter">thread-mode</replaceable></option></term>
               <listitem>
                 <para>
-                    Thread operation mode can be either 'single' or 'multi'. In multi-thread mode packets
-                    are received in separate thread. This allows better utilisation of CPUs.
-                    In single CPU system it is better to run in 1 thread to avoid blocking of threads each other.
-                    If more than 1 CPU is present in the system then multi-thread mode is default otherwise
-                    single-thread is default.
+                  <replaceable class="parameter">thread-mode</replaceable> can be either 'single' or 'multi'.
+                  In multi-thread mode packets are received in separate thread. This allows better utilisation of CPUs.
+                  In single CPU system it is better to run in 1 thread to avoid blocking of threads each other.
+                  If more than 1 CPU is present in the system then multi-thread mode is default otherwise
+                  single-thread is default.
                 </para>
               </listitem>
             </varlistentry>
diff --git a/src/bin/perfdhcp/pkt_transform.cc b/src/bin/perfdhcp/pkt_transform.cc
index d9588b1a65..fad8d1cbd1 100644
--- a/src/bin/perfdhcp/pkt_transform.cc
+++ b/src/bin/perfdhcp/pkt_transform.cc
@@ -6,15 +6,16 @@
 
 #include <config.h>
 
-#include <iostream>
+#include <perfdhcp/pkt_transform.h>
+#include <perfdhcp/localized_option.h>
 
 #include <exceptions/exceptions.h>
 #include <dhcp/option.h>
 #include <dhcp/libdhcp++.h>
 #include <dhcp/dhcp6.h>
 
-#include "pkt_transform.h"
-#include "localized_option.h"
+#include <iostream>
+
 
 using namespace std;
 using namespace isc;
diff --git a/src/bin/perfdhcp/pkt_transform.h b/src/bin/perfdhcp/pkt_transform.h
index 3239b0b081..d1272f1777 100644
--- a/src/bin/perfdhcp/pkt_transform.h
+++ b/src/bin/perfdhcp/pkt_transform.h
@@ -7,9 +7,9 @@
 #ifndef PKT_TRANSFORM_H
 #define PKT_TRANSFORM_H
 
-#include <dhcp/option.h>
+#include <perfdhcp/localized_option.h>
 
-#include "localized_option.h"
+#include <dhcp/option.h>
 
 namespace isc {
 namespace perfdhcp {
diff --git a/src/bin/perfdhcp/rate_control.cc b/src/bin/perfdhcp/rate_control.cc
index 9b7c759a88..8d5ac2453a 100644
--- a/src/bin/perfdhcp/rate_control.cc
+++ b/src/bin/perfdhcp/rate_control.cc
@@ -6,8 +6,10 @@
 
 #include <config.h>
 
+#include <perfdhcp/rate_control.h>
+
 #include <exceptions/exceptions.h>
-#include "rate_control.h"
+
 
 namespace isc {
 namespace perfdhcp {
@@ -18,12 +20,8 @@ RateControl::RateControl()
     : rate_(0), total_pkts_sent_count_(0) {
 }
 
-RateControl::RateControl(const int rate)
+RateControl::RateControl(const unsigned int rate)
     : rate_(rate), total_pkts_sent_count_(0) {
-    if (rate_ < 0) {
-        isc_throw(isc::BadValue, "invalid value of rate " << rate
-                  << ", expected non-negative value");
-    }
 }
 
 uint64_t
diff --git a/src/bin/perfdhcp/rate_control.h b/src/bin/perfdhcp/rate_control.h
index 347e67359d..bbd9f9ea4a 100644
--- a/src/bin/perfdhcp/rate_control.h
+++ b/src/bin/perfdhcp/rate_control.h
@@ -44,7 +44,7 @@ public:
     /// \brief Constructor which sets desired rate.
     ///
     /// \param rate A desired rate.
-    RateControl(const int rate);
+    RateControl(const unsigned int rate);
 
     /// \brief Returns number of messages to be sent "now".
     ///
@@ -72,7 +72,7 @@ public:
     uint64_t getOutboundMessageCount();
 
     /// \brief Returns the rate.
-    int getRate() const {
+    unsigned int getRate() const {
         return (rate_);
     }
 
@@ -90,7 +90,7 @@ protected:
     boost::posix_time::ptime currentTime();
 
     /// \brief Holds a desired rate value.
-    int rate_;
+    unsigned int rate_;
 
     /// \brief Holds number of packets send from the beginning.
 
diff --git a/src/bin/perfdhcp/receiver.cc b/src/bin/perfdhcp/receiver.cc
index dce28f1dd4..0473e58c88 100644
--- a/src/bin/perfdhcp/receiver.cc
+++ b/src/bin/perfdhcp/receiver.cc
@@ -4,12 +4,13 @@
 // License, v. 2.0. If a copy of the MPL was not distributed with this
 // file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
-#include "receiver.h"
-#include "command_options.h"
+#include <perfdhcp/receiver.h>
+#include <perfdhcp/command_options.h>
 
 #include <dhcp/iface_mgr.h>
 
 using namespace std;
+using namespace isc::dhcp;
 
 namespace isc {
 namespace perfdhcp {
@@ -50,16 +51,15 @@ PktPtr
 Receiver::getPkt() {
     if (single_threaded_) {
         // In single thread mode read packet directly from the socket and return it.
-        auto pkt = readPktFromSocket();
-        return pkt;
+        return readPktFromSocket();
     } else {
         // In multi thread mode read packet from the queue which is feed by Receiver thread.
         unique_lock<mutex> lock(pkt_queue_mutex_);
         if (pkt_queue_.empty()) {
             if (CommandOptions::instance().getIpVersion() == 4) {
-                return Pkt4Ptr{nullptr};
+                return Pkt4Ptr();
             } else {
-                return Pkt6Ptr{nullptr};
+                return Pkt6Ptr();
             }
         }
         auto pkt = pkt_queue_.front();
@@ -76,8 +76,12 @@ Receiver::run() {
         while (run_flag_.test_and_set()) {
             receivePackets();
         }
+    } catch (const exception& e) {
+        cerr << "Something went wrong: " << e.what() << endl;
+        usleep(1000);
     } catch (...) {
-        cout << "SOMETHING WENT WRONG" << endl;
+        cerr << "Something went wrong" << endl;
+        usleep(1000);
     }
 }
 
@@ -101,7 +105,7 @@ Receiver::readPktFromSocket() {
             pkt = IfaceMgr::instance().receive6(0, timeout);
         }
     } catch (const Exception& e) {
-        cerr << "Failed to receive DHCP packet: " << e.what() <<  endl;
+        cerr << "Failed to receive DHCP packet: " << e.what() << endl;
     }
     if (!pkt) {
         return nullptr;
@@ -121,10 +125,10 @@ Receiver::receivePackets() {
             break;
         }
 
-        // Drop packet if not supported.
+        // Drop the packet if not supported. Do not bother main thread about it.
         if (pkt->getType() == DHCPOFFER || pkt->getType() == DHCPACK ||
             pkt->getType() == DHCPV6_ADVERTISE || pkt->getType() == DHCPV6_REPLY) {
-            // Otherwise push to another thread.
+            // Otherwise push the packet to the queue, to main thread.
             unique_lock<mutex> lock(pkt_queue_mutex_);
             pkt_queue_.push(pkt);
         }
diff --git a/src/bin/perfdhcp/receiver.h b/src/bin/perfdhcp/receiver.h
index ce7fc4b114..bda6a9dced 100644
--- a/src/bin/perfdhcp/receiver.h
+++ b/src/bin/perfdhcp/receiver.h
@@ -6,8 +6,8 @@
 
 #pragma once
 
-#include "better_socket.h"
-#include "command_options.h"
+#include <perfdhcp/better_socket.h>
+#include <perfdhcp/command_options.h>
 
 #include <dhcp/pkt4.h>
 #include <dhcp/pkt6.h>
@@ -17,13 +17,10 @@
 #include <mutex>
 #include <boost/atomic.hpp>
 
-using namespace std;
-using namespace isc::dhcp;
-
 namespace isc {
 namespace perfdhcp {
 
-typedef boost::shared_ptr<Pkt> PktPtr;
+typedef boost::shared_ptr<isc::dhcp::Pkt> PktPtr;
 
 /// \brief A receviving DHCP packets class.
 ///
@@ -46,13 +43,13 @@ private:
     boost::atomic_flag run_flag_;
 
     /// \brief Thread for receiving packets.
-    unique_ptr<thread> recv_thread_;
+    std::unique_ptr<std::thread> recv_thread_;
 
     /// \brief Queue for passing packets from receiver thread to main thread.
-    queue<PktPtr> pkt_queue_;
+    std::queue<PktPtr> pkt_queue_;
 
     /// \brief Mutex for controlling access to the queue.
-    mutex pkt_queue_mutex_;
+    std::mutex pkt_queue_mutex_;
 
     /// \brief Single- or thread-mode indicator.
     bool single_threaded_;
diff --git a/src/bin/perfdhcp/stats_mgr.h b/src/bin/perfdhcp/stats_mgr.h
index cf69c760dc..00a58904f9 100644
--- a/src/bin/perfdhcp/stats_mgr.h
+++ b/src/bin/perfdhcp/stats_mgr.h
@@ -684,7 +684,7 @@ public:
             using namespace std;
             auto sent = getSentPacketsNum();
             auto drops = getDroppedPacketsNum();
-            double drops_ratio = 100 * static_cast<double>(drops) / static_cast<double>(sent);
+            double drops_ratio = 100.0 * static_cast<double>(drops) / static_cast<double>(sent);
 
             cout << "sent packets: " << sent << endl
                  << "received packets: " << getRcvdPacketsNum() << endl
diff --git a/src/bin/perfdhcp/test_control.cc b/src/bin/perfdhcp/test_control.cc
index d890edae5b..c1a9b665bc 100644
--- a/src/bin/perfdhcp/test_control.cc
+++ b/src/bin/perfdhcp/test_control.cc
@@ -6,6 +6,12 @@
 
 #include <config.h>
 
+#include <perfdhcp/test_control.h>
+#include <perfdhcp/receiver.h>
+#include <perfdhcp/command_options.h>
+#include <perfdhcp/perf_pkt4.h>
+#include <perfdhcp/perf_pkt6.h>
+
 #include <exceptions/exceptions.h>
 #include <asiolink/io_address.h>
 #include <dhcp/libdhcp++.h>
@@ -13,14 +19,8 @@
 #include <dhcp/dhcp4.h>
 #include <dhcp/option6_ia.h>
 #include <util/unittests/check_valgrind.h>
-#include "test_control.h"
-#include "receiver.h"
-#include "command_options.h"
-#include "perf_pkt4.h"
-#include "perf_pkt6.h"
 
 #include <boost/date_time/posix_time/posix_time.hpp>
-
 #include <algorithm>
 #include <fstream>
 #include <stdio.h>
@@ -1235,10 +1235,12 @@ TestControl::processReceivedPacket6(const BetterSocket& socket,
     }
 }
 
-void
+unsigned int
 TestControl::consumeReceivedPackets(Receiver& receiver, const BetterSocket& socket) {
+    unsigned int pkt_count = 0;
     PktPtr pkt;
     while ((pkt = receiver.getPkt())) {
+        pkt_count += 1;
         if (CommandOptions::instance().getIpVersion() == 4) {
             Pkt4Ptr pkt4 = boost::dynamic_pointer_cast<Pkt4>(pkt);
             processReceivedPacket4(socket, pkt4);
@@ -1247,6 +1249,7 @@ TestControl::consumeReceivedPackets(Receiver& receiver, const BetterSocket& sock
             processReceivedPacket6(socket, pkt6);
         }
     }
+    return pkt_count;
 }
 void
 TestControl::registerOptionFactories4() const {
@@ -1421,7 +1424,14 @@ TestControl::run() {
 
         // Pull some packets from receiver thread, process them, update some stats
         // and respond to the server if needed.
-        consumeReceivedPackets(receiver, socket);
+        auto pkt_count = consumeReceivedPackets(receiver, socket);
+
+        // If there is nothing to do in this loop iteration then do some sleep to make
+        // CPU idle for a moment, to not consume 100% CPU all the time
+        // but only if it is not that high request rate expected.
+        if (options.getRate() < 10000 && packets_due == 0 && pkt_count == 0) {
+            usleep(1);
+        }
 
         // If test period finished, maximum number of packet drops
         // has been reached or test has been interrupted we have to
diff --git a/src/bin/perfdhcp/test_control.h b/src/bin/perfdhcp/test_control.h
index cbbe403fdf..2c40b36a62 100644
--- a/src/bin/perfdhcp/test_control.h
+++ b/src/bin/perfdhcp/test_control.h
@@ -7,10 +7,10 @@
 #ifndef TEST_CONTROL_H
 #define TEST_CONTROL_H
 
-#include "packet_storage.h"
-#include "rate_control.h"
-#include "stats_mgr.h"
-#include "receiver.h"
+#include <perfdhcp/packet_storage.h>
+#include <perfdhcp/rate_control.h>
+#include <perfdhcp/stats_mgr.h>
+#include <perfdhcp/receiver.h>
 
 #include <dhcp/iface_mgr.h>
 #include <dhcp/dhcp6.h>
@@ -523,7 +523,7 @@ protected:
     /// \brief Pull packets from receiver and process them.
 
     /// It runs in a loop until there are no packets in receiver.
-    void consumeReceivedPackets(Receiver& receiver, const BetterSocket& socket);
+    unsigned int consumeReceivedPackets(Receiver& receiver, const BetterSocket& socket);
 
     /// \brief Process received DHCPv4 packet.
     ///
diff --git a/src/bin/perfdhcp/tests/Makefile.am b/src/bin/perfdhcp/tests/Makefile.am
index ca65a9d2a1..73c78ee8d0 100644
--- a/src/bin/perfdhcp/tests/Makefile.am
+++ b/src/bin/perfdhcp/tests/Makefile.am
@@ -1,6 +1,6 @@
 SUBDIRS = . testdata
 
-AM_CPPFLAGS = -I$(top_builddir)/src/lib -I$(top_srcdir)/src/lib
+AM_CPPFLAGS = -I$(top_builddir)/src/lib -I$(top_srcdir)/src/lib -I$(top_builddir)/src/bin  -I$(top_srcdir)/src/bin
 AM_CPPFLAGS += -I$(srcdir)/.. -I$(builddir)/..
 AM_CPPFLAGS += -DTEST_DATA_DIR=\"$(abs_srcdir)/testdata\"
 AM_CPPFLAGS += $(BOOST_INCLUDES)
diff --git a/src/bin/perfdhcp/tests/rate_control_unittest.cc b/src/bin/perfdhcp/tests/rate_control_unittest.cc
index 43b695a38a..69e6db5099 100644
--- a/src/bin/perfdhcp/tests/rate_control_unittest.cc
+++ b/src/bin/perfdhcp/tests/rate_control_unittest.cc
@@ -53,9 +53,6 @@ TEST(RateControl, constructor) {
     // will be set correctly.
     NakedRateControl rc2(5);
     EXPECT_EQ(5, rc2.getRate());
-
-    // The negative value of rate is not acceptable.
-    EXPECT_THROW(RateControl(-1), isc::BadValue);
 }
 
 // Check the rate accessor.