BIND 10 #3221: DHCP4 should send generated NameChangeRequests to b10-dhcp-ddns

[BIND 10 Development]

#3221: DHCP4 should send generated NameChangeRequests to b10-dhcp-ddns
-------------------------------------+-------------------------------------
            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:  40            |                 CVSS Scoring:
         Total Hours:  59            |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  6
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by tmark):

 * hours:  5 => 6
 * owner:  tmark => marcin
 * totalhours:  53 => 59


Comment:

 Replying to [comment:7 marcin]:
 > Reviewed commit 400bf9a1f5d4e667c72a51255a2477721876d103
 >
 > You may need to update copyright headers here and there.
 >

 Oops, I forgot about that.

 > I think it is acceptable to have one !ChangeLog entry which aggregates a
 couple of tickets. So, it makes sense to create the entry when you
 implement all outstanding tickets for this task.
 >

 Makes sense. Will create the appropriate !ChangeLog entry with Trac #3329.

 > '''src/lib/dhcp_ddns/ncr_io.cc'''
 > CamelCase notation for the assumeQueue parameter.

 Yikes, how did that happen?  Fixed.

 >
 > The cppcheck may complain about inefficient check for the queue being
 empty in:
 > {{{
 >     if (send_queue_.size() != 0) {
 >         isc_throw(NcrSenderError, "Cannot assume queue:"
 >                   " target queue is not empty");
 >     }
 > }}}
 >
 > You may use .empty() instead.

 Good catch. Done.

 >
 > '''src/lib/dhcp_ddns/ncr_io.h'''
 > CamelCase for naming a parameter passed to assumeQueue. Also, the name
 of this parameter in the doxygen is wrong.
 >

 Hey when I muck up a name, I do it right!  Fixed.

 > getSelectFd: replace !''description!'' with !''descriptor!''.


 >
 > I suggest that the @note is replaced with @warning in the getSelectFd
 description.
 >

 Done.

 > Question: if the !NameChangeRequest is an abstract class, could you just
 make the getSelectFd method abstract, instead of providing an
 implementation that throws !NotImplemented?
 >

 You mean, NameChangeSender. But yes, it should just be abstract. Done.

 > '''src/lib/dhcp_ddns/ncr_udp.cc'''
 > Technically, the operations on watch socket are not exception safe. In
 order words, WatchSocket class may throw an exception in
 !NameChangeUDPSender::open, doSend and sendCompletionHandler. From the
 !NameChangeSender documentation it seems ok, that the first two throw an
 exception. I am not certain if it is ok that sendCompletionHandler does,
 as a result of failure to clear the watch socket (line 314).
 >
 > '''src/lib/dhcp_ddns/ncr_udp.h'''
 > Nits:
 > - Replace !''description!'' with !''descriptor!'' in the getSelectFd
 doc?
 > - Suggest to replace a @note tag with @warning tag
 > - watch_socket_ should be documented
 > - Not strictly related to this ticket but the ncr parameter of doSend
 appears to be not documented
 >

 Got em.

 > '''src/lib/dhcp_ddns/tests/ncr_udp_unittests.cc'''
 > Should this be removed:
 > {{{
 > //const char* TEST_ADDRESS = "192.0.2.10";
 > }}}
 > ?

 Yes, boy am I sloppy or what?. Gone.

 >
 > Why this change in basicSendTests:
 > {{{
 >  TEST(NameChangeUDPSenderBasicTest, basicSendTests) {
 > -    isc::asiolink::IOAddress ip_address(TEST_ADDRESS);
 > -    uint32_t port = SENDER_PORT;
 > +    isc::asiolink::IOAddress ip_address("127.0.0.1");
 > +    uint32_t port = 5301;
 >
 > }}}

 Oops. Was not meant to be permanent.

 >
 > ?
 >
 > A little bit more commentary in the unit test about this:
 > {{{
 > -    NameChangeUDPSender sender(ip_address, port, ip_address, port,
 > +    isc::asiolink::IOAddress any("0.0.0.0");
 > +    NameChangeUDPSender sender(any, 0, ip_address, port,
 >
 > }}}
 >
 > would be useful. In particular, is port 0 reserved for testing?
 >

 Actually I restored this test to its original form and added a second test
 that specifically tests binding the sender's outbound socket to INADDR_ANY
 and port 0.  "0" tells the OS to find an avaible port.


 > '''src/lib/dhcp_ddns/tests/watch_socket_unittests.cc'''
 > Nit: Update copyright header.
 >
 Fixed it.

 > '''src/lib/dhcp_ddns/watch_socket.cc'''
 > Do you think that the clearReady function should react if the value read
 from the pipe is not matching the marker? That would mean that something
 is spoofing your pipe. I think it is unlikely but what if it happens?
 >

 Since the pipe is accessible only within the process space which owns it
 there is little chance for it to be tampered with other than by
 programmatic error.  There is no way to write to it from outside the
 !WatchSocket instance.  One can however close or read from the select_fd.
 I have shored up its behavior to make it a bit more predictable if it does
 get corrupted. The primary goals were:

 # Avoid it being stuck always evaluating as ready
 # Always clear it if it needs to be cleared
 # Always Fail to mark it ready if it has been compromised


 > Nits:
 > - Since you already added @file tag, you could also add some brief
 description of the file. But, this is up to you, since we rarely remember
 to put a file tag.

 The class description is sufficient, plus I'm feeling lazy.

 > - in the commentary !''Plus no likes destructors that throw!'',
 shouldn't it be !''No one likes...!'' or so?
 >

 Fixed, although the comment is now located with new closeSocket() method.


 > '''src/lib/dhcp_ddns/watch_socket.h'''
 > Nits:
 > - Please consider explaining in the description of the MARKER why you
 picked this megic number. Something like: !''A meat has a lot of proteins,
 even if it is dead. Also, I simply like a beaf.!'' would do the trick.
 > - Please consider a @warning tag, instead of @note in the description of
 getSelectFd.

 Done.
 >
 > '''src/lib/dhcpsrv/d2_client.cc'''
 > setD2ClientConfig: rename queueMax to queue_max.
 >

 Weird how I do that sometimes. Got it.

 > '''src/lib/dhcpsrv/d2_client.h'''
 > One line runs on another in the description of the !D2ClientMgr class.

 Oops. Got it.

 >
 > '''src/lib/dhcpsrv/tests/d2_udp_unittest.cc'''
 > Do you need ioctl.h header for anything?

 No it was left over from some experimental stuff.

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