[2786] Address review comments.

diff --git a/src/lib/dhcp/option_string.cc b/src/lib/dhcp/option_string.cc
index 9c3eae35b9f661050f1e744e11c4b4cfa673b87b..ac1e4b5ee032792195fb1a7be37aafcbdb531ec8 100644 (file)
--- a/src/lib/dhcp/option_string.cc
+++ b/src/lib/dhcp/option_string.cc
@@ -19,7 +19,10 @@ namespace dhcp {
 
 OptionString::OptionString(const Option::Universe u, const uint16_t type,
                            const std::string& value)
-    : Option(u, type), value_(value) {
+    : Option(u, type) {
+    // Try to assign the provided string value. This will throw exception
+    // if the provided value is empty.
+    setValue(value);
 }
 
 OptionString::OptionString(const Option::Universe u, const uint16_t type,
@@ -31,25 +34,36 @@ OptionString::OptionString(const Option::Universe u, const uint16_t type,
     unpack(begin, end);
 }
 
-uint16_t
-OptionString::len() {
-    return (getHeaderLen() + value_.size());
+std::string
+OptionString::getValue() const {
+    return std::string(data_.begin(), data_.end());
 }
 
 void
-OptionString::pack(isc::util::OutputBuffer& buf) {
+OptionString::setValue(const std::string& value) {
     // Sanity check that the string value is at least one byte long.
     // This is a requirement for all currently defined options which
     // carry a string value.
-    if (value_.empty()) {
-        isc_throw(isc::OutOfRange, "string value carried in the option"
-                  << " must not be empty");
+    if (value.empty()) {
+        isc_throw(isc::OutOfRange, "string value carried by the option '"
+                  << getType() << "' must not be empty");
     }
 
+    data_.assign(value.begin(), value.end());
+}
+
+
+uint16_t
+OptionString::len() {
+    return (getHeaderLen() + data_.size());
+}
+
+void
+OptionString::pack(isc::util::OutputBuffer& buf) {
     // Pack option header.
     packHeader(buf);
     // Pack data.
-    buf.writeData(value_.c_str(), value_.size());
+    buf.writeData(&data_[0], data_.size());
 
     // That's it. We don't pack any sub-options here, because this option
     // must not contain sub-options.
@@ -62,7 +76,7 @@ OptionString::unpack(OptionBufferConstIter begin,
         isc_throw(isc::OutOfRange, "failed to parse an option holding string value"
                   << " - empty value is not accepted");
     }
-    value_.assign(begin, end);
+    data_.assign(begin, end);
 }
 
 } // end of isc::dhcp namespace

diff --git a/src/lib/dhcp/option_string.h b/src/lib/dhcp/option_string.h
index ca72769ae027cbd2dcf17d0bca41301c95cb627f..3d0aa9afa9ac06c63cb0dc7776004fc86d9dbf7c 100644 (file)
--- a/src/lib/dhcp/option_string.h
+++ b/src/lib/dhcp/option_string.h
@@ -72,14 +72,14 @@ public:
     /// @brief Returns the string value held by the option.
     ///
     /// @return string value held by the option.
-    std::string getValue() const {
-        return (value_);
-    }
+    std::string getValue() const;
 
     /// @brief Sets the string value to be held by the option.
-    void setValue(const std::string& value) {
-        value_ = value;
-    }
+    ///
+    /// @param value string value to be set.
+    ///
+    /// @throw isc::OutOfRange if a string value to be set is empty.
+    void setValue(const std::string& value);
 
     /// @brief Creates on-wire format of the option.
     ///
@@ -103,14 +103,6 @@ public:
     /// @throw isc::OutOfRange if provided buffer is truncated.
     virtual void unpack(OptionBufferConstIter begin, OptionBufferConstIter end);
 
-private:
-    /// String value being held by the option.
-    std::string value_;
-
-    // Change scope of the getData function to private as we want
-    // getValue is called instead.
-    using Option::getData;
-
 };
 
 /// Pointer to the OptionString object.

diff --git a/src/lib/dhcp/tests/libdhcp++_unittest.cc b/src/lib/dhcp/tests/libdhcp++_unittest.cc
index 511cd7e36ebae9fd810190de88ce6da7ca84d033..d523158dd637f1c94cab83f1f98965cab07daa94 100644 (file)
--- a/src/lib/dhcp/tests/libdhcp++_unittest.cc
+++ b/src/lib/dhcp/tests/libdhcp++_unittest.cc
@@ -373,7 +373,7 @@ TEST_F(LibDhcpTest, unpackOptions6) {
 /// is no restriction on the data length being carried by them.
 /// For simplicity, we assign data of the length 3 for each
 /// of them.
-static uint8_t v4Opts[] = {
+static uint8_t v4_opts[] = {
     12,  3, 0,   1,  2, // Hostname
     60,  3, 10, 11, 12, // Class Id
     14,  3, 20, 21, 22, // Merit Dump File
@@ -404,18 +404,18 @@ TEST_F(LibDhcpTest, packOptions4) {
     opts.insert(make_pair(opt1->getType(), opt4));
     opts.insert(make_pair(opt1->getType(), opt5));
 
-    vector<uint8_t> expVect(v4Opts, v4Opts + sizeof(v4Opts));
+    vector<uint8_t> expVect(v4_opts, v4_opts + sizeof(v4_opts));
 
     OutputBuffer buf(100);
     EXPECT_NO_THROW(LibDHCP::packOptions(buf, opts));
-    ASSERT_EQ(buf.getLength(), sizeof(v4Opts));
-    EXPECT_EQ(0, memcmp(v4Opts, buf.getData(), sizeof(v4Opts)));
+    ASSERT_EQ(buf.getLength(), sizeof(v4_opts));
+    EXPECT_EQ(0, memcmp(v4_opts, buf.getData(), sizeof(v4_opts)));
 
 }
 
 TEST_F(LibDhcpTest, unpackOptions4) {
 
-    vector<uint8_t> v4packed(v4Opts, v4Opts + sizeof(v4Opts));
+    vector<uint8_t> v4packed(v4_opts, v4_opts + sizeof(v4_opts));
     isc::dhcp::Option::OptionCollection options; // list of options
 
     ASSERT_NO_THROW(
@@ -430,14 +430,14 @@ TEST_F(LibDhcpTest, unpackOptions4) {
     EXPECT_EQ(12, option12->getType());  // this should be option 12
     ASSERT_EQ(3, option12->getValue().length()); // it should be of length 3
     EXPECT_EQ(5, option12->len()); // total option length 5
-    EXPECT_EQ(0, memcmp(&option12->getValue()[0], v4Opts+2, 3)); // data len=3
+    EXPECT_EQ(0, memcmp(&option12->getValue()[0], v4_opts + 2, 3)); // data len=3
 
     x = options.find(60);
     ASSERT_FALSE(x == options.end()); // option 2 should exist
     EXPECT_EQ(60, x->second->getType());  // this should be option 60
     ASSERT_EQ(3, x->second->getData().size()); // it should be of length 3
     EXPECT_EQ(5, x->second->len()); // total option length 5
-    EXPECT_EQ(0, memcmp(&x->second->getData()[0], v4Opts+7, 3)); // data len=3
+    EXPECT_EQ(0, memcmp(&x->second->getData()[0], v4_opts + 7, 3)); // data len=3
 
     x = options.find(14);
     ASSERT_FALSE(x == options.end()); // option 3 should exist
@@ -446,21 +446,21 @@ TEST_F(LibDhcpTest, unpackOptions4) {
     EXPECT_EQ(14, option14->getType());  // this should be option 14
     ASSERT_EQ(3, option14->getValue().length()); // it should be of length 3
     EXPECT_EQ(5, option14->len()); // total option length 5
-    EXPECT_EQ(0, memcmp(&option14->getValue()[0], v4Opts+12, 3)); // data len=3
+    EXPECT_EQ(0, memcmp(&option14->getValue()[0], v4_opts + 12, 3)); // data len=3
 
     x = options.find(254);
     ASSERT_FALSE(x == options.end()); // option 3 should exist
     EXPECT_EQ(254, x->second->getType());  // this should be option 254
     ASSERT_EQ(3, x->second->getData().size()); // it should be of length 3
     EXPECT_EQ(5, x->second->len()); // total option length 5
-    EXPECT_EQ(0, memcmp(&x->second->getData()[0], v4Opts+17, 3)); // data len=3
+    EXPECT_EQ(0, memcmp(&x->second->getData()[0], v4_opts + 17, 3)); // data len=3
 
     x = options.find(128);
     ASSERT_FALSE(x == options.end()); // option 3 should exist
     EXPECT_EQ(128, x->second->getType());  // this should be option 254
     ASSERT_EQ(3, x->second->getData().size()); // it should be of length 3
     EXPECT_EQ(5, x->second->len()); // total option length 5
-    EXPECT_EQ(0, memcmp(&x->second->getData()[0], v4Opts+22, 3)); // data len=3
+    EXPECT_EQ(0, memcmp(&x->second->getData()[0], v4_opts + 22, 3)); // data len=3
 
     x = options.find(0);
     EXPECT_TRUE(x == options.end()); // option 0 not found

diff --git a/src/lib/dhcp/tests/option_string_unittest.cc b/src/lib/dhcp/tests/option_string_unittest.cc
index 24c599267187a0d60a64c4e16faff781853f31dc..0be3c3d4a3f6a42c78c332d46a758ca709168d3f 100644 (file)
--- a/src/lib/dhcp/tests/option_string_unittest.cc
+++ b/src/lib/dhcp/tests/option_string_unittest.cc
@@ -59,6 +59,10 @@ TEST_F(OptionStringTest, constructorFromString) {
     EXPECT_EQ(234, optv6.getType());
     EXPECT_EQ("other option", optv6.getValue());
     EXPECT_EQ(Option::OPTION6_HDR_LEN + optv6_value.size(), optv6.len());
+
+    // Check that an attempt to use empty string in the constructor
+    // will result in an exception.
+    EXPECT_THROW(OptionString(Option::V6, 123, ""), isc::OutOfRange);
 }
 
 // This test verifies that the constructor which creates an option instance
@@ -119,6 +123,8 @@ TEST_F(OptionStringTest, setValue) {
     // been successful.
     EXPECT_NO_THROW(optv4.setValue("new option value"));
     EXPECT_EQ("new option value", optv4.getValue());
+    // Try to set to an empty string. It should throw exception.
+    EXPECT_THROW(optv4.setValue(""), isc::OutOfRange);
 }
 
 // This test verifies that the pack function encodes the option in

diff --git a/src/lib/dhcp/tests/pkt4_unittest.cc b/src/lib/dhcp/tests/pkt4_unittest.cc
index 67145e90a3766e7f0acc88cbe749434938864261..e9c7dd227fd01e7913b1bd3dd640f20345da525a 100644 (file)
--- a/src/lib/dhcp/tests/pkt4_unittest.cc
+++ b/src/lib/dhcp/tests/pkt4_unittest.cc
@@ -560,12 +560,12 @@ TEST(Pkt4Test, unpackOptions) {
     EXPECT_EQ(0, memcmp(&option12->getValue()[0], v4Opts + 2, 3)); // data len=3
 
     x = pkt->getOption(14);
-    ASSERT_TRUE(x); // option 13 should exist
+    ASSERT_TRUE(x); // option 14 should exist
     // Option 14 is represented by the OptionString class so let's do
     // the appropriate conversion.
     OptionStringPtr option14 = boost::static_pointer_cast<OptionString>(x);
     ASSERT_TRUE(option14);
-    EXPECT_EQ(14, option14->getType());  // this should be option 13
+    EXPECT_EQ(14, option14->getType());  // this should be option 14
     ASSERT_EQ(3, option14->getValue().length()); // it should be of length 3
     EXPECT_EQ(5, option14->len()); // total option length 5
     EXPECT_EQ(0, memcmp(&option14->getValue()[0], v4Opts + 7, 3)); // data len=3