BIND 10 #3008: Develop NameChange{Sender,Listener} classes.

[BIND 10 Development]

#3008: Develop NameChange{Sender,Listener} classes.
-------------------------------------+-------------------------------------
            Reporter:  tmark         |                        Owner:  tmark
                Type:  enhancement   |                       Status:
            Priority:  medium        |  reviewing
           Component:  dhcp-ddns     |                    Milestone:
            Keywords:                |  Sprint-DHCP-20130731
           Sensitive:  0             |                   Resolution:
         Sub-Project:  DHCP          |                 CVSS Scoring:  n
Estimated Difficulty:  0             |              Defect Severity:  N/A
         Total Hours:  0             |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by marcin):

 * owner:  marcin => tmark
 * cvss:   => n


Comment:

 Reviewed commit a765638850588cc8980c622351124f1319c46365

 In general, a piece of very good and useful code.

 '''Review comments'''

 This commit contains invalid email address:
 {{{
 commit a765638850588cc8980c622351124f1319c46365
 Author: Thomas Markwalder <tmark at debian-tkm-01.tmark>
 Date:   Thu Jul 18 07:21:22 2013 -0400

     [3008] Removed extraneous const function return qualifiers the debian
     complained about.

 }}}

 Perhaps committed from the VM.

 General comment: I suggest that !''queue!'' or !''QUEUE!'' is used,
 instead of !''que!'' or !''QUE!'' throughout the code. I understand that
 !''que!'' is shorter and thus easier to type but when I think about the
 variable or function name that refers to a queue I would rather type the
 longer form and would be surprised to see that the symbol does not exist.
 Besides that, the longer form is more widely used in the code.

 '''src/bin/d2/d2_messages.mes'''
 Suggest that the following messages: DHCP_DDNS_NCR_UDP_(SEND |
 LISTEN)_CLOSE is renamed to something like DHCP_DDNS_NCR_UDP_(SEND |
 LISTEN)_CLOSE_ERROR to reflect that it is an error. The other name
 suggests it is an informational message that something has been closed.

 '''DHCP_DDNS_NCR_UDP_RECV_ERROR''': Looks like spurious !''in the
 request!''.

 '''src/bin/d2/ncr_io.cc'''
 '''!NameChangeListener Constructor''': invalid alignment of the second
 line of parameters of ''isc_throw''.

 Neat pick: suggest use of !''constructor!'' instead of ctor in the
 exception string.

 '''NameChangeListener::stopListening''': I am wondering if you should call
 {{{
 setListening(false)
 }}}
 if exception has occured during invocation of ''close()''. It looks to me
 that exception is a sign of error that causes the listener to continue to
 run (because it couldn't be closed).

 '''NameChangeListener::invokeRecvHandler''': I don't understand why
 recv_handler_ must be declared const here and thus why const_cast is
 used?. If you want to make sure not to modify he handler, you could
 provide an accessor function and use it to access the pointer. However, I
 am still not sure if it is required here.

 '''!NameChangeSender Constructor''': Neat pick: Suggest that you replace
 !''ctor!'' with !''constructor!''.

 '''NameChangeSender::startSending''': spurious space between
 !''!IOService!'' and ampersand.
 Also, there should be a space after !''Open failed:!''.

 '''NameChangeSender::stopSending''': similar comment as previously for
 !''NameChangeListener::stopListening!
 ''.

 '''NameChangeSender::invokeSendHandler''':

 What happens if the Completion Handler throws an exception? It is not
 handled here and will be propagated down to the thread where
 io_service.run() was called. The !''sendNext!'' function will not be
 called.

 The same issue applies to NameChangeListener::invokeSendHandler

 '''src/bin/d2/ncr_io.h'''
 In the file description, one line runs on another.
 Also, it would be more logical to me if the layers were described/listed
 from the top level layer to bottom level layer. In other words, perhaps
 you could swap IO Layer with !NameChangeRequest layer.

 Duplicate !''implement!'' in !''The abstract classes defined...!''.

 '''!NcrListenerError class''': the description could be extended. For
 example:
 {{{
 @brief Exception thrown if an error occurs other than covered by @c
 NcrListenerOpenError and @c NcrListenerReceiveError.
 }}}
 or something like this.

 '''!RequestReceiveHandler''': Technically, the !NameChangeRequestPtr could
 be passed by a reference.

 Neat pick: Result parameter could be declared const. But it doesn't have
 to.

 '''!NameChangeListener Constructor''': Are there any use cases when
 recv_handler could be NULL, indicating that no callback should be called
 on request completion? If not, throwning exception when recv_handler is
 NULL is fine.

 '''invokeRecvHandler''': !NameChangeRequestPtr could be passed by
 reference.

 '''RequestSendHandler::operator()''': NameChangeRequestPtr could be passed
 by reference.

 '''!RequestSendHandler constructor''': are there any use cases when
 callback could be NULL to indicate that no callback should be called upon
 request completion?

 '''sendRequest''': !NameChangeRequestPtr could be passed by reference.

 '''invokeSendHandler''': argument could be const. It doesn't have to
 though.

 '''flushSendQue''': the name starting with !''flush!''suggests that all
 requests in the queue are dispatched while they are actually removed.

 '''getNcrToSend''': I am wondering if getNcrToSend should return a const
 reference to a pointer or maybe it shoulnd rather return NCR (not a
 pointer) by value. The reason is that when you return a reference it may
 invalidate when ncr_to_send_ is replaced with the new NCR by the logic
 processing the queue. As a result, the value held by the caller may get
 changed after he has got it. Something like this could work:
 {{{
 NameChangeRequest getNcrToSend() const {
     return (*ncr_to_send_);
 }
 }}}
 Also, whatever you choose, this method should be const.

 '''src/bin/d2/ncr_udp.cc'''
 '''UDPCallback::operator()''': Neatpick: invalid alignment in the second
 line of the parameters list.

 '''!NameChangeUDPListener ctor''': Please check the line breaks etc. in
 the constructor against coding guidelines.

 '''NameChangeUDPListener::open''': typo !''leve!'', instead of
 !''level!''.

 '''NameChangeListener::open''':  The comment in this section is
 misleading:
 {{{
     // If in test mode, enable address reuse.
     if (reuse_address_) {
         asio_socket_->set_option(asio::socket_base::reuse_address(true));
     }
 }}}
 It is should be mentioned that reuse_address_ is only used by unit tests
 (if it is true). Perhaps, reuse_address_ (and reuse_address_ in the
 constructor) could be then renamed to test_mode_ (test_mode). At least it
 should be mentioned in the constructor that address reuse should be set to
 true in the unit tests only.

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