]> git.ipfire.org Git - thirdparty/kea.git/commitdiff
[#1730] Address cosmetic review comments
authorThomas Markwalder <tmark@isc.org>
Thu, 18 Mar 2021 19:05:09 +0000 (15:05 -0400)
committerThomas Markwalder <tmark@isc.org>
Mon, 22 Mar 2021 17:51:50 +0000 (13:51 -0400)
src/lib/Makefile.am
    - fixed over-looked library order issue, http must build
    before config

src/lib/config/cmd_http_listener.cc
src/lib/config/cmd_http_listener.h
src/lib/config/cmd_response_creator.h
src/lib/config/cmd_response_creator_factory.h
src/lib/config/tests/cmd_http_listener_unittests.cc
src/lib/config/tests/cmd_response_creator_unittests.cc
src/lib/http/tests/test_http_client.h
    - Doxygen and formatting corrections

src/lib/Makefile.am
src/lib/config/cmd_http_listener.cc
src/lib/config/cmd_http_listener.h
src/lib/config/cmd_response_creator.h
src/lib/config/cmd_response_creator_factory.h
src/lib/config/tests/cmd_http_listener_unittests.cc
src/lib/config/tests/cmd_response_creator_unittests.cc
src/lib/http/tests/test_http_client.h

index 9cecf356c390c6bc47e02c6077b8e91598b6b960..7189389916ec59b48adbaf6353142a6a44c73200 100644 (file)
@@ -13,10 +13,10 @@ if HAVE_CQL
 SUBDIRS += cql
 endif
 
-SUBDIRS += config_backend hooks dhcp config stats
+SUBDIRS += config_backend hooks dhcp http config stats
 
 if HAVE_SYSREPO
 SUBDIRS += yang
 endif
 
-SUBDIRS += asiodns dhcp_ddns eval cfgrpt process dhcpsrv http
+SUBDIRS += asiodns dhcp_ddns eval cfgrpt process dhcpsrv
index c40adc2202080fc76889c9c5e9becf67ed756357..cd469a80462178df7080d0969ea314ec2a81e7ee 100644 (file)
@@ -28,7 +28,8 @@ namespace config {
 
 CmdHttpListener::CmdHttpListener(const IOAddress& address, const uint16_t port,
                                  const uint16_t thread_pool_size /* = 1 */)
-    : address_(address), port_(port), http_listener_(), thread_pool_size_(thread_pool_size), threads_() {
+    : address_(address), port_(port), io_service_(), http_listener_(),
+      thread_pool_size_(thread_pool_size), threads_() {
 }
 
 CmdHttpListener::~CmdHttpListener() {
@@ -64,8 +65,7 @@ CmdHttpListener::start() {
                                               HttpListener::IdleTimeout(TIMEOUT_AGENT_IDLE_CONNECTION_TIMEOUT)));
 
         // Create a pool of threads, each calls run on our IOService_service instance.
-        for (std::size_t i = 0; i < thread_pool_size_; ++i)
-        {
+        for (std::size_t i = 0; i < thread_pool_size_; ++i) {
             boost::shared_ptr<std::thread> thread(new std::thread(
                 std::bind(&IOService::run, io_service_)));
             threads_.push_back(thread);
@@ -100,8 +100,8 @@ CmdHttpListener::stop() {
     io_service_->stop();
 
     // Stop the threads next.
-    for (std::size_t i = 0; i < threads_.size(); ++i) {
-            threads_[i]->join();
+    for (auto const& thread : threads_) {
+        thread->join();
     }
 
     threads_.clear();
index ce3b2a550a2fe72e8dc32d7e762dec8c27879a0a..a9191bb51e9846c7c72008e83c59d9456bf457b6 100644 (file)
@@ -45,21 +45,21 @@ public:
 
     /// @brief Fetches the port number on which to listen.
     ///
-    /// @return unit16_t containing the port number on which to listen.
+    /// @return uint16_t containing the port number on which to listen.
     uint16_t getPort() {
         return (port_);
     }
 
     /// @brief Fetches the maximum size of the thread pool.
     ///
-    /// @return unit16_t containing the maximum size of the thread pool.
+    /// @return uint16_t containing the maximum size of the thread pool.
     uint16_t getThreadPoolSize() {
         return (thread_pool_size_);
     }
 
     /// @brief Fetches the number of threads in the pool.
     ///
-    /// @return unit16_t containing the number of running threads.
+    /// @return uint16_t containing the number of running threads.
     uint16_t getThreadCount() {
         return (threads_.size());
     }
@@ -87,7 +87,7 @@ private:
 /// @brief Defines a shared pointer to CmdHttpListener.
 typedef boost::shared_ptr<CmdHttpListener> CmdHttpListenerPtr;
 
-}; // namespace isc::config
-}; // namespace isc
+} // namespace isc::config
+} // namespace isc
 
 #endif // CMD_HTTP_LISTENER_H
index 2a6d8973a3c7e71b33e52d2ac0fb1a4113856fc0..136d7213882407ceee96a5f6c2bf77566003ee94 100644 (file)
@@ -35,9 +35,9 @@ public:
     ///
     /// @param emulate_agent_response if true, responses for normal
     /// command outcomes are guaranteed to be wrapped in an Element::list.
-    /// This emulates how kea-ctrl-agent forms responses.  Defualts to true.
+    /// This emulates how kea-ctrl-agent forms responses.  Defaults to true.
     CmdResponseCreator(bool emulate_agent_response = true)
-    : emulate_agent_response_(emulate_agent_response){};
+    : emulate_agent_response_(emulate_agent_response) {};
 
     /// @brief Create a new request.
     ///
@@ -68,14 +68,14 @@ public:
     /// @return an empty HttpAuthConfigPtr.
     const http::HttpAuthConfigPtr& getHttpAuthConfig() {
         static http::HttpAuthConfigPtr no_config;
-        return(no_config);
+        return (no_config);
     }
 
     /// @brief Indicates whether or not agent response emulation is enabled.
     ///
     /// @return true if emulation is enabled.
     bool emulateAgentResponse() {
-        return(emulate_agent_response_);
+        return (emulate_agent_response_);
     }
 
 private:
index 4b25f47cac5de7c2a0a2d0eb170de3efcfd87f03..f9c412692f15d15714e0da3508e17cb60d516bca 100644 (file)
@@ -38,7 +38,7 @@ public:
     ///
     /// @param emulate_agent_response if true, responses for normal
     /// command outcomes are guaranteed to be wrapped in an Element::list.
-    /// This emulates how kea-ctrl-agent forms responses.  Defualts to true.
+    /// This emulates how kea-ctrl-agent forms responses.  Defaults to true.
     CmdResponseCreatorFactory(bool emulate_agent_response = true)
         : sole_creator_(new CmdResponseCreator(emulate_agent_response)) {
     }
index c0f9184ca07544277effba15bb7c3a042cf770eb..0594bcd89064123bf7e2781b7d6e81b45a61359d 100644 (file)
@@ -43,80 +43,6 @@ const unsigned short SERVER_PORT = 18123;
 /// @brief Test timeout (ms).
 const long TEST_TIMEOUT = 10000;
 
-/// Verifies the construction, starting, stopping, and destruction
-/// of CmdHttpListener.
-TEST(CmdHttpListener, basics) {
-    CmdHttpListenerPtr listener;
-    asiolink::IOAddress address(SERVER_ADDRESS);
-    uint16_t port = SERVER_PORT;
-
-    // Make sure we can create one.
-    ASSERT_NO_THROW_LOG(listener.reset(new CmdHttpListener(address, port)));
-    ASSERT_TRUE(listener);
-
-    // Verify the getters get what we expect.
-    EXPECT_EQ(listener->getAddress(), address);
-    EXPECT_EQ(listener->getPort(), port);
-    EXPECT_EQ(listener->getThreadPoolSize(), 1);
-
-    // It should not be listening and have no threads.
-    EXPECT_FALSE(listener->isListening());
-    EXPECT_EQ(listener->getThreadCount(), 0);
-
-    // Verify that we cannot start it when multi-threading is disabled.
-    ASSERT_FALSE(MultiThreadingMgr::instance().getMode());
-    ASSERT_THROW_MSG(listener->start(), InvalidOperation,
-                     "CmdHttpListener cannot be started"
-                     " when multi-threading is disabled");
-
-    // It should still not be listening and have no threads.
-    EXPECT_FALSE(listener->isListening());
-    EXPECT_EQ(listener->getThreadCount(), 0);
-
-    // Enable multi-threading.
-    MultiThreadingMgr::instance().setMode(true);
-
-    // Make sure we can start it and it's listening with 1 thread.
-    ASSERT_NO_THROW_LOG(listener->start());
-    ASSERT_TRUE(listener->isListening());
-    EXPECT_EQ(listener->getThreadCount(), 1);
-
-    // Trying to start it again should fail.
-    ASSERT_THROW_MSG(listener->start(), InvalidOperation,
-                     "CmdHttpListener is already listening!");
-
-    // Stop it and verify we're no longer listening.
-    ASSERT_NO_THROW_LOG(listener->stop());
-    ASSERT_FALSE(listener->isListening());
-    EXPECT_EQ(listener->getThreadCount(), 0);
-
-    // Make sure we can call stop again without problems.
-    ASSERT_NO_THROW_LOG(listener->stop());
-
-    // We should be able to restart it.
-    ASSERT_NO_THROW_LOG(listener->start());
-    ASSERT_TRUE(listener->isListening());
-    EXPECT_EQ(listener->getThreadCount(), 1);
-
-    // Destroying it should also stop it.
-    // If the test timeouts we know it didn't!
-    ASSERT_NO_THROW_LOG(listener.reset());
-
-    // Verify we can construct with more than one thread.
-    ASSERT_NO_THROW_LOG(listener.reset(new CmdHttpListener(address, port, 4)));
-    ASSERT_NO_THROW_LOG(listener->start());
-    EXPECT_EQ(listener->getAddress(), address);
-    EXPECT_EQ(listener->getPort(), port);
-    EXPECT_EQ(listener->getThreadPoolSize(), 4);
-    ASSERT_TRUE(listener->isListening());
-    EXPECT_EQ(listener->getThreadCount(), 4);
-
-    // Stop it and verify we're no longer listening.
-    ASSERT_NO_THROW_LOG(listener->stop());
-    ASSERT_FALSE(listener->isListening());
-    EXPECT_EQ(listener->getThreadCount(), 0);
-}
-
 /// @brief Test fixture class for @ref CmdHttpListener.
 class CmdHttpListenerTest : public ::testing::Test {
 public:
@@ -134,7 +60,7 @@ public:
         // Deregisters commands.
         CommandMgr::instance().deregisterAll();
 
-        // Ensure we're in MT mode.
+        // Enable multi-threading.
         MultiThreadingMgr::instance().setMode(true);
     }
 
@@ -143,25 +69,29 @@ public:
     /// Removes HTTP clients, unregisters commands, disables MT.
     virtual ~CmdHttpListenerTest() {
         // Destroy all remaining clients.
-        for (auto client = clients_.begin(); client != clients_.end();
-             ++client) {
-            (*client)->close();
+        for (auto const& client : clients_) {
+            client->close();
         }
 
         // Deregisters commands.
         config::CommandMgr::instance().deregisterAll();
 
-        // Shut of MT.
+        // Disable multi-threading.
         MultiThreadingMgr::instance().setMode(false);
     }
 
+    /// @brief Constructs a complete HTTP POST given a request body.
+    ///
+    /// @param request_body string containing the desired request body.
+    ///
+    /// @return string containing the constructed POST.
     std::string buildPostStr(const std::string& request_body) {
         // Create the command string.
         std::stringstream ss;
         ss << "POST /foo/bar HTTP/1.1\r\n"
               "Content-Type: application/json\r\n"
               "Content-Length: "
-              << request_body.size()<< "\r\n\r\n"
+              << request_body.size() << "\r\n\r\n"
               << request_body;
         return (ss.str());
     }
@@ -174,7 +104,7 @@ public:
     /// thread and be driven by the test's IOService instance.
     ///
     /// @param request_body JSON String containing the API command
-    /// to be be sent.
+    /// to be sent.
     void startRequest(const std::string& request_body = "{ }") {
         std::string request_str = buildPostStr(request_body);
 
@@ -258,7 +188,7 @@ public:
 
             // If all the clients are done receiving, the test is done.
             keep_going = false;
-            for ( auto client : clients_ ) {
+            for (auto const& client : clients_) {
                 if (!client->receiveDone()) {
                     keep_going = true;
                     break;
@@ -269,7 +199,7 @@ public:
 
     /// @brief Create an HttpResponse from a response string.
     ///
-    /// @param response_str  a string containing the whole HTTP
+    /// @param response_str a string containing the whole HTTP
     /// response received.
     ///
     /// @return An HttpResponse constructed from by parsing the
@@ -316,7 +246,7 @@ public:
     /// @return Returns response with map of arguments containing
     /// a string value 'thread-id': <thread id>
     ConstElementPtr threadCommandHandler(const std::string& /*command_name*/,
-                                       const ConstElementPtr& command_arguments) {
+                                         const ConstElementPtr& command_arguments) {
         // If the number of in progress commands is less than the number
         // of threads, then wait here until we're notified.  Otherwise,
         // notify everyone and finish.  The idea is to force each thread
@@ -352,7 +282,7 @@ public:
         return (createAnswer(CONTROL_RESULT_SUCCESS, arguments));
     }
 
-    /// @brief Submits one or more thread commands to a CmdHttpListener
+    /// @brief Submits one or more thread commands to a CmdHttpListener.
     ///
     /// This function command will creates a CmdHttpListener
     /// with the given number of threads, initiates the given
@@ -371,7 +301,6 @@ public:
     /// thread command.  Each client is used to carry out a single thread
     /// command request.  Must be greater than 0 and a multiple of num_threads
     /// if it is greater than num_threads.
-    ///
     void threadListenAndRespond(size_t num_threads, size_t num_clients) {
         // First we makes sure the parameter rules apply.
         ASSERT_TRUE(num_threads > 0);
@@ -403,7 +332,7 @@ public:
 
         // Initiate the prescribed number of command requests.
         num_in_progress_ = 0;
-        for ( auto i = 0; clients_.size() < num_clients; ++i) {
+        for (auto i = 0; clients_.size() < num_clients; ++i) {
             ASSERT_NO_THROW_LOG(startThreadCommand());
         }
 
@@ -418,7 +347,7 @@ public:
 
         // Iterate over the clients, checking their outcomes.
         size_t total_responses = 0;
-        for (auto client : clients_) {
+        for (auto const& client : clients_) {
             // Client should have completed its receive successfully.
             ASSERT_TRUE(client->receiveDone());
 
@@ -500,7 +429,7 @@ public:
         ASSERT_EQ(clients_per_thread.size(), expected_thread_count);
 
         // Each thread-id ought to have handled the same number of clients.
-        for (auto it : clients_per_thread) {
+        for (auto const& it : clients_per_thread) {
             EXPECT_EQ(it.second, num_clients / clients_per_thread.size())
                       << "thread-id: " << it.first
                       << ", clients: " << it.second << std::endl;
@@ -520,13 +449,99 @@ public:
     /// @brief List of client connections.
     std::list<TestHttpClientPtr> clients_;
 
+    /// @brief Number of threads the listener should use for the test.
     size_t num_threads_;
+
+    /// @brief Number of client requests to make during the test.
     size_t num_clients_;
+
+    /// @brief Number of requests currently in progress.
     size_t num_in_progress_;
+
+    /// @brief Mutex used to lock during thread coordination.
     std::mutex mutex_;
+
+    /// @brief Condition variable used to coordinate threads.
     std::condition_variable cv_;
 };
 
+/// Verifies the construction, starting, stopping, and destruction
+/// of CmdHttpListener.
+TEST_F(CmdHttpListenerTest, basics) {
+    // Make sure multi-threading is off.
+    MultiThreadingMgr::instance().setMode(false);
+    CmdHttpListenerPtr listener;
+    asiolink::IOAddress address(SERVER_ADDRESS);
+    uint16_t port = SERVER_PORT;
+
+    // Make sure we can create one.
+    ASSERT_NO_THROW_LOG(listener.reset(new CmdHttpListener(address, port)));
+    ASSERT_TRUE(listener);
+
+    // Verify the getters do what we expect.
+    EXPECT_EQ(listener->getAddress(), address);
+    EXPECT_EQ(listener->getPort(), port);
+    EXPECT_EQ(listener->getThreadPoolSize(), 1);
+
+    // It should not be listening and have no threads.
+    EXPECT_FALSE(listener->isListening());
+    EXPECT_EQ(listener->getThreadCount(), 0);
+
+    // Verify that we cannot start it when multi-threading is disabled.
+    ASSERT_FALSE(MultiThreadingMgr::instance().getMode());
+    ASSERT_THROW_MSG(listener->start(), InvalidOperation,
+                     "CmdHttpListener cannot be started"
+                     " when multi-threading is disabled");
+
+    // It should still not be listening and have no threads.
+    EXPECT_FALSE(listener->isListening());
+    EXPECT_EQ(listener->getThreadCount(), 0);
+
+    // Enable multi-threading.
+    MultiThreadingMgr::instance().setMode(true);
+
+    // Make sure we can start it and it's listening with 1 thread.
+    ASSERT_NO_THROW_LOG(listener->start());
+    ASSERT_TRUE(listener->isListening());
+    EXPECT_EQ(listener->getThreadCount(), 1);
+
+    // Trying to start it again should fail.
+    ASSERT_THROW_MSG(listener->start(), InvalidOperation,
+                     "CmdHttpListener is already listening!");
+
+    // Stop it and verify we're no longer listening.
+    ASSERT_NO_THROW_LOG(listener->stop());
+    ASSERT_FALSE(listener->isListening());
+    EXPECT_EQ(listener->getThreadCount(), 0);
+
+    // Make sure we can call stop again without problems.
+    ASSERT_NO_THROW_LOG(listener->stop());
+
+    // We should be able to restart it.
+    ASSERT_NO_THROW_LOG(listener->start());
+    ASSERT_TRUE(listener->isListening());
+    EXPECT_EQ(listener->getThreadCount(), 1);
+
+    // Destroying it should also stop it.
+    // If the test timeouts we know it didn't!
+    ASSERT_NO_THROW_LOG(listener.reset());
+
+    // Verify we can construct with more than one thread.
+    ASSERT_NO_THROW_LOG(listener.reset(new CmdHttpListener(address, port, 4)));
+    ASSERT_NO_THROW_LOG(listener->start());
+    EXPECT_EQ(listener->getAddress(), address);
+    EXPECT_EQ(listener->getPort(), port);
+    EXPECT_EQ(listener->getThreadPoolSize(), 4);
+    ASSERT_TRUE(listener->isListening());
+    EXPECT_EQ(listener->getThreadCount(), 4);
+
+    // Stop it and verify we're no longer listening.
+    ASSERT_NO_THROW_LOG(listener->stop());
+    ASSERT_FALSE(listener->isListening());
+    EXPECT_EQ(listener->getThreadCount(), 0);
+}
+
+
 // This test verifies that an HTTP connection can be established and used to
 // transmit an HTTP request and receive the response.
 TEST_F(CmdHttpListenerTest, basicListenAndRespond) {
index 125da71cf89598547c5413738485a6afdfcbbbbd..d2126e5e88ea7de4bc70fb48dcab173240b3d466 100644 (file)
@@ -26,7 +26,7 @@ namespace ph = std::placeholders;
 namespace {
 
 /// @brief Test fixture class for @ref CmdResponseCreator.
-class CmdResponseCreatorTest :  public ::testing::Test {
+class CmdResponseCreatorTest : public ::testing::Test {
 public:
 
     /// @brief Constructor.
@@ -39,12 +39,19 @@ public:
         config::CommandMgr::instance().deregisterAll();
         // Register our "foo" command.
         config::CommandMgr::instance().
-            registerCommand("foo", std::bind(&CmdResponseCreatorTest::
-                                             fooCommandHandler,
+            registerCommand("foo", std::bind(&CmdResponseCreatorTest::fooCommandHandler,
                                              this, ph::_1, ph::_2));
     }
 
+    /// @brief Destructor.
+    ///
+    /// Removes registered commands from the command manager.
+    virtual ~CmdResponseCreatorTest() {
+        config::CommandMgr::instance().deregisterAll();
+    }
+
     /// @brief SetUp function that wraps call to initCreator.
+    ///
     /// Creates a default CmdResponseCreator and new HttpRequest.
     virtual void SetUp() {
         initCreator();
@@ -60,13 +67,6 @@ public:
         ASSERT_TRUE(request_) << "initCreator failed to create request";
     }
 
-    /// @brief Destructor.
-    ///
-    /// Removes registered commands from the command manager.
-    virtual ~CmdResponseCreatorTest() {
-        config::CommandMgr::instance().deregisterAll();
-    }
-
     /// @brief Fills request context with required data to create new request.
     ///
     /// @param request Request which context should be configured.
@@ -99,7 +99,7 @@ public:
         HttpResponsePtr response;
         ASSERT_NO_THROW(
             response = response_creator_->createStockHttpResponse(request_,
-                                                                 status_code)
+                                                                  status_code)
         );
         ASSERT_TRUE(response);
         HttpResponseJsonPtr response_json = boost::dynamic_pointer_cast<
@@ -221,7 +221,7 @@ TEST_F(CmdResponseCreatorTest, createDynamicHttpResponseNoEmulation) {
     // Response should be a map that is not enclosed in a list.
     ASSERT_FALSE(response_creator_->emulateAgentResponse());
     ASSERT_TRUE(response_json->getBodyAsJson()->getType() == Element::map)
-                << "response is not a list: " << response_json->toString();
+                << "response is not a map: " << response_json->toString();
 
     // Response must be successful.
     EXPECT_TRUE(response_json->toString().find("HTTP/1.1 200 OK") !=
@@ -232,7 +232,6 @@ TEST_F(CmdResponseCreatorTest, createDynamicHttpResponseNoEmulation) {
                 std::string::npos);
 }
 
-
 // This test verifies that Internal Server Error is returned when invalid C++
 // request type is used. This is considered an error in the server logic.
 TEST_F(CmdResponseCreatorTest, createDynamicHttpResponseInvalidType) {
index c5ba0eb1ac0125d09a3e849efeb136d2ab42d4f3..f249d9f48df09b7f18ca70f1ad61b748a5ef22b9 100644 (file)
@@ -29,6 +29,8 @@ public:
     /// connect() to connect to the server.
     ///
     /// @param io_service IO service to be stopped on error or completion.
+    /// @param server_address string containing the IP address of the server.
+    /// @param port port number of the server.
     explicit TestHttpClient(IOService& io_service,
                             const std::string& server_address = "127.0.0.1",
                             uint16_t port = 18123)
@@ -129,7 +131,6 @@ public:
             if (ec) {
                 // IO service stopped so simply return.
                 if (ec.value() == boost::asio::error::operation_aborted) {
-                    std::cout << "this: " << this << "IO service stopped" << std::endl;
                     return;
 
                 } else if ((ec.value() == boost::asio::error::try_again) ||
@@ -227,11 +228,17 @@ public:
         socket_.close();
     }
 
+    /// @brief Returns the HTTP response string.
+    ///
+    /// @retrurn string containg the response.
     std::string getResponse() const {
         return (response_);
     }
 
     /// @brief Returns true if the receive completed without error.
+    ///
+    /// @return True if the receive completed succesfully, false
+    /// otherwise.
     bool receiveDone() {
         return (receive_done_);
     }
@@ -256,7 +263,7 @@ private:
     /// @brief IP port of the server.
     uint16_t server_port_;
 
-    /// @brief Set to true when the receive as completed successfully.
+    /// @brief Set to true when the receive has completed successfully.
     bool receive_done_;
 };