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      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:24 stephen]:

 It now mostly looks okay (and I'm happy that it's now much simplified
 and more understandable).

 I made some fixes and directly pushed them.  Please check.  I bevelive
 they are trivial and should be non controversial.

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

 It's mostly okay, but there's still some subtle case.  Consider the
 case where first inet_pton() fails with -1 but the input was actually
 valid.  Then the second inet_pton() will also fail, this time with 0,
 so we'll see a "not valid" exception what() message, which is
 misleading.  That's why I checked status == 0 in my suggested patch.
 On the other hand, your version can rescue the case where the first
 inet_pton() fails with -1, the input is a valid IPv4 address, and the
 second inet_pton() succeeds.

 We could make this perfect addressing both two cases, but all of these
 are quite unlikely scenario anyway, so maybe it's not worth the
 additional code complexity.  I'd leave it to you.

 > I assume you mean needless iterations.  Fixed.

 There's still a possible needless iteration.  I'd suggest revise it
 to:
 {{{
         for (size_t i = 0; i < IPV6_SIZE; ++i) {
             if (mask_[i] == 0xff) {
                 // All bits set in this byte
                 count += 8;
                 continue;

             } else if (mask_[i] != 0) {
                 // Only some bits set in this byte.  Count them.
                 uint8_t byte = mask_[i];
                 for (int j = 0; j < 8; ++j) {
                     count += byte & 0x01;   // Add one if the bit is set
                     byte >>= 1;             // Go for next bit
                 }
             }
             // Unless the all bits set, there are no more remaining bits
             // that is set, so exit.
             break;
         }
 }}}

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

 Hmm, this seems to me to try catching an unseen need, but I don't
 oppose it.  But if you want to leave that possibility,
 - please document the intent
 - I'd suggest making it protected, not public.

 When these minor things are addressed, I think it can be merged (I
 don't think more explicit review is needed - unless you want to
 discuss something about them).

 Finally, please create tickets about open points.

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