BIND 10 #1528: Interface detection on Linux refactoring

[BIND 10 Development]

#1528: Interface detection on Linux refactoring
-------------------------------------+-------------------------------------
                   Reporter:  tomek  |                 Owner:  tomek
                       Type:         |                Status:  reviewing
  enhancement                        |             Milestone:  Sprint-
                   Priority:  major  |  DHCP-20120305
                  Component:         |            Resolution:
  libdhcp                            |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DHCP
            Defect Severity:  N/A    |  Estimated Difficulty:  0
Feature Depending on Ticket:         |           Total Hours:  0
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by stephen):

 * owner:  UnAssigned => tomek


Comment:

 '''src/lib/dhcp/iface_mgr_linux.cc'''[[BR]]
 Should update copyright to 2011-2012.

 Really needs an introduction explaining the logic of Linux interfaces,
 e.g. what is netlink, what messages are being used, what needs to happen
 etc.

 It is not clear why a vector of pointers is required to have many nlmsg
 structures pointing to the same header.  Can this comment be expanded?

 There is no need to use the keyword "struct" in the declaration of a
 vector of pointers to nlmsghdr objects.

 "Holds information about interface or address attributes."  This does not
 hold the information, it merely points to it. Also, what is the
 significance of "rt" in "rtaddtr", "RTattribs" and rtnl_* - Real-Time?
 (What I'm trying to get across is that the reader may not know much about
 the Netlink interface as it is not as widely-used as, say, the socket
 interface.  Adding comments to guide them makes it much easier to follow
 the logic and to spot errors.)

 Both !NetlinkMessages and RTattribs objects hold pointers to other
 objects.  This immediately raises the question of what is expected to free
 up the memory pointed to by them.  This should be documented.  (In fact,
 looking further on in the code I note that there is a special function
 that deletes the pointed-to objects.  In this case, why not declare the
 vectors as vectors of scoped_ptrs?  Then the objects will be automatically
 deleted when the vector is deleted.)

 Typo: "addres info", should be "address info".  Also "any code reuse"
 should be "any code reusable".

 The assumption that IFA_MAX is shorter than IFLA_MAX can be explicitly
 checked during compilation using BOOST_STATIC_ASSERT.

 No need to use "struct" in the declaration of rtnl_handle.

 Should document why !__u32 is used as a data type rather than uint32_t in
 rtnl_handle.

 sndbuf/rcvbuf.  These variables sound like send buffer and receive buffer,
 whereas in fact they are the size of the buffer.  SNDBUF_SIZE and
 RCVBUF_SIZE (or xxx_MAX) would be better names for them.

 ''rtnl_send_request''[[BR]]
 The members of "struct req" do not need to include the "struct"
 declaration in C++.

 In rtnl, two structs are declared with another one.  The enclosing struct
 is then sent via a sendto() call with sizeof(enclosing-struct) as the
 size.  Are we sure that the compiler is not aligning the member structs on
 natural boundaries? In other words, in some circumstances could there be
 unused bytes between the two?  Addition of an assertion to check that
 would be useful.

 Spaces around the binary operator "|" are required.

 A comment as to why handle.dump is set to ++handle.seq would be useful -
 that is non-obvious, as handle is just described as a "context structure".
 Also, I would explicitly put that assignment as a separate statement -
 it's easy to overlook.

 In the call to sendto(), the casting should be done using C++ constructs.

 ''rtnl_store_reply''[[BR]]
 "Appends nlmsg to a storage" should be "Appends a copy of a !NetLink
 message to the message store".

 Why the redundant parentheses in the call to "new"?

 No need to check if {{{copy == NULL}}} as the "new" operation would throw
 an exception if this were true.  (But if it didn't, the check should
 follow the memcpy() call, which would segfault if the destination were
 null.)

 ''parse_rtattr''[[BR]]
 Reading this raised the following question: why is a boost array used to
 store pointers to the rtattr structure rather than a vector (as is the
 case for a !NetlinkMessage pointer)?  (As it is, if the number of rtattr
 structures is too many for the array, the excess are silently ignored.)

 In the "if" block, braces are required even for single-line statements.

 What are the RTA_OK and RTA_NEXT macros?  Some comments are required here.

 ''ipaddrs_get''[[BR]]
 This parses addr_info - but addr_info is described as a collection of
 parsed netlink messages.  More information about what this function
 actually does is required.

 Why is addr declared with a length of 16?  A symbolic constant should be
 used.

 {{{struct ifaddrmsg *ifa"}}} should be {{{ifaddrmsg* ifa}}} (note the
 absence of "struct" and the position of the "*".)

 Should use C++ construct to cast to "ifaddrmsg*".

 Redundant space after "(" in the "if" statement.

 No need to use "struct rtattr*" in call to fill() - "rtattr*" should be
 sufficient.

 The two "if" blocks checking ifa->family are identical, with the exception
 of the address family in the call to IOAddress::from_bytes and the size of
 the data in the memcpy() call.  The common code should be abstracted into
 a separate function.  (Additionally: (a) The "if" blocks in this code
 should include braces, (b) C++ casts should be used in the call to memcpy
 (and the second argument should be cast to {{{const void*}}}) and (c)
 symbolic constants should be used as the length of data to be copied in
 the call to memcpy.

 ''rtnl_process_reply''[[BR]]
 It would be helpful if the header were to say what processing was going to
 be done. (And comments in the code to say that as well.)

 Using "rth" as the designation for "netlink parameters" seems a bit
 obscure.

 No need to use the "struct" keyword when declaring variables.

 Declaration of "status" should be at first use. (The same goes for the
 declaration of "header".)

 Does iov.iov_len vary between iterations of the loop?  If not, why not set
 it outside?

 The "if" statement checking for EINTR should have the body within braces.

 If recvmsg() returns a value less than zero, it does not automatically
 mean that there is an overrun - it could be any one of a number of errors
 (determined by errno).  Including errno in the exception thrown would be
 useful for diagnostic purposes.

 I notice that here, if recvmsg fails due to an interrupted system call
 status, the call is retried.  This does not happen in the recvmsg() calls
 in iface_mgr.cc.  If this is an omission, I would create a separate
 function that calls recvmsg and handles the case of an interrupted system
 call.

 Assignment of address of "buf" to "header" - use C++ casts.

 The comment "Why we received this anyway?" does not give much information.
 From the code, better would be something like "Received a message not
 addressed to our process, or not with a sequence number we are expecting.
 Ignore, and look at the next one."

 ''release_list''[[BR]]
 This would not be needed if the object were a vector of scoped_ptrs - a
 call to vector::clear() would handle the destruction of the pointed-to
 objects.

 ''detectIfaces''[[BR]]
 The comment "link info" on a variable named "link_info" is not really
 helpful (same for addr_info).

 Start comments with a capital letter.

 "len" can be declared within the main loop, at the point of first use.

 Within the main "for" loop, why not set "len" with a single statement.
 And what is "len" being set to?

 The "valgrind" report has to be bogus - iface_name is scope-limited to the
 "for" loop and takes a copy of its argument.  However, it is not clear
 what the string represents - a comment would be useful.  And are we sure
 the string is non-null?

 Looking at the declaration of iface:
  * The index is set in the constructor
  * The hardware type is set via a direct access to a member of Iface
  * The flags are set through a call to the method setFlagsx().
 This is inconsistent.

 The inconsistency is carried through to the setting of the mac_len_ and
 mac_ members: these should be zeroed in the the constructor.  Similarly
 the copying of the MAC from the attribs_table should be handled by a
 single call to an Iface method, passing to it the address of the data and
 length of the data.

 Note that if exceptions are thrown, the socket will not be closed.  This
 suggests that rtnl_handle should contain a destructor that closes the
 socket.  Also, since a socket is opened with rtnl_open_socket(), it would
 be symmetrical for there to be a rtnl_close_socket() call to avoid the
 need to expose handle.fd.  In fact, going further, I would suggest that
 rtnl_handle should be a fully-fledged class, and that the all the rtnl_xxx
 functions should be class methods.  This would give maximum information
 hiding.

 ''IfaceMgr::setFlags''[[BR]]
 Why are some members of !IfaceMgr public?  In particular, why have the
 flags_xxx_ members, which effectively duplicate the contents of flags_?

 '''src/lib/dhcp/tests/iface_mgr_unittest.cc'''[[BR]]
 When processing ifconfig output, you may want to consider use of
 isc::util::str::tokens, which splits output into separate tokens,
 regardless of the number of intervening spaces.  Processing tokens one at
 a time rather than searching through the string for the first space etc.
 may be easier. (This is a comment, no action required.)

 parse_ifconfig: looking at the output of ifconfig on both FreeBSD
 (bikeshed) and Solaris (sol-10), it might be safer to regard the output
 for an interface as starting at the line where the first character is not
 whitespace.

 Use spaces around binary operators (e.g. {{{line.find("inet")+5}}}).

 '''src/lib/dhcp/tests/pkt4_unittest.cc'''[[BR]]
 Test metaFields: other tests used boost::shared_ptr to store the newly-
 created Pkt4 object and to ensure destruction when the test exits, it
 would be logical for this to do so. (As an aside, use of shared_ptr in
 this context is an unnecessary overhead - if the pointer is not going to
 be shared, scoped_ptr would be better.)

-- 
Ticket URL: <http://bind10.isc.org/ticket/1528#comment:6>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development