From: Thomas Markwalder Date: Thu, 18 Mar 2021 19:05:09 +0000 (-0400) Subject: [#1730] Address cosmetic review comments X-Git-Tag: Kea-1.9.6~155 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=4b00eb229bb35010ab46b8dd5c5600001a86fb18;p=thirdparty%2Fkea.git [#1730] Address cosmetic review comments 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 --- diff --git a/src/lib/Makefile.am b/src/lib/Makefile.am index 9cecf356c3..7189389916 100644 --- a/src/lib/Makefile.am +++ b/src/lib/Makefile.am @@ -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 diff --git a/src/lib/config/cmd_http_listener.cc b/src/lib/config/cmd_http_listener.cc index c40adc2202..cd469a8046 100644 --- a/src/lib/config/cmd_http_listener.cc +++ b/src/lib/config/cmd_http_listener.cc @@ -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 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(); diff --git a/src/lib/config/cmd_http_listener.h b/src/lib/config/cmd_http_listener.h index ce3b2a550a..a9191bb51e 100644 --- a/src/lib/config/cmd_http_listener.h +++ b/src/lib/config/cmd_http_listener.h @@ -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 CmdHttpListenerPtr; -}; // namespace isc::config -}; // namespace isc +} // namespace isc::config +} // namespace isc #endif // CMD_HTTP_LISTENER_H diff --git a/src/lib/config/cmd_response_creator.h b/src/lib/config/cmd_response_creator.h index 2a6d8973a3..136d721388 100644 --- a/src/lib/config/cmd_response_creator.h +++ b/src/lib/config/cmd_response_creator.h @@ -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: diff --git a/src/lib/config/cmd_response_creator_factory.h b/src/lib/config/cmd_response_creator_factory.h index 4b25f47cac..f9c412692f 100644 --- a/src/lib/config/cmd_response_creator_factory.h +++ b/src/lib/config/cmd_response_creator_factory.h @@ -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)) { } diff --git a/src/lib/config/tests/cmd_http_listener_unittests.cc b/src/lib/config/tests/cmd_http_listener_unittests.cc index c0f9184ca0..0594bcd890 100644 --- a/src/lib/config/tests/cmd_http_listener_unittests.cc +++ b/src/lib/config/tests/cmd_http_listener_unittests.cc @@ -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': 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 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) { diff --git a/src/lib/config/tests/cmd_response_creator_unittests.cc b/src/lib/config/tests/cmd_response_creator_unittests.cc index 125da71cf8..d2126e5e88 100644 --- a/src/lib/config/tests/cmd_response_creator_unittests.cc +++ b/src/lib/config/tests/cmd_response_creator_unittests.cc @@ -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) { diff --git a/src/lib/http/tests/test_http_client.h b/src/lib/http/tests/test_http_client.h index c5ba0eb1ac..f249d9f48d 100644 --- a/src/lib/http/tests/test_http_client.h +++ b/src/lib/http/tests/test_http_client.h @@ -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_; };