BIND 10 #1237: Interface detection on Linux

[BIND 10 Development]

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

 * owner:  stephen => tomek


Comment:

 Review done in two parts to avoid the files included by the merge of 992.

 Reviewing change from commit ea709c77cdab1d2d91a923b913af869f865477bd to
 commit  0ca7dca47750297e7477223093da8159d7f48f6c

 '''configure.ac'''[[BR]]
 When selecting an operating system, rather than updating the definition of
 CPPFLAGS, it is probably better to add a #define to config.h and include
 config.h when you need to reference it.  To do this, replace:

 {{{
 AM_CONDITIONAL(OS_LINUX, test $OS_TYPE = Linux)
 AM_CONDITIONAL(OS_BSD, test $OS_TYPE = BSD)
 AM_CONDITIONAL(OS_SOLARIS, test $OS_TYPE = Solaris)
 }}}
 With
 {{{
 AM_CONDITIONAL(OS_LINUX, test $OS_TYPE = LINUX)
 AM_COND_IF([OS_LINUX], [AC_DEFINE([OS_LINUX], [1], [Running on Linux?])])
 AM_CONDITIONAL(OS_BSD, test $OS_TYPE = BSD)
 AM_COND_IF([OS_BSD], [AC_DEFINE([OS_BSD], [1], [Running on BSD?])])
 AM_CONDITIONAL(OS_SOLARIS, test $OS_TYPE = Solaris)
 AM_COND_IF([OS_SOLARIS], [AC_DEFINE([OS_SOLARIS], [1], [Running on
 Solaris?])])
 }}}
 (I would also change LINUX to Linux in the test, so that the result of
 testing the operating system type is reported as "Linux" rather than
 "LINUX").

 '''src/bin/dhcp6/dhcp6_srv.cc'''[[BR]]
 If IfaceMgr::instance() throws an exception (e), suggest that e.what() is
 included in the message.  If the message is not wanted, remove the
 exception variable "e" from the declaration.

 '''src/lib/dhcp/iface_mgr.cc'''[[BR]]
 Use static_cast<int>() to convert mac_[i] to int.

 If re-throwing the same exception, "throw" by itself is sufficient.

 If changes listed above are made to include the macros in config.h, the
 config.h needs to be included before all other include files (including
 system include files).

 '''src/lib/dhcp/iface_mgr.h'''[[BR]]
 IfaceMgr::Iface::setFlags() needs a Doxygen header.

 '''src/lib/dhcp/iface_mgr_bsd.cc'''[[BR]]
 An alternative way of handling operating-system specific compilation would
 be to use the construct
 {{{
 #include <config.h>
 #ifdef OS_XXX
    (code)
 #endif
 }}}
 ... in the file (it should compile to an empty object file that can be
 included in any link with no ill-effect).  Such an approach would
 eliminate the conditionals in Makefile.am.

 '''src/lib/dhcp/iface_mgr_linux.cc'''[[BR]]
 I'm guessing that a lot of this code is copied from elsewhere (e.g. use of
 "malloc", embedded comments, cryptic variable names ("h", "lp" etc.),
 indentation etc.).  It doesn't conform to the BIND 10 coding standards and
 does need many format changes and better comments.  Also, it some changes
 to the code itself (e.g. why use nlmsg_list instead of
 std::list<nlmsghdr>?).   In summary, it needs refactoring.


 Review of d1a22e481da930ca3151ce704305d7995f9d8fcc to
 aaea8925c243f6ead71f368de54ad27cfd19ef6b

 '''src/lib/dhcp/tests/iface_mgr_unittest.cc'''[[BR]]
 Is it likely that the parsing of a mac address will be needed in the
 server?  If so, it seems that we really need a !MacAddress class with
 fromText() and toText() methods.

 parse_ifconfig(): you may want to look at isc::util::str::tokens() (see
 src/lib/util/strutil.h).  This should eliminate minor variations in
 formatting.

 When creating a test file, you should ensure that the file is placed in a
 suitable directory.  Have a look at src/lib/log/tests/tempdir.h.in:  this
 is used to ensure that the file used in the log tests is placed in the
 build directory (which is known to be writeable).

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