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:  14            |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:  3251
                                     |          Add Hours to Ticket:  1
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by tmark):

 * hours:  2 => 1
 * owner:  tmark => marcin
 * totalhours:  13 => 14


Comment:

 Replying to [comment:6 marcin]:
 > 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.

 If you could just sharpen those precognitive skills a bit, you wouln't
 need me to reivew your tickets anymore. :)

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

 A practice left over from yesteryear when STL didn't exist and macros were
 all the rage.  You weren't born yet.

 >
 > >
 > >
 ---------------------------------------------------------------------------
 > >
 > > 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 warning should suffice given the intent of these methods. To be clear
 thoughmy point is that an instance of Kea fails to open a socket on an
 interface that it has already opened.  That shouldn't constitute an error.
 What is an error is if something else has the socket open.


 Please merge in your changes.

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