BIND 10 trac1528, updated. d8db35358ab97a466ea61e9624edf8d736364211 [1528] Another of review changes:

BIND 10 source code commits bind10-changes at lists.isc.org
Fri Mar 16 15:59:30 UTC 2012


The branch, trac1528 has been updated
       via  d8db35358ab97a466ea61e9624edf8d736364211 (commit)
       via  ce7abc70e21119826398943593b327f12b4bc751 (commit)
      from  dbedca057de99f85a373b918cae849174128e17b (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.

- Log -----------------------------------------------------------------
commit d8db35358ab97a466ea61e9624edf8d736364211
Author: Tomek Mrugalski <tomasz at isc.org>
Date:   Fri Mar 16 16:59:19 2012 +0100

    [1528] Another of review changes:
    
    - setMac(), getMac(), getMacLen(), getHWType(), setHWType() implemented.
    - clean-ups in interface detection code on Linux

commit ce7abc70e21119826398943593b327f12b4bc751
Author: Tomek Mrugalski <tomasz at isc.org>
Date:   Fri Mar 16 15:14:33 2012 +0100

    [1528] Changes after review: comments about netlink interface
    
    - new comments added
    - unnecessary struct keywords removed
    - several other smaller clean-ups

-----------------------------------------------------------------------

Summary of changes:
 src/lib/dhcp/iface_mgr.cc                |   21 ++-
 src/lib/dhcp/iface_mgr.h                 |   37 ++++-
 src/lib/dhcp/iface_mgr_linux.cc          |  296 ++++++++++++++++++------------
 src/lib/dhcp/tests/iface_mgr_unittest.cc |    8 +-
 4 files changed, 235 insertions(+), 127 deletions(-)

-----------------------------------------------------------------------
diff --git a/src/lib/dhcp/iface_mgr.cc b/src/lib/dhcp/iface_mgr.cc
index b704370..3aa54e8 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 5eba76b..58b2914 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 38b2134..15dc867 100644
--- a/src/lib/dhcp/iface_mgr_linux.cc
+++ b/src/lib/dhcp/iface_mgr_linux.cc
@@ -1,4 +1,4 @@
-// Copyright (C) 2011  Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2011-2012 Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -15,13 +15,13 @@
 #include <config.h>
 
 #if defined(OS_LINUX)
-
-#include <dhcp/iface_mgr.h>
-#include <exceptions/exceptions.h>
-
 #include <net/if.h>
 #include <linux/rtnetlink.h>
 #include <boost/array.hpp>
+#include <boost/static_assert.hpp>
+#include <dhcp/iface_mgr.h>
+#include <exceptions/exceptions.h>
+#include <asiolink/io_address.h>
 
 using namespace std;
 using namespace isc;
@@ -32,31 +32,53 @@ namespace {
 /// @brief Holds pointers to netlink messages.
 ///
 /// netlink (a Linux interface for getting information about network
-/// interfaces) uses memory aliasing. There are many nlmsg structures
-/// with varying size that all have the same nlmsghdr. The only
-/// reasonable way to represent this in C++ is to use vector of
-/// pointers to nlmsghdr (the common structure).
-typedef vector<struct nlmsghdr*> NetlinkMessages;
-
-/// @brief Holds information about interface or address attributes.
+/// interfaces) uses memory aliasing. Linux kernel returns a memory
+/// blob that should be interpreted as series of nlmessages. There
+/// are different nlmsg structures defined with varying size. They
+/// have one thing common - inital fields are laid out in the same
+/// way as nlmsghdr. Therefore different messages can be represented
+/// as nlmsghdr with followed variable number of bytes that are
+/// message-specific. The only reasonable way to represent this in
+/// C++ is to use vector of pointers to nlmsghdr (the common structure).
+typedef vector<nlmsghdr*> NetlinkMessages;
+
+/// @brief Holds pointers to interface or address attributes.
 ///
-/// Note that to get addres info, a shorter (IFA_MAX rather than IFLA_MAX)
+/// Note that to get address info, a shorter (IFA_MAX rather than IFLA_MAX)
 /// table could be used, but we will use the bigger one anyway to
-/// make any code reuse
-typedef boost::array<struct rtattr*, IFLA_MAX+1> RTattribs;
+/// make the code reusable.
+///
+/// rtattr is a generic structure, similar to sockaddr. It is defined
+/// in linux/rtnetlink.h and shown here for documentation purposes only:
+///
+/// struct rtattr {
+///     unsigned short<>rta_len;
+///     unsigned short<>rta_type;
+/// };
+///
+typedef boost::array<struct rtattr*, IFLA_MAX+1> RTattribPtrs;
+
+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
-    struct sockaddr_nl local;
-    struct sockaddr_nl peer;
-    __u32 seq;
-    __u32 dump;
+    sockaddr_nl local; // local and remote addresses
+    sockaddr_nl peer;
+    __u32 seq; // counter used for generating unique sequence numbers
+    __u32 dump; // number of expected message response
 };
 
 const size_t sndbuf = 32768;
@@ -104,37 +126,61 @@ 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(struct rtnl_handle& handle, int family, int type) {
+void rtnl_send_request(rtnl_handle& handle, int family, int type) {
     struct {
-        struct nlmsghdr netlink_header;
-        struct rtgenmsg generic;
+        nlmsghdr netlink_header;
+        rtgenmsg generic;
     } req;
     struct sockaddr_nl nladdr;
 
+    // This doesn't work as gcc is confused with coma appearing in
+    // the expression and thinks that there are 2 parameters passed to
+    // BOOST_STATIC_ASSERT macro, while it only takes one.
+    // BOOST_STATIC_ASSERT(sizeof(nlmsghdr) == offsetof(req,generic) );
+
     memset(&nladdr, 0, sizeof(nladdr));
     nladdr.nl_family = AF_NETLINK;
 
+    // according to netlink(7) manpage, mlmsg_seq must be set to
+    // sequence number and is used to track messages. That is just a
+    // value that is opaque to kernel and user-space code is supposed
+    // to use it to match incoming responses to sent requests. That is
+    // not really useful, as we send a single request and get a single
+    // response at a time, but still it better to obey man page suggestion
+    // and just set this to monotonically increasing numbers.
+    handle.seq++;
+
+    // this will be used to finding correct response (responses
+    // sent by kernel are supposed to have the same sequence number
+    // as the request we sent)
+    handle.dump = handle.seq;
+
     memset(&req, 0, sizeof(req));
     req.netlink_header.nlmsg_len = sizeof(req);
     req.netlink_header.nlmsg_type = type;
-    req.netlink_header.nlmsg_flags = NLM_F_ROOT|NLM_F_MATCH|NLM_F_REQUEST;
+    req.netlink_header.nlmsg_flags = NLM_F_ROOT | NLM_F_MATCH | NLM_F_REQUEST;
     req.netlink_header.nlmsg_pid = 0;
-    req.netlink_header.nlmsg_seq = handle.dump = ++handle.seq;
+    req.netlink_header.nlmsg_seq = handle.seq;
     req.generic.rtgen_family = family;
 
-    int status =  sendto(handle.fd, (void*)&req, sizeof(req), 0,
-                         (struct sockaddr*)&nladdr, sizeof(nladdr));
+    int status =  sendto(handle.fd, static_cast<void*>(&req), sizeof(req), 0,
+                         static_cast<struct sockaddr*>(static_cast<void*>(&nladdr)),
+                         sizeof(nladdr));
 
     if (status<0) {
-        isc_throw(Unexpected, "Failed to send " << sizeof(nladdr) << " bytes over netlink socket.");
+        isc_throw(Unexpected, "Failed to send " << sizeof(nladdr)
+                  << " bytes over netlink socket.");
     }
 }
 
 /// @brief Appends nlmsg to a storage.
 ///
+/// This method copies pointed nlmsg to a newly allocated memory
+/// and adds it to storage.
+///
 /// @param storage a vector that holds netlink messages
 /// @param msg a netlink message to be added
 void rtnl_store_reply(NetlinkMessages& storage, const struct nlmsghdr *msg)
@@ -142,12 +188,8 @@ void rtnl_store_reply(NetlinkMessages& storage, const struct nlmsghdr *msg)
     // we need to make a copy of this message. We really can't allocate
     // nlmsghdr directly as it is only part of the structure. There are
     // many message types with varying lengths and a common header.
-    struct nlmsghdr* copy = reinterpret_cast<struct nlmsghdr*>(new char[(msg->nlmsg_len)]);
+    struct nlmsghdr* copy = reinterpret_cast<struct nlmsghdr*>(new char[msg->nlmsg_len]);
     memcpy(copy, msg, msg->nlmsg_len);
-    if (copy == NULL) {
-        isc_throw(Unexpected, "Failed to allocate " << msg->nlmsg_len
-                  << " bytes.");
-    }
 
     // push_back copies only pointer content, not the pointed object
     storage.push_back(copy);
@@ -155,18 +197,27 @@ void rtnl_store_reply(NetlinkMessages& storage, const struct nlmsghdr *msg)
 
 /// @brief Parses rtattr message.
 ///
-/// Netlink can return a concatenated list of rtattr structures. This function iterates
-/// over that list and stores pointers to those messages in flat array (table).
+/// Some netlink messages represent address information. Such messages
+/// are concatenated collection of rtaddr structures. This function
+/// iterates over that list and stores pointers to those messages in
+/// flat array (table).
 ///
 /// @param table rtattr messages will be stored here
 /// @param rta pointer to first rtattr object
 /// @param len length (in bytes) of concatenated rtattr list.
-void parse_rtattr(RTattribs& table, struct rtattr * rta, int len)
+void parse_rtattr(RTattribPtrs& table, struct rtattr * rta, int len)
 {
     std::fill(table.begin(), table.end(), static_cast<struct rtattr*>(NULL));
+    // RTA_OK and RTA_NEXT() are macros defined in linux/rtnetlink.h
+    // they are used to handle rtattributes. RTA_OK checks if the structure
+    // pointed by rta is reasonable and passes all sanity checks.
+    // RTA_NEXT() returns pointer to the next rtattr structure that
+    // immediately follows pointed rta structure. See aforementioned
+    // header for details.
     while (RTA_OK(rta, len)) {
-        if (rta->rta_type <= table.size()-1)
+        if (rta->rta_type <= table.size()-1) {
             table[rta->rta_type] = rta;
+        }
         rta = RTA_NEXT(rta,len);
     }
     if (len) {
@@ -176,61 +227,63 @@ void parse_rtattr(RTattribs& table, struct rtattr * rta, int len)
 
 /// @brief Parses addr_info and appends appropriate addresses to Iface object.
 ///
+/// Netlink is a fine, but convoluted interface. It returns concatenated
+/// collection of netlink messages. Some of those messages convey information
+/// about addresses. Those messages are in fact appropriate header followed
+/// by concatenated lists of rtattr structures that define various pieces
+/// of address information.
+///
 /// @param iface interface representation (addresses will be added here)
 /// @param addr_info collection of parsed netlink messages
 void ipaddrs_get(IfaceMgr::Iface& iface, NetlinkMessages& addr_info) {
-    uint8_t addr[16];
-    RTattribs rta_tb;
+    uint8_t addr[V6ADDRESS_LEN];
+    RTattribPtrs rta_tb;
 
     for (NetlinkMessages::const_iterator msg = addr_info.begin();
          msg != addr_info.end(); ++msg) {
-        struct ifaddrmsg *ifa = (ifaddrmsg*)NLMSG_DATA(*msg);
+        ifaddrmsg* ifa = static_cast<ifaddrmsg*>(NLMSG_DATA(*msg));
 
         // these are not the addresses you are looking for
-        if ( ifa->ifa_index != iface.getIndex()) {
+        if (ifa->ifa_index != iface.getIndex()) {
             continue;
         }
 
-        if ( ifa->ifa_family == AF_INET6 ) {
-            std::fill(rta_tb.begin(), rta_tb.end(), static_cast<struct rtattr*>(NULL));
+        if ((ifa->ifa_family == AF_INET6) || (ifa->ifa_family == AF_INET)) {
+            std::fill(rta_tb.begin(), rta_tb.end(), static_cast<rtattr*>(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])
+            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]),16);
-            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<struct rtattr*>(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
         }
     }
 }
 
 /// @brief Processes reply received over netlink socket.
 ///
-/// @param rth netlink parameters
+/// 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 container.
+///
+/// Make sure to release this memory, e.g. using release_info() function.
+///
+/// @param context netlink parameters
 /// @param info received netlink messages will be stored here
-void rtnl_process_reply(const struct 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;
@@ -239,43 +292,44 @@ void rtnl_process_reply(const struct 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<nlmsghdr*>(static_cast<void*>(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<nlmsgerr*>(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 {
@@ -318,71 +372,87 @@ namespace isc {
 
 namespace dhcp {
 
+/// @brief Detect available interfaces on Linux systesm.
+///
+/// For detailed information about netlink interface, please refer to
+/// http://en.wikipedia.org/wiki/Netlink and RFC3549.  Following
+/// comments in the core is an overview on how netlink interface is
+/// used here. Please note that this interface is very robust and
+/// allows many operations: add/get/set/delete links, addresses,
+/// routes, queuing, manipulate traffic classes, manipulate
+/// neithborhood tables and even do something with address
+/// labels. Getting list of interfaces with addresses configured on it
+/// is just a small subset of all possible actions.
 void IfaceMgr::detectIfaces() {
     cout << "Linux: detecting interfaces." << endl;
 
-    NetlinkMessages link_info; // link info
-    NetlinkMessages addr_info; // address info
-    struct rtnl_handle handle; // socket descriptors other rtnl-related parameters
+    // Copies of netlink messages about links will be stored here.
+    NetlinkMessages link_info;
+
+    // Copies of netlink messages about addresses will be stored here.
+    NetlinkMessages addr_info;
+
+    // Socket descriptors and other rtnl-related parameters.
+    struct rtnl_handle handle;
 
-    // required to display information about interface
-    struct ifinfomsg* interface_info = NULL;
-    RTattribs attribs_table; // table with address attributes
-    int len = 0;
-    std::fill(attribs_table.begin(), attribs_table.end(), static_cast<struct rtattr*>(NULL));
+    RTattribPtrs attribs_table; // table with pointers to address attributes
+    std::fill(attribs_table.begin(), attribs_table.end(),
+              static_cast<struct rtattr*>(NULL));
 
     // open socket
     rtnl_open_socket(handle);
 
     // now we have open functional socket, let's use it!
-    // ask for list of interface...
+    // ask for list of network interfaces...
     rtnl_send_request(handle, AF_PACKET, RTM_GETLINK);
 
-    // Get reply and store it in link_info list.
+    // Get reply and store it in link_info list:
+    // response is received as with any other socket - just a series
+    // of bytes. They are representing collection of netlink messages
+    // concatenated together. rtnl_process_reply will parse this
+    // buffer, copy each message to a newly allocated memory and
+    // store pointers to it in link_info. This allocated memory will
+    // be released later. See release_info(link_info) below.
     rtnl_process_reply(handle, link_info);
 
     // Now ask for list of addresses (AF_UNSPEC = of any family)
+    // Let's repeat, but this time ask for any addresses.
+    // That includes IPv4, IPv6 and any other address families that
+    // are happen to be supported by this system.
     rtnl_send_request(handle, AF_UNSPEC, RTM_GETADDR);
 
     // Get reply and store it in addr_info list.
+    // Again, we will allocate new memory and store messages in
+    // addr_info. It will be released later using release_info(addr_info).
     rtnl_process_reply(handle, addr_info);
 
     // Now build list with interface names
     for (NetlinkMessages::iterator msg = link_info.begin(); msg != link_info.end(); ++msg) {
-        interface_info = static_cast<ifinfomsg*>(NLMSG_DATA(*msg));
-        len = (*msg)->nlmsg_len;
+        // required to display information about interface
+        struct ifinfomsg* interface_info = static_cast<ifinfomsg*>(NLMSG_DATA(*msg));
+        int len = (*msg)->nlmsg_len;
         len -= NLMSG_LENGTH(sizeof(*interface_info));
         parse_rtattr(attribs_table, IFLA_RTA(interface_info), len);
 
-        // valgrind reports *possible* memory leak in the line below, but I do believe that this
-        // report is bogus. Nevertheless, I've split the whole interface definition into
-        // three separate steps for easier debugging.
+        // valgrind reports *possible* memory leak in the line below,
+        // but I do believe that this report is bogus. Nevertheless,
+        // I've split the whole interface definition into three
+        // separate steps for easier debugging.
         const char* tmp = static_cast<const char*>(RTA_DATA(attribs_table[IFLA_IFNAME]));
         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<const uint8_t*>(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);
@@ -393,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 8175a19..0570bf7 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());



More information about the bind10-changes mailing list