BIND 10 #3251: libdhcp++: the openSockets6 function should be unit tested.
BIND 10 Development
do-not-reply at isc.org
Thu Dec 19 17:20:02 UTC 2013
#3251: libdhcp++: the openSockets6 function should be unit tested.
-------------------------------------+-------------------------------------
Reporter: marcin | Owner: tomek
Type: defect | Status:
Priority: medium | reviewing
Component: dhcp | Milestone: DHCP-
Keywords: | Kea1.0-alpha
Sensitive: 0 | Resolution:
Sub-Project: DHCP | CVSS Scoring:
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 => tomek
Comment:
Replying to [comment:4 tomek]:
> The code did not compile on Ubuntu 13.04 with g++ 4.8.1 and gtest 1.7:
> {{{
> thomson at billabong:~/devel/bind10/src/lib/dhcp$ make -j8
> Making all in .
> make[1]: Wejście do katalogu `/home/thomson/devel/bind10/src/lib/dhcp'
> make[1]: Nie ma nic do zrobienia w `all-am'.
> make[1]: Opuszczenie katalogu `/home/thomson/devel/bind10/src/lib/dhcp'
> Making all in tests
> make[1]: Wejście do katalogu
`/home/thomson/devel/bind10/src/lib/dhcp/tests'
> Making all in .
> make[2]: Wejście do katalogu
`/home/thomson/devel/bind10/src/lib/dhcp/tests'
> CXX libdhcp___unittests-iface_mgr_unittest.o
> In file included from
../../../../src/lib/dhcp/tests/pkt_filter6_test_utils.h:21:0,
> from iface_mgr_unittest.cc:22:
> /home/thomson/devel/gtest-1.7.0/include/gtest/gtest.h: In member
function ‘virtual void
{anonymous}::IfaceMgrTest_openSockets6Unicast_Test::TestBody()’:
> /home/thomson/devel/gtest-1.7.0/include/gtest/gtest.h:262:60: error:
‘success’ may be used uninitialized in this function [-Werror=maybe-
uninitialized]
> explicit AssertionResult(bool success) : success_(success) {}
> ^
> iface_mgr_unittest.cc:1525:10: note: ‘success’ was declared here
> bool success;
> ^
> In file included from
../../../../src/lib/dhcp/tests/pkt_filter6_test_utils.h:21:0,
> from iface_mgr_unittest.cc:22:
> /home/thomson/devel/gtest-1.7.0/include/gtest/gtest.h: In member
function ‘virtual void
{anonymous}::IfaceMgrTest_openSockets6IfaceDown_Test::TestBody()’:
> /home/thomson/devel/gtest-1.7.0/include/gtest/gtest.h:262:60: error:
‘success’ may be used uninitialized in this function [-Werror=maybe-
uninitialized]
> explicit AssertionResult(bool success) : success_(success) {}
> ^
> iface_mgr_unittest.cc:1572:10: note: ‘success’ was declared here
> bool success;
> ^
> In file included from
../../../../src/lib/dhcp/tests/pkt_filter6_test_utils.h:21:0,
> from iface_mgr_unittest.cc:22:
> /home/thomson/devel/gtest-1.7.0/include/gtest/gtest.h: In member
function ‘virtual void
{anonymous}::IfaceMgrTest_openSockets6IfaceInactive_Test::TestBody()’:
> /home/thomson/devel/gtest-1.7.0/include/gtest/gtest.h:262:60: error:
‘success’ may be used uninitialized in this function [-Werror=maybe-
uninitialized]
> explicit AssertionResult(bool success) : success_(success) {}
> ^
> iface_mgr_unittest.cc:1621:10: note: ‘success’ was declared here
> bool success;
> ^
> cc1plus: all warnings being treated as errors
> make[2]: *** [libdhcp___unittests-iface_mgr_unittest.o] Błąd 1
> }}}
>
> Fix was easy - initialize bool variables. Fix pushed.
Thanks a lot. I am using gtest 1.6 and I didn't observe these errors on my
system.
>
> --------------
> Not exactly part of the ticket, but found in related code:
>
> '''pkt_filter_lpf.h''':30 - @warning about being not implemented is no
> longer applicable. Please remove it.
Removed
>
> Please also add a comment that it is used to handle DHCPv4 traffic
> and is necessary to handle packets sents from/to IPv4 hosts without
> IPv4 address assigned.
Added.
>
> ---------------
>
> '''pkt_filter6.h''' - In the class comment. Add an explanation that is
has nothing
> to do with with Linux or BSD Packet Filter. In general, I'd prefer the
code to be
> named AbstractSocketUDP6 and SocketUDP6 or similar, but I won't object
if you insist
> to keep the current naming. Just keep in mind that we will be adding TCP
sockets to the
> mix one day (for bulk leasequery and for failover).
Added additional comment.
The IfaceMgr holds a common code for V4 and V6. Although, the other naming
could be considered I thought it may be useful to have the naming
consistent for V4 and V6 because both classes serve pretty much the same
purpose and expose similar API.
>
> All function descriptions should be in 3rd person (e.g. Open a
> socket=> Opens a socket).
Changed.
>
> PktFilter6::openSocket() - This function open => This function opens
> and IP address => and IPv6 address
Fixed.
>
> '''pkt_filter6.cc'''
> PktFilter6::joinMulticast() please clear mreq before using. There's no
> guarantee that mreq struct will always consist of only
> ipv6mr_multiaddr and imv6mr_interface fields.
Added memset.
>
> '''pkt_filter_inet6.h'''
> All function descriptions should be in 3rd person (e.g. Open a socket=>
Opens a socket).
Changed.
>
> PktFilterInet6::receive() - the comment should mention that if there
> is no data waiting on a given socket, it will block. (That's ok, the
> IfaceMgr::receive() will take care of that.)
Added a comment.
>
> '''pkt_filter_inet.cc'''
> PktFilterInet6::openSocket() - comment in line 58 seems wrong. If we
> are being restarted, the old instance should close the socket, so the
> new one can open it up immediately.
It was your comment. I just copy-pasted the code from IfaceMgr. I agree it
is wrong, so I removed it.
>
> Please use @todo (rather than "TODO") in line 50. When using @todo
syntax, the issue
> appears on this list:
http://git.bind10.isc.org/~tester/doxygen/dd/da0/todo.html
> I'm secretly hoping that one day we'll start working through that list.
I agree. Again it was your code though. :)
>
> '''iface_mgr.cc'''
> bool IfaceMgr::openSockets6():488 and following. joinMulticast is now
> done elsewhere, please remove that commented out code.
Ooops. I was planning to remove it but simply forgot.
>
> Nothing to do here just yet, but the @todo in line 480 is
> interesting. Ticket #2246 has been merged to master already. After the
> review is complete, you have merged it to master (but not pushed yet),
> can you try if this ifdef is still necessary? This is just an
experiment.
> If it fails, then keep it. It may be a potential work for a separate
> ticket.
It is interesting. I don't know the origin of this todo. I also don't
understand what this todo is doing there. The commentary you had put there
says that binding a socket to multicast address doesn't work on NetBSD so
if this is true, removing a todo will break NetBSD. But, we can obviously
make some experiments here.
>
> '''iface_mgr.h'''
> pkt_filter6_ field should be documented.
Added documentation.
>
> '''libdhcp++.dox'''
> Good update. Very informative.
>
Thanks.
> '''test/pkt_filter6_test_utils.h'''
> There is no dot after ifname_ description, but there is one after all
> other members.
There is now.
>
> '''test/iface_mgr_unittest.cc'''
> createIface() description: fictious => fictitious
Ooops. Fixed.
>
> checkSocketsCount6(): Why is there +2 for Linux and +1 for non-linux?
> It should be +1 for Linux and exact match for non-linux (as the
> comments suggest).
>
> This is especially true in context of
IfaceMgrTest.openSockets6LinkLocal.
> The test in its current form is self-contradicting.
CallsCheckSocketCount6
> suggest that the number of expected sockets is 0 on all interfaces. I
> would expect that the parameters passed be:
>
> {{{
> checkSocketsCount6(*ifacemgr.getIface("lo"), 0);
> checkSocketsCount6(*ifacemgr.getIface("eth0"), 1);
> checkSocketsCount6(*ifacemgr.getIface("eth1"), 1);
> }}}
> (don't open anything on loopback, open one socket on eth0 and eth1).
No. The second parameter specifies how many unicast addresses have been
specified (see function documentation). So the value of 0 means, there is
no unicast address to which the socket should be bound. In this case, the
function expects that there will be two sockets: one bound to link local
address, another one bound to multicast address (on linux).
>
> Alternatively, you can rename checkSocketsCount6 to
> countUnicastSockets6, but I don't think this is what the current code
does.
No. The purpose of this function is to check that there is an expected
number of sockets opened on the interface: whether they are bound to
unicast, link-local or multicast address. This function doesn't check
unicast only.
>
> Please add a test for case where there is an interface, but it does
> not have any link-local addresses. Link local addresses are necessary
> for DHCPv6 to work, so such an interface should be skipped. One real
> life case of such odd interfaces is 6rd.
Added new test case. I also had to extend the checkSocketsCount6 to
control the number of link-local addresses on an interface.
>
> It is questionable whether to also add a test for skipping interfaces
> that are not multicast capable. On one hand, this is a requirement for
> DHCPv6, but on some point to point interfaces (e.g. ppp) it is still
> useful to run DHCPv6 and is actually a business advantage (ISC DHCP
> refuses to work on such interfaces, but Dibbler does that and works
> correctly). So perhaps add a note that we chose to omit multicast
> check to enable support for point-to-point interfaces?
I modified the IfaceMgr to open a socket and DO NOT join multicast group
nor bind a socket to multicast address if the flag_multicast_ is not set.
Previously the code would throw an exception. In the future, I am planning
to issue a warning logger message when the interface is multicast-
incapable.
>
> openSockets6IfaceInactive - comment before setIfaceFlags() seems
> invalid (active/inactive for v4/v6 is flipped)
They are not really. The inactive = true means that the interface is
active, and vice versa. Setting a value to true means do not use this
interface.
>
> IfaceMgrTest.openSockets6NoIfaces - typo in test comment
> "That that" => "Tests that"
Corrected.
>
> ------------------------------
>
> The ChangeLog looks fine.
Great.
--
Ticket URL: <https://bind10.isc.org/ticket/3251#comment:5>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list