[trac997] Address review comments

src/lib/acl/acl.h

@@ -19,6 +19,7 @@
 #include <vector>
 
 #include <boost/shared_ptr.hpp>
+#include <boost/noncopyable.hpp>
 
 namespace isc {
 namespace acl {
@@ -28,9 +29,10 @@ namespace acl {
  *
  * This is the default for the ACL class. It is possible to specify any other
  * data type, as the ACL class does nothing about them, but these look
- * reasonable, so they are provided for convenience.
+ * reasonable, so they are provided for convenience. It is not specified what
+ * exactly these mean and it's up to whoever uses them.
  */
-enum Action {
+enum BasicAction {
     ACCEPT,
     REJECT,
     DROP
@@ -44,42 +46,31 @@ enum Action {
  * whenever the action matches. They are tested in the order and first
  * match counts.
  *
- * \note There are protected members. In fact, you should consider them
+ * This is non-copyable. It seems that there's no need to copy them (even
+ * when it would be technically possible), so we forbid it just to prevent
+ * copying it by accident. If there really is legitimate use, this restriction
+ * can be removed.
+ *
+ * The class is template. It is possible to specify on which context the checks
+ * match and which actions it returns. The actions must be copyable
+ * for this to work and it is expected to be something small, usually an enum
+ * (but other objects are also possible).
+ *
+ * \note There are protected functions. In fact, you should consider them
  *     private, they are protected so tests can get inside. This class
  *     is not expected to be subclassed in real applications.
  */
-template<typename Context, typename Action = isc::acl::Action> class Acl {
-private:
-    /**
-     * \brief Copy constructor.
-     *
-     * It is private on purpose, this class is not supposed to be copied.
-     * It is technically possible though, so if the need arises, it can be
-     * added (or, more correctly, this privade one removed so default one
-     * is created).
-     */
-    Acl(const Acl<Context, Action>& other);
-
-    /**
-     * \brief Assignment operator.
-     *
-     * It is private on purpose, this class is not supposed to be copied.
-     * It is technically possible though, so if the need arises, it can be
-     * added (or, more correctly, this privade one removed so default one
-     * is created).
-     */
-    Acl& operator=(const Acl<Context, Action>& other);
-
+template<typename Context, typename Action = BasicAction> class ACL :
+    public boost::noncopyable {
 public:
     /**
      * \brief Constructor.
      *
-     * \param policy It is the action that is returned when the checked things
-     *     "falls off" the end of the list (when no rule matched).
+     * \param default_action It is the action that is returned when the checked
+     *     things "falls off" the end of the list (when no rule matched).
      */
-    Acl(Action policy) : policy_(policy)
+    ACL(const Action& default_action) : default_action_(default_action)
     {}
-
     /**
      * \brief Pointer to the check.
      *
@@ -87,26 +78,25 @@ public:
      * However, we might need to copy the entries (when we concatenate ACLs
      * together in future).
      */
-    typedef boost::shared_ptr<Check<Context> > CheckPtr;
-
+    typedef boost::shared_ptr<const Check<Context> > ConstCheckPtr;
     /**
      * \brief The actual main function that decides.
      *
      * This is the function that takes the entries one by one, checks
      * the context against conditions and if it matches, returns the
-     * action that belongs to the first matched entry or policy action
+     * action that belongs to the first matched entry or default action
      * if nothing matches.
      * \param context The thing that should be checked. It is directly
      *     passed to the checks.
      */
-    Action execute(const Context& context) const {
+    const Action& execute(const Context& context) const {
         for (typename Entries::const_iterator i(entries_.begin());
              i != entries_.end(); ++i) {
             if (i->first->matches(context)) {
                 return (i->second);
             }
         }
-        return (policy_);
+        return (default_action_);
     }
 
     /**
@@ -119,18 +109,26 @@ public:
      * \param check The check to test if the thing matches.
      * \param action The action to return when the thing matches this check.
      */
-    void append(CheckPtr check, const Action& action) {
+    void append(ConstCheckPtr check, const Action& action) {
         entries_.push_back(Entry(check, action));
     }
 private:
     // Just type abbreviations.
-    typedef std::pair<CheckPtr, Action> Entry;
+    typedef std::pair<ConstCheckPtr, Action> Entry;
     typedef std::vector<Entry> Entries;
-protected:
-    /// \brief The policy.
-    Action policy_;
+    /// \brief The default action, when nothing mathes.
+    const Action default_action_;
     /// \brief The entries we have.
     Entries entries_;
+protected:
+    /**
+     * \brief Get the default action.
+     *
+     * This is for testing purposes only.
+     */
+    const Action& getDefaultAction() const {
+        return (default_action_);
+    }
 };
 
 }

src/lib/acl/tests/acl_test.cc

@@ -28,7 +28,7 @@ const size_t LOG_SIZE = 10;
 // This will remember which checks did run already.
 struct Log {
     // The actual log cells, if i-th check did run
-    bool run[LOG_SIZE];
+    mutable bool run[LOG_SIZE];
     Log() {
         // Nothing run yet
         for (size_t i(0); i < LOG_SIZE; ++i) {
@@ -57,7 +57,7 @@ struct Log {
 
 // This returns true or false every time, no matter what is passed to it.
 // But it logs that it did run.
-class ConstCheck : public Check<Log*> {
+class ConstCheck : public Check<Log> {
 public:
     ConstCheck(bool accepts, size_t log_num) :
         log_num_(log_num),
@@ -65,9 +65,14 @@ public:
     {
         assert(log_num < LOG_SIZE); // If this fails, the LOG_SIZE is too small
     }
-    typedef Log* LPtr;
-    virtual bool matches(const LPtr& log) const {
-        log->run[log_num_] = true;
+    /*
+     * This use of mutable log context is abuse for testing purposes.
+     * It is expected that the context will not be modified in the real
+     * applications of ACLs, but we want to know which checks were called
+     * and this is an easy way.
+     */
+    virtual bool matches(const Log& log) const {
+        log.run[log_num_] = true;
         return (accepts_);
     }
 private:
@@ -77,14 +82,14 @@ private:
 
 // Test version of the Acl class. It adds few methods to examine the protected
 // data, but does not change the implementation.
-class TestAcl : public Acl<Log*> {
+class TestACL : public ACL<Log> {
 public:
-    TestAcl() :
-        Acl(DROP)
+    TestACL() :
+        ACL(DROP)
     {}
     // Check the stored policy there
-    void checkPolicy(Action ac) {
-        EXPECT_EQ(policy_, ac);
+    void checkPolicy(BasicAction ac) {
+        EXPECT_EQ(getDefaultAction(), ac);
     }
 };
 
@@ -95,11 +100,11 @@ public:
     AclTest() :
         next_check_(0)
     {}
-    TestAcl acl_;
+    TestACL acl_;
     Log log_;
     size_t next_check_;
-    shared_ptr<Check<Log*> > getCheck(bool accepts) {
-        return (shared_ptr<Check<Log*> >(new ConstCheck(accepts,
+    shared_ptr<Check<Log> > getCheck(bool accepts) {
+        return (shared_ptr<Check<Log> >(new ConstCheck(accepts,
                                                         next_check_++)));
     }
 };
@@ -112,7 +117,7 @@ public:
  */
 TEST_F(AclTest, emptyPolicy) {
     acl_.checkPolicy(DROP);
-    EXPECT_EQ(DROP, acl_.execute(&log_));
+    EXPECT_EQ(DROP, acl_.execute(log_));
     // No test was run
     log_.checkFirst(0);
 }
@@ -123,7 +128,7 @@ TEST_F(AclTest, emptyPolicy) {
 TEST_F(AclTest, policy) {
     acl_.append(getCheck(false), ACCEPT);
     acl_.append(getCheck(false), REJECT);
-    EXPECT_EQ(DROP, acl_.execute(&log_));
+    EXPECT_EQ(DROP, acl_.execute(log_));
     // The first two checks were actually run (and didn't match)
     log_.checkFirst(2);
 }
@@ -136,7 +141,7 @@ TEST_F(AclTest, check) {
     acl_.append(getCheck(false), ACCEPT);
     acl_.append(getCheck(true), REJECT);
     acl_.append(getCheck(true), ACCEPT);
-    EXPECT_EQ(REJECT, acl_.execute(&log_));
+    EXPECT_EQ(REJECT, acl_.execute(log_));
     log_.checkFirst(2);
 }