From: Thomas Markwalder Date: Tue, 22 Oct 2019 14:46:10 +0000 (-0400) Subject: [#900,!561] Addressed review comments X-Git-Tag: Kea-1.7.1~53 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=c75a7c10bde74f59d481a7717b4e03b9cb2268a6;p=thirdparty%2Fkea.git [#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 44f06ed436..6c37b77efd 100644 --- 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 1d484f46e1..1c77397456 100644 --- 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 1297e4a608..3e1fa740af 100644 --- 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 64d4499350..a35a021abf 100644 --- 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 13c1c0d122..e7b10b2812 100644 --- 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"; \ } \ } \