BIND 10 #3242: Kea4: check if only remote traffic can be supported
BIND 10 Development
do-not-reply at isc.org
Thu Feb 6 17:46:46 UTC 2014
#3242: Kea4: check if only remote traffic can be supported
-------------------------------------+-------------------------------------
Reporter: tomek | Owner: tomek
Type: defect | Status:
Priority: high | reviewing
Component: dhcp4 | Milestone: DHCP-
Keywords: | Kea0.9-alpha
Sensitive: 0 | Resolution:
Sub-Project: DHCP | CVSS Scoring:
Estimated Difficulty: 24 | Defect Severity: N/A
Total Hours: 43 | Feature Depending on Ticket:
| Add Hours to Ticket: 8
| Internal?: 0
-------------------------------------+-------------------------------------
Changes (by marcin):
* hours: 6 => 8
* owner: marcin => tomek
* totalhours: 35 => 43
Comment:
Replying to [comment:13 tomek]:
> Reviewing 1b522d63a842654c680602d3ee89eac324333d48.
Thanks for the thorough review.
>
> You submitted #3320 about using option 50. Do you think it should go to
> Kea-proposed? It is now in DHCP-Outstanding-tickets where it won't
> be looked at for at least the next 3 mothns.
We have already sorted this out.
>
> This is very helpful work. Thanks for taking care of this.
Thank you and you're welcome! :)
>
> '''src/bin/dhcp4/dhcpv4_srv.cc''':
> selectSubnet(): Please move the comment starting in line 1505 into the
scope
> it comments on. The same if true for the follow up comment starting in
1515.
Some time ago somebody (maybe Stephen) pointed out that the comment should
be before the section it describes. So I got aligned with this suggestion.
It actually makes sense for me to do it this way when there are multiple
elseif statements - we put the comment before if, why not have before
elseif? If you insist that I change it, I can change it. Do you?
>
> accept(): Since the accept() method became very similar to
> sanityCheck() methods, perhaps you could merge those two together?
I agree with that, but this is a bit more tricky in DHCPv4. The Request
message sometimes must include a server id, sometimes not. Making this
decision in the accept method (common for all messages received) requires
quite a change to the logic in sanityCheck. There is a ticket #3116 which
I am hoping will result in a separate class for making decisions on
accepting or rejecting packets and the merge will be done there.
>
> Also, accept() checks if the message includes message type option, but
> ignores value returned by query->getType(). Perhaps it could further
> sanity check returned value, so we drop bogus (with invalid type)
> packets early?
Added a check.
>
> acceptDirectRequest(): The last line can be simplified as:
> {{{
> return (!(pkt->getLocalAddr() == bcast && !selectSubnet(pkt)));
> }}}
I simplified it.
>
> Thank you for doing the clean up in dhcp4_test_utils.
You're welcome. More cleanup to come in the appropriate time.
>
> '''src/bin/dhcp4/tests/direct_client_unittest.cc'''
>
> DirectClientTest::configure() seems very handy. Can you move it to
> base Dhcpv4SrvTest?
Moved. Although, I had to swap !Dhcpv4SrvTest and !NakedDhcpv4Srv in the
header file because one become dependent on another.
>
> configureSubnet, configureTwoSubnets() - please add a comment that
> the real configuration would exclude .0 (network address) and .255
> (broadcast) address, but we ignore that fact for the sake of test
> simplicity.
Added.
>
> !DirectClientTest.twoSubnets:
> Interesting use of ASSERT_NO_FATAL_FAILURE. I must admit that I
> haven't thought about its use before. I'm afraid there are quite a few
> tests scattered all around that may not behave properly upon failure.
Yup. I have learnt this trick recently and realized the same thing.
>
> '''src/lib/dhcp/tests/iface_mgr_test_config.h'''
> It would be useful to add a pointer in the !IfaceMgrTestConfig class
> description to the place where the fake interfaces are created.
> If you don't expect it to change, perhaps even copying the interfaces
> into would be useful?
Added some more documentation there.
>
> Why did you define !FlagLoopback, !FlagUp, !FlagRunning and other flags?
> These are simple boolean flags and they will never be any more complex
> than that. Unless, there's a good reason for them, please replace
> them will booleans.
I explained that in the comment. The general point is that when you call a
function that takes 5 booleans as argument, it is very likely that you'll
screw up. By making objects from these parameters you employ compiler to
guard you against it.
See the comments.
>
> '''src/lib/dhcp/tests/iface_mgr_test_config.cc'''
> createIfaces(): eth0 and eth1 have exactly the same link-local
> address, which implies they have the same MAC address. Is this
> intentional?
That was a mistake.
>
> '''src/lib/dhcp/tests/iface_mgr_unittest.cc'''
> !IfaceMgrTest.ifaceGetAddress: Tests in general should restrict
> themselves to using reserved pools specified in RFC5735 and RFC5737
>
(http://en.wikipedia.org/wiki/Reserved_IP_addresses#Reserved_IPv4_addresses),
> but I understand that in many cases that is not possible.
I know that. But I didn't do anything about it in this ticket. I left it
as is.
>
> '''src/lib/dhcpsrv/cfgmgr.cc'''
> CfgMgr::getSubnet4(iface_name): What sort of sorcery is this? ;)
> IPv6 loopback address in IPv4 methods?
It actually didn't matter. The address had to be initialized to something.
It is later overridden anyway. But I changed that to 0 now.
>
> '''src/lib/dhcpsrv/tests/cfgmgr_unittest.cc'''
> !CfgMgrTest.getSubnet4ForInterface: the checks in lines 721,722 could
> be EXPECT_FALSE. ASSERT_FALSE is not necessary.
Changed.
>
> -----------------
>
> The clean build fails for me:
> make -j8
>
> {{{
> make[6]: Wejście do katalogu
`/home/thomson/devel/bind10/src/lib/dhcp/tests'
> CXX libdhcptest_la-iface_mgr_test_config.lo
> CXX libdhcptest_la-pkt_filter_test_stub.lo
> CXX libdhcp___unittests-run_unittests.o
> CXX libdhcp___unittests-hwaddr_unittest.o
> CXX libdhcp___unittests-iface_mgr_unittest.o
> CXX libdhcp___unittests-libdhcp++_unittest.o
> CXX libdhcp___unittests-option4_addrlst_unittest.o
> CXX libdhcp___unittests-option4_client_fqdn_unittest.o
> CXX libdhcp___unittests-option6_addrlst_unittest.o
> CXX libdhcp___unittests-option6_client_fqdn_unittest.o
> CXX libdhcp___unittests-option6_ia_unittest.o
> CXX libdhcp___unittests-option6_iaaddr_unittest.o
> CXX libdhcp___unittests-option6_iaprefix_unittest.o
> CXX libdhcp___unittests-option_int_unittest.o
> CXX libdhcp___unittests-option_int_array_unittest.o
> CXX libdhcp___unittests-option_data_types_unittest.o
> CXX libdhcp___unittests-option_definition_unittest.o
> CXX libdhcp___unittests-option_custom_unittest.o
> CXX libdhcp___unittests-option_unittest.o
> CXX libdhcp___unittests-option_vendor_unittest.o
> CXX libdhcp___unittests-pkt4_unittest.o
> CXX libdhcp___unittests-pkt6_unittest.o
> CXX libdhcp___unittests-pkt_filter_unittest.o
> CXX libdhcp___unittests-pkt_filter_inet_unittest.o
> CXX libdhcp___unittests-pkt_filter_inet6_unittest.o
> CXX libdhcp___unittests-pkt_filter_test_utils.o
> CXX libdhcp___unittests-pkt_filter6_test_utils.o
> CXX libdhcp___unittests-pkt_filter_lpf_unittest.o
> CXX libdhcp___unittests-protocol_util_unittest.o
> make[6]: *** Brak reguł do zrobienia obiektu
`../../../../src/lib/dhcp/tests/libdhcptest.la', wymaganego przez
`libdhcp++_unittests'. Stop.
> make[6]: *** Oczekiwanie na niezakończone zadania....
> CXX libdhcp___unittests-duid_unittest.o
> make[6]: Opuszczenie katalogu
> `/home/thomson/devel/bind10/src/lib/dhcp/tests'
> }}}
>
> Repeated make -j8 calls managed to finally build it.
> Once they compiled, all tests passed.
Good catch. The test library was building as a separate job, while the
unit tests started to build and they didn't have the library available
yet. I made a workaround to include the cc files in the libdhcp++ tests,
instead of the library. The library is built anyway and is linked with
other unit tests.
>
> ------------------
>
> Please describe the subnet selection logic in the BIND10 Guide.
> Copying appropriate parts of your comment:10 would probably address
> most of that request.
Described.
--
Ticket URL: <http://bind10.isc.org/ticket/3242#comment:14>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list