From 6887a4a0efd7feb8fa31b46337ac170038bc0020 Mon Sep 17 00:00:00 2001 From: Simon Fels Date: Thu, 26 Jan 2017 09:19:28 +0100 Subject: [PATCH 1/5] Filter log messages by severity --- src/anbox/logger.cpp | 28 +++++++++++++--------------- src/anbox/logger.h | 3 +++ 2 files changed, 16 insertions(+), 15 deletions(-) diff --git a/src/anbox/logger.cpp b/src/anbox/logger.cpp index c2c2aed..bb92783 100644 --- a/src/anbox/logger.cpp +++ b/src/anbox/logger.cpp @@ -30,8 +30,7 @@ namespace { namespace attrs { -BOOST_LOG_ATTRIBUTE_KEYWORD(Severity, "anbox::Severity", - anbox::Logger::Severity) +BOOST_LOG_ATTRIBUTE_KEYWORD(Severity, "anbox::Severity", anbox::Logger::Severity) BOOST_LOG_ATTRIBUTE_KEYWORD(Location, "Location", anbox::Logger::Location) BOOST_LOG_ATTRIBUTE_KEYWORD(Timestamp, "Timestamp", boost::posix_time::ptime) } @@ -39,8 +38,7 @@ BOOST_LOG_ATTRIBUTE_KEYWORD(Timestamp, "Timestamp", boost::posix_time::ptime) struct BoostLogLogger : public anbox::Logger { BoostLogLogger() : initialized_(false) {} - void Init(const anbox::Logger::Severity& severity = - anbox::Logger::Severity::kWarning) override { + void Init(const anbox::Logger::Severity& severity = anbox::Logger::Severity::kWarning) override { if (initialized_) return; boost::log::formatter formatter = @@ -58,25 +56,24 @@ struct BoostLogLogger : public anbox::Logger { auto logger = boost::log::add_console_log(std::cout); logger->set_formatter(formatter); - // FIXME need to enable this once we found how we wrap this - // properly into our service architecture. For now left as - // it is. - boost::ignore_unused_variable_warning(severity); - // logger->set_filter(attrs::Severity < severity); - + severity_ = severity; initialized_ = true; } - void Log(Severity severity, const std::string& message, - const boost::optional& loc) { + void Log(Severity severity, const std::string& message, const boost::optional& loc) override { if (!initialized_) Init(); + // FIXME somehow set_filter doesn't work with the trivial logger. If + // we set a filter based on the severity attribute open_record will + // not return a new record. Because of that we do a poor man filtering + // here until we have a proper way to do this via boost. + if (severity < severity_) + return; + if (auto rec = boost::log::trivial::logger::get().open_record()) { boost::log::record_ostream out{rec}; out << boost::log::add_value(attrs::Severity, severity) - << boost::log::add_value( - attrs::Timestamp, - boost::posix_time::microsec_clock::universal_time()) + << boost::log::add_value(attrs::Timestamp, boost::posix_time::microsec_clock::universal_time()) << message; if (loc) { @@ -91,6 +88,7 @@ struct BoostLogLogger : public anbox::Logger { } private: + Severity severity_; bool initialized_; }; diff --git a/src/anbox/logger.h b/src/anbox/logger.h index 8f72064..7a177eb 100644 --- a/src/anbox/logger.h +++ b/src/anbox/logger.h @@ -49,6 +49,9 @@ class Logger : public DoNotCopyOrMove { virtual void Init(const Severity& severity = Severity::kWarning) = 0; + void SetSeverity(const std::string &severity); + virtual void SetSeverity(const Severity& severity) = 0; + virtual void Log(Severity severity, const std::string& message, const boost::optional& location) = 0; From ddc022466e868ee6fc2265185341440b51634eab Mon Sep 17 00:00:00 2001 From: Simon Fels Date: Thu, 26 Jan 2017 09:19:48 +0100 Subject: [PATCH 2/5] Set log level on startup from environment variable --- src/anbox/daemon.cpp | 6 ++++++ src/anbox/logger.cpp | 19 +++++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/src/anbox/daemon.cpp b/src/anbox/daemon.cpp index 248a70c..d246d7c 100644 --- a/src/anbox/daemon.cpp +++ b/src/anbox/daemon.cpp @@ -39,6 +39,12 @@ Daemon::Daemon() .command(std::make_shared()) .command(std::make_shared()) .command(std::make_shared()); + + Log().Init(anbox::Logger::Severity::kWarning); + + const auto log_level = utils::get_env_value("ANBOX_LOG_LEVEL", ""); + if (!log_level.empty()) + Log().SetSeverity(log_level); } int Daemon::Run(const std::vector &arguments) try { diff --git a/src/anbox/logger.cpp b/src/anbox/logger.cpp index bb92783..8dd7866 100644 --- a/src/anbox/logger.cpp +++ b/src/anbox/logger.cpp @@ -60,6 +60,10 @@ struct BoostLogLogger : public anbox::Logger { initialized_ = true; } + void SetSeverity(const Severity& severity) override { + severity_ = severity; + } + void Log(Severity severity, const std::string& message, const boost::optional& loc) override { if (!initialized_) Init(); @@ -103,6 +107,21 @@ void SetInstance(const std::shared_ptr& logger) { } namespace anbox { +void Logger::SetSeverity(const std::string& severity) { + if (severity == "trace") + SetSeverity(Severity::kTrace); + else if (severity == "debug") + SetSeverity(Severity::kDebug); + else if (severity == "info") + SetSeverity(Severity::kInfo); + else if (severity == "warning") + SetSeverity(Severity::kWarning); + else if (severity == "error") + SetSeverity(Severity::kError); + else if (severity == "fatal") + SetSeverity(Severity::kFatal); +} + void Logger::Trace(const std::string& message, const boost::optional& location) { Log(Severity::kTrace, message, location); From 1b1b2c35a113245248aa2d63723f596c15a3c94b Mon Sep 17 00:00:00 2001 From: Simon Fels Date: Thu, 26 Jan 2017 09:21:53 +0100 Subject: [PATCH 3/5] Pass log levels from emugl layer --- .../host/tools/emugen/ApiGen.cpp | 9 ++----- .../ChecksumCalculatorThreadInfo.cpp | 2 +- .../shared/emugl/common/crash_reporter.cpp | 6 ++--- .../shared/emugl/common/crash_reporter.h | 6 ++--- .../shared/emugl/common/logging.cpp | 2 +- .../shared/emugl/common/logging.h | 18 +++++++++++++- src/anbox/graphics/emugl/RenderApi.cpp | 3 +-- src/anbox/graphics/emugl/RenderApi.h | 9 ++++--- src/anbox/graphics/gl_renderer_server.cpp | 24 +++++++++++++++++-- 9 files changed, 54 insertions(+), 25 deletions(-) diff --git a/external/android-emugl/host/tools/emugen/ApiGen.cpp b/external/android-emugl/host/tools/emugen/ApiGen.cpp index 7e87c07..c7713d2 100644 --- a/external/android-emugl/host/tools/emugen/ApiGen.cpp +++ b/external/android-emugl/host/tools/emugen/ApiGen.cpp @@ -823,12 +823,7 @@ int ApiGen::genDecoderImpl(const std::string &filename) fprintf(fp, "typedef unsigned int tsize_t; // Target \"size_t\", which is 32-bit for now. It may or may not be the same as host's size_t when emugen is compiled.\n\n"); // helper macros - fprintf(fp, - "#ifdef OPENGL_DEBUG_PRINTOUT\n" - "# define DEBUG(...) do { if (emugl_cxt_logger) { emugl_cxt_logger(__VA_ARGS__); } } while(0)\n" - "#else\n" - "# define DEBUG(...) ((void)0)\n" - "#endif\n\n"); + fprintf(fp, "# define DEBUG(...) do { if (emugl_cxt_logger) { emugl_cxt_logger(LogLevel::TRACE, __VA_ARGS__); } } while(0)\n\n"); fprintf(fp, "#ifdef CHECK_GLERROR\n" @@ -913,7 +908,7 @@ int ApiGen::genDecoderImpl(const std::string &filename) } } else if (pass == PASS_DebugPrint) { fprintf(fp, - "\t\t\tDEBUG(\"%s(%%p): %s(%s)\\n\", stream", + "\t\t\tDEBUG(\"%s(%%p): %s(%s)\", stream", m_basename.c_str(), e->name().c_str(), printString.c_str()); diff --git a/external/android-emugl/shared/OpenglCodecCommon/ChecksumCalculatorThreadInfo.cpp b/external/android-emugl/shared/OpenglCodecCommon/ChecksumCalculatorThreadInfo.cpp index 6a35c92..aedfea1 100644 --- a/external/android-emugl/shared/OpenglCodecCommon/ChecksumCalculatorThreadInfo.cpp +++ b/external/android-emugl/shared/OpenglCodecCommon/ChecksumCalculatorThreadInfo.cpp @@ -98,6 +98,6 @@ void ChecksumCalculatorThreadInfo::validOrDie(void* buf, // We should actually call crashhandler_die(message), but I don't think we // can link to that library from here if (!validate(buf, bufLen, checksum, checksumLen)) { - emugl_crash_reporter(message); + emugl_crash_reporter(emugl::LogLevel::FATAL, message); } } diff --git a/external/android-emugl/shared/emugl/common/crash_reporter.cpp b/external/android-emugl/shared/emugl/common/crash_reporter.cpp index b4e0267..2df5d1b 100644 --- a/external/android-emugl/shared/emugl/common/crash_reporter.cpp +++ b/external/android-emugl/shared/emugl/common/crash_reporter.cpp @@ -18,13 +18,13 @@ #include -void default_crash_reporter(const char* format, ...) { +void default_crash_reporter(const emugl::LogLevel &level, const char* format, ...) { abort(); } -crash_reporter_t emugl_crash_reporter = default_crash_reporter; +logger_t emugl_crash_reporter = default_crash_reporter; -void set_emugl_crash_reporter(crash_reporter_t crash_reporter) { +void set_emugl_crash_reporter(logger_t crash_reporter) { if (crash_reporter) { emugl_crash_reporter = crash_reporter; } else { diff --git a/external/android-emugl/shared/emugl/common/crash_reporter.h b/external/android-emugl/shared/emugl/common/crash_reporter.h index 81bb21a..3f189dd 100644 --- a/external/android-emugl/shared/emugl/common/crash_reporter.h +++ b/external/android-emugl/shared/emugl/common/crash_reporter.h @@ -16,7 +16,7 @@ #pragma once -typedef void (*crash_reporter_t)(const char* format, ...); +#include "emugl/common/logging.h" -extern crash_reporter_t emugl_crash_reporter; -void set_emugl_crash_reporter(crash_reporter_t crash_reporter); +extern logger_t emugl_crash_reporter; +void set_emugl_crash_reporter(logger_t crash_reporter); diff --git a/external/android-emugl/shared/emugl/common/logging.cpp b/external/android-emugl/shared/emugl/common/logging.cpp index 1f40ea8..69a24da 100644 --- a/external/android-emugl/shared/emugl/common/logging.cpp +++ b/external/android-emugl/shared/emugl/common/logging.cpp @@ -16,7 +16,7 @@ #include "emugl/common/logging.h" -void default_logger(const char* fmt, ...) { } +void default_logger(const emugl::LogLevel &level, const char* fmt, ...) { } logger_t emugl_logger = default_logger; logger_t emugl_cxt_logger = default_logger; diff --git a/external/android-emugl/shared/emugl/common/logging.h b/external/android-emugl/shared/emugl/common/logging.h index d8b00bd..2fc1f15 100644 --- a/external/android-emugl/shared/emugl/common/logging.h +++ b/external/android-emugl/shared/emugl/common/logging.h @@ -14,10 +14,24 @@ * limitations under the License. */ +#ifndef EMUGL_COMMON_LOGGING_H_ +#define EMUGL_COMMON_LOGGING_H_ + #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wredundant-decls" -typedef void (*logger_t)(const char* fmt, ...); +namespace emugl { +enum class LogLevel { + TRACE, + DEBUG, + INFO, + WARNING, + ERROR, + FATAL, +}; +} // namespace emugl + +typedef void (*logger_t)(const emugl::LogLevel &level, const char* fmt, ...); extern logger_t emugl_logger; extern logger_t emugl_cxt_logger; void set_emugl_logger(logger_t f); @@ -40,3 +54,5 @@ void set_emugl_cxt_logger(logger_t f); #endif #pragma GCC diagnostic pop + +#endif diff --git a/src/anbox/graphics/emugl/RenderApi.cpp b/src/anbox/graphics/emugl/RenderApi.cpp index 93f343f..934be25 100644 --- a/src/anbox/graphics/emugl/RenderApi.cpp +++ b/src/anbox/graphics/emugl/RenderApi.cpp @@ -23,7 +23,6 @@ #include "OpenGLESDispatch/GLESv2Dispatch.h" #include "emugl/common/crash_reporter.h" -#include "emugl/common/logging.h" #include @@ -50,7 +49,7 @@ std::vector default_gl_libraries(bool no_glesv1) { }; } -bool initialize(const std::vector &libs, emugl_logger_struct log_funcs, emugl_crash_func_t crash_func) { +bool initialize(const std::vector &libs, emugl_logger_struct log_funcs, logger_t crash_func) { set_emugl_crash_reporter(crash_func); set_emugl_logger(log_funcs.coarse); set_emugl_cxt_logger(log_funcs.fine); diff --git a/src/anbox/graphics/emugl/RenderApi.h b/src/anbox/graphics/emugl/RenderApi.h index 7d1e93d..61c2c28 100644 --- a/src/anbox/graphics/emugl/RenderApi.h +++ b/src/anbox/graphics/emugl/RenderApi.h @@ -21,12 +21,11 @@ #include -typedef void (*emugl_logger_func_t)(const char* fmt, ...); -typedef void (*emugl_crash_func_t)(const char* format, ...); +#include "emugl/common/logging.h" typedef struct { - emugl_logger_func_t coarse; - emugl_logger_func_t fine; + logger_t coarse; + logger_t fine; } emugl_logger_struct; namespace anbox { @@ -40,7 +39,7 @@ struct GLLibrary { std::vector default_gl_libraries(bool no_glesv1 = false); -bool initialize(const std::vector &libs, emugl_logger_struct log_funcs, emugl_crash_func_t crash_func); +bool initialize(const std::vector &libs, emugl_logger_struct log_funcs, logger_t crash_func); } // namespace emugl } // namespace graphics } // namespace anbox diff --git a/src/anbox/graphics/gl_renderer_server.cpp b/src/anbox/graphics/gl_renderer_server.cpp index f8337d4..4da77f5 100644 --- a/src/anbox/graphics/gl_renderer_server.cpp +++ b/src/anbox/graphics/gl_renderer_server.cpp @@ -29,7 +29,9 @@ #include namespace { -void logger_write(const char *format, ...) { +void logger_write(const emugl::LogLevel &level, const char *format, ...) { + (void)level; + char message[2048]; va_list args; @@ -37,7 +39,25 @@ void logger_write(const char *format, ...) { vsnprintf(message, sizeof(message) - 1, format, args); va_end(args); - DEBUG("%s", message); + switch (level) { + case emugl::LogLevel::WARNING: + WARNING("%s", message); + break; + case emugl::LogLevel::ERROR: + ERROR("%s", message); + break; + case emugl::LogLevel::FATAL: + FATAL("%s", message); + break; + case emugl::LogLevel::DEBUG: + DEBUG("%s", message); + break; + case emugl::LogLevel::TRACE: + TRACE("%s", message); + break; + default: + break; + } } } From 9646a5fbc33d6b0a9fd2e7237af18bcce087dc85 Mon Sep 17 00:00:00 2001 From: Simon Fels Date: Wed, 22 Feb 2017 17:05:37 +0100 Subject: [PATCH 4/5] Remove unneeded log messages from translator libraries --- .../android-emugl/host/libs/Translator/GLES_CM/GLEScmImp.cpp | 1 - .../android-emugl/host/libs/Translator/GLES_V2/GLESv2Imp.cpp | 1 - 2 files changed, 2 deletions(-) diff --git a/external/android-emugl/host/libs/Translator/GLES_CM/GLEScmImp.cpp b/external/android-emugl/host/libs/Translator/GLES_CM/GLEScmImp.cpp index c9b2c22..2c1dea2 100644 --- a/external/android-emugl/host/libs/Translator/GLES_CM/GLEScmImp.cpp +++ b/external/android-emugl/host/libs/Translator/GLES_CM/GLEScmImp.cpp @@ -71,7 +71,6 @@ static GLESiface s_glesIface = { extern "C" { static void initGLESx() { - DBG("No special initialization necessary for GLES_CM\n"); return; } diff --git a/external/android-emugl/host/libs/Translator/GLES_V2/GLESv2Imp.cpp b/external/android-emugl/host/libs/Translator/GLES_V2/GLESv2Imp.cpp index 0e73b9d..c49d4b7 100644 --- a/external/android-emugl/host/libs/Translator/GLES_V2/GLESv2Imp.cpp +++ b/external/android-emugl/host/libs/Translator/GLES_V2/GLESv2Imp.cpp @@ -70,7 +70,6 @@ static GLESiface s_glesIface = { extern "C" { static void initGLESx() { - DBG("No special initialization necessary for GLES_V2\n"); return; } From ef5cdf172598dd58c99d0d01dd2d35df3cbdfd5e Mon Sep 17 00:00:00 2001 From: Simon Fels Date: Fri, 24 Feb 2017 19:07:53 +0100 Subject: [PATCH 5/5] Rename SetSeverity to SetSeverityFromString and check for valid severity --- src/anbox/daemon.cpp | 4 ++-- src/anbox/logger.cpp | 5 ++++- src/anbox/logger.h | 2 +- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/anbox/daemon.cpp b/src/anbox/daemon.cpp index d246d7c..6bb4e84 100644 --- a/src/anbox/daemon.cpp +++ b/src/anbox/daemon.cpp @@ -43,8 +43,8 @@ Daemon::Daemon() Log().Init(anbox::Logger::Severity::kWarning); const auto log_level = utils::get_env_value("ANBOX_LOG_LEVEL", ""); - if (!log_level.empty()) - Log().SetSeverity(log_level); + if (!log_level.empty() || !Log().SetSeverityFromString(log_level)) + WARNING("Failed to set logging severity to '%s'", log_level); } int Daemon::Run(const std::vector &arguments) try { diff --git a/src/anbox/logger.cpp b/src/anbox/logger.cpp index 8dd7866..f59803d 100644 --- a/src/anbox/logger.cpp +++ b/src/anbox/logger.cpp @@ -107,7 +107,7 @@ void SetInstance(const std::shared_ptr& logger) { } namespace anbox { -void Logger::SetSeverity(const std::string& severity) { +bool Logger::SetSeverityFromString(const std::string& severity) { if (severity == "trace") SetSeverity(Severity::kTrace); else if (severity == "debug") @@ -120,6 +120,9 @@ void Logger::SetSeverity(const std::string& severity) { SetSeverity(Severity::kError); else if (severity == "fatal") SetSeverity(Severity::kFatal); + else + return false; + return true; } void Logger::Trace(const std::string& message, diff --git a/src/anbox/logger.h b/src/anbox/logger.h index 7a177eb..6e797b6 100644 --- a/src/anbox/logger.h +++ b/src/anbox/logger.h @@ -49,7 +49,7 @@ class Logger : public DoNotCopyOrMove { virtual void Init(const Severity& severity = Severity::kWarning) = 0; - void SetSeverity(const std::string &severity); + bool SetSeverityFromString(const std::string &severity); virtual void SetSeverity(const Severity& severity) = 0; virtual void Log(Severity severity, const std::string& message,