From: Michal 'vorner' Vaner Date: Tue, 3 Jan 2012 12:37:31 +0000 (+0100) Subject: [805] Details based on review X-Git-Tag: trac2351_base~304^2~16^2~20 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=e031e1adcfa042da3cb6baa9834e42f573ea66fb;p=thirdparty%2Fkea.git [805] Details based on review * Use the address family instead of v6 boolean. * Check it by exception. * Documentation. --- diff --git a/src/lib/asiodns/dns_service.cc b/src/lib/asiodns/dns_service.cc index 964b77e706..c5a460c3e2 100644 --- a/src/lib/asiodns/dns_service.cc +++ b/src/lib/asiodns/dns_service.cc @@ -78,8 +78,8 @@ public: DNSLookup *lookup_; DNSAnswer *answer_; - template void addServerFromFD(int fd, bool v6) { - Ptr server(new Server(io_service_.get_io_service(), fd, v6, checkin_, + template void addServerFromFD(int fd, int af) { + Ptr server(new Server(io_service_.get_io_service(), fd, af, checkin_, lookup_, answer_)); (*server)(); servers_.push_back(server); @@ -196,12 +196,12 @@ DNSService::addServer(uint16_t port, const std::string& address) { impl_->addServer(port, convertAddr(address)); } -void DNSService::addServerTCP(int fd, bool v6) { - impl_->addServerFromFD(fd, v6); +void DNSService::addServerTCPFromFD(int fd, int af) { + impl_->addServerFromFD(fd, af); } -void DNSService::addServerUDP(int fd, bool v6) { - impl_->addServerFromFD(fd, v6); +void DNSService::addServerUDPFromFD(int fd, int af) { + impl_->addServerFromFD(fd, af); } void diff --git a/src/lib/asiodns/dns_service.h b/src/lib/asiodns/dns_service.h index 75b688eb1f..d05a89bb3a 100644 --- a/src/lib/asiodns/dns_service.h +++ b/src/lib/asiodns/dns_service.h @@ -88,10 +88,34 @@ public: /// \brief Add another server to the service void addServer(uint16_t port, const std::string &address); void addServer(const char &port, const std::string &address); - /// \brief Add another server to the service from already opened file - /// descriptor - void addServerTCP(int fd, bool v6); - void addServerUDP(int fd, bool v6); + /// \brief Add another TCP server/listener to the service from already opened + /// file descriptor + /// + /// Adds a new TCP server using an already opened file descriptor (eg. it + /// only wraps it so the file descriptor is usable within the event loop). + /// + /// If you pass bad data (eg. opened UDP socket as fd), this will not detect + /// it right away, but successfull operations might fail. + /// + /// \param fd the file descriptor to be used. + /// \param af the address family of the file descriptor. Must be either + /// AF_INET or AF_INET6. + /// \throw isc::InvalidParameter if af is neither AF_INET nor AF_INET6. + void addServerTCPFromFD(int fd, int af); + /// \brief Add another UDP server to the service from already opened + /// file descriptor + /// + /// Adds a new UDP server using an already opened file descriptor (eg. it + /// only wraps it so the file descriptor is usable within the event loop). + /// + /// If you pass bad data (eg. opened TCP socket as fd), this will not detect + /// it right away, but successfull operations might fail. + /// + /// \param fd the file descriptor to be used. + /// \param af the address family of the file descriptor. Must be either + /// AF_INET or AF_INET6. + /// \throw isc::InvalidParameter if af is neither AF_INET nor AF_INET6. + void addServerUDPFromFD(int fd, int af); /// \brief Remove all servers from the service void clearServers(); diff --git a/src/lib/asiodns/tcp_server.cc b/src/lib/asiodns/tcp_server.cc index 05cf14041f..620696aa90 100644 --- a/src/lib/asiodns/tcp_server.cc +++ b/src/lib/asiodns/tcp_server.cc @@ -69,7 +69,7 @@ TCPServer::TCPServer(io_service& io_service, acceptor_->listen(); } -TCPServer::TCPServer(io_service& io_service, int fd, bool v6, +TCPServer::TCPServer(io_service& io_service, int fd, int af, const SimpleCallback* checkin, const DNSLookup* lookup, const DNSAnswer* answer) : @@ -77,8 +77,12 @@ TCPServer::TCPServer(io_service& io_service, int fd, bool v6, checkin_callback_(checkin), lookup_callback_(lookup), answer_callback_(answer) { + if (af != AF_INET && af != AF_INET6) { + isc_throw(InvalidParameter, "Address family must be either AF_INET " + "or AF_INET6, not " << af); + } acceptor_.reset(new tcp::acceptor(io_service)); - acceptor_->assign(v6 ? tcp::v6() : tcp::v4(), fd); + acceptor_->assign(af == AF_INET6 ? tcp::v6() : tcp::v4(), fd); acceptor_->listen(); } diff --git a/src/lib/asiodns/tcp_server.h b/src/lib/asiodns/tcp_server.h index c7fac8703a..116c709b4d 100644 --- a/src/lib/asiodns/tcp_server.h +++ b/src/lib/asiodns/tcp_server.h @@ -46,11 +46,12 @@ public: /// \brief Constructor /// \param io_service the asio::io_service to work with /// \param fd the file descriptor of opened TCP socket - /// \param v6 the socket in fd is ipv6 one (if false, it is ipv4) + /// \param af address family of the socket, either AF_INET or AF_INET6 /// \param checkin the callbackprovider for non-DNS events /// \param lookup the callbackprovider for DNS lookup events /// \param answer the callbackprovider for DNS answer events - TCPServer(asio::io_service& io_service, int fd, bool v6, + /// \throw isc::InvalidParameter if af is neither AF_INET nor AF_INET6 + TCPServer(asio::io_service& io_service, int fd, int af, const isc::asiolink::SimpleCallback* checkin = NULL, const DNSLookup* lookup = NULL, const DNSAnswer* answer = NULL); diff --git a/src/lib/asiodns/tests/dns_server_unittest.cc b/src/lib/asiodns/tests/dns_server_unittest.cc index e67f4a21e5..a64aa1b697 100644 --- a/src/lib/asiodns/tests/dns_server_unittest.cc +++ b/src/lib/asiodns/tests/dns_server_unittest.cc @@ -320,9 +320,17 @@ class TCPClient : public SimpleClient { // the same. class DNSServerTestBase : public::testing::Test { protected: + DNSServerTestBase() : + udp_server_(NULL), + tcp_server_(NULL) + { } void TearDown() { - udp_server_->stop(); - tcp_server_->stop(); + if (udp_server_) { + udp_server_->stop(); + } + if (tcp_server_) { + tcp_server_->stop(); + } delete checker_; delete lookup_; delete answer_; @@ -360,7 +368,7 @@ class DNSServerTestBase : public::testing::Test { answer_ = new SimpleAnswer(); udp_client_ = new UDPClient(service, ip::udp::endpoint(server_address_, - server_port)); + server_port)); tcp_client_ = new TCPClient(service, ip::tcp::endpoint(server_address_, server_port)); @@ -452,12 +460,12 @@ protected: commonSetup(); int fdUDP(getFd(SOCK_DGRAM)); ASSERT_NE(-1, fdUDP) << strerror(errno); - udp_server_ = new UDPServer(service, fdUDP, false, checker_, lookup_, - answer_); + udp_server_ = new UDPServer(service, fdUDP, AF_INET6, checker_, + lookup_, answer_); int fdTCP(getFd(SOCK_STREAM)); ASSERT_NE(-1, fdTCP) << strerror(errno); - tcp_server_ = new TCPServer(service, fdTCP, false, checker_, lookup_, - answer_); + tcp_server_ = new TCPServer(service, fdTCP, AF_INET6, checker_, + lookup_, answer_); } }; @@ -594,4 +602,16 @@ TYPED_TEST(DNSServerTest, stopTCPServeMoreThanOnce) { EXPECT_TRUE(this->serverStopSucceed()); } +// It raises an exception when invalid address family is passed +TEST_F(DNSServerTestBase, exceptions) { + // We abuse DNSServerTestBase for this test, as we don't need the + // initialization. + commonSetup(); + // We use the 0 fd as it should not be touched anyway + EXPECT_THROW(UDPServer(service, 0, AF_UNIX, checker_, lookup_, + answer_), isc::InvalidParameter); + EXPECT_THROW(TCPServer(service, 0, AF_UNIX, checker_, lookup_, + answer_), isc::InvalidParameter); +} + } diff --git a/src/lib/asiodns/udp_server.cc b/src/lib/asiodns/udp_server.cc index 176853d542..3ec81b1bf7 100644 --- a/src/lib/asiodns/udp_server.cc +++ b/src/lib/asiodns/udp_server.cc @@ -74,15 +74,19 @@ struct UDPServer::Data { } socket_->bind(udp::endpoint(addr, port)); } - Data(io_service& io_service, int fd, bool v6, - SimpleCallback* checkin, DNSLookup* lookup, DNSAnswer* answer) : - io_(io_service), done_(false), - checkin_callback_(checkin),lookup_callback_(lookup), - answer_callback_(answer) + Data(io_service& io_service, int fd, int af, SimpleCallback* checkin, + DNSLookup* lookup, DNSAnswer* answer) : + io_(io_service), done_(false), + checkin_callback_(checkin),lookup_callback_(lookup), + answer_callback_(answer) { + if (af != AF_INET && af != AF_INET6) { + isc_throw(InvalidParameter, "Address family must be either AF_INET " + "or AF_INET6, not " << af); + } // We must use different instantiations for v4 and v6; // otherwise ASIO will bind to both - udp proto = v6 ? udp::v6() : udp::v4(); + udp proto = af == AF_INET6 ? udp::v6() : udp::v4(); socket_.reset(new udp::socket(io_service)); socket_->assign(proto, fd); } @@ -179,10 +183,10 @@ UDPServer::UDPServer(io_service& io_service, const ip::address& addr, data_(new Data(io_service, addr, port, checkin, lookup, answer)) { } -UDPServer::UDPServer(io_service& io_service, int fd, bool v6, +UDPServer::UDPServer(io_service& io_service, int fd, int af, SimpleCallback* checkin, DNSLookup* lookup, DNSAnswer* answer) : - data_(new Data(io_service, fd, v6, checkin, lookup, answer)) + data_(new Data(io_service, fd, af, checkin, lookup, answer)) { } /// The function operator is implemented with the "stackless coroutine" diff --git a/src/lib/asiodns/udp_server.h b/src/lib/asiodns/udp_server.h index f5a9b856f8..b8456e64f3 100644 --- a/src/lib/asiodns/udp_server.h +++ b/src/lib/asiodns/udp_server.h @@ -55,11 +55,12 @@ public: /// \brief Constructor /// \param io_service the asio::io_service to work with /// \param fd the file descriptor of opened UDP socket - /// \param v6 the socket in fd is ipv6 one (if false, it is ipv4) + /// \param af address family, either AF_INET or AF_INET6 /// \param checkin the callbackprovider for non-DNS events /// \param lookup the callbackprovider for DNS lookup events /// \param answer the callbackprovider for DNS answer events - UDPServer(asio::io_service& io_service, int fd, bool v6, + /// \throw isc::InvalidParameter if af is neither AF_INET nor AF_INET6 + UDPServer(asio::io_service& io_service, int fd, int af, isc::asiolink::SimpleCallback* checkin = NULL, DNSLookup* lookup = NULL, DNSAnswer* answer = NULL); diff --git a/src/lib/server_common/portconfig.cc b/src/lib/server_common/portconfig.cc index 929ef71523..589db60a7a 100644 --- a/src/lib/server_common/portconfig.cc +++ b/src/lib/server_common/portconfig.cc @@ -92,7 +92,7 @@ setAddresses(DNSService& service, const AddressList& addresses) { } current_sockets.clear(); BOOST_FOREACH(const AddressPair &address, addresses) { - bool is_v6(IOAddress(address.first).getFamily() == AF_INET6); + int af(IOAddress(address.first).getFamily()); // TODO: Support sharing somehow in future. const SocketRequestor::SocketID tcp(socketRequestor().requestSocket(SocketRequestor::TCP, @@ -101,7 +101,7 @@ setAddresses(DNSService& service, const AddressList& addresses) { "dummy_app")); current_sockets.push_back(tcp.second); if (!test_mode) { - service.addServerTCP(tcp.first, is_v6); + service.addServerTCPFromFD(tcp.first, af); } const SocketRequestor::SocketID udp(socketRequestor().requestSocket(SocketRequestor::UDP, @@ -110,7 +110,7 @@ setAddresses(DNSService& service, const AddressList& addresses) { "dummy_app")); current_sockets.push_back(udp.second); if (!test_mode) { - service.addServerUDP(udp.first, is_v6); + service.addServerUDPFromFD(udp.first, af); } } }