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:  tmark
                Type:  enhancement   |                       Status:
            Priority:  medium        |  reviewing
           Component:  dhcp          |                    Milestone:  DHCP-
            Keywords:                |  Kea1.0-alpha
           Sensitive:  0             |                   Resolution:
         Sub-Project:  DHCP          |                 CVSS Scoring:
Estimated Difficulty:  0             |              Defect Severity:  N/A
         Total Hours:  11            |  Feature Depending on Ticket:  3251
                                     |          Add Hours to Ticket:  4
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by marcin):

 * owner:  marcin => tmark


Comment:

 Replying to [comment:5 tmark]:
 > 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.

 The funny thing about it is that I realized that before I read your
 comment :) Fixed.

 >
 >
 ------------------------------------------------------------------------------
 >
 > 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...

 Excellent suggestion to enclose in brackets!

 >
 >
 ---------------------------------------------------------------------------
 >
 > 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())

 I fixed it either here or in the v4 test.

 >
 > * 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;
 >     }
 > }}}
 >

 I do this too often! Fixed.

 > * 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.

 The goal wasn't to simulate the case when Kea opens two sockets on the
 same address and port. I had in mind the case when there is another
 instance of the server with a socket bound to the same address and port
 (and yes, I know we actually open the socket within the same !IfaceMgr
 instance, but this is only a test to check specific behavior). So, it is
 not really an issue that the count of sockets opened is 1. I'd even say
 that it is an intended behavior if you keep in mind that the socket on
 eth0 was opened by another server instance. We only count sockets opened
 by us.

 >
 > 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.

 I added a warning to both openSockets4 and openSockets6.


 >
 >
 >
 -------------------------------------------------------------------------------
 >
 > 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."/>
 >

 Thanks for that. The problem is that the sock variable is needed on Linux
 only. That is because we close the socket a few lines down in the Linux-
 only code. I didn't want to create more ifdef sections with and without
 this variable, so I rather suppressed this warning and I see no harm in
 this suppression.


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

 Thank you QA team!

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