[2313] Changes as a result of the review.

diff --git a/src/lib/dhcpsrv/option_space.cc b/src/lib/dhcpsrv/option_space.cc
index 88f05bed55e40d1482bf5069a90a30acb11fbb71..c244268ca0d5daa0067f23c378f6478d5175e732 100644 (file)
--- a/src/lib/dhcpsrv/option_space.cc
+++ b/src/lib/dhcpsrv/option_space.cc
@@ -30,14 +30,22 @@ OptionSpace::OptionSpace(const std::string& name, const bool vendor_space)
 
 bool
 OptionSpace::validateName(const std::string& name) {
-    // Allowed digits are: lower or upper case letters, digits,
+
+    using namespace boost::algorithm;
+
+    // Allowed characters are: lower or upper case letters, digits,
     // underscores and dashes. Empty option spaces are not allowed.
-    if (boost::algorithm::all(name, boost::is_from_range('a', 'z') ||
-                              boost::is_from_range('A', 'Z') ||
-                              boost::is_digit() ||
-                              boost::is_any_of("-_")) &&
-        !name.empty()) {
+    if (all(name, boost::is_from_range('a', 'z') ||
+            boost::is_from_range('A', 'Z') ||
+            boost::is_digit() ||
+            boost::is_any_of("-_")) &&
+        !name.empty() &&
+        // Hyphens are not allowed at the beginning and at
+        // the end of the option space name.
+        !all(find_head(name, 1), boost::is_any_of("-_")) &&
+        !all(find_tail(name, 1), boost::is_any_of("-_"))) {
         return (true);
+
     }
     return (false);
 }

diff --git a/src/lib/dhcpsrv/option_space.h b/src/lib/dhcpsrv/option_space.h
index 0a07f27013e2a93a7c4d401e673c9cd1efe52944..9eebd76df9dd0e0e55c74c7750099ff74516c103 100644 (file)
--- a/src/lib/dhcpsrv/option_space.h
+++ b/src/lib/dhcpsrv/option_space.h
@@ -49,10 +49,26 @@ typedef std::map<std::string, OptionSpacePtr> OptionSpaceCollection;
 /// assigned to option instances represented by isc::dhcp::Option and
 /// other classes derived from it. Each particular option may belong to
 /// multiple option spaces.
-/// This class may be used to represent any DHCPv4 option space. In theory
-/// this class can be also used to represent non-vendor specific DHCPv6
-/// option space but this is disencouraged. For DHCPv6 option spaces the
-/// OptionSpace6 class should be used instead.
+/// This class may be used to represent any DHCPv4 option space. If the
+/// option space is to group DHCPv4 Vendor Encapsulated Options then
+/// "vendor space" flag must be set using \ref OptionSpace::setVendorSpace
+/// or the argument passed to the constructor. In theory, this class can
+/// be also used to represent non-vendor specific DHCPv6 option space
+/// but this is discouraged. For DHCPv6 option spaces the OptionSpace6
+/// class should be used instead.
+///
+/// @note this class is intended to be used to represent DHCPv4 option
+/// spaces only. However, it hasn't been called OptionSpace4 (that would
+/// suggest that it is specific to DHCPv4) because it can be also
+/// used to represent some DHCPv6 option spaces and is a base class
+/// for \ref OptionSpace6. Thus, if one declared the container as follows:
+/// @code
+/// std::vector<OptionSpace4> container;
+/// @endcode
+/// it would suggest that the container holds DHCPv4 option spaces while
+/// it could hold both DHCPv4 and DHCPv6 option spaces as the OptionSpace6
+/// object could be upcast to OptionSpace4. This confusion does not appear
+/// when OptionSpace is used as a name for the base class.
 class OptionSpace {
 public:
 
@@ -68,16 +84,16 @@ public:
     /// correct.
     OptionSpace(const std::string& name, const bool vendor_space = false);
 
-    /// @brief Mark option space as non-vendor space.
-    void clearVendorSpace() {
-        vendor_space_ = false;
-    }
-
     /// @brief Return option space name.
     ///
     /// @return option space name.
     const std::string& getName() const { return (name_); }
 
+    /// @brief Mark option space as non-vendor space.
+    void clearVendorSpace() {
+        vendor_space_ = false;
+    }
+
     /// @brief Check if option space is vendor specific.
     ///
     /// @return boolean value that indicates if the object describes

diff --git a/src/lib/dhcpsrv/tests/option_space_unittest.cc b/src/lib/dhcpsrv/tests/option_space_unittest.cc
index 6c0653a788cffb47414ed4d5cc419edc666dd9f0..9ee6fcaaf5ac7aab7a897c6c7e1123198c30ad33 100644 (file)
--- a/src/lib/dhcpsrv/tests/option_space_unittest.cc
+++ b/src/lib/dhcpsrv/tests/option_space_unittest.cc
@@ -77,6 +77,13 @@ TEST(OptionSpaceTest, validateName) {
     EXPECT_FALSE(OptionSpace::validateName(" isc"));
     EXPECT_FALSE(OptionSpace::validateName("isc with-space"));
 
+    // Hyphens are not allowed at the beginning and at the end
+    // of the option space name.
+    EXPECT_FALSE(OptionSpace::validateName("-isc"));
+    EXPECT_FALSE(OptionSpace::validateName("isc-"));
+    EXPECT_FALSE(OptionSpace::validateName("_isc"));
+    EXPECT_FALSE(OptionSpace::validateName("isc_"));
+
     // Test other special characters
     const char specials[] = { '!', '@', '#', '$', '%', '^', '&', '*', '(', ')',
                               '+', '=', '[', ']', '{', '}', ';', ':', '"', '\'',