BIND 10 #3329: Integrate use of D2ClientMgr IO services into b10-dhcp4

[BIND 10 Development]

#3329: Integrate use of D2ClientMgr IO services into b10-dhcp4
-------------------------------------+-------------------------------------
            Reporter:  tmark         |                        Owner:
                Type:  enhancement   |  marcin
            Priority:  medium        |                       Status:
           Component:  dhcp-ddns     |  reviewing
            Keywords:                |                    Milestone:  DHCP-
           Sensitive:  0             |  Kea0.9-alpha
         Sub-Project:  DHCP          |                   Resolution:
Estimated Difficulty:  24            |                 CVSS Scoring:
         Total Hours:  35            |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  1
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by tmark):

 * hours:  2 => 1
 * owner:  tmark => marcin
 * totalhours:  34 => 35


Comment:

 Replying to [comment:5 marcin]:
 > Reviewed commit e8513d7199db5ba79b1f91087f57eb54ab81ef74
 >
 > '''src/bin/dhcp4/ctrl_dhcp4_srv.cc'''
 > establishSession: you should change the type of the exception being
 caught in this function so as it traps exceptions being thrown by
 startD2() function (at least function documentation says that it may throw
 !D2ClientError). Currently !DhcpConfigError is caught in establishSession.
 >

 Good catch (no pun intended). I changed to simply catch std::exception.

 > '''src/bin/dhcp4/dhcp4_messages.mes'''
 > I think it would be better to refer the DHCP server as !''DHCPv4
 server!'', instead of !''IPv4 DHCP server!''.
 >

 Changed all them.

 > Also, spurious space before %1.

 Got it.

 >
 > '''src/bin/dhcp4/tests/d2_unittest.cc'''
 > There is an unspecified todo in line 32.

 Oops. That was carried over from the .h file when I first added the
 method.
 todo removed, no action necessary.

 >
 > '''src/lib/dhcp_ddns/dhcp_ddns_messages.mes'''
 > In this: !'' This is a programmatic error, highly unlikely to occur, and
 should not impair the application's to process requests.!'' - should it be
 !''application's ability!'', or so? Sounds like something is missing.
 >

 Sorry, I lapsed into Polglish there. Fixed.

 > '''src/lib/dhcp_ddns/ncr_io.h'''
 > I don't understand why you have this warning: !''Running all ready
 handlers, in theory, could process all messages currently queued.!'' in
 the context of runReadyIO? This function is supposed to run one ready
 handler (not all). Even if I run it multiple times, what is the problem
 with processing all queued messages?
 >

 I added the following commentary along with the warning:

 {{{
     ///
     /// NameChangeSender daisy chains requests together in its completion
     /// by one message completion's handler initiating the next message's
 send.
     /// When using UDP, a send immediately marks its event handler as
 ready
     /// to run.  If this occurs inside a call to ioservice::poll() or
 run(),
     /// that event will also be run.  If that handler calls UDP send then
     /// that send's handler will be marked ready and executed and so on.
 If
     /// there were 1000 messages in the queue then all them would be sent
 from
     /// within the context of one call to runReadyIO().
     /// By running only one handler at time, we ensure that NCR IO
 activity
     /// doesn't starve other processing.  It is unclear how much of a real
     /// threat this poses but for now it is best to err on the side of
 caution.
     ///
 }}}

 > '''src/lib/dhcp_ddns/tests/ncr_udp_unittests.cc'''
 > The comment in line 492 is partly true as it doesn't check ready state
 via select but siumply calls ioReady:
 > {{{
 > // Make sure select_fd does evaluates to ready via select and
 > // that ioReady() method agrees.
 > ASSERT_TRUE(sender.ioReady());
 > }}}
 >
 > Perhaps the comments should be modified to just say that we call
 ioReady.
 >

 Copy and paste error there. Fixed it.

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