BIND 10 #3008: Develop NameChange{Sender,Listener} classes.
BIND 10 Development
do-not-reply at isc.org
Tue Jul 23 18:41:30 UTC 2013
#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
Comment:
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?
NameChangeUDPListener ctor: Duplicate !''when!'' in the doxygen comment.
Description of the parameter !''port!'': It says ''IP port''. I guess it
should rather be ''UDP port''.
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.
Also, why !''successful!'' parameter can't be const?
NameChangeUDPSender::open: In the Doxygen comment the:
{{{
@throw throws a NcrUDPError if the open fails
}}}
should rather be
{{{
@throw NcrUDPError if the open fails
}}}
Also, spurious space between !''IOService!'' and ampersand.
NameChangeUDPSender::doSend: please consider passing the parameter via
reference.
NameChangeUDPSender:send_completion_handler: It should be
!''sendCompletionHandler!''
'''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.
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)
}}}
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());
}}}
--
Ticket URL: <https://bind10.isc.org/ticket/3008#comment:8>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list