From a9ac326d83c45cef21798a31a5c744f82a1ed1bc Mon Sep 17 00:00:00 2001 From: Andrei Pavel Date: Wed, 24 Aug 2022 16:43:11 +0300 Subject: [PATCH] [#1955] don't revert logging on initial config fail --- src/bin/dhcp4/ctrl_dhcp4_srv.cc | 17 +++++++++-------- src/bin/dhcp4/main.cc | 10 +++++++++- src/bin/dhcp6/ctrl_dhcp6_srv.cc | 17 +++++++++-------- src/bin/dhcp6/main.cc | 10 +++++++++- src/lib/log/logger.cc | 5 +++++ src/lib/log/logger.h | 8 ++++++++ src/lib/log/logger_impl.cc | 31 ++++++++++++++++++++++++++++++- src/lib/log/logger_impl.h | 8 ++++++++ 8 files changed, 87 insertions(+), 19 deletions(-) diff --git a/src/bin/dhcp4/ctrl_dhcp4_srv.cc b/src/bin/dhcp4/ctrl_dhcp4_srv.cc index 6b219fc971..b068d3863e 100644 --- a/src/bin/dhcp4/ctrl_dhcp4_srv.cc +++ b/src/bin/dhcp4/ctrl_dhcp4_srv.cc @@ -414,18 +414,19 @@ ControlledDhcpv4Srv::commandConfigSetHandler(const string&, // Use new configuration. CfgMgr::instance().commit(); - } else { + } else if (CfgMgr::instance().getCurrentCfg()->getSequence() != 0) { // Ok, we applied the logging from the upcoming configuration, but // there were problems with the config. As such, we need to back off - // and revert to the previous logging configuration. + // and revert to the previous logging configuration. This is not done if + // sequence == 0, because that would mean always reverting to stdout by + // default, and it is arguably more helpful to have the error in a + // potential file or syslog configured in the upcoming configuration. CfgMgr::instance().getCurrentCfg()->applyLoggingCfg(); - if (CfgMgr::instance().getCurrentCfg()->getSequence() != 0) { - // Not initial configuration so someone can believe we reverted - // to the previous configuration. It is not the case so be clear - // about this. - LOG_FATAL(dhcp4_logger, DHCP4_CONFIG_UNRECOVERABLE_ERROR); - } + // Not initial configuration so someone can believe we reverted + // to the previous configuration. It is not the case so be clear + // about this. + LOG_FATAL(dhcp4_logger, DHCP4_CONFIG_UNRECOVERABLE_ERROR); } return (result); diff --git a/src/bin/dhcp4/main.cc b/src/bin/dhcp4/main.cc index 219fb53da1..7984c75d15 100644 --- a/src/bin/dhcp4/main.cc +++ b/src/bin/dhcp4/main.cc @@ -16,6 +16,7 @@ #include #include #include +#include #include #include @@ -240,8 +241,15 @@ main(int argc, char* argv[]) { server.init(config_file); } catch (const std::exception& ex) { + // Let's log out what went wrong. try { - // Let's log out what went wrong. + // Log with the current logger, but only if it's not + // configured with console output so as to not log twice. + if (!dhcp4_logger.hasAppender(isc::log::OutputOption::DEST_CONSOLE)) { + LOG_ERROR(dhcp4_logger, DHCP4_INIT_FAIL).arg(ex.what()); + } + + // Log on the console as well. isc::log::LoggerManager log_manager; log_manager.process(); LOG_ERROR(dhcp4_logger, DHCP4_INIT_FAIL).arg(ex.what()); diff --git a/src/bin/dhcp6/ctrl_dhcp6_srv.cc b/src/bin/dhcp6/ctrl_dhcp6_srv.cc index bdddcb7478..48e63333f7 100644 --- a/src/bin/dhcp6/ctrl_dhcp6_srv.cc +++ b/src/bin/dhcp6/ctrl_dhcp6_srv.cc @@ -417,18 +417,19 @@ ControlledDhcpv6Srv::commandConfigSetHandler(const string&, // Use new configuration. CfgMgr::instance().commit(); - } else { + } else if (CfgMgr::instance().getCurrentCfg()->getSequence() != 0) { // Ok, we applied the logging from the upcoming configuration, but // there were problems with the config. As such, we need to back off - // and revert to the previous logging configuration. + // and revert to the previous logging configuration. This is not done if + // sequence == 0, because that would mean always reverting to stdout by + // default, and it is arguably more helpful to have the error in a + // potential file or syslog configured in the upcoming configuration. CfgMgr::instance().getCurrentCfg()->applyLoggingCfg(); - if (CfgMgr::instance().getCurrentCfg()->getSequence() != 0) { - // Not initial configuration so someone can believe we reverted - // to the previous configuration. It is not the case so be clear - // about this. - LOG_FATAL(dhcp6_logger, DHCP6_CONFIG_UNRECOVERABLE_ERROR); - } + // Not initial configuration so someone can believe we reverted + // to the previous configuration. It is not the case so be clear + // about this. + LOG_FATAL(dhcp6_logger, DHCP6_CONFIG_UNRECOVERABLE_ERROR); } return (result); diff --git a/src/bin/dhcp6/main.cc b/src/bin/dhcp6/main.cc index e3e98431a6..7baa2b5c60 100644 --- a/src/bin/dhcp6/main.cc +++ b/src/bin/dhcp6/main.cc @@ -16,6 +16,7 @@ #include #include #include +#include #include #include @@ -240,8 +241,15 @@ main(int argc, char* argv[]) { server.init(config_file); } catch (const std::exception& ex) { + // Let's log out what went wrong. try { - // Let's log out what went wrong. + // Log with the current logger, but only if it's not + // configured with console output so as to not log twice. + if (!dhcp6_logger.hasAppender(isc::log::OutputOption::DEST_CONSOLE)) { + LOG_ERROR(dhcp6_logger, DHCP6_INIT_FAIL).arg(ex.what()); + } + + // Log on the console as well. isc::log::LoggerManager log_manager; log_manager.process(); LOG_ERROR(dhcp6_logger, DHCP6_INIT_FAIL).arg(ex.what()); diff --git a/src/lib/log/logger.cc b/src/lib/log/logger.cc index d0cd19609f..0ee4ef534d 100644 --- a/src/lib/log/logger.cc +++ b/src/lib/log/logger.cc @@ -204,6 +204,11 @@ Logger::setInterprocessSync(isc::log::interprocess::InterprocessSync* sync) { getLoggerPtr()->setInterprocessSync(sync); } +bool +Logger::hasAppender(OutputOption::Destination const destination) { + return getLoggerPtr()->hasAppender(destination); +} + // Comparison (testing only) bool diff --git a/src/lib/log/logger.h b/src/lib/log/logger.h index 5983b2450b..9654dbb798 100644 --- a/src/lib/log/logger.h +++ b/src/lib/log/logger.h @@ -17,6 +17,7 @@ #include #include #include +#include namespace isc { namespace log { @@ -294,6 +295,13 @@ public: /// a BadInterprocessSync exception is thrown. void setInterprocessSync(isc::log::interprocess::InterprocessSync* sync); + /// @brief Check if this logger has an appender of the given type. + /// + /// @param destination the appender type to be checked: console, file or syslog + /// + /// @return true if an appender of the given type is found, false otherwise + bool hasAppender(OutputOption::Destination const destination); + /// \brief Equality /// /// Check if two instances of this logger refer to the same stream. diff --git a/src/lib/log/logger_impl.cc b/src/lib/log/logger_impl.cc index 853cba056d..51cc4c15f9 100644 --- a/src/lib/log/logger_impl.cc +++ b/src/lib/log/logger_impl.cc @@ -20,9 +20,12 @@ #include #include -#include #include +#include +#include #include +#include +#include #include #include @@ -198,5 +201,31 @@ LoggerImpl::outputRaw(const Severity& severity, const string& message) { } } +bool +LoggerImpl::hasAppender(OutputOption::Destination const destination) { + // Get the appender for the name under which this logger is registered. + log4cplus::SharedAppenderPtrList appenders( + log4cplus::Logger::getInstance(name_).getAllAppenders()); + + // If there are no appenders, they might be under the root name. + if (appenders.size() == 0) { + appenders = log4cplus::Logger::getInstance(getRootLoggerName()).getAllAppenders(); + } + + for (log4cplus::helpers::SharedObjectPtr logger : appenders) { + if (destination == OutputOption::DEST_CONSOLE && + dynamic_cast(logger.get())) { + return true; + } else if (destination == OutputOption::DEST_FILE && + dynamic_cast(logger.get())) { + return true; + } else if (destination == OutputOption::DEST_SYSLOG && + dynamic_cast(logger.get())) { + return true; + } + } + return false; +} + } // namespace log } // namespace isc diff --git a/src/lib/log/logger_impl.h b/src/lib/log/logger_impl.h index b41bbcb16f..682ba91f63 100644 --- a/src/lib/log/logger_impl.h +++ b/src/lib/log/logger_impl.h @@ -26,6 +26,7 @@ #include #include #include +#include namespace isc { namespace log { @@ -177,6 +178,13 @@ public: /// If NULL is passed, a BadInterprocessSync exception is thrown. void setInterprocessSync(isc::log::interprocess::InterprocessSync* sync); + /// @brief Check if this logger has an appender of the given type. + /// + /// @param destination the appender type to be checked: console, file or syslog + /// + /// @return true if an appender of the given type is found, false otherwise + bool hasAppender(OutputOption::Destination const destination); + /// \brief Equality /// /// Check if two instances of this logger refer to the same stream.