BIND 10 #3252: libdhcp++: The IfaceMgr::openSockets6 shouldn't throw an exception when failed to open socket.

[BIND 10 Development]

#3252: libdhcp++: The IfaceMgr::openSockets6 shouldn't throw an exception when
failed to open socket.
-------------------------------------+-------------------------------------
            Reporter:  marcin        |                        Owner:
                Type:  enhancement   |  marcin
            Priority:  medium        |                       Status:
           Component:  dhcp          |  reviewing
            Keywords:                |                    Milestone:  DHCP-
           Sensitive:  0             |  Kea1.0-alpha
         Sub-Project:  DHCP          |                   Resolution:
Estimated Difficulty:  0             |                 CVSS Scoring:
         Total Hours:  11            |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:  3251
                                     |          Add Hours to Ticket:  4
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by tmark):

 * hours:  7 => 4
 * owner:  tmark => marcin
 * totalhours:  7 => 11


Comment:

 src/lib/dhcp/iface_mgr.h

 Commentary for  openSockets6 needs to updated.  It still has a @todo (line
 632) talking about exception throwing.  Tsk Tsk.

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

 src/lib/dhcp/iface_mgr.cc
 iface_mgr.cc

 ifacemgr_error -  You should capitalize this so it is clear that it a
 macro.  Also you should add a set of enclosing brackets {}, to ensure
 everything the macro does is within its own scope like so:

 {{{

 #define ifacemgr_error(ex_type, handler, stream) \
 {
     std::ostringstream oss__; \
     oss__ << stream; \
     if (handler) { \
         handler(oss__.str()); \
     } else { \
         isc_throw(ex_type, oss__); \
     }
 }
 }}}

 This ensures oss__ cannot collide with another variable etc...

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

 src/lib/dhcp/tests/ifage_mgr_unittest.cc

     openSocket6ErrorHandler()

 * Comment at line 1852 is a little confusing. Here you are opening an
 initial
 socket which should succeed. The commentary makes one think this open
 should
 fail when in reality it is a subsequent open that should fail.

 This one should perhaps say "Open a socket, so subsequent open will fail."
 Something like that.  (Similar situation with comments in
 openSocket6NoErrorHandler())

 * line 1863 is commented out, this is followed by a try-catch block with
 an and std::cout.  The try-catch appears to be the debug version and line
 1863 should be live code (looks like something I would do ;) ):

 {{{
     //    ASSERT_NO_THROW(ifacemgr.openSockets6(DHCP6_SERVER_PORT,
 error_handler));
     try {
         ifacemgr.openSockets6(DHCP6_SERVER_PORT, error_handler);
     } catch (const Exception& ex) {
         std::cout << ex.what() << std::endl;
     }
 }}}

 * In reading the comment at line 1876 something occurred to me:

 openSockets6 does not take into account whether or not IfaceMgr already
 has a socket open on a given interface.  This is obvious in that your test
 counts on this but it is an inconsistency in behavior.

 This test first uses openSocket to open eth0. This succeeds and eth0's
 socket collection has 1 member in it.  Then the test adds the error
 handler and calls openSockets6.  This fails to open on eth0 but succeeds
 on eth1.  IfaceMgr reports the error and returns a count of 1 socket
 opened.  This is technically true but misleading.  Eth0 does in fact have
 a open socket on it.  In fact, messages arriving on eth0 would get
 processed.

 When we call openSockets6 again,  we report two failures an return a count
 of 0 open when in fact, both eth0 and eth1 have open sockets.

 This is a little like Message::fromWire in that openSockets6 isn't
 accurate unless it starts from a state of all sockets on all interfaces
 closed.

 Maybe this is a non-issue but it is at least worth a mention in the method
 commentaries.

 BTW, openSockets4() appears to behave the same way.


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

 The following cppcheck errors showed up under OS-X:

     <error file="src/lib/dhcp/iface_mgr.cc" line="520" id="variableScope"
 severity="style" msg="The scope of the variable 'sock' can be
 reduced."/>
     <error file="src/lib/dhcp/iface_mgr.cc" line="522" id="unreadVariable"
 severity="style" msg="Variable 'sock' is assigned a value that
 is never used."/>


 Unit tests pass on OS-X; and pass with valgrind on Fedora 19

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