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

[BIND 10 Development]

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

 * owner:  tmark => marcin


Comment:

 Replying to [comment:8 marcin]:
 > Thanks for addressing my comments above. I will have a look at them
 tomorrow. I just realized that some of my comments got truncated, so I am
 adding them now. I apologize for it. I copied the review comments from the
 local file to trac and probably they got lost during copy-paste.
 >
 >
 > '''src/bin/d2/ncr_udp.h'''
 > Typo: line 64 - !''aiso!''
 >
 > General comment: I think we tend to use !''@return!'' tag in the doxygen
 comments describing functions that return something. It is true, that in
 case of getters the text in the !''@return!'' tag often duplicates
 !''@brief!'' but, in other cases it may be good to consider having
 !''@return!''.
 >


 > UDPCallback::Data constructor: could RawBufferPtr be passed by reference
 (as well as !UDPEndpointPtr)? They are later copied in the ctor's
 initialization list anyway.
 > UDPCallback constructor: Could pointer objects be passed by reference,
 or maybe const reference? Also could buf_size be const? They are not
 modified by the constructor.
 >
 > UDPCallback::operator(): please check alignment of the second line of
 the operator declaration.
 >
 > UDPCallback::setDataSource: can UDPEndpointPtr be passed by reference?
 >

 UDPCallback changes done.


 > NameChangeUDPListener ctor: Duplicate !''when!'' in the doxygen comment.

 Done.

 > Description of the parameter !''port!'': It says ''IP port''. I guess it
 should rather be ''UDP port''.
 >

 Picky, picky. ;)

 > NameChangeUDPListener::recv_completion_handler: The format of the name
 of this method is invalid. It should rather be:
 !''recvCompletionHandler!''. Although, it is a special purpose function
 and this format is used in boost, we don't do exceptions for handlers in
 bind10. I belive and they should follow the normal naming convention.
 >

 I agree. Some how I was mixing the concept of having an instance member
 pointer to a handler and the handler itself.   Oops.  Fixed this.

 > Also, why !''successful!'' parameter can't be const?

 It can be, I was just testing you.

 >
 > NameChangeUDPSender::open: In the Doxygen comment the:
 > {{{
 > @throw throws a NcrUDPError if the open fails
 > }}}
 > should rather be
 > {{{
 > @throw NcrUDPError if the open fails

 Fixed all @throw throws in ncr_io.h and ncr_udp.h

 > }}}
 > Also, spurious space between !''IOService!'' and ampersand.

 got it

 >
 > NameChangeUDPSender::doSend: please consider passing the parameter via
 reference.
 >
 > NameChangeUDPSender:send_completion_handler: It should be
 !''sendCompletionHandler!''

 Yep, I fixed it.

 >
 > '''src/bin/d2/tests/ncr_udp_unittests.cc'''
 > checkSendVsReceived: It should be fine to compare the text form until we
 implement more proper way to compare these two. However, I would suggest
 that the operator is now created and that the text comparison is moved to
 this new operator. That way, you could be already using the equality
 operator to compre name change request. Once you finally implement proper
 comparison, it would be transparent change for the code. With the current
 code you would have to identify all places in the code where text
 comparison is made and replace it.

 As part of the work I am doing on the queue manager (trac 3052), I
 implemented == and !=.  I have
 pulled that change into 3008.

 >


 > NameChangeUDPListenerTest constructor: the listener_ is allocated but it
 is never deleted. This is what Valgrind tells about it:
 > {{{
 > =22700==
 > ==22700== HEAP SUMMARY:
 > ==22700==     in use at exit: 4,696 bytes in 14 blocks
 > ==22700==   total heap usage: 21,918 allocs, 21,904 frees, 2,428,072
 bytes allocated
 > ==22700==
 > ==22700== 4,696 (120 direct, 4,576 indirect) bytes in 1 blocks are
 definitely lost in loss record 14 of 14
 > ==22700==    at 0x4A09361: operator new(unsigned long)
 (vg_replace_malloc.c:298)
 > ==22700==    by 0x498B69: testing::internal::TestFactoryImpl<(anonymous
 namespace)::NameChangeUDPListenerTest_basicReceivetest_Test>::CreateTest()
 (ncr_udp_unittests.cc:168)
 > ==22700==    by 0x4C47E49: testing::Test*
 testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::TestFactoryBase,
 testing::Test*>(testing::internal::TestFactoryBase*, testing::Test*
 (testing::internal::TestFactoryBase::*)(), char const*) (gtest.cc:2090)
 > ==22700==    by 0x4C3CCBD: testing::TestInfo::Run() (gtest.cc:2331)
 > ==22700==    by 0x4C3CE14: testing::TestCase::Run() (gtest.cc:2445)
 > ==22700==    by 0x4C3D184:
 testing::internal::UnitTestImpl::RunAllTests() (gtest.cc:4237)
 > ==22700==    by 0x4C47B49: bool
 testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl,
 bool>(testing::internal::UnitTestImpl*, bool
 (testing::internal::UnitTestImpl::*)(), char const*) (gtest.cc:2090)
 > ==22700==    by 0x4C3C460: testing::UnitTest::Run() (gtest.cc:3874)
 > ==22700==    by 0x42776D: main (d2_unittests.cc:28)
 > ==22700==
 > ==22700== LEAK SUMMARY:
 > ==22700==    definitely lost: 120 bytes in 1 blocks
 > ==22700==    indirectly lost: 4,576 bytes in 13 blocks
 > ==22700==      possibly lost: 0 bytes in 0 blocks
 > ==22700==    still reachable: 0 bytes in 0 blocks
 > ==22700==         suppressed: 0 bytes in 0 blocks
 > ==22700==
 > ==22700== For counts of detected and suppressed errors, rerun with: -v
 > ==22700== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 2 from 2)
 >
 > }}}


 I had intended that listener_ be a smart pointer.  It is now.

 >
 > NameChangeUDPListenerTest::basicReceivetest: I guess it should be
 !''basicReceiveTest!''.
 > The following section of this test:
 > {{{
 >     EXPECT_FALSE(listener_->amListening());
 >     ASSERT_NO_THROW(listener_->startListening(io_service_));
 >     ASSERT_TRUE(listener_->amListening());
 > }}}
 > may need to be corrected:
 > {{{
 >     ASSERT_FALSE(listener_->amListening());
 >     ASSERT_NO_THROW(listener_->startListening(io_service_));
 >     EXPECT_TRUE(listener_->amListening());
 > }}}

 Actually all three should be asserts. If any of those three fail the rest
 of test is
 meaningless.

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