BIND 10 #997: Create the ACL class

[BIND 10 Development]

#997: Create the ACL class
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  vorner                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:  major  |  Sprint-20110614
                  Component:         |            Resolution:
  Unclassified                       |             Sensitive:  0
                   Keywords:         |           Sub-Project:  Core
            Defect Severity:  N/A    |  Estimated Difficulty:  0.0
Feature Depending on Ticket:         |           Total Hours:  0
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 I did't see anything obviously wrong, but (as usual?) I have some
 points to discuss in details.

 First, I made some trivial editorial changes and pushed them.
 Having/removing a white space, e.g. from "{ }" may be purely matter of
 taste (and the coding guideline doesn't say anything about this
 point), but it seems we don't have a space for these cases in the vast
 majority of the code, so I made them consistent.

 == Action enum ==

 - It seems the ACL module does intentionally not define the semantics
   of these (and the caller application can use it for whatever purpose
   it wants).  I think this is reasonable, but it would be better if
   the description clarifies that point.

 - Since "Action" is a template parameter, and an application may
   introduce its own set of actions, I'd name "Action" something more
   basic, e.g. "BaseAction", "BasicAction", "BuiltInAction", etc.  It's
   just a comment, though.

 == Acl class ==

 - maybe "ACL" instead of Acl is a better name considering it's an
   acronym.  but this is just a comment; I don't insist on it.

 - why not using boost::noncopyable to prohibit copy?  we agreed on
   using instead of in-house privatization of the copy constructor and
   operator= for that purpose.

 - maybe a matter of taste, but I'd call the parameter to the
   constructor "default_action" instead of "policy" (and would name the
   corresponding member variable accordingly) as the former seems to
   better describe what it is.

 - "policy_" doesn't seem to be intended to be changed after the
   construction of the Acl class.  If that's the intent, I'd suggest
   making it const.

 - as you may have anticipated, I'd generally avoid using protected
   member variables.  In this case, if (as documented) the only reason
   for making them protected is to refer to them from tests, I'd
   introduce accessor methods, and, desirably, have them return a const
   reference (or a copy of the object).  That way we can be sure it's a
   bug within the main ACL implementation (not in the test code or
   possible in a non-compliant application) when we encounter a strange
   bug where these member variables have been magically changed.  If
   you insist on keeping them member variables, perhaps for brevity in
   the test code, I don't agree with the brevity argument, but I could
   at least live with that for "policy_" as long as it's const (see the
   previous point).  I'd still argue the access to "entries_" should be
   more restricted because it cannot be immutable, but I'd also note
   that at least for now it doesn't have to be protected (the test code
   doesn't use it).

 - CheckPtr: shouldn't it (better) be a shared pointer of a "const"
   object?  i.e.
 {{{
     typedef boost::shared_ptr<const Check<Context> > CheckPtr;
 }}}
   (and, perhaps, name it ConstCheckPtr following our convention)

 - execute(): is it reasonable to return an Action "object"?  This
   means it must be copyable, and, depending on the internal detail of
   the actual Action type, the copy could be very expensive.  On a
   related note, maybe it would be useful to describe the minimal
   requirement (such as copyability) that the Action template parameter
   must meet, and, if it's basically intended to be integral types,
   mention that, too.

 - execute(): this type of for loop is a bit more expensive because it
   involves a call to entries_.end() in each iteration.  I'd not
   necessarily request it be addressed right now, but since this could
   be a performance sensitive path, I believe we should be careful.

 == Test ==

 The use of reference of pointer seems to be abuse in the apparent
 sense of Check class:

 {{{
 class ConstCheck : public Check<Log*> {
 public:
 [...]
     typedef Log* LPtr;
     virtual bool matches(const LPtr& log) const {
         log->run[log_num_] = true;
         return (accepts_);
     }
 }}}

 That is, while matches() is basically expected to not alter the
 context, this implementation bypasses the design intent by hiding the
 real context in the pointer.  Maybe this is only for tests, and if so,
 I'd at least add comments about the intent and/or (pass a normal
 reference and) explicitly use const_cast to break the constness to
 highlight the intent.  Or, maybe in some cases contexts are actually
 expected to be modified within matches().  Although I'd avoid that if
 we could do so by design, but if there are really cases we want to do
 that, I'd introduce a non-const version of matches() and let the
 application use that with an explicit care.

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