[#3141] addressed review comments

- refactored code of SvcParam config parser
- added new 2 UTs
- added some more comments in code

src/lib/dhcp/option4_dnr.h

@@ -291,6 +291,9 @@ class DnrInstance {
     ///
     /// @param begin beginning of the buffer from which the field will be read
     /// @param end end of the buffer from which the field will be read
+    ///
+    /// @throw OutOfRange Thrown when truncated data is detected.
+    /// @throw InvalidOptionDnrSvcParams Thrown when invalid SvcParams syntax is detected.
     void unpackSvcParams(OptionBufferConstIter& begin, OptionBufferConstIter end);
 
     /// @brief Adds IP address to @c ip_addresses_ container.
@@ -322,8 +325,6 @@ class DnrInstance {
     /// @throw BadValue Thrown in case parser found wrong format of received string.
     /// @throw InvalidOptionDnrDomainName Thrown in case parser had problems with extracting ADN
     /// FQDN.
-    /// @throw InvalidOptionDnrSvcParams Thrown in case parser had problems with extracting
-    /// SvcParams.
     void parseDnrInstanceConfigData(const std::string& config_txt);
 
 protected:
@@ -431,6 +432,45 @@ class DnrInstance {
     /// @throw BadValue thrown when there is a problem with reading alpn SvcParamVal from
     ///                 @c svc_params_map_
     std::string svcParamValAsText(const std::pair<uint16_t, OpaqueDataTuple>& svc_param) const;
+
+    /// @brief Parses DNR resolver IP address/es from a piece of convenient notation option config.
+    ///
+    /// @param txt_addresses a piece of convenient notation option config holding IP address/es
+    ///
+    /// @throw BadValue Thrown in case parser found wrong format of received string.
+    void parseIpAddresses(const std::string& txt_addresses);
+
+    /// @brief Parses Service Parameters from a piece of convenient notation option config.
+    ///
+    /// @param txt_svc_params a piece of convenient notation option config holding SvcParams
+    ///
+    /// @throw InvalidOptionDnrSvcParams Thrown in case parser had problems with extracting
+    /// SvcParams.
+    void parseSvcParams(const std::string& txt_svc_params);
+
+    /// @brief Parses ALPN Service Parameter from a piece of convenient notation option config.
+    ///
+    /// @param svc_param_val a piece of convenient notation option config holding ALPN SvcParam
+    ///
+    /// @throw InvalidOptionDnrSvcParams Thrown in case parser had problems with extracting
+    /// SvcParams.
+    void parseAlpnSvcParam(const std::string& svc_param_val);
+
+    /// @brief Parses Port Service Parameter from a piece of convenient notation option config.
+    ///
+    /// @param svc_param_val a piece of convenient notation option config holding Port SvcParam
+    ///
+    /// @throw InvalidOptionDnrSvcParams Thrown in case parser had problems with extracting
+    /// SvcParams.
+    void parsePortSvcParam(const std::string& svc_param_val);
+
+    /// @brief Parses Dohpath Service Parameter from a piece of convenient notation option config.
+    ///
+    /// @param svc_param_val a piece of convenient notation option config holding Dohpath SvcParam
+    ///
+    /// @throw InvalidOptionDnrSvcParams Thrown in case parser had problems with extracting
+    /// SvcParams.
+    void parseDohpathSvcParam(const std::string& svc_param_val);
 };
 
 /// @brief Represents DHCPv4 Encrypted DNS %Option (code 162).

src/lib/dhcp/option6_dnr.h

@@ -104,6 +104,7 @@ class Option6Dnr : public Option, public DnrInstance {
     ///
     /// @throw OutOfRange Thrown in case of malformed data detected during parsing e.g.
     /// Addr Len not divisible by 16, Addr Len is 0, addresses data truncated etc.
+    /// @throw BadValue Thrown when trying to unpack address which is not an IPv6 address
     void unpackAddresses(OptionBufferConstIter& begin, OptionBufferConstIter end) override;
 
 private:

src/lib/dhcp/option_definition.cc

@@ -319,8 +319,11 @@ OptionDefinition::optionFactory(Option::Universe u, uint16_t type,
             // true, so that the factory will have a chance to handle it in a special way.
 
             // At this stage any escape backslash chars were lost during last call of
-            // isc::util::str::tokens(), so we must restore them. Some INTERNAL options may use
-            // escaped delimiters, e.g. DNR options.
+            // isc::util::str::tokens() inside of
+            // OptionDataParser::createOption(ConstElementPtr option_data), so we must
+            // restore them. Some INTERNAL options may use escaped delimiters, e.g. DNR options.
+            // Values are concatenated back to comma separated format and will be written to
+            // the OptionBuffer as one String type option.
             std::ostringstream stream;
             bool first = true;
             for (auto val : values) {

src/lib/dhcp/tests/option4_dnr_unittest.cc

@@ -685,6 +685,23 @@ TEST(Option4DnrTest, fromConfigCtorWrongSvcParamsMissingVarInDohpath) {
     ASSERT_FALSE(option);
 }
 
+// This test verifies that option constructor throws
+// an exception when config provided via ctor is malformed
+// - IPv6 address given.
+TEST(Option4DnrTest, fromConfigCtorIPv6Address) {
+    // Prepare example config.
+    const std::string config = "100, dot1.example.org., 2001:db8::1";
+
+    OptionBuffer buf;
+    buf.assign(config.begin(), config.end());
+
+    // Create option instance. Check that constructor throws.
+    Option4DnrPtr option;
+    EXPECT_THROW(option.reset(new Option4Dnr(buf.begin(), buf.end(), true)),
+                 BadValue);
+    ASSERT_FALSE(option);
+}
+
 // This test verifies option packing into wire data.
 // Provided data to pack contains 2 DNR instances:
 // 1. ADN only mode

src/lib/dhcp/tests/option6_dnr_unittest.cc

@@ -590,6 +590,23 @@ TEST(Option6DnrTest, fromConfigCtorSvcParamsKeyRepeated) {
     ASSERT_FALSE(option);
 }
 
+// This test verifies that option constructor throws
+// an exception when config provided via ctor is malformed
+// - IPv4 address given.
+TEST(Option6DnrTest, fromConfigCtorIPv4Address) {
+    // Prepare example config.
+    const std::string config = "100, dot1.example.org., 10.0.2.3";
+
+    OptionBuffer buf;
+    buf.assign(config.begin(), config.end());
+
+    // Create option instance. Check that constructor throws.
+    Option6DnrPtr option;
+    EXPECT_THROW(option.reset(new Option6Dnr(buf.begin(), buf.end(), true)),
+                 BadValue);
+    ASSERT_FALSE(option);
+}
+
 // This test verifies that string representation of the option returned by
 // toText method is correctly formatted.
 TEST(Option6DnrTest, toText) {