From: Tomek Mrugalski Date: Fri, 16 Mar 2012 15:59:19 +0000 (+0100) Subject: [1528] Another of review changes: X-Git-Tag: trac2351_base~240^2~13 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=d8db35358ab97a466ea61e9624edf8d736364211;p=thirdparty%2Fkea.git [1528] Another of review changes: - setMac(), getMac(), getMacLen(), getHWType(), setHWType() implemented. - clean-ups in interface detection code on Linux --- diff --git a/src/lib/dhcp/iface_mgr.cc b/src/lib/dhcp/iface_mgr.cc index b704370c90..3aa54e8aaf 100644 --- a/src/lib/dhcp/iface_mgr.cc +++ b/src/lib/dhcp/iface_mgr.cc @@ -54,9 +54,9 @@ IfaceMgr::instance() { } IfaceMgr::Iface::Iface(const std::string& name, int ifindex) - :name_(name), ifindex_(ifindex), mac_len_(0), flag_loopback_(false), - flag_up_(false), flag_running_(false), flag_multicast_(false), - flag_broadcast_(false), flags_(0), hardware_type_(0) + :name_(name), ifindex_(ifindex), mac_len_(0), hardware_type_(0), + flag_loopback_(false), flag_up_(false), flag_running_(false), + flag_multicast_(false), flag_broadcast_(false), flags_(0) { memset(mac_, 0, sizeof(mac_)); } @@ -83,6 +83,17 @@ IfaceMgr::Iface::getPlainMac() const { return (tmp.str()); } +void IfaceMgr::Iface::setMac(const uint8_t* mac, size_t len) { + if (len > IfaceMgr::MAX_MAC_LEN) { + isc_throw(OutOfRange, "Interface " << getFullName() + << " was detected to have link address of length " + << len << ", but maximum supported length is " + << IfaceMgr::MAX_MAC_LEN); + } + mac_len_ = len; + memcpy(mac_, mac, len); +} + bool IfaceMgr::Iface::delAddress(const isc::asiolink::IOAddress& addr) { for (AddressCollection::iterator a = addrs_.begin(); a!=addrs_.end(); ++a) { @@ -305,8 +316,8 @@ IfaceMgr::printIfaces(std::ostream& out /*= std::cout*/) { const AddressCollection& addrs = iface->getAddresses(); out << "Detected interface " << iface->getFullName() - << ", hwtype=" << iface->hardware_type_ << ", maclen=" << iface->mac_len_ - << ", mac=" << iface->getPlainMac(); + << ", hwtype=" << iface->getHWType() + << ", mac=" << iface->getPlainMac(); out << ", flags=" << hex << iface->flags_ << dec << "(" << (iface->flag_loopback_?"LOOPBACK ":"") << (iface->flag_up_?"UP ":"") diff --git a/src/lib/dhcp/iface_mgr.h b/src/lib/dhcp/iface_mgr.h index 5eba76b0da..58b29149bf 100644 --- a/src/lib/dhcp/iface_mgr.h +++ b/src/lib/dhcp/iface_mgr.h @@ -87,6 +87,23 @@ public: /// @return MAC address as a plain text (string) std::string getPlainMac() const; + /// @brief Sets MAC address of the interface. + /// + /// @param mac pointer to MAC address buffer + /// @param macLen length of mac address + void setMac(const uint8_t* mac, size_t macLen); + + /// @brief Returns MAC length. + /// + /// @return length of MAC address + size_t getMacLen() const { return mac_len_; } + + /// @brief Returns pointer to MAC address. + /// + /// Note: Returned pointer is only valid as long as the interface object + /// that returned it. + const uint8_t* getMac() const { return mac_; } + /// @brief Sets flag_*_ fields based on bitmask value returned by OS /// /// Note: Implementation of this method is OS-dependent as bits have @@ -105,6 +122,16 @@ public: /// @return interface name std::string getName() const { return name_; }; + /// @brief Sets up hardware type of the interface. + /// + /// @param type hardware type + void setHWType(uint16_t type ) { hardware_type_ = type; } + + /// @brief Returns hardware type of the interface. + /// + /// @return hardware type + uint16_t getHWType() const { return hardware_type_; } + /// @brief Returns all interfaces available on an interface. /// /// Care should be taken to not use this collection after Iface object @@ -167,13 +194,16 @@ public: /// list of assigned addresses AddressCollection addrs_; - public: /// link-layer address uint8_t mac_[MAX_MAC_LEN]; /// length of link-layer address (usually 6) - int mac_len_; + size_t mac_len_; + + /// hardware type + uint16_t hardware_type_; + public: /// specifies if selected interface is loopback bool flag_loopback_; @@ -193,9 +223,6 @@ public: /// interface flags (this value is as is returned by OS, /// it may mean different things on different OSes) uint32_t flags_; - - /// hardware type - uint16_t hardware_type_; }; // TODO performance improvement: we may change this into diff --git a/src/lib/dhcp/iface_mgr_linux.cc b/src/lib/dhcp/iface_mgr_linux.cc index c51465421f..15dc86769d 100644 --- a/src/lib/dhcp/iface_mgr_linux.cc +++ b/src/lib/dhcp/iface_mgr_linux.cc @@ -63,10 +63,17 @@ BOOST_STATIC_ASSERT(IFLA_MAX>=IFA_MAX); /// @brief This structure defines context for netlink connection. struct rtnl_handle { - rtnl_handle() :fd(0), seq(0), dump(0) { + rtnl_handle() :fd(-1), seq(0), dump(0) { memset(&local, 0, sizeof(struct sockaddr_nl)); memset(&peer, 0, sizeof(struct sockaddr_nl)); } + + ~rtnl_handle() { + if (fd != -1) { + close(fd); + } + } + int fd; // netlink file descriptor sockaddr_nl local; // local and remote addresses sockaddr_nl peer; @@ -119,7 +126,7 @@ void rtnl_open_socket(struct rtnl_handle& handle) { /// @brief Sends request over NETLINK socket. /// -/// @param handle context structure +/// @param handle structure that contains necessary information about netlink /// @param family requested information family /// @param type request type (RTM_GETLINK or RTM_GETADDR) void rtnl_send_request(rtnl_handle& handle, int family, int type) { @@ -241,7 +248,7 @@ void ipaddrs_get(IfaceMgr::Iface& iface, NetlinkMessages& addr_info) { continue; } - if (ifa->ifa_family == AF_INET6) { + if ((ifa->ifa_family == AF_INET6) || (ifa->ifa_family == AF_INET)) { std::fill(rta_tb.begin(), rta_tb.end(), static_cast(NULL)); parse_rtattr(rta_tb, IFA_RTA(ifa), (*msg)->nlmsg_len - NLMSG_LENGTH(sizeof(*ifa))); if (!rta_tb[IFA_LOCAL]) { @@ -251,24 +258,12 @@ void ipaddrs_get(IfaceMgr::Iface& iface, NetlinkMessages& addr_info) { rta_tb[IFA_ADDRESS] = rta_tb[IFA_LOCAL]; } - memcpy(addr,(char*)RTA_DATA(rta_tb[IFLA_ADDRESS]), V6ADDRESS_LEN); - IOAddress a = IOAddress::from_bytes(AF_INET6, addr); + memcpy(addr, RTA_DATA(rta_tb[IFLA_ADDRESS]), + ifa->ifa_family==AF_INET?V4ADDRESS_LEN:V6ADDRESS_LEN); + IOAddress a = IOAddress::from_bytes(ifa->ifa_family, addr); iface.addAddress(a); - /// TODO: Read lifetimes of configured addresses - } - - if ( ifa->ifa_family == AF_INET ) { - std::fill(rta_tb.begin(), rta_tb.end(), static_cast(NULL)); - parse_rtattr(rta_tb, IFA_RTA(ifa), (*msg)->nlmsg_len - NLMSG_LENGTH(sizeof(*ifa))); - if (!rta_tb[IFA_LOCAL]) - rta_tb[IFA_LOCAL] = rta_tb[IFA_ADDRESS]; - if (!rta_tb[IFA_ADDRESS]) - rta_tb[IFA_ADDRESS] = rta_tb[IFA_LOCAL]; - - memcpy(addr,(char*)RTA_DATA(rta_tb[IFLA_ADDRESS]),4); - IOAddress a = IOAddress::from_bytes(AF_INET, addr); - iface.addAddress(a); + /// TODO: Read lifetimes of configured IPv6 addresses } } } @@ -277,18 +272,18 @@ void ipaddrs_get(IfaceMgr::Iface& iface, NetlinkMessages& addr_info) { /// /// This method parses received buffer (a collection of concatenated /// netlink messages), copies each received message to newly allocated -/// memory and stores pointers to it in info. +/// memory and stores pointers to it in info container. /// /// Make sure to release this memory, e.g. using release_info() function. /// -/// @param rth netlink parameters +/// @param context netlink parameters /// @param info received netlink messages will be stored here -void rtnl_process_reply(const rtnl_handle& rth, NetlinkMessages& info) { +void rtnl_process_reply(const rtnl_handle& handle, NetlinkMessages& info) { - struct sockaddr_nl nladdr; - struct iovec iov; - struct msghdr msg; - memset(&msg, 0, sizeof(struct msghdr)); + sockaddr_nl nladdr; + iovec iov; + msghdr msg; + memset(&msg, 0, sizeof(msghdr)); msg.msg_name = &nladdr; msg.msg_namelen = sizeof(nladdr); msg.msg_iov = &iov; @@ -297,43 +292,44 @@ void rtnl_process_reply(const rtnl_handle& rth, NetlinkMessages& info) { char buf[rcvbuf]; iov.iov_base = buf; + iov.iov_len = sizeof(buf); while (true) { - int status; - struct nlmsghdr* header; - - iov.iov_len = sizeof(buf); - status = recvmsg(rth.fd, &msg, 0); + int status = recvmsg(handle.fd, &msg, 0); if (status < 0) { - if (errno == EINTR) + if (errno == EINTR) { continue; - isc_throw(Unexpected, "Overrun while processing reply from netlink socket."); + } + isc_throw(Unexpected, "Error " << errno + << " while processing reply from netlink socket."); } if (status == 0) { isc_throw(Unexpected, "EOF while reading netlink socket."); } - header = (struct nlmsghdr*)buf; + nlmsghdr* header = static_cast(static_cast(buf)); while (NLMSG_OK(header, status)) { - // why we received this anyway? + // Received a message not addressed to our process, or not + // with a sequence number we are expecting. Ignore, and + // look at the next one. if (nladdr.nl_pid != 0 || - header->nlmsg_pid != rth.local.nl_pid || - header->nlmsg_seq != rth.dump) { + header->nlmsg_pid != handle.local.nl_pid || + header->nlmsg_seq != handle.dump) { header = NLMSG_NEXT(header, status); continue; } if (header->nlmsg_type == NLMSG_DONE) { - // end of message + // End of message. return; } if (header->nlmsg_type == NLMSG_ERROR) { - struct nlmsgerr* err = (struct nlmsgerr*)NLMSG_DATA(header); + nlmsgerr* err = static_cast(NLMSG_DATA(header)); if (header->nlmsg_len < NLMSG_LENGTH(sizeof(struct nlmsgerr))) { - // we are really out of luck here. We can't even say what is + // We are really out of luck here. We can't even say what is // wrong as error message is truncated. D'oh. isc_throw(Unexpected, "Netlink reply read failed."); } else { @@ -396,11 +392,10 @@ void IfaceMgr::detectIfaces() { // Copies of netlink messages about addresses will be stored here. NetlinkMessages addr_info; - // socket descriptors other rtnl-related parameters + // Socket descriptors and other rtnl-related parameters. struct rtnl_handle handle; RTattribPtrs attribs_table; // table with pointers to address attributes - int len = 0; std::fill(attribs_table.begin(), attribs_table.end(), static_cast(NULL)); @@ -435,7 +430,7 @@ void IfaceMgr::detectIfaces() { for (NetlinkMessages::iterator msg = link_info.begin(); msg != link_info.end(); ++msg) { // required to display information about interface struct ifinfomsg* interface_info = static_cast(NLMSG_DATA(*msg)); - len = (*msg)->nlmsg_len; + int len = (*msg)->nlmsg_len; len -= NLMSG_LENGTH(sizeof(*interface_info)); parse_rtattr(attribs_table, IFLA_RTA(interface_info), len); @@ -447,28 +442,17 @@ void IfaceMgr::detectIfaces() { string iface_name(tmp); // <--- (probably bogus) valgrind warning here Iface iface = Iface(iface_name, interface_info->ifi_index); - iface.hardware_type_ = interface_info->ifi_type; + iface.setHWType(interface_info->ifi_type); iface.setFlags(interface_info->ifi_flags); - iface.mac_len_ = 0; - memset(iface.mac_, 0, IfaceMgr::MAX_MAC_LEN); // Does inetface has LL_ADDR? if (attribs_table[IFLA_ADDRESS]) { - iface.mac_len_ = RTA_PAYLOAD(attribs_table[IFLA_ADDRESS]); - if (iface.mac_len_ > IfaceMgr::MAX_MAC_LEN) { - iface.mac_len_ = 0; - isc_throw(Unexpected, "Interface " << iface.getFullName() - << " was detected to have link address of length " - << RTA_PAYLOAD(attribs_table[IFLA_ADDRESS]) - << ", but maximum supported length is " << IfaceMgr::MAX_MAC_LEN); - } - memcpy(iface.mac_, RTA_DATA(attribs_table[IFLA_ADDRESS]), iface.mac_len_); + iface.setMac(static_cast(RTA_DATA(attribs_table[IFLA_ADDRESS])), + RTA_PAYLOAD(attribs_table[IFLA_ADDRESS])); } else { // Tunnels can have no LL_ADDR. RTA_PAYLOAD doesn't check it and try to // dereference it in this manner - // #define RTA_PAYLOAD(rta) ((int)((rta)->rta_len) - RTA_LENGTH(0)) - iface.mac_len_ = 0; } ipaddrs_get(iface, addr_info); @@ -479,8 +463,6 @@ void IfaceMgr::detectIfaces() { release_list(addr_info); printIfaces(); - - close(handle.fd); } /// @brief sets flag_*_ fields. diff --git a/src/lib/dhcp/tests/iface_mgr_unittest.cc b/src/lib/dhcp/tests/iface_mgr_unittest.cc index 8175a19978..0570bf71fe 100644 --- a/src/lib/dhcp/tests/iface_mgr_unittest.cc +++ b/src/lib/dhcp/tests/iface_mgr_unittest.cc @@ -728,7 +728,9 @@ void parse_ifconfig(const std::string& textFile, IfaceMgr::IfaceCollection& ifac mac = line.substr(offset, string::npos); mac = mac.substr(0, mac.find_first_of(" ")); - iface->mac_len_ = parse_mac(mac, iface->mac_, IfaceMgr::MAX_MAC_LEN); + uint8_t buf[IfaceMgr::MAX_MAC_LEN]; + int mac_len = parse_mac(mac, buf, IfaceMgr::MAX_MAC_LEN); + iface->setMac(buf, mac_len); } } @@ -870,8 +872,8 @@ TEST_F(IfaceMgrTest, DISABLED_detectIfaces_linux) { // skip MAC comparison for loopback as netlink returns MAC // 00:00:00:00:00:00 for lo if (!detected->flag_loopback_) { - ASSERT_EQ(detected->mac_len_, i->mac_len_); - EXPECT_EQ(0, memcmp(detected->mac_, i->mac_, i->mac_len_)); + ASSERT_EQ(detected->getMacLen(), i->getMacLen()); + EXPECT_EQ(0, memcmp(detected->getMac(), i->getMac(), i->getMacLen())); } EXPECT_EQ(detected->getAddresses().size(), i->getAddresses().size());