BIND 10 #3242: Kea4: check if only remote traffic can be supported

[BIND 10 Development]

#3242: Kea4: check if only remote traffic can be supported
-------------------------------------+-------------------------------------
            Reporter:  tomek         |                        Owner:
                Type:  defect        |  marcin
            Priority:  high          |                       Status:
           Component:  dhcp4         |  reviewing
            Keywords:                |                    Milestone:  DHCP-
           Sensitive:  0             |  Kea0.9-alpha
         Sub-Project:  DHCP          |                   Resolution:
Estimated Difficulty:  24            |                 CVSS Scoring:
         Total Hours:  43            |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  8
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by tomek):

 * owner:  tomek => marcin


Comment:

 Replying to [comment:14 marcin]:

 Removed sections that I agree with, consider them addressed and have no
 further comments.

 > > 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.
 The change looks good, but please add a debug log that informs why the
 message was dropped. The last if clause in accept() looks like a good
 place to do that. This is quite important, actually. Otherwise people
 will be confused why server does not respond to something and won't have
 any good way to debugging it.

 > > 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.
 Ok. Personally, I wouldn't do that as it makes the code bigger, but
 that's more of a personal preference thing. The code and the comments
 as you wrote them are fine.

 > > '''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.
 Except that you used 10.0.0.1 in IfaceMgrTestConfig. But that's ok. Those
 RFCs
 are only a guidance. We need to disregard them anyway, e.g. when testing
 link-local addresses. So leave the code as it is.

 > > -----------------
 > >
 > > The clean build fails for me:
 > > Repeated make -j8 calls managed to finally build it.
 > > Once they compiled, all tests passed.
 >
 > Good catch.
 It was rather easy to spot. :)

 > 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.
 It builds (clean build) fine for me now.

 > > ------------------
 > > 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.
 Looks fine.

 Ok, this is almost ready to go. Just add the debug log message and go
 ahead
 with the merge. I don't need to see it again.

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