From: Marcin Siodelski Date: Fri, 30 Jun 2017 13:23:04 +0000 (+0200) Subject: [5317] Addressed review comments. X-Git-Tag: trac5227_base~26^2 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=c7f86fdad0ab748098ddd2accb6d5b5fd649228b;p=thirdparty%2Fkea.git [5317] Addressed review comments. --- diff --git a/src/bin/dhcp4/tests/ctrl_dhcp4_srv_unittest.cc b/src/bin/dhcp4/tests/ctrl_dhcp4_srv_unittest.cc index 918455df2b..cc3d3da9b9 100644 --- a/src/bin/dhcp4/tests/ctrl_dhcp4_srv_unittest.cc +++ b/src/bin/dhcp4/tests/ctrl_dhcp4_srv_unittest.cc @@ -130,7 +130,7 @@ public: // make the actual configuration text. std::string config_txt = header + socket_path_ + footer; - server_.reset(new NakedControlledDhcpv4Srv()); + ASSERT_NO_THROW(server_.reset(new NakedControlledDhcpv4Srv())); ConstElementPtr config; ASSERT_NO_THROW(config = parseDHCP4(config_txt)); @@ -190,11 +190,14 @@ public: client.reset(new UnixControlClient()); ASSERT_TRUE(client); - // Connect. + // Connect to the server. This is expected to trigger server's acceptor + // handler when IOService::poll() is run. ASSERT_TRUE(client->connectToServer(socket_path_)); ASSERT_NO_THROW(getIOService()->poll()); - // Send the command. + // Send the command. This will trigger server's handler which receives + // data over the unix domain socket. The server will start sending + // response to the client. ASSERT_TRUE(client->sendCommand(command)); ASSERT_NO_THROW(getIOService()->poll()); @@ -683,7 +686,7 @@ TEST_F(CtrlChannelDhcpv4SrvTest, configSet) { EXPECT_EQ("{ \"result\": 0, \"text\": \"Configuration successful.\" }", response); - /// Check that the config was indeed applied. + // Check that the config was indeed applied. const Subnet4Collection* subnets = CfgMgr::instance().getCurrentCfg()->getCfgSubnets4()->getAll(); EXPECT_EQ(1, subnets->size()); @@ -1080,7 +1083,7 @@ TEST_F(CtrlChannelDhcpv4SrvTest, concurrentConnections) { ASSERT_TRUE(client1); boost::scoped_ptr client2(new UnixControlClient()); - ASSERT_TRUE(client1); + ASSERT_TRUE(client2); // Client 1 connects. ASSERT_TRUE(client1->connectToServer(socket_path_)); diff --git a/src/bin/dhcp6/tests/ctrl_dhcp6_srv_unittest.cc b/src/bin/dhcp6/tests/ctrl_dhcp6_srv_unittest.cc index eae45a4a30..a477fdefdb 100644 --- a/src/bin/dhcp6/tests/ctrl_dhcp6_srv_unittest.cc +++ b/src/bin/dhcp6/tests/ctrl_dhcp6_srv_unittest.cc @@ -200,11 +200,14 @@ public: client.reset(new UnixControlClient()); ASSERT_TRUE(client); - // Connect. + // Connect to the server. This is expected to trigger server's acceptor + // handler when IOService::poll() is run. ASSERT_TRUE(client->connectToServer(socket_path_)); ASSERT_NO_THROW(getIOService()->poll()); - // Send the command. + // Send the command. This will trigger server's handler which receives + // data over the unix domain socket. The server will start sending + // response to the client. ASSERT_TRUE(client->sendCommand(command)); ASSERT_NO_THROW(getIOService()->poll()); @@ -1102,7 +1105,7 @@ TEST_F(CtrlChannelDhcpv6SrvTest, concurrentConnections) { ASSERT_TRUE(client1); boost::scoped_ptr client2(new UnixControlClient()); - ASSERT_TRUE(client1); + ASSERT_TRUE(client2); // Client 1 connects. ASSERT_TRUE(client1->connectToServer(socket_path_)); diff --git a/src/lib/asiolink/io_acceptor.h b/src/lib/asiolink/io_acceptor.h index 4597f61d41..c493d3427e 100644 --- a/src/lib/asiolink/io_acceptor.h +++ b/src/lib/asiolink/io_acceptor.h @@ -22,6 +22,11 @@ namespace asiolink { /// This is a wrapper class for ASIO acceptor service. Classes implementing /// services for specific protocol types should derive from this class. /// +/// Acceptor is an IO object which accepts incoming connections into a socket +/// object. This socket is then used for data transmission from the client +/// to server and back. The acceptor is continued to be used to accept new +/// connections while the accepted connection is active. +/// /// @tparam ProtocolType ASIO protocol type, e.g. stream_protocol /// @tparam CallbackType Callback function type which should have the following /// signature: @c void(const boost::system::error_code&). diff --git a/src/lib/asiolink/unix_domain_socket_acceptor.h b/src/lib/asiolink/unix_domain_socket_acceptor.h index 1cec340c67..8aa11cadca 100644 --- a/src/lib/asiolink/unix_domain_socket_acceptor.h +++ b/src/lib/asiolink/unix_domain_socket_acceptor.h @@ -19,6 +19,9 @@ namespace isc { namespace asiolink { /// @brief Implements acceptor service for @ref UnixDomainSocket. +/// +/// This class is used to accept new incoming connections over unix domain +/// sockets. class UnixDomainSocketAcceptor : public IOAcceptor > { public: diff --git a/src/lib/asiolink/unix_domain_socket_endpoint.h b/src/lib/asiolink/unix_domain_socket_endpoint.h index 85f019cf5f..9378a8371e 100644 --- a/src/lib/asiolink/unix_domain_socket_endpoint.h +++ b/src/lib/asiolink/unix_domain_socket_endpoint.h @@ -18,7 +18,9 @@ namespace asiolink { /// @brief Endpoint for @ref UnixDomainSocket. /// -/// This is a simple class encapsulating ASIO unix domain socket. +/// This is a simple class encapsulating ASIO unix domain socket endpoint. +/// It is used to represent endpoints taking part in communication via +/// unix domain sockets. class UnixDomainSocketEndpoint { public: diff --git a/src/lib/config/command_mgr.cc b/src/lib/config/command_mgr.cc index f04a88470f..2be8124e94 100644 --- a/src/lib/config/command_mgr.cc +++ b/src/lib/config/command_mgr.cc @@ -200,32 +200,33 @@ Connection::receiveHandler(const boost::system::error_code& ec, // No response generated. Connection will be closed. if (!rsp) { LOG_WARN(command_logger, COMMAND_RESPONSE_ERROR); + rsp = createAnswer(CONTROL_RESULT_ERROR, + "internal server error: no response generated"); - } else { + } - // Let's convert JSON response to text. Note that at this stage - // the rsp pointer is always set. - std::string txt = rsp->str(); - size_t len = txt.length(); - if (len > 65535) { - // Hmm, our response is too large. Let's send the first - // 64KB and hope for the best. - LOG_ERROR(command_logger, COMMAND_SOCKET_RESPONSE_TOOLARGE).arg(len); + // Let's convert JSON response to text. Note that at this stage + // the rsp pointer is always set. + std::string txt = rsp->str(); + size_t len = txt.length(); + if (len > 65535) { + // Hmm, our response is too large. Let's send the first + // 64KB and hope for the best. + LOG_ERROR(command_logger, COMMAND_SOCKET_RESPONSE_TOOLARGE).arg(len); - len = 65535; - } + len = 65535; + } - try { - // Send the data back over socket. - socket_->write(txt.c_str(), len); + try { + // Send the data back over socket. + socket_->write(txt.c_str(), len); - } catch (const std::exception& ex) { - // Response transmission failed. Since the response failed, it doesn't - // make sense to send any status codes. Let's log it and be done with - // it. - LOG_ERROR(command_logger, COMMAND_SOCKET_WRITE_FAIL) - .arg(len).arg(socket_->getNative()).arg(ex.what()); - } + } catch (const std::exception& ex) { + // Response transmission failed. Since the response failed, it doesn't + // make sense to send any status codes. Let's log it and be done with + // it. + LOG_ERROR(command_logger, COMMAND_SOCKET_WRITE_FAIL) + .arg(len).arg(socket_->getNative()).arg(ex.what()); } connection_pool_.stop(shared_from_this()); diff --git a/src/lib/dhcpsrv/tests/timer_mgr_unittest.cc b/src/lib/dhcpsrv/tests/timer_mgr_unittest.cc index d84215d4e9..e0d95dd2fc 100644 --- a/src/lib/dhcpsrv/tests/timer_mgr_unittest.cc +++ b/src/lib/dhcpsrv/tests/timer_mgr_unittest.cc @@ -367,4 +367,11 @@ TEST_F(TimerMgrTest, callbackWithException) { doWait(500); } +// This test verifies that IO service specified for the TimerMgr +// must not be null. +TEST_F(TimerMgrTest, setIOService) { + EXPECT_THROW(timer_mgr_->setIOService(IOServicePtr()), + BadValue); +} + } // end of anonymous namespace diff --git a/src/lib/dhcpsrv/timer_mgr.cc b/src/lib/dhcpsrv/timer_mgr.cc index 6590688417..aaf7c729cf 100644 --- a/src/lib/dhcpsrv/timer_mgr.cc +++ b/src/lib/dhcpsrv/timer_mgr.cc @@ -160,6 +160,9 @@ TimerMgrImpl::TimerMgrImpl() : void TimerMgrImpl::setIOService(const IOServicePtr& io_service) { + if (!io_service) { + isc_throw(BadValue, "IO service object must not be null for TimerMgr"); + } io_service_ = io_service; } diff --git a/src/lib/dhcpsrv/timer_mgr.h b/src/lib/dhcpsrv/timer_mgr.h index 63ca7bb9b4..28ba36f85d 100644 --- a/src/lib/dhcpsrv/timer_mgr.h +++ b/src/lib/dhcpsrv/timer_mgr.h @@ -24,7 +24,7 @@ class TimerMgr; /// @brief Type definition of the shared pointer to @c TimerMgr. typedef boost::shared_ptr TimerMgrPtr; -/// @brief Manages a pool of asynchronous interval timers for Kea server. +/// @brief Manages a pool of asynchronous interval timers. /// /// This class holds a pool of asynchronous interval timers. ///