BIND 10 #998: IP based ACL check

[BIND 10 Development]

#998: IP based ACL check
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  vorner                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:  major  |  Sprint-20110628
                  Component:         |            Resolution:
  Unclassified                       |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DNS
            Defect Severity:  N/A    |  Estimated Difficulty:  5.0
Feature Depending on Ticket:         |           Total Hours:  0
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by stephen):

 * owner:  stephen => jinmei


Comment:

 '''ip_check'''
 >> (about default constructor)
 > Apparently the intended usage is to use shared pointers of Check objects
 (see the ACL class merged in the master), so it's less likely to store
 bare Check objects in STL containers in practice. In general, I'd prefer
 minimalist approach: don't introduce a feature due to a guessed (but yet
 non existent) usage.
 Unfortunately, at the time of writing, very little detail was specified
 about how the ACL objects would be used, hence the guesses.  I've removed
 the default constructor.

 > You seem to have intended to apply my editorial suggestion on IPv[46] vs
 IPV[46], but the actual result seems to be different from what I actually
 suggested: my suggestion is to use "IPv4/6" (lower v) in comments. You
 applied it in the reverse direction. Is that intentional?
 Misread it - changed.

 > acl/Makefile.am
 > Why did you disable -Werror for libacl?
 Because I missed it when I copied and edited another Makefile.am after I
 introduced the .cc file.  Corrected.

 > prefix_size: I'd personally defer defining a mutable variable until we
 really start using it..
 Done.

 > so, are we (now?) allowing beginning and ending spaces (including '\n')?
 Why not?  Semantically, " 192.0.2.0 " means the same as "192.0.2.0".
 (Also, it follows Postel's dictum of being liberal in what you accept.)

 > mod_prefix could (should) be const.
 Done.

 > prefix could simply be initialized using the default constructor (which
 would be a bit more efficient than constructing string("") and coping it)
 True, but this is a minuscule overhead that I think is outweighed by the
 documentation effect provided by having two statements together where the
 initial values are clearly shown.

 > slashpos could (should) be const
 Done.

 > lexical_cast.hpp and scoped_ptr.hpp should be removed
 Done, with lexical_cast.hpp being moved to ip_check.cc

 > From IPv4 constructor: I don't think it a good idea to use the host byte
 order. And, on further thought, I'd even wonder whether we need these
 "from address" constructors in the first place for now...
 As to the constructors, I don't mind.  As I said, there was a certain
 amount of second-guessing when trying to create this class and my guess
 that non-string constructors were needed was obviously wrong.  I've got
 rid of these so a number of your other comments now no longer apply.

 > I know I may sound inconsistent regarding when to worry about
 performance and when to not, but I'm afraid using vectors are suboptimal
 ...
 Inconsistent? Never! :-)  However, now that we only have a string
 constructor, we can fix array sizes.  Both address and mask have been
 converted to a fixed-size uint8_t array.

 > constructors: address_() and mask_() could be omitted, but this is
 probably a matter of personal taste.
 Removed.

 The following no longer apply:
 > From IPv4 constructor: as far as I can see we don't need static_cast
 here.
 > From string constructor: this pattern doesn't seem to be the most common
 idiom in this case:

 Continuing...
 > From string constructor: technically this is not 100% correct:
 > :
 >       when status == -1. To be 100% pedantic we should do something like
 the attached diff.
 Message has been changed and a different exception thrown for a negative
 status return.

 > getPrefixlen(): even though it's legal, I'd avoid shadowing the same
 name of variable 'i'.
 Mistake - changed to j.

 > getPrefixlen(): this for loop can make meaningless iterations once the
 mask byte is 0.
 I assume you mean needless iterations.  Fixed.

 > getPrefixlen(): the condition seems to be wrong. It should be "i < 8":
 Only because we know that uint8_t is one byte wide.  However, the use of
 the multiplier was inconsistent so I have removed it.

 > compare(): (should have noticed this before) shouldn't this be non
 virtual?
 It should.  However, if we derive classes from IPPcheck, then we may want
 to access it and possibly override it, so I have made it public.

 '''ip_check_unittest'''
 > The behavior of >> for signed integer is implementation dependent (at
 least it is so for plain C). To be very safe, I'd use a uint16_t
 initialized with 0xff00 with >>. This way we could also integrate the case
 of i = 0 in the for loop.
 > Same for the 32-bit version, but I now guess we need this in the first
 place, both for the main code and test (and whether createMask needs to be
 templated) because we don't use the 32-bit version.
 I've removed the templated version - it's no longer needed.

 > for comparison tests, I'd also use prefix lengths that are not for a 4
 or 8-bit boundary, e.g. /19, /45, /127, etc.
 Added some that are not on nibble boundaries.

 > !MixedMode test: I don't see a point in doing EXPECT_NO_THROW before
 EXPECT_FALSE for the same expression.
 If there was an exception generated I wanted to clearly see where the
 problem was.  However, removed the EXPECT_NO_THROW.

 > !MixedMode test: for a more meaningful test, I'd use data that would
 confuse the compare method without the explicit family check, e.g. an IPv4
 address to be tested and IPv6 prefix in the check whose first 32 bits are
 identical to the IPv4 address.
 Done.

-- 
Ticket URL: <https://bind10.isc.org/ticket/998#comment:24>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development