[#900,!561] Addressed review comments

src/lib/dhcp/libdhcp++.cc
    Cleaned up necessary exception decls

src/lib/dhcp/option.h
    Added commentary for SkipThisOptionError

src/lib/dhcp/option_definition.cc
    Cleaned up unnecessary exception decls

src/lib/dhcp/option_string.cc
    Replaced NULL with nul

src/lib/testutils/gtest_utils.h
    Added emissions of exception type name

diff --git a/src/lib/dhcp/libdhcp++.cc b/src/lib/dhcp/libdhcp++.cc
index 44f06ed436fb6de1265021e195e5b6a90810fdbe..6c37b77efdb605d0672d12872c8cdbe1be627da1 100644 (file)
--- a/src/lib/dhcp/libdhcp++.cc
+++ b/src/lib/dhcp/libdhcp++.cc
@@ -456,7 +456,7 @@ size_t LibDHCP::unpackOptions6(const OptionBuffer& buf,
                 opt = def->optionFactory(Option::V6, opt_type,
                                          buf.begin() + offset,
                                          buf.begin() + offset + opt_len);
-            } catch (const SkipThisOptionError& ex)  {
+            } catch (const SkipThisOptionError&)  {
                 opt.reset();
             }
         }
@@ -599,7 +599,7 @@ size_t LibDHCP::unpackOptions4(const OptionBuffer& buf,
                 opt = def->optionFactory(Option::V4, opt_type,
                                          buf.begin() + offset,
                                          buf.begin() + offset + opt_len);
-            } catch (const SkipThisOptionError& ex)  {
+            } catch (const SkipThisOptionError&)  {
                 opt.reset();
             }
         }

diff --git a/src/lib/dhcp/option.h b/src/lib/dhcp/option.h
index 1d484f46e15987a13fc64ed819447b9d31f598b9..1c77397456e5ad55c01f3865440cbbd387d2e65f 100644 (file)
--- a/src/lib/dhcp/option.h
+++ b/src/lib/dhcp/option.h
@@ -55,6 +55,15 @@ public:
         isc::Exception(file, line, what) { };
 };
 
+/// @brief Exception thrown during option unpacking
+/// This exception is thrown when an error has occurred unpacking
+/// an option from a packet and rather than drop the whole packet, we
+/// wish to simply skip over the option (i.e. omit it from the unpacked
+/// results), and resume unpacking with the next option in the buffer.
+/// The intent is to allow us to be liberal with what we receive, and
+/// skip nonsensical options rather than drop the whole packet. This
+/// exception is thrown, for instance, when string options are found to
+/// be empty or to contain only nuls.
 class SkipThisOptionError : public Exception {
 public:
     SkipThisOptionError (const char* file, size_t line, const char* what) :

diff --git a/src/lib/dhcp/option_definition.cc b/src/lib/dhcp/option_definition.cc
index 1297e4a60804e573df7bf6ddc62c81729f389761..3e1fa740af1eab684b47aa22c63d091dc3e0dea8 100644 (file)
--- a/src/lib/dhcp/option_definition.cc
+++ b/src/lib/dhcp/option_definition.cc
@@ -264,12 +264,12 @@ OptionDefinition::optionFactory(Option::Universe u, uint16_t type,
             ;
         }
         return (OptionPtr(new OptionCustom(*this, u, begin, end)));
-    } catch (const SkipThisOptionError& ex) {
+    } catch (const SkipThisOptionError&) {
         // We need to throw this one as is.
-        throw ex;
-    } catch (const SkipRemainingOptionsError& ex) {
+        throw;
+    } catch (const SkipRemainingOptionsError&) {
         // We need to throw this one as is.
-        throw ex;
+        throw;
     } catch (const Exception& ex) {
         isc_throw(InvalidOptionValue, ex.what());
     }

diff --git a/src/lib/dhcp/option_string.cc b/src/lib/dhcp/option_string.cc
index 64d4499350c3920c98263029f3d8bf840f48618d..a35a021abf6adce1a97ef0da14a47cb17fd34d4e 100644 (file)
--- a/src/lib/dhcp/option_string.cc
+++ b/src/lib/dhcp/option_string.cc
@@ -51,13 +51,13 @@ OptionString::setValue(const std::string& value) {
                   << getType() << "' must not be empty");
     }
 
-    // Trim off any trailing NULLs.
+    // Trim off any trailing nuls.
     auto begin = value.begin();
     auto end = util::str::seekTrimmed(begin, value.end(), 0x0);
 
     if (std::distance(begin, end) == 0) {
         isc_throw(isc::OutOfRange, "string value carried by the option '"
-                  << getType() << "' contained only NULLs");
+                  << getType() << "' contained only nuls");
     }
 
     // Now set the value.
@@ -85,12 +85,12 @@ OptionString::pack(isc::util::OutputBuffer& buf) const {
 void
 OptionString::unpack(OptionBufferConstIter begin,
                      OptionBufferConstIter end) {
-    // Trim off trailing null(s)
+    // Trim off trailing nul(s)
     end = util::str::seekTrimmed(begin, end, 0x0);
     if (std::distance(begin, end) == 0) {
         isc_throw(isc::dhcp::SkipThisOptionError, "failed to parse an option '"
                   << getType() << "' holding string value"
-                  << "' was empty or contained only NULLs");
+                  << "' was empty or contained only nuls");
     }
 
     // Now set the data.

diff --git a/src/lib/testutils/gtest_utils.h b/src/lib/testutils/gtest_utils.h
index 13c1c0d122c2b1d32912bc3b4efa10a146a4a8c6..e7b10b2812374c812af34badd3be340dd1f6434a 100644 (file)
--- a/src/lib/testutils/gtest_utils.h
+++ b/src/lib/testutils/gtest_utils.h
@@ -54,33 +54,35 @@ namespace test {
     } \
 } \
 
-/// @brief Adds a non-fatal failure with exception info, if the given 
-/// expression throws
-/// 
+/// @brief Adds a non-fatal failure with exception info, if the given
+/// expression throws.  Note the type name emitted may be mangled.
+///
 /// @param statement - statement block to execute
 #define EXPECT_NO_THROW_LOG(statement) \
 { \
     try { \
         statement; \
     } catch (const std::exception& ex)  { \
-        ADD_FAILURE() << #statement <<  "threw: " << ex.what(); \
+        ADD_FAILURE() << #statement <<  " threw type: " << typeid(ex).name() \
+                      << ", what: " << ex.what(); \
     } catch (...) { \
         ADD_FAILURE() << #statement <<  "threw non-std::exception"; \
     } \
 } \
 
-/// @brief Generates a fatal failure with exception info, if the given 
-/// expression throws
-/// 
+/// @brief Generates a fatal failure with exception info, if the given
+/// expression throws.  Note the type name emitted may be mangled.
+///
 /// @param statement - statement block to execute
 #define ASSERT_NO_THROW_LOG(statement) \
 { \
     try { \
         statement; \
     } catch (const std::exception& ex)  { \
-        GTEST_FAIL() << #statement <<  "threw: " << ex.what(); \
+        GTEST_FAIL() << #statement <<  " threw type: " << typeid(ex).name() \
+                     << ", what: " << ex.what(); \
     } catch (...) { \
-        GTEST_FAIL() << #statement <<  "threw non-std::exception"; \
+        GTEST_FAIL() << #statement <<  " threw non-std::exception"; \
     } \
 } \