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

 * hours:  48 => 5
 * owner:  marcin => tmark
 * totalhours:  0 => 53


Comment:

 Reviewed commit 400bf9a1f5d4e667c72a51255a2477721876d103

 You may need to update copyright headers here and there.

 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.

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

 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.

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

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

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

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

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

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

 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;

 }}}

 ?

 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?

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

 '''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?

 Nits:
 - Since you already added @file tag, you could also add some brief
 desription of the file. But, this is up to you, since we rarely remember
 to put a file tag.
 - in the commentary !''Plus no likes destructors that throw!'', shouldn't
 it be !''No one likes...!'' or so?

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

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

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

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

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