BIND 10 #2246: Implement cross-OS interface detection routines

[BIND 10 Development]

#2246: Implement cross-OS interface detection routines
-------------------------------------+-------------------------------------
            Reporter:  tomek         |                        Owner:
                Type:  task          |  dclink
            Priority:  medium        |                       Status:
           Component:  libdhcp       |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-DHCP-20131016
         Sub-Project:  DHCP          |                   Resolution:
Estimated Difficulty:  0             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by tomek):

 * owner:  tomek => dclink


Comment:

 Thanks a lot for the patch. Sorry it took so long.

 It looks good in general, but there are couple things that have to be
 fixed before we can merge it.

 Documentation has to be updated. Please take a look at
 doc/guide/bind10-guide.html, section 19.1.

 Developer's guide (cd doc; make devel; open doc/html/index.html) needs
 minor update, too. See src/lib/dhcp/libdhcp++.dox.

 I've cleaned up your patch a bit and checked it into trac2246. I've
 submitted it to run on our test farm
 (http://git.bind10.isc.org/~tester/builder/builder.html or
 http://git.bind10.isc.org/~tester/builder/builder-new.html). Please check
 out if there are any build failures reported on trac2246.

 Both BSD and Solaris versions detect all interfaces, but report only those
 with at least one address. They should report all interfaces, regardless
 if there are addresses or not. That is useful for cases when interface is
 not fully configured (down; not associated wifis, etc)

 '''iface_mgr_bsd.cc''' and '''iface_msg_sun.cc'''
 Please try to use more meaningful names. Instead of itf, use iface.

 '''iface_mgr_unittest.cc'''
 The line
 {{{
 #if defined(OS_LINUX) || defined(OS_BSD) || defined(OS_SUN)
 }}}
 does not make sense. We don't run on any other OSes, besides Linux, BSD or
 Solaris.

 Functions should have Doxygen comments. They should include parameter
 names. We typically keep function prototypes in .h files, but your
 decision to keep checkIfIndex, checkIfFlags and checkIfAddrs in .cc file
 was correct. This way we don't need to include headers to have struct
 ifaddrs in a header.

 The code that checks MAC address needs improvement:
 {{{
         int i = 0;
         while(i < 6) {
             if(* (p + i) != * (iface.getMac() + i)) {
                 return (false);
             }
             ++ i;
         }
 }}}

 I've added a @todo that explains the issue: MAC address is 6 bytes long
 for IEEE802 family interfaces only (Ethernet, Wifi, Bluetooth, etc.). For
 some interfaces, e.g. IPv6-over-IPv4 tunnels, it is only 4 bytes long. For
 other, more exotic ones, like infiniband, it is 20 bytes long. That length
 has to be checked. And the loop cited above can be done with a single call
 to memcmp().

 We try to keep the number of printfs and couts in unit-tests to minimum.
 Please consider removing some of the messages printed. This is only a
 suggestion. If you think they should stay, I won't object.

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

 Please propose !ChangeLog entry.

 Ok, the ticket is back with you, dclink. Please address the issues
 mentioned and provide a patch against trac2246 branch. Once you do that,
 reassign the ticket back to me.

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