BIND 10 trac997, updated. fe244439fca78ce558073b856dac69290baf67c5 [trac997] Address review comments
BIND 10 source code commits
bind10-changes at lists.isc.org
Tue Jun 14 10:01:59 UTC 2011
The branch, trac997 has been updated
via fe244439fca78ce558073b856dac69290baf67c5 (commit)
from a6c21ad6d670896283a941b5bfb233ff5987c50b (commit)
Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.
- Log -----------------------------------------------------------------
commit fe244439fca78ce558073b856dac69290baf67c5
Author: Michal 'vorner' Vaner <michal.vaner at nic.cz>
Date: Tue Jun 14 12:01:36 2011 +0200
[trac997] Address review comments
-----------------------------------------------------------------------
Summary of changes:
src/lib/acl/acl.h | 76 ++++++++++++++++++++---------------------
src/lib/acl/tests/acl_test.cc | 37 +++++++++++---------
2 files changed, 58 insertions(+), 55 deletions(-)
-----------------------------------------------------------------------
diff --git a/src/lib/acl/acl.h b/src/lib/acl/acl.h
index 382f00a..ec3dbbb 100644
--- a/src/lib/acl/acl.h
+++ b/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_);
+ }
};
}
diff --git a/src/lib/acl/tests/acl_test.cc b/src/lib/acl/tests/acl_test.cc
index 8dd8004..1c32d31 100644
--- a/src/lib/acl/tests/acl_test.cc
+++ b/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);
}
More information about the bind10-changes
mailing list