From 3187dbf0aa0943d9eca0c5c1259f8a8ca549709b Mon Sep 17 00:00:00 2001 From: tycho Date: Sun, 30 Aug 2015 22:57:43 +0100 Subject: Increase robustness of the logging subsystem --- Tools/MCADefrag/MCADefrag.cpp | 20 +++---- Tools/ProtoProxy/ProtoProxy.cpp | 14 +++-- src/CompositeChat.h | 1 + src/Globals.h | 20 ++++++- src/Logger.cpp | 24 ++++++-- src/Logger.h | 59 +++++++++++--------- src/LoggerListeners.cpp | 120 ++++++++++++++++++++++++---------------- src/LoggerListeners.h | 26 +-------- src/Root.cpp | 19 ++++--- src/World.cpp | 6 -- src/main.cpp | 1 + 11 files changed, 175 insertions(+), 135 deletions(-) diff --git a/Tools/MCADefrag/MCADefrag.cpp b/Tools/MCADefrag/MCADefrag.cpp index 80c6f5be2..cf1db85d5 100644 --- a/Tools/MCADefrag/MCADefrag.cpp +++ b/Tools/MCADefrag/MCADefrag.cpp @@ -22,26 +22,26 @@ static const Byte g_Zeroes[4096] = {0}; int main(int argc, char ** argv) { - cLogger::cListener * consoleLogListener = MakeConsoleListener(false); - cLogger::cListener * fileLogListener = new cFileListener(); - cLogger::GetInstance().AttachListener(consoleLogListener); - cLogger::GetInstance().AttachListener(fileLogListener); + auto consoleLogListener = MakeConsoleListener(false); + auto consoleAttachment = cLogger::GetInstance().AttachListener(std::move(consoleLogListener)); + auto fileLogListenerRet = MakeFileListener(); + if (!fileLogListenerRet.first) + { + LOGERROR("Failed to open log file, aborting"); + return EXIT_FAILURE; + } + auto fileAttachment = cLogger::GetInstance().AttachListener(std::move(fileLogListenerRet.second)); cLogger::InitiateMultithreading(); cMCADefrag Defrag; if (!Defrag.Init(argc, argv)) { - return 1; + return EXIT_FAILURE; } Defrag.Run(); - cLogger::GetInstance().DetachListener(consoleLogListener); - delete consoleLogListener; - cLogger::GetInstance().DetachListener(fileLogListener); - delete fileLogListener; - return 0; } diff --git a/Tools/ProtoProxy/ProtoProxy.cpp b/Tools/ProtoProxy/ProtoProxy.cpp index 2d27d7556..06a486778 100644 --- a/Tools/ProtoProxy/ProtoProxy.cpp +++ b/Tools/ProtoProxy/ProtoProxy.cpp @@ -15,11 +15,17 @@ int main(int argc, char ** argv) { // Initialize logging subsystem: - cLogger::InitiateMultithreading(); auto consoleLogListener = MakeConsoleListener(false); - auto fileLogListener = new cFileListener(); - cLogger::GetInstance().AttachListener(consoleLogListener); - cLogger::GetInstance().AttachListener(fileLogListener); + auto consoleAttachment = cLogger::GetInstance().AttachListener(std::move(consoleLogListener)); + auto fileLogListenerRet = MakeFileListener(); + if (!fileLogListenerRet.first) + { + LOGERROR("Failed to open log file, aborting"); + return EXIT_FAILURE; + } + auto fileAttachment = cLogger::GetInstance().AttachListener(std::move(fileLogListenerRet.second)); + + cLogger::InitiateMultithreading(); int ListenPort = (argc > 1) ? atoi(argv[1]) : 25564; int ConnectPort = (argc > 2) ? atoi(argv[2]) : 25565; diff --git a/src/CompositeChat.h b/src/CompositeChat.h index 369eed196..becf0a91e 100644 --- a/src/CompositeChat.h +++ b/src/CompositeChat.h @@ -5,6 +5,7 @@ #include "Defines.h" #include "json/json.h" +#include "Logger.h" diff --git a/src/Globals.h b/src/Globals.h index 1afcc928c..a69a64452 100644 --- a/src/Globals.h +++ b/src/Globals.h @@ -271,7 +271,25 @@ template class SizeChecker; #include "OSSupport/StackTrace.h" #ifndef TEST_GLOBALS - #include "Logger.h" + +// These fiunctions are defined in Logger.cpp, but are declared here to avoid including all of logger.h +extern void LOG (const char * a_Format, ...) FORMATSTRING(1, 2); +extern void LOGINFO (const char * a_Format, ...) FORMATSTRING(1, 2); +extern void LOGWARNING(const char * a_Format, ...) FORMATSTRING(1, 2); +extern void LOGERROR (const char * a_Format, ...) FORMATSTRING(1, 2); + + + + + +// In debug builds, translate LOGD to LOG, otherwise leave it out altogether: +#ifdef _DEBUG + #define LOGD LOG +#else + #define LOGD(...) +#endif // _DEBUG + +#define LOGWARN LOGWARNING #else // Logging functions void inline LOGERROR(const char * a_Format, ...) FORMATSTRING(1, 2); diff --git a/src/Logger.cpp b/src/Logger.cpp index 292622a46..60f5a88d2 100644 --- a/src/Logger.cpp +++ b/src/Logger.cpp @@ -1,5 +1,6 @@ #include "Globals.h" // NOTE: MSVC stupidness requires this to be the same across all modules +#include "Logger.h" #include "OSSupport/IsThread.h" #ifdef _WIN32 @@ -73,10 +74,14 @@ void cLogger::Log(const char * a_Format, eLogLevel a_LogLevel, va_list a_ArgList -void cLogger::AttachListener(cListener * a_Listener) +cLogger::cAttachment cLogger::AttachListener(std::unique_ptr a_Listener) { - cCSLock Lock(m_CriticalSection); - m_LogListeners.push_back(a_Listener); + auto nonOwning = a_Listener.get(); + { + cCSLock Lock(m_CriticalSection); + m_LogListeners.push_back(std::move(a_Listener)); + } + return cAttachment{nonOwning}; } @@ -86,7 +91,16 @@ void cLogger::AttachListener(cListener * a_Listener) void cLogger::DetachListener(cListener * a_Listener) { cCSLock Lock(m_CriticalSection); - m_LogListeners.erase(std::remove(m_LogListeners.begin(), m_LogListeners.end(), a_Listener)); + m_LogListeners.erase( + std::remove_if( + m_LogListeners.begin(), + m_LogListeners.end(), + [=](std::unique_ptr & a_OtherListener) -> bool + { + return a_OtherListener.get() == a_Listener; + } + ) + ); } @@ -120,7 +134,7 @@ void LOGINFO(const char * a_Format, ...) -void LOGWARN(const char * a_Format, ...) +void LOGWARNING(const char * a_Format, ...) { va_list argList; va_start(argList, a_Format); diff --git a/src/Logger.h b/src/Logger.h index 176c6e810..a0c2917a9 100644 --- a/src/Logger.h +++ b/src/Logger.h @@ -23,13 +23,35 @@ public: virtual ~cListener(){} }; + class cAttachment + { + public: + + cAttachment(cAttachment && a_other) + : m_listener(a_other.m_listener) + { + } + + ~cAttachment() + { + cLogger::GetInstance().DetachListener(m_listener); + } + + private: + + cListener * m_listener; + + friend class cLogger; + + cAttachment(cListener * a_listener) : m_listener(a_listener) {} + }; + void Log (const char * a_Format, eLogLevel a_LogLevel, va_list a_ArgList) FORMATSTRING(2, 0); /** Logs the simple text message at the specified log level. */ void LogSimple(AString a_Message, eLogLevel a_LogLevel = llRegular); - void AttachListener(cListener * a_Listener); - void DetachListener(cListener * a_Listener); + cAttachment AttachListener(std::unique_ptr a_Listener); static cLogger & GetInstance(void); // Must be called before calling GetInstance in a multithreaded context @@ -37,37 +59,20 @@ public: private: cCriticalSection m_CriticalSection; - std::vector m_LogListeners; - -}; - - - - - - -extern void LOG (const char * a_Format, ...) FORMATSTRING(1, 2); -extern void LOGINFO (const char * a_Format, ...) FORMATSTRING(1, 2); -extern void LOGWARN (const char * a_Format, ...) FORMATSTRING(1, 2); -extern void LOGERROR(const char * a_Format, ...) FORMATSTRING(1, 2); - - - - - -// In debug builds, translate LOGD to LOG, otherwise leave it out altogether: -#ifdef _DEBUG - #define LOGD LOG -#else - #define LOGD(...) -#endif // _DEBUG + std::vector> m_LogListeners; + void DetachListener(cListener * a_Listener); +}; -#define LOGWARNING LOGWARN +// These declarations are duplicated in globals.h +extern void LOG (const char * a_Format, ...) FORMATSTRING(1, 2); +extern void LOGINFO (const char * a_Format, ...) FORMATSTRING(1, 2); +extern void LOGWARNING(const char * a_Format, ...) FORMATSTRING(1, 2); +extern void LOGERROR (const char * a_Format, ...) FORMATSTRING(1, 2); diff --git a/src/LoggerListeners.cpp b/src/LoggerListeners.cpp index ba8139678..3b6ed2147 100644 --- a/src/LoggerListeners.cpp +++ b/src/LoggerListeners.cpp @@ -251,11 +251,11 @@ class cNullConsoleListener -cLogger::cListener * MakeConsoleListener(bool a_IsService) +std::unique_ptr MakeConsoleListener(bool a_IsService) { if (a_IsService) { - return new cNullConsoleListener; + return cpp14::make_unique(); } #ifdef _WIN32 @@ -267,25 +267,25 @@ cLogger::cListener * MakeConsoleListener(bool a_IsService) HANDLE Console = GetStdHandle(STD_OUTPUT_HANDLE); GetConsoleScreenBufferInfo(Console, &sbi); WORD DefaultConsoleAttrib = sbi.wAttributes; - return new cWindowsConsoleListener(Console, DefaultConsoleAttrib); + return cpp14::make_unique(Console, DefaultConsoleAttrib); } else { - return new cVanillaCPPConsoleListener; + return cpp14::make_unique(); } #elif defined (__linux) && !defined(ANDROID_NDK) // TODO: lookup terminal in terminfo if (isatty(fileno(stdout))) { - return new cLinuxConsoleListener(); + return cpp14::make_unique(); } else { - return new cVanillaCPPConsoleListener(); + return cpp14::make_unique(); } #else - return new cVanillaCPPConsoleListener(); + return cpp14::make_unique(); #endif } @@ -296,60 +296,82 @@ cLogger::cListener * MakeConsoleListener(bool a_IsService) //////////////////////////////////////////////////////////////////////////////// // cFileListener: -cFileListener::cFileListener(void) +class cFileListener + : public cLogger::cListener { - cFile::CreateFolder(FILE_IO_PREFIX + AString("logs")); - m_File.Open( - FILE_IO_PREFIX + Printf( - "logs/LOG_%d.txt", - std::chrono::duration_cast>>( - std::chrono::system_clock::now().time_since_epoch() - ).count() - ), - cFile::fmAppend - ); -} - - +public: + cFileListener(void) {} + bool Open() + { + // Assume creation succeeds, as the API does not provide a way to tell if the folder exists. + cFile::CreateFolder(FILE_IO_PREFIX + AString("logs")); + bool success = m_File.Open( + FILE_IO_PREFIX + Printf( + "logs/LOG_%d.txt", + std::chrono::duration_cast>>( + std::chrono::system_clock::now().time_since_epoch() + ).count() + ), + cFile::fmAppend + ); + return success; + } -void cFileListener::Log(AString a_Message, cLogger::eLogLevel a_LogLevel) -{ - const char * LogLevelPrefix = "Unkn "; - bool ShouldFlush = false; - switch (a_LogLevel) + virtual void Log(AString a_Message, cLogger::eLogLevel a_LogLevel) override { - case cLogger::llRegular: - { - LogLevelPrefix = " "; - break; - } - case cLogger::llInfo: - { - LogLevelPrefix = "Info "; - break; - } - case cLogger::llWarning: + const char * LogLevelPrefix = "Unkn "; + bool ShouldFlush = false; + switch (a_LogLevel) { - LogLevelPrefix = "Warn "; - ShouldFlush = true; - break; + case cLogger::llRegular: + { + LogLevelPrefix = " "; + break; + } + case cLogger::llInfo: + { + LogLevelPrefix = "Info "; + break; + } + case cLogger::llWarning: + { + LogLevelPrefix = "Warn "; + ShouldFlush = true; + break; + } + case cLogger::llError: + { + LogLevelPrefix = "Err "; + ShouldFlush = true; + break; + } } - case cLogger::llError: + m_File.Printf("%s%s", LogLevelPrefix, a_Message.c_str()); + if (ShouldFlush) { - LogLevelPrefix = "Err "; - ShouldFlush = true; - break; + m_File.Flush(); } } - m_File.Printf("%s%s", LogLevelPrefix, a_Message.c_str()); - if (ShouldFlush) + +private: + + cFile m_File; +}; + + + + + +std::pair> MakeFileListener() +{ + auto listener = cpp14::make_unique(); + if (!listener->Open()) { - m_File.Flush(); + return {false, nullptr}; } + return {true, std::move(listener)}; } - - diff --git a/src/LoggerListeners.h b/src/LoggerListeners.h index a7f9a35a5..3d5628caa 100644 --- a/src/LoggerListeners.h +++ b/src/LoggerListeners.h @@ -2,30 +2,8 @@ #include "Logger.h" #include "OSSupport/File.h" - - - - -class cFileListener - : public cLogger::cListener -{ -public: - - cFileListener(); - cFileListener(AString a_Filename); - - virtual void Log(AString a_Message, cLogger::eLogLevel a_LogLevel) override; - -private: - - cFile m_File; -}; - - - - - -cLogger::cListener * MakeConsoleListener(bool a_IsService); +std::unique_ptr MakeConsoleListener(bool a_IsService); +std::pair> MakeFileListener(); diff --git a/src/Root.cpp b/src/Root.cpp index dc90671fb..13166d883 100644 --- a/src/Root.cpp +++ b/src/Root.cpp @@ -22,6 +22,7 @@ #include "SettingsRepositoryInterface.h" #include "OverridesSettingsRepository.h" #include "SelfTests.h" +#include "Logger.h" #include @@ -107,10 +108,15 @@ void cRoot::Start(std::unique_ptr a_OverridesRepo) EnableMenuItem(ConsoleMenu, SC_CLOSE, MF_GRAYED); // Disable close button when starting up; it causes problems with our CTRL-CLOSE handling #endif - cLogger::cListener * consoleLogListener = MakeConsoleListener(m_RunAsService); - cLogger::cListener * fileLogListener = new cFileListener(); - cLogger::GetInstance().AttachListener(consoleLogListener); - cLogger::GetInstance().AttachListener(fileLogListener); + auto consoleLogListener = MakeConsoleListener(m_RunAsService); + auto consoleAttachment = cLogger::GetInstance().AttachListener(std::move(consoleLogListener)); + auto fileLogListenerRet = MakeFileListener(); + if (!fileLogListenerRet.first) + { + LOGERROR("Failed to open log file, aborting"); + return; + } + auto fileAttachment = cLogger::GetInstance().AttachListener(std::move(fileLogListenerRet.second)); LOG("--- Started Log ---"); @@ -317,11 +323,6 @@ void cRoot::Start(std::unique_ptr a_OverridesRepo) LOG("Shutdown successful - restarting..."); } LOG("--- Stopped Log ---"); - - cLogger::GetInstance().DetachListener(consoleLogListener); - delete consoleLogListener; - cLogger::GetInstance().DetachListener(fileLogListener); - delete fileLogListener; } diff --git a/src/World.cpp b/src/World.cpp index f929e2d1a..eb96eb57a 100644 --- a/src/World.cpp +++ b/src/World.cpp @@ -255,8 +255,6 @@ int cWorld::GetDefaultWeatherInterval(eWeather a_Weather) return 2400 + (m_TickRand.randInt() % 4800); // 2 - 6 minutes } } - LOGWARNING("%s: Missing default weather interval for weather %d.", __FUNCTION__, a_Weather); - return -1; } @@ -646,10 +644,6 @@ eWeather cWorld::ChooseNewWeather() return ((m_TickRand.randInt() % 256) < 32) ? eWeather_ThunderStorm : eWeather_Sunny; } } - - LOGWARNING("Unknown current weather: %d. Setting sunny.", m_Weather); - ASSERT(!"Unknown weather"); - return eWeather_Sunny; } diff --git a/src/main.cpp b/src/main.cpp index bc2439616..34285c767 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -14,6 +14,7 @@ #include "OSSupport/NetworkSingleton.h" #include "BuildInfo.h" +#include "Logger.h" #include "MemorySettingsRepository.h" -- cgit v1.2.3