From 4111989bb81641ee36fa94bf5cb181aa18f5477f Mon Sep 17 00:00:00 2001 From: Tomek Mrugalski Date: Thu, 18 Aug 2011 17:59:12 +0200 Subject: [PATCH] [878] Many improvements in IfaceMgr after review --- doc/Doxyfile | 2 +- src/bin/dhcp6/iface_mgr.cc | 95 ++++++++++++++++++----- src/bin/dhcp6/iface_mgr.h | 50 +++++++----- src/bin/dhcp6/tests/iface_mgr_unittest.cc | 44 +++++++---- 4 files changed, 137 insertions(+), 54 deletions(-) diff --git a/doc/Doxyfile b/doc/Doxyfile index b8a4656058..8857c1688c 100644 --- a/doc/Doxyfile +++ b/doc/Doxyfile @@ -574,7 +574,7 @@ INPUT = ../src/lib/cc ../src/lib/config \ ../src/lib/log ../src/lib/asiolink/ ../src/lib/nsas \ ../src/lib/testutils ../src/lib/cache ../src/lib/server_common/ \ ../src/bin/sockcreator/ ../src/lib/util/ \ - ../src/lib/resolve ../src/lib/acl + ../src/lib/resolve ../src/lib/acl ../src/bin/dhcp6 # This tag can be used to specify the character encoding of the source files # that doxygen parses. Internally doxygen uses the UTF-8 encoding, which is diff --git a/src/bin/dhcp6/iface_mgr.cc b/src/bin/dhcp6/iface_mgr.cc index 831b4c3800..da50ba4463 100644 --- a/src/bin/dhcp6/iface_mgr.cc +++ b/src/bin/dhcp6/iface_mgr.cc @@ -48,11 +48,6 @@ IfaceMgr::instance() { return (*instance_); } -IfaceMgr::Iface::Iface() - : name_(""), ifindex_(0), mac_len_(0) { - memset(mac_, 0, 20); -} - IfaceMgr::Iface::Iface(const std::string& name, int ifindex) :name_(name), ifindex_(ifindex), mac_len_(0) { memset(mac_, 0, 20); @@ -84,6 +79,9 @@ IfaceMgr::IfaceMgr() { cout << "IfaceMgr initialization." << endl; try { + // required for sending/receiving packets + // let's keep it in front, just in case someone + // wants to send anything during initialization control_buf_len_ = CMSG_SPACE(sizeof(struct in6_pktinfo)); control_buf_ = new char[control_buf_len_]; @@ -140,6 +138,11 @@ IfaceMgr::detectIfaces() { ifaces_.push_back(iface); interfaces.close(); } catch (std::exception& ex) { + // TODO: deallocate whatever memory we used + // not that important, since this function is going to be + // thrown away as soon as we get proper interface detection + // implemented + // TODO Do LOG_FATAL here std::cerr << "Interface detection failed." << std::endl; throw ex; @@ -171,6 +174,7 @@ IfaceMgr::openSockets() { DHCP6_SERVER_PORT, true); if (sock<0) { cout << "Failed to open multicast socket." << endl; + close(sendsock_); return (false); } recvsock_ = sock; @@ -181,18 +185,18 @@ IfaceMgr::openSockets() { } void -IfaceMgr::printIfaces() { +IfaceMgr::printIfaces(std::ostream& out /*= std::cout*/) { for (IfaceLst::const_iterator iface=ifaces_.begin(); iface!=ifaces_.end(); ++iface) { - cout << "Detected interface " << iface->getFullName() << endl; - cout << " " << iface->addrs_.size() << " addr(s):" << endl; + out << "Detected interface " << iface->getFullName() << endl; + out << " " << iface->addrs_.size() << " addr(s):" << endl; for (Addr6Lst::const_iterator addr=iface->addrs_.begin(); addr != iface->addrs_.end(); ++addr) { - cout << " " << addr->toText() << endl; + out << " " << addr->toText() << endl; } - cout << " mac: " << iface->getPlainMac() << endl; + out << " mac: " << iface->getPlainMac() << endl; } } @@ -205,7 +209,7 @@ IfaceMgr::getIface(int ifindex) { return (&(*iface)); } - return 0; // not found + return (NULL); // not found } IfaceMgr::Iface* @@ -217,12 +221,24 @@ IfaceMgr::getIface(const std::string& ifname) { return (&(*iface)); } - return (0); // not found + return (NULL); // not found } + +/** + * Opens UDP/IPv6 socket and binds it to specific address, interface nad port. + * + * @param ifname name of the interface + * @param addr address to be bound. + * @param port UDP port. + * @param mcast Should multicast address also be bound? + * + * @return socket descriptor, if socket creation, binding and multicast + * group join were all successful. -1 otherwise. + */ int IfaceMgr::openSocket(const std::string& ifname, - const IOAddress & addr, + const IOAddress& addr, int port, bool mcast) { struct sockaddr_storage name; @@ -247,7 +263,7 @@ IfaceMgr::openSocket(const std::string& ifname, #endif name_len = sizeof(*addr6); - // XXX: use sockcreator once it becomes available + // TODO: use sockcreator once it becomes available // make a socket int sock = socket(AF_INET6, SOCK_DGRAM, 0); @@ -262,12 +278,14 @@ IfaceMgr::openSocket(const std::string& ifname, if (setsockopt(sock, SOL_SOCKET, SO_REUSEADDR, (char *)&flag, sizeof(flag)) < 0) { cout << "Can't set SO_REUSEADDR option on dhcpv6 socket." << endl; + close(sock); return (-1); } if (bind(sock, (struct sockaddr *)&name, name_len) < 0) { cout << "Failed to bind socket " << sock << " to " << addr.toText() << "/port=" << port << endl; + close(sock); return (-1); } @@ -276,6 +294,7 @@ IfaceMgr::openSocket(const std::string& ifname, if (setsockopt(sock, IPPROTO_IPV6, IPV6_RECVPKTINFO, &flag, sizeof(flag)) != 0) { cout << "setsockopt: IPV6_RECVPKTINFO failed." << endl; + close(sock); return (-1); } #else @@ -283,6 +302,7 @@ IfaceMgr::openSocket(const std::string& ifname, if (setsockopt(sock, IPPROTO_IPV6, IPV6_PKTINFO, &flag, sizeof(flag)) != 0) { cout << "setsockopt: IPV6_PKTINFO: failed." << endl; + close(sock); return (-1); } #endif @@ -307,6 +327,15 @@ IfaceMgr::openSocket(const std::string& ifname, return (sock); } +/** + * joins multicast group + * + * @param sock socket file descriptor + * @param ifname name of the interface (DHCPv6 uses link-scoped mc groups) + * @param mcast multicast address to join (string) + * + * @return true if joined successfully, false otherwise + */ bool IfaceMgr::joinMcast(int sock, const std::string& ifname, const std::string & mcast) { @@ -332,6 +361,17 @@ const std::string & mcast) { return (true); } +/** + * Sends UDP packet over IPv6. + * + * All parameters for actual transmission are specified in + * Pkt6 structure itself. That includes destination address, + * src/dst port and interface over which data will be sent. + * + * @param pkt A packet object that is going to be sent. + * + * @return True, if transmission was successful. False otherwise. + */ bool IfaceMgr::send(Pkt6 &pkt) { struct msghdr m; @@ -405,6 +445,16 @@ IfaceMgr::send(Pkt6 &pkt) { return (result); } + +/** + * Attempts to receive UDP/IPv6 packet over open sockets. + * + * TODO Start using select() and add timeout to be able + * to not wait infinitely, but rather do something useful + * (e.g. remove expired leases) + * + * @return Object prepresenting received packet. + */ Pkt6* IfaceMgr::receive() { struct msghdr m; @@ -418,7 +468,14 @@ IfaceMgr::receive() { char addr_str[INET6_ADDRSTRLEN]; try { - pkt = new Pkt6(1500); + // RFC3315 states that server responses may be + // fragmented if they are over MTU. There is no + // text whether client's packets may be larger + // than 1500. Nevertheless to be on the safe side + // we use larger buffer. This buffer limit is checked + // during reception (see iov_len below), so we are + // safe + pkt = new Pkt6(65536); } catch (std::exception& ex) { cout << "Failed to create new packet." << endl; return (0); @@ -495,9 +552,8 @@ IfaceMgr::receive() { return (0); } - // That's ugly. - // TODO add IOAddress constructor that will take struct in6_addr* parameter + // TODO add IOAddress constructor that will take struct in6_addr* inet_ntop(AF_INET6, &to_addr, addr_str,INET6_ADDRSTRLEN); pkt->local_addr_ = IOAddress(string(addr_str)); @@ -511,8 +567,9 @@ IfaceMgr::receive() { pkt->iface_ = received->name_; } else { cout << "Received packet over unknown interface (ifindex=" - << pkt->ifindex_ << endl; - pkt->iface_ = "[unknown]"; + << pkt->ifindex_ << ")." << endl; + delete pkt; + return (0); } pkt->data_len_ = result; diff --git a/src/bin/dhcp6/iface_mgr.h b/src/bin/dhcp6/iface_mgr.h index 3569da9f8d..c251fdca9c 100644 --- a/src/bin/dhcp6/iface_mgr.h +++ b/src/bin/dhcp6/iface_mgr.h @@ -30,61 +30,73 @@ namespace isc { class IfaceMgr { public: typedef std::list Addr6Lst; - struct Iface { // XXX: could be a class as well - std::string name_; - int ifindex_; + struct Iface { // TODO: could be a class as well + std::string name_; // network interface name + int ifindex_; // interface index (a value that uniquely indentifies + // an interface Addr6Lst addrs_; - char mac_[20]; // + char mac_[20]; // Infiniband used 20 bytes indentifiers int mac_len_; - Iface(); Iface(const std::string& name, int ifindex); std::string getFullName() const; std::string getPlainMac() const; + int sendsock_; // socket used to sending data + int recvsock_; // socket used for receiving data + // next field is not needed, let's keep it in cointainers }; - // TODO performance improvement: we may change this into - // 2 maps (ifindex-indexed and name-indexed) + // TODO performance improvement: we may change this into + // 2 maps (ifindex-indexed and name-indexed) and + // also hide it (make it public make tests easier for now) typedef std::list IfaceLst; static IfaceMgr& instance(); - static void instanceCreate(); Iface * getIface(int ifindex); Iface * getIface(const std::string& ifname); - bool openSockets(); - void printIfaces(); - - int openSocket(const std::string& ifname, - const isc::asiolink::IOAddress& addr, - int port, bool multicast); - bool joinMcast(int sock, const std::string& ifname, - const std::string& mcast); + void printIfaces(std::ostream& out = std::cout); bool send(Pkt6& pkt); Pkt6* receive(); - // don't use private, we need derived classes in tests + // don't use private, we need derived classes in tests protected: IfaceMgr(); // don't create IfaceMgr directly, use instance() method ~IfaceMgr(); void detectIfaces(); - // XXX: having 2 maps (ifindex->iface and ifname->iface would) + int openSocket(const std::string& ifname, + const isc::asiolink::IOAddress& addr, + int port, bool multicast); + + // TODO: having 2 maps (ifindex->iface and ifname->iface would) // probably be better for performance reasons IfaceLst ifaces_; static IfaceMgr * instance_; - int recvsock_; // XXX: should be fd_set eventually, but we have only + // TODO: Also keep this interface on Iface once interface detection + // is implemented. We may need it e.g. to close all sockets on + // specific interface + int recvsock_; // TODO: should be fd_set eventually, but we have only int sendsock_; // 2 sockets for now. Will do for until next release + // we can't use the same socket, as receiving socket + // is bound to multicast address. And we all know what happens + // to people who try to use multicast as source address. char * control_buf_; int control_buf_len_; + + private: + bool openSockets(); + static void instanceCreate(); + bool joinMcast(int sock, const std::string& ifname, + const std::string& mcast); }; }; diff --git a/src/bin/dhcp6/tests/iface_mgr_unittest.cc b/src/bin/dhcp6/tests/iface_mgr_unittest.cc index 834ee0d62a..6c2dd0a1e6 100644 --- a/src/bin/dhcp6/tests/iface_mgr_unittest.cc +++ b/src/bin/dhcp6/tests/iface_mgr_unittest.cc @@ -30,12 +30,19 @@ using namespace isc::asiolink; namespace { class NakedIfaceMgr: public IfaceMgr { - // "naked" Interface Manager, exposes internal fields + // "naked" Interface Manager, exposes internal fields public: NakedIfaceMgr() { } IfaceLst & getIfacesLst() { return ifaces_; } void setSendSock(int sock) { sendsock_ = sock; } void setRecvSock(int sock) { recvsock_ = sock; } + + int openSocket(const std::string& ifname, + const isc::asiolink::IOAddress& addr, + int port, bool multicast) { + return IfaceMgr::openSocket(ifname, addr, port, multicast); + } + }; // dummy class for now, but this will be expanded when needed @@ -102,29 +109,29 @@ TEST_F(IfaceMgrTest, getIface) { TEST_F(IfaceMgrTest, detectIfaces) { - // test detects that interfaces can be detected + // test detects that interfaces can be detected // there is no code for that now, but interfaces are // read from file fstream fakeifaces("interfaces.txt", ios::out); fakeifaces << "eth0 fe80::1234"; fakeifaces.close(); - + // this is not usable on systems that don't have eth0 // interfaces. Nevertheless, this fake interface should // be on list, but if_nametoindex() will fail. - + IfaceMgr & ifacemgr = IfaceMgr::instance(); - + ASSERT_TRUE( ifacemgr.getIface("eth0") != NULL ); - + IfaceMgr::Iface * eth0 = ifacemgr.getIface("eth0"); - + // there should be one address EXPECT_EQ(1, eth0->addrs_.size()); - + IOAddress * addr = &(*eth0->addrs_.begin()); ASSERT_TRUE( addr != NULL ); - + EXPECT_STREQ( "fe80::1234", addr->toText().c_str() ); } @@ -132,28 +139,34 @@ TEST_F(IfaceMgrTest, sockets) { // testing socket operation in a portable way is tricky // without interface detection implemented - IfaceMgr & ifacemgr = IfaceMgr::instance(); + NakedIfaceMgr * ifacemgr = new NakedIfaceMgr(); IOAddress loAddr("::1"); // bind multicast socket to port 10547 - int socket1 = ifacemgr.openSocket("lo", loAddr, 10547, true); + int socket1 = ifacemgr->openSocket("lo", loAddr, 10547, true); EXPECT_GT(socket1, 0); // socket > 0 // bind unicast socket to port 10548 - int socket2 = ifacemgr.openSocket("lo", loAddr, 10548, false); + int socket2 = ifacemgr->openSocket("lo", loAddr, 10548, false); EXPECT_GT(socket2, 0); // good to check that both sockets can be opened at once close(socket1); close(socket2); + + delete ifacemgr; } TEST_F(IfaceMgrTest, sendReceive) { // testing socket operation in a portable way is tricky // without interface detection implemented + fstream fakeifaces("interfaces.txt", ios::out); + fakeifaces << "lo ::1"; + fakeifaces.close(); + NakedIfaceMgr * ifacemgr = new NakedIfaceMgr(); // let's assume that every supported OS have lo interface @@ -168,14 +181,14 @@ TEST_F(IfaceMgrTest, sendReceive) { // prepare dummy payload for (int i=0;i<128; i++) { - sendPkt.data_[i] = i; + sendPkt.data_[i] = i; } sendPkt.remote_port_ = 10547; sendPkt.remote_addr_ = IOAddress("::1"); sendPkt.ifindex_ = 1; sendPkt.iface_ = "lo"; - + Pkt6 * rcvPkt; EXPECT_EQ(true, ifacemgr->send(sendPkt)); @@ -186,7 +199,8 @@ TEST_F(IfaceMgrTest, sendReceive) { // let's check that we received what was sent EXPECT_EQ(sendPkt.data_len_, rcvPkt->data_len_); - EXPECT_EQ(0, memcmp(&sendPkt.data_[0], &rcvPkt->data_[0], rcvPkt->data_len_) ); + EXPECT_EQ(0, memcmp(&sendPkt.data_[0], &rcvPkt->data_[0], + rcvPkt->data_len_) ); EXPECT_EQ(sendPkt.remote_addr_, rcvPkt->remote_addr_); EXPECT_EQ(rcvPkt->remote_port_, 10546);