BIND 10 #3251: libdhcp++: the openSockets6 function should be unit tested.
BIND 10 Development
do-not-reply at isc.org
Tue Dec 17 18:13:26 UTC 2013
#3251: libdhcp++: the openSockets6 function should be unit tested.
-------------------------------------+-------------------------------------
Reporter: marcin | Owner:
Type: defect | marcin
Priority: medium | Status:
Component: dhcp | reviewing
Keywords: | Milestone: DHCP-
Sensitive: 0 | Kea1.0-alpha
Sub-Project: DHCP | Resolution:
Estimated Difficulty: 0 | CVSS Scoring:
Total Hours: 0 | Defect Severity: N/A
| Feature Depending on Ticket:
| Add Hours to Ticket: 0
| Internal?: 0
-------------------------------------+-------------------------------------
Changes (by tomek):
* owner: tomek => marcin
Comment:
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.
--------------
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.
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.
---------------
'''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).
All function descriptions should be in 3rd person (e.g. Open a
socket=> Opens a socket).
PktFilter6::openSocket() - This function open => This function opens
and IP address => and IPv6 address
'''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.
'''pkt_filter_inet6.h'''
All function descriptions should be in 3rd person (e.g. Open a socket=>
Opens a socket).
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.)
'''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.
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.
'''iface_mgr.cc'''
bool IfaceMgr::openSockets6():488 and following. joinMulticast is now
done elsewhere, please remove that commented out code.
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.
'''iface_mgr.h'''
pkt_filter6_ field should be documented.
'''libdhcp++.dox'''
Good update. Very informative.
'''test/pkt_filter6_test_utils.h'''
There is no dot after ifname_ description, but there is one after all
other members.
'''test/iface_mgr_unittest.cc'''
createIface() description: fictious => fictitious
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).
Alternatively, you can rename checkSocketsCount6 to
countUnicastSockets6, but I don't think this is what the current code
does.
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.
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?
openSockets6IfaceInactive - comment before setIfaceFlags() seems
invalid (active/inactive for v4/v6 is flipped)
IfaceMgrTest.openSockets6NoIfaces - typo in test comment
"That that" => "Tests that"
------------------------------
The ChangeLog looks fine.
--
Ticket URL: <http://bind10.isc.org/ticket/3251#comment:4>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list