[3436] D2 TSIGKeyInfo configuration errors now include position info.

Modified DCfgContextBase::getParam() variants to return the parameter's
Element::Position.  This makes it available during parsing.

Modified TSIGKeyInfoParser::build to validate parameters and use position
in error messages.

src/bin/d2/d2_config.cc

@@ -356,36 +356,59 @@ TSIGKeyInfoParser::build(isc::data::ConstElementPtr key_config) {
     std::string name;
     std::string algorithm;
     std::string secret;
+    std::vector<isc::data::Element::Position> pos;
 
-    // Fetch the key configuration's parsed scalar values from parser's
+    // Fetch the key's parsed scalar values from parser's local storage.
-    // local storage.
+    // All are required, if any are missing we'll throw.
-    local_scalars_.getParam("name", name);
+    try {
-    local_scalars_.getParam("algorithm", algorithm);
+        pos.push_back(local_scalars_.getParam("name", name));
-    local_scalars_.getParam("secret", secret);
+        pos.push_back(local_scalars_.getParam("algorithm", algorithm));
+        pos.push_back(local_scalars_.getParam("secret", secret));
+    } catch (const std::exception& ex) {
+        isc_throw(D2CfgError, "TSIG Key incomplete : " << ex.what()
+                  << " : " << key_config->getPosition());
+    }
 
     // Name cannot be blank.
     if (name.empty()) {
-        isc_throw(D2CfgError, "TSIG Key Info must specify name");
+        isc_throw(D2CfgError, "TSIG key must specify name : " << pos[0]);
     }
 
-    // Algorithm cannot be blank.
+    // Currently, the premise is that key storage is always empty prior to
-    if (algorithm.empty()) {
+    // parsing so we are always adding keys never replacing them. Duplicates
-        isc_throw(D2CfgError, "TSIG Key Info must specify algorithm");
+    // are not allowed and should be flagged as a configuration error.
+    if (keys_->find(name) != keys_->end()) {
+        isc_throw(D2CfgError, "Duplicate TSIG key name specified : " << name
+                              << " : " << pos[0]);
+    }
+
+    // Algorithm must be valid.
+    try {
+        TSIGKeyInfo::stringToAlgorithmName(algorithm);
+    } catch (const std::exception& ex) {
+        isc_throw(D2CfgError, "TSIG key invalid algorithm : "
+                  << algorithm << " : " << pos[1]);
     }
 
     // Secret cannot be blank.
+    // Cryptolink lib doesn't offer anyway to validate these. As long as it
+    // isn't blank we'll accept it.  If the content is bad, the call to in
+    // TSIGKeyInfo::remakeKey() made in the TSIGKeyInfo ctor will throw.
+    // We'll deal with that below.
     if (secret.empty()) {
-        isc_throw(D2CfgError, "TSIG Key Info must specify secret");
+        isc_throw(D2CfgError, "TSIG key must specify secret : " << pos[2]);
     }
 
-    // Currently, the premise is that key storage is always empty prior to
+    // Everything should be valid, so create the key instance.
-    // parsing so we are always adding keys never replacing them. Duplicates
+    // It is possible for the asiodns::dns::TSIGKey create to fail such as
-    // are not allowed and should be flagged as a configuration error.
+    // with an invalid secret content.
-    if (keys_->find(name) != keys_->end()) {
+    TSIGKeyInfoPtr key_info;
-        isc_throw(D2CfgError, "Duplicate TSIG key specified:" << name);
+    try {
-    }
+        key_info.reset(new TSIGKeyInfo(name, algorithm, secret));
+    } catch (const std::exception& ex) {
+        isc_throw(D2CfgError, ex.what() << " : " << key_config->getPosition());
 
-    TSIGKeyInfoPtr key_info(new TSIGKeyInfo(name, algorithm, secret));
+    }
 
     // Add the new TSIGKeyInfo to the key storage.
     (*keys_)[name]=key_info;

src/bin/d2/d_cfg_mgr.cc

@@ -52,43 +52,51 @@ DCfgContextBase::DCfgContextBase(const DCfgContextBase& rhs):
         string_values_(new StringStorage(*(rhs.string_values_))) {
 }
 
-void
+const data::Element::Position&
 DCfgContextBase::getParam(const std::string& name, bool& value, bool optional) {
     try {
         value = boolean_values_->getParam(name);
+        return (boolean_values_->getPosition(name));
     } catch (DhcpConfigError& ex) {
         // If the parameter is not optional, re-throw the exception.
         if (!optional) {
             throw;
         }
     }
-}
 
+    return (data::Element::ZERO_POSITION());
+}
 
-void
+const data::Element::Position&
 DCfgContextBase::getParam(const std::string& name, uint32_t& value,
                           bool optional) {
     try {
         value = uint32_values_->getParam(name);
+        return (uint32_values_->getPosition(name));
     } catch (DhcpConfigError& ex) {
         // If the parameter is not optional, re-throw the exception.
         if (!optional) {
             throw;
         }
     }
+
+    return (data::Element::ZERO_POSITION());
 }
 
-void
+const data::Element::Position&
 DCfgContextBase::getParam(const std::string& name, std::string& value,
                           bool optional) {
     try {
         value = string_values_->getParam(name);
+        return (string_values_->getPosition(name));
     } catch (DhcpConfigError& ex) {
         // If the parameter is not optional, re-throw the exception.
         if (!optional) {
             throw;
         }
     }
+
+    return (data::Element::ZERO_POSITION());
 }
 
 DCfgContextBase::~DCfgContextBase() {

src/bin/d2/d_cfg_mgr.h

@@ -80,9 +80,14 @@ public:
     /// will not throw if the parameter is not found in the context. The
     /// contents of the output parameter, value, will not be altered.
     /// It defaults to false if not specified.
+    ///
+    /// @return The parameter's element's position information if found,
+    /// otherwise it returns isc::data::Element::ZERO_POSITION().
+    ///
     /// @throw throws DhcpConfigError if the context does not contain the
     /// parameter and optional is false.
-    void getParam(const std::string& name, bool& value, bool optional=false);
+    const data::Element::Position&
+    getParam(const std::string& name, bool& value, bool optional=false);
 
     /// @brief Fetches the value for a given uint32_t configuration parameter
     /// from the context.
@@ -93,9 +98,14 @@ public:
     /// @param optional if true, the parameter is optional and the method
     /// will not throw if the parameter is not found in the context. The
     /// contents of the output parameter, value, will not be altered.
+    ///
+    /// @return The parameter's element's position information if found,
+    /// otherwise it returns isc::data::Element::ZERO_POSITION().
+    ///
     /// @throw throws DhcpConfigError if the context does not contain the
     /// parameter and optional is false.
-    void getParam(const std::string& name, uint32_t& value,
+    const data::Element::Position&
+    getParam(const std::string& name, uint32_t& value,
                  bool optional=false);
 
     /// @brief Fetches the value for a given string configuration parameter
@@ -107,9 +117,14 @@ public:
     /// @param optional if true, the parameter is optional and the method
     /// will not throw if the parameter is not found in the context. The
     /// contents of the output parameter, value, will not be altered.
+    ///
+    /// @return The parameter's element's position information if found,
+    /// otherwise it returns isc::data::Element::ZERO_POSITION().
+    ///
     /// @throw throws DhcpConfigError if the context does not contain the
     /// parameter and optional is false.
-    void getParam(const std::string& name, std::string& value,
+    const data::Element::Position&
+    getParam(const std::string& name, std::string& value,
                   bool optional=false);
 
     /// @brief Fetches the Boolean Storage. Typically used for passing

src/bin/d2/tests/d_cfg_mgr_unittests.cc

@@ -439,4 +439,50 @@ TEST_F(DStubCfgMgrTest, rollBackTest) {
     EXPECT_TRUE(object);
 }
 
+// Tests that configuration element position is returned by getParam variants.
+TEST_F(DStubCfgMgrTest, paramPosition) {
+    // Create a configuration with one of each scalar types.  We end them
+    // with line feeds so we can test position value.
+    string config = "{ \"bool_test\": true , \n"
+                    "  \"uint32_test\": 77 , \n"
+                    "  \"string_test\": \"hmmm chewy\" }";
+    ASSERT_TRUE(fromJSON(config));
+
+    // Verify that the configuration parses without error.
+    answer_ = cfg_mgr_->parseConfig(config_set_);
+    ASSERT_TRUE(checkAnswer(0));
+    DStubContextPtr context = getStubContext();
+    ASSERT_TRUE(context);
+
+    // Verify that the boolean parameter was parsed correctly by retrieving
+    // its value from the context.
+    bool actual_bool = false;
+    isc::data::Element::Position pos;
+    EXPECT_NO_THROW(pos = context->getParam("bool_test", actual_bool));
+    EXPECT_EQ(true, actual_bool);
+    EXPECT_EQ(1, pos.line_);
+
+    // Verify that the uint32 parameter was parsed correctly by retrieving
+    // its value from the context.
+    uint32_t actual_uint32 = 0;
+    EXPECT_NO_THROW(pos = context->getParam("uint32_test", actual_uint32));
+    EXPECT_EQ(77, actual_uint32);
+    EXPECT_EQ(2, pos.line_);
+
+    // Verify that the string parameter was parsed correctly by retrieving
+    // its value from the context.
+    std::string actual_string = "";
+    EXPECT_NO_THROW(pos = context->getParam("string_test", actual_string));
+    EXPECT_EQ("hmmm chewy", actual_string);
+    EXPECT_EQ(3, pos.line_);
+
+    // Verify that an optional parameter that is not defined, returns the
+    // zero position.
+    pos = isc::data::Element::ZERO_POSITION();
+    EXPECT_NO_THROW(pos = context->getParam("bogus_value",
+                                            actual_string, true));
+    EXPECT_EQ(pos.file_, isc::data::Element::ZERO_POSITION().file_);
+}
+
+
 } // end of anonymous namespace