[1528] Modifications to some of the comments

src/lib/dhcp/iface_mgr_linux.cc

@@ -12,6 +12,21 @@
 // OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
 // PERFORMANCE OF THIS SOFTWARE.
 
+/// @file
+/// Access to interface information on Linux is via netlink, a socket-based
+/// method for transferring information between the kernel and user processes.
+///
+/// For detailed information about netlink interface, please refer to
+/// http://en.wikipedia.org/wiki/Netlink and RFC3549.  Comments in the
+/// detectIfaces() method (towards the end of this file) provide an overview
+/// on how the netlink interface is used here.
+///
+/// Note that this interface is very robust and allows many operations:
+/// add/get/set/delete links, addresses, routes, queuing, manipulation of
+/// traffic classes, manipulation of neighbourhood tables and even the ability
+/// to do something with address labels. Getting a list of interfaces with
+/// addresses configured on it is just a small subset of all possible actions.
+
 #include <config.h>
 
 #if defined(OS_LINUX)
@@ -34,8 +49,8 @@ namespace {
 
 /// @brief This class offers utility methods for netlink connection.
 ///
-/// See IfaceMgr::detectIfaces() (Linux implementation) for example
-/// usage.
+/// See IfaceMgr::detectIfaces() (Linux implementation, towards the end of this
+/// file) for example usage.
 class Netlink
 {
 public:
@@ -65,10 +80,9 @@ public:
 ///     unsigned short<>rta_len;
 ///     unsigned short<>rta_type;
 /// };
-///
-    typedef boost::array<struct rtattr*, IFLA_MAX+1> RTattribPtrs;
+    typedef boost::array<struct rtattr*, IFLA_MAX + 1> RTattribPtrs;
 
-    Netlink() :fd_(-1), seq_(0), dump_(0) {
+    Netlink() : fd_(-1), seq_(0), dump_(0) {
         memset(&local_, 0, sizeof(struct sockaddr_nl));
         memset(&peer_, 0, sizeof(struct sockaddr_nl));
     }
@@ -87,11 +101,11 @@ public:
     void rtnl_close_socket();
 
 private:
-    int fd_; // netlink file descriptor
-    sockaddr_nl local_; // local and remote addresses
-    sockaddr_nl peer_;
-    uint32_t seq_; // counter used for generating unique sequence numbers
-    uint32_t dump_; // number of expected message response
+    int fd_;            // Netlink file descriptor
+    sockaddr_nl local_; // Local addresses
+    sockaddr_nl peer_;  // Remote address
+    uint32_t seq_;      // Counter used for generating unique sequence numbers
+    uint32_t dump_;     // Number of expected message response
 };
 
 const size_t sndbuf = 32768;
@@ -100,10 +114,8 @@ const size_t rcvbuf = 32768;
 /// @brief Opens netlink socket and initializes handle structure.
 ///
 /// @exception Unexpected Thrown if socket configuration fails.
-///
-/// @param handle Context will be stored in this structure.
 void Netlink::rtnl_open_socket() {
-    // equivalent of rtnl_open
+
     fd_ = socket(AF_NETLINK, SOCK_RAW, NETLINK_ROUTE);
     if (fd_ < 0) {
         isc_throw(Unexpected, "Failed to create NETLINK socket.");
@@ -136,6 +148,7 @@ void Netlink::rtnl_open_socket() {
     }
 }
 
+/// @brief Closes netlink communication socket
 void Netlink::rtnl_close_socket() {
     if (fd_ != -1) {
         close(fd_);
@@ -145,9 +158,8 @@ void Netlink::rtnl_close_socket() {
 
 /// @brief Sends request over NETLINK socket.
 ///
-/// @param handle structure that contains necessary information about netlink
-/// @param family requested information family
-/// @param type request type (RTM_GETLINK or RTM_GETADDR)
+/// @param family requested information family.
+/// @param type request type (RTM_GETLINK or RTM_GETADDR).
 void Netlink::rtnl_send_request(int family, int type) {
     struct {
         nlmsghdr netlink_header;
@@ -155,26 +167,25 @@ void Netlink::rtnl_send_request(int family, int type) {
     } 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
+    // This doesn't work as gcc is confused with a comma appearing in
+    // the expression and thinks that there are two parameters passed to
     // BOOST_STATIC_ASSERT macro, while it only takes one.
-    // BOOST_STATIC_ASSERT(sizeof(nlmsghdr) == offsetof(req,generic) );
-
+    // 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.
+    // According to netlink(7) manpage, mlmsg_seq must be set to a 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. However, we
+    // obey the man page suggestion and just set this to monotonically
+    // increasing numbers.
     seq_++;
 
-    // this will be used to finding correct response (responses
+    // 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)
+    // as the request we sent).
     dump_ = seq_;
 
     memset(&req, 0, sizeof(req));
@@ -200,8 +211,9 @@ void Netlink::rtnl_send_request(int family, int type) {
 /// 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
+/// @param storage A vector that holds pointers to netlink messages. The caller
+///        is responsible for freeing the pointed-to messages.
+/// @param msg A netlink message to be added.
 void Netlink::rtnl_store_reply(NetlinkMessages& storage, const struct nlmsghdr *msg)
 {
     // we need to make a copy of this message. We really can't allocate
@@ -210,7 +222,7 @@ void Netlink::rtnl_store_reply(NetlinkMessages& storage, const struct nlmsghdr *
     struct nlmsghdr* copy = reinterpret_cast<struct nlmsghdr*>(new char[msg->nlmsg_len]);
     memcpy(copy, msg, msg->nlmsg_len);
 
-    // push_back copies only pointer content, not the pointed object
+    // push_back copies only pointer content, not the pointed-to object.
     storage.push_back(copy);
 }
 
@@ -221,10 +233,10 @@ void Netlink::rtnl_store_reply(NetlinkMessages& storage, const struct nlmsghdr *
 /// 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 Netlink::parse_rtattr(RTattribPtrs& table, struct rtattr * rta, int len)
+/// @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 Netlink::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
@@ -246,7 +258,7 @@ void Netlink::parse_rtattr(RTattribPtrs& 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
+/// Netlink is a fine, but convoluted interface. It returns a 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
@@ -262,7 +274,7 @@ void Netlink::ipaddrs_get(IfaceMgr::Iface& iface, NetlinkMessages& addr_info) {
          msg != addr_info.end(); ++msg) {
         ifaddrmsg* ifa = static_cast<ifaddrmsg*>(NLMSG_DATA(*msg));
 
-        // these are not the addresses you are looking for
+        // These are not the addresses you are looking for
         if (ifa->ifa_index != iface.getIndex()) {
             continue;
         }
@@ -289,13 +301,13 @@ void Netlink::ipaddrs_get(IfaceMgr::Iface& iface, NetlinkMessages& addr_info) {
 
 /// @brief Processes reply received over netlink socket.
 ///
-/// This method parses received buffer (a collection of concatenated
+/// This method parses the received buffer (a collection of concatenated
 /// netlink messages), copies each received message to newly allocated
-/// memory and stores pointers to it in info container.
+/// memory and stores pointers to it in the "info" container.
 ///
-/// Make sure to release this memory, e.g. using release_info() function.
-///
-/// @param info received netlink messages will be stored here
+/// @param info received netlink messages will be stored here.  It is the
+///        caller's responsibility to release the memory associated with the
+///        messages by calling the release_list() method.
 void Netlink::rtnl_process_reply(NetlinkMessages& info) {
     sockaddr_nl nladdr;
     iovec iov;
@@ -352,7 +364,7 @@ void Netlink::rtnl_process_reply(NetlinkMessages& info) {
                 } else {
                     isc_throw(Unexpected, "Netlink reply read error " << -err->error);
                 }
-                // never happens we throw before we reach here
+                // Never happens we throw before we reach here
                 return;
             }
 
@@ -372,7 +384,7 @@ void Netlink::rtnl_process_reply(NetlinkMessages& info) {
 
 /// @brief releases nlmsg structure
 ///
-/// @param messages first element of the list to be released
+/// @param messages Set of messages to be freed.
 void Netlink::release_list(NetlinkMessages& messages) {
     // let's free local copies of stored messages
     for (NetlinkMessages::iterator msg = messages.begin(); msg != messages.end(); ++msg) {
@@ -389,17 +401,10 @@ namespace isc {
 
 namespace dhcp {
 
-/// @brief Detect available interfaces on Linux systesm.
+/// @brief Detect available interfaces on Linux systems.
 ///
-/// 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.
+/// Uses the socket-based netlink protocol to retrieve the list of interfaces
+/// from the Linux kernel.
 void IfaceMgr::detectIfaces() {
     cout << "Linux: detecting interfaces." << endl;
 
@@ -412,16 +417,16 @@ void IfaceMgr::detectIfaces() {
     // Socket descriptors and other rtnl-related parameters.
     Netlink nl;
 
-    // table with pointers to address attributes
+    // Table with pointers to address attributes.
     Netlink::RTattribPtrs attribs_table;
     std::fill(attribs_table.begin(), attribs_table.end(),
               static_cast<struct rtattr*>(NULL));
 
-    // open socket
+    // Open socket
     nl.rtnl_open_socket();
 
-    // now we have open functional socket, let's use it!
-    // ask for list of network interfaces...
+    // Now we have open functional socket, let's use it!
+    // Ask for list of network interfaces...
     nl.rtnl_send_request(AF_PACKET, RTM_GETLINK);
 
     // Get reply and store it in link_info list:
@@ -447,15 +452,15 @@ void IfaceMgr::detectIfaces() {
     // Now build list with interface names
     for (Netlink::NetlinkMessages::iterator msg = link_info.begin();
          msg != link_info.end(); ++msg) {
-        // required to display information about interface
+        // 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));
         nl.parse_rtattr(attribs_table, IFLA_RTA(interface_info), len);
 
-        // valgrind reports *possible* memory leak in the line below,
-        // but it 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 it is
+        // bogus. Nevertheless, the whole interface definition has been split
+        // into three separate steps for easier debugging.
         const char* tmp = static_cast<const char*>(RTA_DATA(attribs_table[IFLA_IFNAME]));
         string iface_name(tmp); // <--- bogus valgrind warning here
         Iface iface = Iface(iface_name, interface_info->ifi_index);
@@ -463,14 +468,14 @@ void IfaceMgr::detectIfaces() {
         iface.setHWType(interface_info->ifi_type);
         iface.setFlags(interface_info->ifi_flags);
 
-        // Does inetface has LL_ADDR?
+        // Does inetface have LL_ADDR?
         if (attribs_table[IFLA_ADDRESS]) {
             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
+            // Tunnels can have no LL_ADDR. RTA_PAYLOAD doesn't check it and
+            // try to dereference it in this manner
         }
 
         nl.ipaddrs_get(iface, addr_info);