From c27170ab2747f7854a06c4dc533c2ad3c88dd758 Mon Sep 17 00:00:00 2001 From: Ratan Gupta Date: Wed, 22 Nov 2017 15:44:42 +0530 Subject: Reduce journal logs Presently the code was creating the elog during reading of the values from the config parser which creates the journal log internally("Internal error Occured") Apart from creating elog,Code also logs into the journal more descriptive message saying which section and key was not found. The code which catches the exception that also logs into the journal. In the changed approach instead of throwing exception we are returning the return code as not finding a section or key in the network config file is not an error. Change-Id: Ib89a3af6541fdf93fb5fa9a8e583d6c6ed88da79 Signed-off-by: Ratan Gupta --- config_parser.cpp | 57 ++++++++++++++++++++++------------------ config_parser.hpp | 26 +++++++++++++----- ethernet_interface.cpp | 32 +++++++++++----------- test/test_config_parser.cpp | 40 ++++++++++------------------ test/test_ethernet_interface.cpp | 8 ++++-- test/test_vlan_interface.cpp | 22 ++++++++++------ timer.cpp | 1 - util.cpp | 22 +++++++++------- 8 files changed, 113 insertions(+), 95 deletions(-) diff --git a/config_parser.cpp b/config_parser.cpp index 9ab601c..3852b6c 100644 --- a/config_parser.cpp +++ b/config_parser.cpp @@ -1,7 +1,5 @@ #include "config_parser.hpp" -#include "xyz/openbmc_project/Common/error.hpp" #include -#include #include #include @@ -18,7 +16,6 @@ namespace config { using namespace phosphor::logging; -using namespace sdbusplus::xyz::openbmc_project::Common::Error; Parser::Parser(const fs::path& filePath) { @@ -26,58 +23,66 @@ Parser::Parser(const fs::path& filePath) } -KeyValues Parser::getSection(const std::string& section) +std::tuple Parser::getSection(const std::string& section) { auto it = sections.find(section); if (it == sections.end()) { - log("ConfigParser: Section not found", - entry("SECTION=%s",section)); - elog(); + KeyValueMap keyValues; + return std::make_tuple(ReturnCode::SECTION_NOT_FOUND, + std::move(keyValues)); } - return it->second; + + return std::make_tuple(ReturnCode::SUCCESS, it->second); } -std::vector Parser::getValues(const std::string& section, - const std::string& key) +std::tuple Parser::getValues(const std::string& section, + const std::string& key) { - std::vector values; - auto keyValues = getSection(section); + ValueList values; + KeyValueMap keyValues {}; + auto rc = ReturnCode::SUCCESS; + + std::tie(rc, keyValues) = getSection(section); + if (rc != ReturnCode::SUCCESS) + { + return std::make_tuple(rc, std::move(values)); + } + auto it = keyValues.find(key); if (it == keyValues.end()) { - log("ConfigParser: Key not found", - entry("KEY=%s",key)); - elog(); + return std::make_tuple(ReturnCode::KEY_NOT_FOUND, std::move(values)); } + for (; it != keyValues.end() && key == it->first; it++) { values.push_back(it->second); } - return values; + + return std::make_tuple(ReturnCode::SUCCESS, std::move(values)); } bool Parser::isValueExist(const std::string& section, const std::string& key, const std::string& value) { - try - { - auto values = getValues(section, key); - auto it = std::find(values.begin(), values.end(), value); - return it != std::end(values) ? true : false; - } - catch (InternalFailure& e) + auto rc = ReturnCode::SUCCESS; + ValueList values; + std::tie(rc, values) = getValues(section, key); + + if (rc != ReturnCode::SUCCESS) { - commit(); + return false; } - return false; + auto it = std::find(values.begin(), values.end(), value); + return it != std::end(values) ? true : false; } void Parser::setValue(const std::string& section, const std::string& key, const std::string& value) { - KeyValues values; + KeyValueMap values; auto it = sections.find(section); if (it != sections.end()) { diff --git a/config_parser.hpp b/config_parser.hpp index bf5cb2d..f3e4a70 100644 --- a/config_parser.hpp +++ b/config_parser.hpp @@ -2,6 +2,7 @@ #include #include +#include #include #include #include @@ -14,9 +15,18 @@ namespace config { using Section = std::string; -using KeyValues = std::multimap; +using KeyValueMap = std::multimap; +using ValueList = std::vector; + namespace fs = std::experimental::filesystem; +enum class ReturnCode +{ + SUCCESS = 0x0, + SECTION_NOT_FOUND = 0x1, + KEY_NOT_FOUND = 0x2, +}; + class Parser { public: @@ -32,11 +42,13 @@ class Parser /** @brief Get the values of the given key and section. * @param[in] section - section name. * @param[in] key - key to look for. - * @returns the values associated with the key. + * @returns the tuple of return code and the + * values associated with the key. */ - std::vector getValues(const std::string& section, - const std::string& key); + std::tuple getValues( + const std::string& section, + const std::string& key); /** @brief Set the value of the given key and section. * @param[in] section - section name. @@ -64,10 +76,10 @@ class Parser /** @brief Get all the key values of the given section. * @param[in] section - section name. - * @returns the map of the key and value. + * @returns the tuple of return code and the map of (key,value). */ - KeyValues getSection(const std::string& section); + std::tuple getSection(const std::string& section); /** @brief checks that whether the value exist in the * given section. @@ -80,7 +92,7 @@ class Parser bool isValueExist(const std::string& section, const std::string& key, const std::string& value); - std::unordered_map sections; + std::unordered_map sections; fs::path filePath; }; diff --git a/ethernet_interface.cpp b/ethernet_interface.cpp index 15f40fb..7d13c1c 100644 --- a/ethernet_interface.cpp +++ b/ethernet_interface.cpp @@ -398,14 +398,14 @@ ServerList EthernetInterface::getNameServerFromConf() systemd::config::networkFileSuffix; confPath /= fileName; ServerList servers; - try - { - config::Parser parser(confPath.string()); - servers = parser.getValues("Network", "DNS"); - } - catch (InternalFailure& e) + config::Parser parser(confPath.string()); + auto rc = config::ReturnCode::SUCCESS; + + std::tie(rc, servers) = parser.getValues("Network", "DNS"); + if (rc != config::ReturnCode::SUCCESS) { - log("Exception getting DNS value from conf file"); + log("Unable to get the value for network[DNS]", + entry("RC=%d", rc)); } return servers; } @@ -484,18 +484,20 @@ ServerList EthernetInterface::getNTPServersFromConf() fs::path confPath = manager.getConfDir(); std::string fileName = systemd::config::networkFilePrefix + interfaceName() + - systemd::config::networkFileSuffix; + systemd::config::networkFileSuffix; confPath /= fileName; + ServerList servers; - try - { - config::Parser parser(confPath.string()); - servers = parser.getValues("Network", "NTP"); - } - catch (InternalFailure& e) + config::Parser parser(confPath.string()); + auto rc = config::ReturnCode::SUCCESS; + + std::tie(rc, servers) = parser.getValues("Network", "NTP"); + if (rc != config::ReturnCode::SUCCESS) { - log("Unable to find the NTP server configuration."); + log("Unable to get the value for Network[NTP]", + entry("rc=%d", rc)); } + return servers; } diff --git a/test/test_config_parser.cpp b/test/test_config_parser.cpp index a07390b..1c902d2 100644 --- a/test/test_config_parser.cpp +++ b/test/test_config_parser.cpp @@ -46,17 +46,20 @@ class TestConfigParser : public testing::Test TEST_F(TestConfigParser, ReadConfigDataFromFile) { - auto values = parser.getValues("Network", "DHCP"); + config::ReturnCode rc = config::ReturnCode::SUCCESS; + config::ValueList values; + + std::tie(rc, values) = parser.getValues("Network", "DHCP"); std::string expectedValue = "true"; bool found = isValueFound(values, expectedValue); EXPECT_EQ(found, true); - values = parser.getValues("DHCP", "ClientIdentifier"); + std::tie(rc, values) = parser.getValues("DHCP", "ClientIdentifier"); expectedValue = "mac"; found = isValueFound(values, expectedValue); EXPECT_EQ(found, true); - values = parser.getValues("Match", "Name"); + std::tie(rc, values) = parser.getValues("Match", "Name"); expectedValue = "eth0"; found = isValueFound(values, expectedValue); EXPECT_EQ(found, true); @@ -64,35 +67,20 @@ TEST_F(TestConfigParser, ReadConfigDataFromFile) TEST_F(TestConfigParser, SectionNotExist) { - using namespace sdbusplus::xyz::openbmc_project::Common::Error; - bool caughtException = false; - try - { - parser.getValues("abc", "ipaddress"); - } - catch (const std::exception& e) - { - caughtException = true; - } - EXPECT_EQ(true, caughtException); + config::ReturnCode rc = config::ReturnCode::SUCCESS; + config::ValueList values; + std::tie(rc, values) = parser.getValues("abc", "ipaddress"); + EXPECT_EQ(config::ReturnCode::SECTION_NOT_FOUND, rc); } TEST_F(TestConfigParser, KeyNotFound) { - using namespace sdbusplus::xyz::openbmc_project::Common::Error; - bool caughtException = false; - try - { - parser.getValues("Network", "abc"); - } - catch (const std::exception& e) - { - caughtException = true; - } - EXPECT_EQ(true, caughtException); + config::ReturnCode rc = config::ReturnCode::SUCCESS; + config::ValueList values; + std::tie(rc, values) = parser.getValues("Network", "abc"); + EXPECT_EQ(config::ReturnCode::KEY_NOT_FOUND, rc); remove("/tmp/eth0.network"); } }//namespace network }//namespace phosphor - diff --git a/test/test_ethernet_interface.cpp b/test/test_ethernet_interface.cpp index 27fbb67..2181006 100644 --- a/test/test_ethernet_interface.cpp +++ b/test/test_ethernet_interface.cpp @@ -197,7 +197,9 @@ TEST_F(TestEthernetInterface, addNameServers) fs::path filePath = confDir; filePath /= "00-bmc-test0.network"; config::Parser parser(filePath.string()); - auto values = parser.getValues("Network", "DNS"); + config::ReturnCode rc = config::ReturnCode::SUCCESS; + config::ValueList values; + std::tie(rc, values) = parser.getValues("Network", "DNS"); EXPECT_EQ(servers , values); validateResolvFile(values); @@ -210,7 +212,9 @@ TEST_F(TestEthernetInterface, addNTPServers) fs::path filePath = confDir; filePath /= "00-bmc-test0.network"; config::Parser parser(filePath.string()); - auto values = parser.getValues("Network", "NTP"); + config::ReturnCode rc = config::ReturnCode::SUCCESS; + config::ValueList values; + std::tie(rc, values) = parser.getValues("Network", "NTP"); EXPECT_EQ(servers , values); } diff --git a/test/test_vlan_interface.cpp b/test/test_vlan_interface.cpp index 9711a1a..00384f1 100644 --- a/test/test_vlan_interface.cpp +++ b/test/test_vlan_interface.cpp @@ -123,17 +123,20 @@ TEST_F(TestVlanInterface, createVLAN) filePath /= "test0.50.netdev"; config::Parser parser(filePath.string()); - auto values = parser.getValues("NetDev", "Name"); + config::ReturnCode rc = config::ReturnCode::SUCCESS; + config::ValueList values; + + std::tie(rc, values) = parser.getValues("NetDev", "Name"); std::string expectedValue = "test0.50"; bool found = isValueFound(values, expectedValue); EXPECT_EQ(found, true); - values = parser.getValues("NetDev", "Kind"); + std::tie(rc, values) = parser.getValues("NetDev", "Kind"); expectedValue = "vlan"; found = isValueFound(values, expectedValue); EXPECT_EQ(found, true); - values = parser.getValues("VLAN", "Id"); + std::tie(rc, values) = parser.getValues("VLAN", "Id"); expectedValue = "50"; found = isValueFound(values, expectedValue); EXPECT_EQ(found, true); @@ -163,17 +166,20 @@ TEST_F(TestVlanInterface, createMultipleVLAN) fs::path filePath = confDir; filePath /= "test0.50.netdev"; config::Parser parser(filePath.string()); - auto values = parser.getValues("NetDev", "Name"); + config::ReturnCode rc = config::ReturnCode::SUCCESS; + config::ValueList values; + + std::tie(rc, values) = parser.getValues("NetDev", "Name"); std::string expectedValue = "test0.50"; bool found = isValueFound(values, expectedValue); EXPECT_EQ(found, true); - values = parser.getValues("NetDev", "Kind"); + std::tie(rc, values) = parser.getValues("NetDev", "Kind"); expectedValue = "vlan"; found = isValueFound(values, expectedValue); EXPECT_EQ(found, true); - values = parser.getValues("VLAN", "Id"); + std::tie(rc, values) = parser.getValues("VLAN", "Id"); expectedValue = "50"; found = isValueFound(values, expectedValue); EXPECT_EQ(found, true); @@ -181,12 +187,12 @@ TEST_F(TestVlanInterface, createMultipleVLAN) filePath = confDir; filePath /= "test0.60.netdev"; parser.setFile(filePath.string()); - values = parser.getValues("NetDev", "Name"); + std::tie(rc, values) = parser.getValues("NetDev", "Name"); expectedValue = "test0.60"; found = isValueFound(values, expectedValue); EXPECT_EQ(found, true); - values = parser.getValues("VLAN", "Id"); + std::tie(rc, values) = parser.getValues("VLAN", "Id"); expectedValue = "60"; found = isValueFound(values, expectedValue); EXPECT_EQ(found, true); diff --git a/timer.cpp b/timer.cpp index afd4b8d..7052775 100644 --- a/timer.cpp +++ b/timer.cpp @@ -72,7 +72,6 @@ int Timer::timeoutHandler(sd_event_source* eventSource, timer->userCallBack(); } - log("Timer expired"); sd_event_source_set_enabled(eventSource, SD_EVENT_OFF); return 0; } diff --git a/util.cpp b/util.cpp index 5e606a7..4b234f8 100644 --- a/util.cpp +++ b/util.cpp @@ -411,19 +411,21 @@ bool getDHCPValue(const std::string& confDir, const std::string& intf) systemd::config::networkFileSuffix; confPath /= fileName; - try + auto rc = config::ReturnCode::SUCCESS; + config::ValueList values; + config::Parser parser(confPath.string()); + + std::tie(rc, values) = parser.getValues("Network", "DHCP"); + if (rc != config::ReturnCode::SUCCESS) { - config::Parser parser(confPath.string()); - auto values = parser.getValues("Network", "DHCP"); - // There will be only single value for DHCP key. - if (values[0] == "true") - { - dhcp = true; - } + log("Unable to get the value for Network[DHCP]", + entry("RC=%d", rc)); + return dhcp; } - catch (InternalFailure& e) + // There will be only single value for DHCP key. + if (values[0] == "true") { - log("Exception occurred during getting of DHCP value"); + dhcp = true; } return dhcp; } -- cgit v1.2.1