[3186] Addressed review comments.

Mostly minor cosmetics. Added logic to select callouts to
simply return if no subnets are configured.

src/Makefile.am

@@ -1,5 +1,5 @@
 SUBDIRS = lib bin
-# hooks lib could be a configurable switch
+# @todo hooks lib could be a configurable switch
 SUBDIRS += hooks
 
 EXTRA_DIST = \

src/hooks/dhcp/user_chk/load_unload.cc

@@ -30,12 +30,17 @@ UserRegistryPtr user_registry;
 /// @brief Output filestream for recording user check outcomes.
 std::fstream user_chk_output;
 
-/// @brief For now, hard-code registry input file name.
-const char* registry_fname = "/tmp/user_registry.txt";
+/// @brief User registry input file name.
+/// @todo Hard-coded for now, this should be configurable.
+const char* registry_fname = "/tmp/user_chk_registry.txt";
 
-/// @brief For now, hard-code user check outcome file name.
-const char* user_chk_output_fname = "/tmp/user_check_output.txt";
+/// @brief User check outcome file name.
+/// @todo Hard-coded for now, this should be configurable.
+const char* user_chk_output_fname = "/tmp/user_chk_outcome.txt";
 
+// Functions accessed by the hooks framework use C linkage to avoid the name
+// mangling that accompanies use of the C++ compiler as well as to avoid
+// issues related to namespaces.
 extern "C" {
 
 /// @brief Called by the Hooks library manager when the library is loaded.

src/hooks/dhcp/user_chk/subnet_select_co.cc

@@ -33,6 +33,9 @@ extern std::fstream user_chk_output;
 extern const char* registry_fname;
 extern const char* user_chk_output_fname;
 
+// Functions accessed by the hooks framework use C linkage to avoid the name
+// mangling that accompanies use of the C++ compiler as well as to avoid
+// issues related to namespaces.
 extern "C" {
 
 /// @brief Adds an entry to the end of the user check outcome file.
@@ -115,6 +118,13 @@ int subnet4_select(CalloutHandle& handle) {
     }
 
     try {
+        // Get subnet collection. If it's empty just bail nothing to do.
+        const isc::dhcp::Subnet4Collection *subnets = NULL;
+        handle.getArgument("subnet4collection", subnets);
+        if (subnets->size() == 0) {
+            return 0;
+        }
+
         // Refresh the registry.
         user_registry->refresh();
 
@@ -136,8 +146,6 @@ int subnet4_select(CalloutHandle& handle) {
             // User is not in the registry, so assign them to the last subnet
             // in the collection.  By convention we are assuming this is the
             // restricted subnet.
-            const isc::dhcp::Subnet4Collection *subnets = NULL;
-            handle.getArgument("subnet4collection", subnets);
             Subnet4Ptr subnet = subnets->back();
             handle.setArgument("subnet4", subnet);
             // Add the outcome entry to the output file.
@@ -175,6 +183,13 @@ int subnet6_select(CalloutHandle& handle) {
     }
 
     try {
+        // Get subnet collection. If it's empty just bail nothing to do.
+        const isc::dhcp::Subnet6Collection *subnets = NULL;
+        handle.getArgument("subnet6collection", subnets);
+        if (subnets->size() == 0) {
+            return 0;
+        }
+
         // Refresh the registry.
         user_registry->refresh();
 
@@ -204,8 +219,6 @@ int subnet6_select(CalloutHandle& handle) {
             // User is not in the registry, so assign them to the last subnet
             // in the collection.  By convention we are assuming this is the
             // restricted subnet.
-            const isc::dhcp::Subnet6Collection *subnets = NULL;
-            handle.getArgument("subnet6collection", subnets);
             Subnet6Ptr subnet = subnets->back();
             handle.setArgument("subnet6", subnet);
             // Add the outcome entry to the output file.

src/hooks/dhcp/user_chk/tests/Makefile.am

@@ -64,7 +64,6 @@ libdhcp_user_chk_unittests_LDADD += $(top_builddir)/src/lib/hooks/libb10-hooks.l
 libdhcp_user_chk_unittests_LDADD += $(top_builddir)/src/lib/dhcp/libb10-dhcp++.la
 libdhcp_user_chk_unittests_LDADD += $(top_builddir)/src/lib/exceptions/libb10-exceptions.la
 libdhcp_user_chk_unittests_LDADD += $(top_builddir)/src/lib/cc/libb10-cc.la
-#libdhcp_user_chk_unittests_LDADD += $(top_builddir)/src/hooks/dhcp/user_chk/libdhcp_user_chk.la
 libdhcp_user_chk_unittests_LDADD += ${BOTAN_LIBS} ${BOTAN_RPATH}
 libdhcp_user_chk_unittests_LDADD += $(GTEST_LDADD)
 endif

src/hooks/dhcp/user_chk/tests/test_data_files_config.h.in

@@ -1,4 +1,4 @@
-// Copyright (C) 2009  Internet Systems Consortium, Inc. ("ISC")   
+// Copyright (C) 2013 Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -11,6 +11,6 @@
 // LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE
 // OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
 // PERFORMANCE OF THIS SOFTWARE.
-
+//
 /// @brief Path to local dir so tests can locate test data files 
 #define USER_CHK_TEST_DIR "@abs_top_srcdir@/src/hooks/dhcp/user_chk/tests"

src/hooks/dhcp/user_chk/user.cc

@@ -15,6 +15,7 @@
 #include <dhcp/hwaddr.h>
 #include <dhcp/duid.h>
 #include <exceptions/exceptions.h>
+#include <util/encode/hex.h>
 
 #include <user.h>
 
@@ -42,7 +43,7 @@ UserId::UserId(UserIdType id_type, const std::string & id_str) :
     // Convert the id string to vector.
     // Input is expected to be 2-digits per bytes, no delimiters.
     std::vector<uint8_t> addr_bytes;
-    decodeHex(id_str, addr_bytes);
+    isc::util::encode::decodeHex(id_str, addr_bytes);
 
     // Attempt to instantiate the appropriate id class to leverage validation.
     switch (id_type) {
@@ -111,28 +112,6 @@ UserId::operator <(const UserId & other) const {
             ((this->id_type_ == other.id_type_) && (this->id_ < other.id_)));
 }
 
-void
-UserId::decodeHex(const std::string& input, std::vector<uint8_t>& bytes)
-const {
-    int len = input.size();
-    if ((len % 2) != 0) {
-        isc_throw (isc::BadValue,
-                   "input string must be event number of digits: "
-                             << input);
-    }
-
-    for (int i = 0; i < len / 2; i++) {
-        unsigned int tmp;
-        if (sscanf (&input[i*2], "%02x", &tmp) != 1) {
-            isc_throw (isc::BadValue,
-                       "input string contains invalid characters: "
-                                 << input);
-        }
-
-        bytes.push_back(static_cast<uint8_t>(tmp));
-    }
-}
-
 std::string
 UserId::lookupTypeStr(UserIdType type) {
     const char* tmp = NULL;

src/hooks/dhcp/user_chk/user.h

@@ -12,8 +12,8 @@
 // OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
 // PERFORMANCE OF THIS SOFTWARE.
 
-#ifndef _USER_H
-#define _USER_H
+#ifndef USER_H
+#define USER_H
 
 #include <boost/shared_ptr.hpp>
 
@@ -23,7 +23,7 @@
 
 /// @file user.h This file defines classes: UserId and User.
 /// @brief These classes are used to describe and recognize DHCP lease
-/// requestors (i.e. clients).
+/// clients.
 
 /// @brief Encapsulates a unique identifier for a DHCP client.
 /// This class provides a generic wrapper around the information used to
@@ -120,15 +120,6 @@ public:
     static UserIdType lookupType(const std::string& type_str);
 
 private:
-    /// @brief Converts a string of hex digits to vector of bytes
-    ///
-    /// @param input string of hex digits to convert
-    /// @param bytes vector in which to place the result
-    ///
-    /// @throw isc::BadValue if input string contains invalid hex digits
-    /// or has an odd number of digits.
-    void decodeHex(const std::string& input, std::vector<uint8_t>& bytes) const;
-
     /// @brief The type of id value
     UserIdType id_type_;
 

src/hooks/dhcp/user_chk/user_chk_log.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2011  Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2013 Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -12,8 +12,7 @@
 // OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
 // PERFORMANCE OF THIS SOFTWARE.
 
-/// Defines the logger used by the NSAS
-
+/// Defines the logger used by the user check hooks library.
 #include <user_chk_log.h>
 
 isc::log::Logger user_chk_logger("user_chk");

src/hooks/dhcp/user_chk/user_chk_messages.mes

@@ -14,7 +14,8 @@
 
 % USER_CHK_HOOK_LOAD_ERROR DHCP UserCheckHook could not be loaded: %1
 This is an error message issued when the DHCP UserCheckHook could not be loaded.
-The exact cause should be explained in the log message.  User subnet selection will revert to default processing.
+The exact cause should be explained in the log message.  User subnet selection
+will revert to default processing.
 
 % USER_CHK_HOOK_UNLOAD_ERROR DHCP UserCheckHook an error occurred unloading the library: %1
 This is an error message issued when an error occurs while unloading the

src/hooks/dhcp/user_chk/user_file.cc

@@ -20,7 +20,7 @@
 #include <errno.h>
 #include <iostream>
 
-UserFile::UserFile(const std::string& fname) : fname_(fname), ifs_() {
+UserFile::UserFile(const std::string& fname) : fname_(fname), file_() {
     if (fname_.empty()) {
         isc_throw(UserFileError, "file name cannot be blank");
     }
@@ -36,9 +36,9 @@ UserFile::open() {
         isc_throw (UserFileError, "file is already open");
     }
 
-    ifs_.open(fname_.c_str(), std::ifstream::in);
+    file_.open(fname_.c_str(), std::ifstream::in);
     int sav_error = errno;
-    if (!ifs_.is_open()) {
+    if (!file_.is_open()) {
         isc_throw(UserFileError, "cannot open file:" << fname_
                                  << " reason: " << strerror(sav_error));
     }
@@ -50,14 +50,14 @@ UserFile::readNextUser() {
         isc_throw (UserFileError, "cannot read, file is not open");
     }
 
-    if (ifs_.good()) {
-        char buf[4096];
+    if (file_.good()) {
+        char buf[USER_ENTRY_MAX_LEN];
 
         // Get the next line.
-        ifs_.getline(buf, sizeof(buf));
+        file_.getline(buf, sizeof(buf));
 
         // We got something, try to make a user out of it.
-        if (ifs_.gcount() > 0) {
+        if (file_.gcount() > 0) {
             return(makeUser(buf));
         }
     }
@@ -140,14 +140,14 @@ UserFile::makeUser(const std::string& user_string) {
 
 bool
 UserFile::isOpen() const {
-    return (ifs_.is_open());
+    return (file_.is_open());
 }
 
 void
 UserFile::close() {
     try {
-        if (ifs_.is_open()) {
-            ifs_.close();
+        if (file_.is_open()) {
+            file_.close();
         }
     } catch (const std::exception& ex) {
         // Highly unlikely to occur but let's at least spit out an error.

src/hooks/dhcp/user_chk/user_file.h

@@ -64,6 +64,11 @@ public:
 /// @endcode
 class UserFile : public UserDataSource {
 public:
+    /// @brief Maximum length of a single user entry.
+    /// This value is somewhat arbitrary. 4K seems reasonably large.  If it
+    /// goes beyond this, then a flat file is not likely the way to go.
+    static const size_t USER_ENTRY_MAX_LEN = 4096;
+
     /// @brief Constructor
     ///
     /// Create a UserFile for the given file name without opening the file.
@@ -120,7 +125,7 @@ private:
     string fname_;
 
     /// @brief Input file stream.
-    std::ifstream ifs_;
+    std::ifstream file_;
 
 };