From: Tomek Mrugalski Date: Thu, 22 Jun 2023 10:48:50 +0000 (+0200) Subject: [#2707] parseAnswer() cleaned up X-Git-Tag: Kea-2.4.0~123 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=aadbff39abbf5efe57381f04a27fdd5e81ffc88c;p=thirdparty%2Fkea.git [#2707] parseAnswer() cleaned up There are now two versions of this func: - parseAnswer() - returns parameters (if present) or text - parseanswerText() - return text status Previously they were both bundled into one parseAnswer() --- diff --git a/src/bin/dhcp4/tests/config_backend_unittest.cc b/src/bin/dhcp4/tests/config_backend_unittest.cc index 510c2bffcc..f3aa486976 100644 --- a/src/bin/dhcp4/tests/config_backend_unittest.cc +++ b/src/bin/dhcp4/tests/config_backend_unittest.cc @@ -131,7 +131,7 @@ public: ASSERT_TRUE(status); int rcode; - ConstElementPtr comment = parseAnswer(rcode, status); + ConstElementPtr comment = parseAnswerText(rcode, status); ASSERT_EQ(expected_code, rcode) << " comment: " << comment->stringValue(); diff --git a/src/bin/dhcp4/tests/config_parser_unittest.cc b/src/bin/dhcp4/tests/config_parser_unittest.cc index e297063b3e..9e30f23580 100644 --- a/src/bin/dhcp4/tests/config_parser_unittest.cc +++ b/src/bin/dhcp4/tests/config_parser_unittest.cc @@ -294,7 +294,7 @@ public: // Also stores result in rcode_ and comment_. void checkResult(ConstElementPtr status, int expected_code) { ASSERT_TRUE(status); - comment_ = parseAnswer(rcode_, status); + comment_ = parseAnswerText(rcode_, status); EXPECT_EQ(expected_code, rcode_) << "error text:" << comment_->stringValue(); } @@ -332,10 +332,11 @@ public: ASSERT_TRUE(status); int rcode; - ConstElementPtr comment = parseAnswer(rcode, status); + ConstElementPtr comment = parseAnswerText(rcode, status); EXPECT_EQ(expected_code, rcode); string text; + ASSERT_TRUE(comment); ASSERT_NO_THROW(text = comment->stringValue()); if (expected_code != rcode) { @@ -665,7 +666,7 @@ public: // Store the answer if we need it. // Returned value should be 0 (configuration success) - comment_ = parseAnswer(rcode_, status); + comment_ = parseAnswerText(rcode_, status); if (rcode_ != 0) { string reason = ""; if (comment_) { @@ -2327,6 +2328,7 @@ TEST_F(Dhcp4ParserTest, badSubnetValues) { ConstElementPtr status; EXPECT_NO_THROW(status = Dhcpv4SrvTest::configure(*srv_, config)); checkResult(status, 1); + ASSERT_TRUE(comment_); EXPECT_EQ(comment_->stringValue(), (*scenario).exp_error_msg_); } } diff --git a/src/bin/dhcp4/tests/dhcp4_test_utils.cc b/src/bin/dhcp4/tests/dhcp4_test_utils.cc index c4a4db593b..b33c34f531 100644 --- a/src/bin/dhcp4/tests/dhcp4_test_utils.cc +++ b/src/bin/dhcp4/tests/dhcp4_test_utils.cc @@ -854,7 +854,7 @@ Dhcpv4SrvTest::configure(const std::string& config, EXPECT_NO_THROW(status = configureDhcp4Server(srv, json, test)); ASSERT_TRUE(status); int rcode; - ConstElementPtr comment = config::parseAnswer(rcode, status); + ConstElementPtr comment = config::parseAnswerText(rcode, status); ASSERT_EQ(0, rcode) << "configuration failed, test is broken: " << comment->str(); @@ -927,7 +927,7 @@ Dhcpv4SrvTest::configureWithStatus(const std::string& config, NakedDhcpv4Srv& sr } int rcode; - ConstElementPtr comment = config::parseAnswer(rcode, status); + ConstElementPtr comment = config::parseAnswerText(rcode, status); EXPECT_EQ(exp_rcode, rcode) << comment->stringValue(); // Use specified lease database backend. diff --git a/src/bin/dhcp6/tests/config_backend_unittest.cc b/src/bin/dhcp6/tests/config_backend_unittest.cc index 6475ee832b..e2d87762d2 100644 --- a/src/bin/dhcp6/tests/config_backend_unittest.cc +++ b/src/bin/dhcp6/tests/config_backend_unittest.cc @@ -134,7 +134,7 @@ public: ASSERT_TRUE(status); int rcode; - ConstElementPtr comment = parseAnswer(rcode, status); + ConstElementPtr comment = parseAnswerText(rcode, status); ASSERT_EQ(expected_code, rcode) << " comment: " << comment->stringValue(); diff --git a/src/bin/dhcp6/tests/config_parser_unittest.cc b/src/bin/dhcp6/tests/config_parser_unittest.cc index 59c17abb87..7eb5186d3c 100644 --- a/src/bin/dhcp6/tests/config_parser_unittest.cc +++ b/src/bin/dhcp6/tests/config_parser_unittest.cc @@ -405,7 +405,7 @@ public: // Also stores result in rcode_ and comment_. void checkResult(ConstElementPtr status, int expected_code) { ASSERT_TRUE(status); - comment_ = parseAnswer(rcode_, status); + comment_ = parseAnswerText(rcode_, status); EXPECT_EQ(expected_code, rcode_); if (expected_code != rcode_) { cout << "The comment returned was: [" << comment_->stringValue() << "]" << endl; @@ -418,7 +418,7 @@ public: void checkResult(ConstElementPtr status, int expected_code, string expected_txt) { ASSERT_TRUE(status); - comment_ = parseAnswer(rcode_, status); + comment_ = parseAnswerText(rcode_, status); EXPECT_EQ(expected_code, rcode_) << "error text:" << comment_->stringValue(); ASSERT_TRUE(comment_); ASSERT_EQ(Element::string, comment_->getType()); @@ -442,10 +442,11 @@ public: ASSERT_TRUE(status); int rcode; - ConstElementPtr comment = parseAnswer(rcode, status); + ConstElementPtr comment = parseAnswerText(rcode, status); EXPECT_EQ(expected_code, rcode); string text; + ASSERT_TRUE(comment); ASSERT_NO_THROW(text = comment->stringValue()); if (expected_code != rcode) { @@ -708,7 +709,7 @@ public: // Store the answer if we need it. // Returned value should be 0 (configuration success) - comment_ = parseAnswer(rcode_, status); + comment_ = parseAnswerText(rcode_, status); if (rcode_ != 0) { string reason = ""; if (comment_) { @@ -6169,7 +6170,7 @@ TEST_F(Dhcp6ParserTest, notExistDataDir) { // returned value should be 1 (error) int rcode; - ConstElementPtr comment = parseAnswer(rcode, status); + ConstElementPtr comment = parseAnswerText(rcode, status); EXPECT_EQ(1, rcode); string text; ASSERT_NO_THROW(text = comment->stringValue()); @@ -6191,7 +6192,7 @@ TEST_F(Dhcp6ParserTest, notDirDataDir) { // returned value should be 1 (error) int rcode; - ConstElementPtr comment = parseAnswer(rcode, status); + ConstElementPtr comment = parseAnswerText(rcode, status); EXPECT_EQ(1, rcode); string text; ASSERT_NO_THROW(text = comment->stringValue()); @@ -6646,7 +6647,7 @@ TEST_F(Dhcp6ParserTest, invalidPoolRange) { EXPECT_NO_THROW(status = Dhcpv6SrvTest::configure(srv_, json)); ASSERT_TRUE(status); int rcode; - ConstElementPtr comment = parseAnswer(rcode, status); + ConstElementPtr comment = parseAnswerText(rcode, status); string text; ASSERT_NO_THROW(text = comment->stringValue()); @@ -6677,7 +6678,7 @@ TEST_F(Dhcp6ParserTest, outsideSubnetPool) { EXPECT_NO_THROW(status = Dhcpv6SrvTest::configure(srv_, json)); ASSERT_TRUE(status); int rcode; - ConstElementPtr comment = parseAnswer(rcode, status); + ConstElementPtr comment = parseAnswerText(rcode, status); string text; ASSERT_NO_THROW(text = comment->stringValue()); diff --git a/src/lib/cc/command_interpreter.cc b/src/lib/cc/command_interpreter.cc index 5cb1747105..0ea03bcd48 100644 --- a/src/lib/cc/command_interpreter.cc +++ b/src/lib/cc/command_interpreter.cc @@ -65,6 +65,33 @@ createAnswer(const int status_code, const ConstElementPtr& arg) { return (createAnswer(status_code, "", arg)); } +ConstElementPtr +parseAnswerText(int &rcode, const ConstElementPtr& msg) { + if (!msg) { + isc_throw(CtrlChannelError, "invalid answer: no answer specified"); + } + if (msg->getType() != Element::map) { + isc_throw(CtrlChannelError, "invalid answer: expected toplevel entry to be a map, got " + << Element::typeToName(msg->getType()) << " instead"); + } + if (!msg->contains(CONTROL_RESULT)) { + isc_throw(CtrlChannelError, + "invalid answer: does not contain mandatory '" << CONTROL_RESULT << "'"); + } + + ConstElementPtr result = msg->get(CONTROL_RESULT); + if (result->getType() != Element::integer) { + isc_throw(CtrlChannelError, "invalid answer: expected '" << CONTROL_RESULT + << "' to be an integer, got " + << Element::typeToName(result->getType()) << " instead"); + } + + rcode = result->intValue(); + + // If there are arguments, return them. + return (msg->get(CONTROL_TEXT)); +} + ConstElementPtr parseAnswer(int &rcode, const ConstElementPtr& msg) { if (!msg) { @@ -91,17 +118,14 @@ parseAnswer(int &rcode, const ConstElementPtr& msg) { // If there are arguments, return them. ConstElementPtr args = msg->get(CONTROL_ARGUMENTS); if (args) { - // If the arguments contain only a hash (used in config-set/config-get), we can ignore them. - // We don't want to return arguments with just a hash. - if ( (args->getType()!=isc::data::Element::map) || (args->size() > 1) || !args->get("hash") ) { - return (args); - } + return (args); } // There are no arguments, let's try to return just the text status return (msg->get(CONTROL_TEXT)); } + std::string answerToText(const ConstElementPtr& msg) { if (!msg) { diff --git a/src/lib/cc/command_interpreter.h b/src/lib/cc/command_interpreter.h index 6a2b2cb3ae..aea0552f43 100644 --- a/src/lib/cc/command_interpreter.h +++ b/src/lib/cc/command_interpreter.h @@ -101,14 +101,29 @@ isc::data::ConstElementPtr createAnswer(const int status_code, const std::string& status, const isc::data::ConstElementPtr& arg); -/// @brief Parses a standard config/command level answer message. +/// @brief Parses a standard config/command level answer and returns arguments +/// or text status code. +/// +/// If you need to get the text status, please use @ref parseAnswerText. /// /// @param status_code This value will be set to the return code contained in /// the message /// @param msg The message to parse -/// @return The optional argument in the message. -isc::data::ConstElementPtr parseAnswer(int &status_code, - const isc::data::ConstElementPtr& msg); +/// @return The optional argument in the message (or null) +isc::data::ConstElementPtr +parseAnswer(int &status_code, const isc::data::ConstElementPtr& msg); + +/// @brief Parses a standard config/command level answer and returns text status. +/// +/// This method returns the text status. If you need to get the arguments provided, +/// please use @ref parseAnswer. +/// +/// @param status_code This value will be set to the return code contained in +/// the message +/// @param msg The message to parse +/// @return The optional argument in the message (or null) +isc::data::ConstElementPtr +parseAnswerText(int &rcode, const isc::data::ConstElementPtr& msg); /// @brief Converts answer to printable text /// diff --git a/src/lib/cc/tests/command_interpreter_unittests.cc b/src/lib/cc/tests/command_interpreter_unittests.cc index 38910fb441..8738b925c5 100644 --- a/src/lib/cc/tests/command_interpreter_unittests.cc +++ b/src/lib/cc/tests/command_interpreter_unittests.cc @@ -78,15 +78,24 @@ TEST(CommandInterpreterTest, parseAnswer) { EXPECT_EQ(0, rcode); EXPECT_TRUE(isNull(arg)); - answer = el("{ \"result\": 1, \"text\": \"error\" }"); + answer = el("{ \"result\": 3, \"text\": \"error\", \"arguments\": [ \"some\", \"data\" ] }"); arg = parseAnswer(rcode, answer); - EXPECT_EQ(1, rcode); - EXPECT_EQ("error", arg->stringValue()); + ASSERT_TRUE(arg); + EXPECT_EQ(3, rcode); + EXPECT_EQ("[ \"some\", \"data\" ]", arg->str()); +} - answer = el("{ \"result\": 0, \"arguments\": [ \"just\", \"some\", \"data\" ] }"); - arg = parseAnswer(rcode, answer); - EXPECT_EQ(0, rcode); - EXPECT_EQ("[ \"just\", \"some\", \"data\" ]", arg->str()); +// Checks if parseAnswerText can return the text +TEST(CommandInterpreterTest, parseAnswerText) { + ConstElementPtr answer; + ConstElementPtr arg; + int rcode; + + answer = el("{ \"result\": 5, \"text\": \"error\", \"arguments\": [ \"some\", \"data\" ] }"); + arg = parseAnswerText(rcode, answer); + ASSERT_TRUE(arg); + EXPECT_EQ(5, rcode); + EXPECT_EQ("error", arg->stringValue()); } // This checks whether we can convert an answer to easily printable form.