Hi Hans, Please check my response with [Minh]
Thanks Minh On 18/04/18 22:40, Hans Nordeback wrote:
[Minh] Changing Log() and Init() not to be static because I am not using singleton any more. Before this ticket, we have 2 singleton, one for TraceLog and one for MdsLog, they are mostly looking similar, so don't want to make a third singleton for new osaf log.Hi Minh, ack, code review only. Some comments below. /Thanks HansN On 04/12/2018 01:12 AM, Minh Chau wrote:[HansN] instead of static global use unnamed namespaces instead. Also try to avoid globals, why change Log and Init fromUnify TraceLog and MdsLog class to one class (TraceLog) so it can be used as a common log client. Add new instance TraceLog for OpenSAF logging to local file, which can be enabled/disabled via environment variable OSAF_LOCAL_NODE_LOG ---src/base/logtrace.cc | 167 ++++++++++++++++++++++++++-------------------------src/base/logtrace.h | 50 +++++++++++++-- src/mds/mds_log.cc | 114 +++-------------------------------- 3 files changed, 140 insertions(+), 191 deletions(-) diff --git a/src/base/logtrace.cc b/src/base/logtrace.cc index b046fab..857e31c 100644 --- a/src/base/logtrace.cc +++ b/src/base/logtrace.cc @@ -36,15 +36,10 @@ #include <cstdio> #include <cstdlib> #include <cstring> -#include "base/buffer.h" -#include "base/conf.h" -#include "base/log_message.h" -#include "base/macros.h" -#include "base/mutex.h" +#include "base/getenv.h" #include "base/ncsgl_defs.h" #include "base/osaf_utility.h" #include "base/time.h" -#include "base/unix_client_socket.h" #include "dtm/common/osaflog_protocol.h" namespace global {@@ -55,65 +50,38 @@ const char *const prefix_name[] = {"EM", "AL", "CR", "ER", "WA", "NO", "IN","T6", "T7", "T8", ">>", "<<"}; char *msg_id; int logmask; +const char* osaf_log_file = "osaf.log"; +bool enable_osaf_log = false; } // namespace global -class TraceLog { - public: - static bool Init(); - static void Log(base::LogMessage::Severity severity, const char *fmt, - va_list ap); - - private: - TraceLog(const std::string &fqdn, const std::string &app_name, - uint32_t proc_id, const std::string &msg_id, - const std::string &socket_name);- void LogInternal(base::LogMessage::Severity severity, const char *fmt,- va_list ap);- static constexpr const uint32_t kMaxSequenceId = uint32_t{0x7fffffff};- static TraceLog *instance_; - const base::LogMessage::HostName fqdn_; - const base::LogMessage::AppName app_name_; - const base::LogMessage::ProcId proc_id_; - const base::LogMessage::MsgId msg_id_; - uint32_t sequence_id_; - base::UnixClientSocket log_socket_; - base::Buffer<512> buffer_; - base::Mutex mutex_; - - DELETE_COPY_AND_MOVE_OPERATORS(TraceLog); -}; - -TraceLog *TraceLog::instance_ = nullptr; --TraceLog::TraceLog(const std::string &fqdn, const std::string &app_name,- uint32_t proc_id, const std::string &msg_id, - const std::string &socket_name) - : fqdn_{base::LogMessage::HostName{fqdn}}, - app_name_{base::LogMessage::AppName{app_name}}, - proc_id_{base::LogMessage::ProcId{std::to_string(proc_id)}}, - msg_id_{base::LogMessage::MsgId{msg_id}}, - sequence_id_{1}, - log_socket_{socket_name, base::UnixSocket::kBlocking}, - buffer_{}, - mutex_{} {} - -bool TraceLog::Init() { - if (instance_ != nullptr) return false; - char app_name[49]; - char pid_path[1024];static members? (An alternative is to use a singleton instead, if needed)
[Minh] I also don't really like the "extern C++" here together with existing "extern C", but it serves a purpose of including header file at the other places. It only needs "#include "base/logtrace.h" at either C or C++ source to use either legacy C trace functions or new C++ trace class. If you agree this is a convenience that needs to do?[HansN] perhaps move the TraceLog class to a separate file? The extern "C++" is not needed.+static TraceLog gl_trace; +static TraceLog gl_osaflog; +TraceLog::TraceLog() + : sequence_id_{1}, buffer_{} { + mutex_ = nullptr; + log_socket_ = nullptr; +} + +TraceLog::~TraceLog() { + if (mutex_) delete mutex_; + if (log_socket_) delete log_socket_; +} +bool TraceLog::Init(const char *msg_id, WriteMode mode) { + char app_name[NAME_MAX]; + char pid_path[PATH_MAX]; uint32_t process_id = getpid(); char *token, *saveptr; char *pid_name = nullptr; - memset(app_name, 0, 49); - memset(pid_path, 0, 1024); + memset(app_name, 0, NAME_MAX); + memset(pid_path, 0, PATH_MAX);snprintf(pid_path, sizeof(pid_path), "/proc/%" PRIu32 "/cmdline", process_id);FILE *f = fopen(pid_path, "r"); if (f != nullptr) { size_t size; - size = fread(pid_path, sizeof(char), 1024, f); + size = fread(pid_path, sizeof(char), PATH_MAX, f); if (size > 0) { if ('\n' == pid_path[size - 1]) pid_path[size - 1] = '\0'; } @@ -131,19 +99,28 @@ bool TraceLog::Init() { } base::Conf::InitFullyQualifiedDomainName(); const std::string &fqdn = base::Conf::FullyQualifiedDomainName(); - instance_ = new TraceLog{fqdn, app_name, process_id, global::msg_id, - Osaflog::kServerSocketPath}; - return instance_ != nullptr; + + fqdn_ = base::LogMessage::HostName(fqdn); + app_name_ = base::LogMessage::AppName(app_name); + proc_id_ = base::LogMessage::ProcId{std::to_string(process_id)}; + msg_id_ = base::LogMessage::MsgId{msg_id}; + log_socket_ = new base::UnixClientSocket{Osaflog::kServerSocketPath, + static_cast<base::UnixSocket::Mode>(mode)}; + mutex_ = new base::Mutex{}; + + return true; }void TraceLog::Log(base::LogMessage::Severity severity, const char *fmt,va_list ap) { - if (instance_ != nullptr) instance_->LogInternal(severity, fmt, ap); + if (log_socket_ != nullptr && mutex_ != nullptr) { + LogInternal(severity, fmt, ap); + } }void TraceLog::LogInternal(base::LogMessage::Severity severity, const char *fmt,va_list ap) { - base::Lock lock(mutex_); + base::Lock lock(*mutex_); uint32_t id = sequence_id_; sequence_id_ = id < kMaxSequenceId ? id + 1 : 1; buffer_.clear();@@ -154,7 +131,7 @@ void TraceLog::LogInternal(base::LogMessage::Severity severity, const char *fmt,{base::LogMessage::Parameter{base::LogMessage::SdName{"sequenceId"}, std::to_string(id)}}}}, fmt, ap, &buffer_); - log_socket_.Send(buffer_.data(), buffer_.size()); + log_socket_->Send(buffer_.data(), buffer_.size()); } static pid_t gettid() { return syscall(SYS_gettid); } @@ -190,7 +167,7 @@ static void sighup_handler(int sig) { setlogmask(global::logmask); }-void logtrace_output(const char *file, unsigned line, unsigned priority,+void trace_output(const char *file, unsigned line, unsigned priority,unsigned category, const char *format, va_list ap) {char preamble[288];@@ -199,38 +176,59 @@ void logtrace_output(const char *file, unsigned line, unsigned priority,if (strncmp(file, "src/", 4) == 0) file += 4;snprintf(preamble, sizeof(preamble), "%d:%s:%u %s %s", gettid(), file, line,global::prefix_name[priority + category], format);- TraceLog::Log(static_cast<base::LogMessage::Severity>(priority), preamble,- ap);+ gl_trace.Log(static_cast<base::LogMessage::Severity>(priority), preamble,+ ap); +} + +void log_output(const char *file, unsigned line, unsigned priority,+ unsigned category, const char *format, va_list ap) {+ char preamble[288]; + + assert(priority <= LOG_DEBUG && category < CAT_MAX); + + if (strncmp(file, "src/", 4) == 0) file += 4;+ snprintf(preamble, sizeof(preamble), "%d:%s:%u %s %s", gettid(), file, line,+ global::prefix_name[priority + category], format);+ gl_osaflog.Log(static_cast<base::LogMessage::Severity>(priority), preamble,+ ap); } void logtrace_log(const char *file, unsigned line, int priority, const char *format, ...) { va_list ap; va_list ap2; + va_list ap3; - /* Uncondionally send to syslog */ va_start(ap, format); va_copy(ap2, ap); - - char *tmp_str = nullptr; - int tmp_str_len = 0; -- if ((tmp_str_len = asprintf(&tmp_str, "%s %s", global::prefix_name[priority],- format)) < 0) { - vsyslog(priority, format, ap); - } else { - vsyslog(priority, tmp_str, ap); - free(tmp_str); + va_copy(ap3, ap); + + /* Only log to syslog if the env var OSAF_LOCAL_NODE_LOG is not set + * Or with equal or higher priority than LOG_WARNING, + */ + if (global::enable_osaf_log == false || + (global::enable_osaf_log && priority <= LOG_WARNING)) { + char *tmp_str = nullptr; + int tmp_str_len = 0; + if ((tmp_str_len = asprintf(&tmp_str, "%s %s", + global::prefix_name[priority], format)) < 0) { + vsyslog(priority, format, ap); + } else { + vsyslog(priority, tmp_str, ap); + free(tmp_str); + } + } + /* Log to osaf_local_node_log file */ + if (global::enable_osaf_log == true) { + log_output(file, line, priority, CAT_LOG, format, ap2); } - /* Only output to file if configured to */ - if (!(global::category_mask & (1 << CAT_LOG))) goto done; - - logtrace_output(file, line, priority, CAT_LOG, format, ap2); - -done: + if (global::category_mask & (1 << CAT_LOG)) { + trace_output(file, line, priority, CAT_LOG, format, ap3); + } va_end(ap); va_end(ap2); + va_end(ap3); } bool is_logtrace_enabled(unsigned category) {@@ -245,7 +243,7 @@ void logtrace_trace(const char *file, unsigned line, unsigned category,if (is_logtrace_enabled(category) == false) return; va_start(ap, format); - logtrace_output(file, line, LOG_DEBUG, category, format, ap); + trace_output(file, line, LOG_DEBUG, category, format, ap); va_end(ap); }@@ -266,9 +264,12 @@ int logtrace_init(const char *, const char *pathname, unsigned mask) {global::msg_id = nullptr; } if (result && mask != 0) { - result = TraceLog::Init(); + result = gl_trace.Init(global::msg_id, TraceLog::kBlocking); + } + if (base::GetEnv("OSAF_LOCAL_NODE_LOG", uint32_t{0}) == 1) { + global::enable_osaf_log = true; + gl_osaflog.Init(global::osaf_log_file, TraceLog::kBlocking); } - if (result) {syslog(LOG_INFO, "logtrace: trace enabled to file '%s', mask=0x%x",global::msg_id, global::category_mask); @@ -308,7 +309,7 @@ int trace_category_set(unsigned mask) { if (global::category_mask == 0) { syslog(LOG_INFO, "logtrace: trace disabled"); } else { - TraceLog::Init(); + gl_trace.Init(global::msg_id, TraceLog::kBlocking); syslog(LOG_INFO, "logtrace: trace enabled to file %s, mask=0x%x", global::msg_id, global::category_mask); } diff --git a/src/base/logtrace.h b/src/base/logtrace.h index e7c232c..7f5a059 100644 --- a/src/base/logtrace.h +++ b/src/base/logtrace.h @@ -37,6 +37,45 @@ #include <syslog.h> #include <stdarg.h> #include "base/ncsgl_defs.h" +
+#ifdef __cplusplus +extern "C++" { +#include "base/log_message.h" +#include "base/unix_client_socket.h" +#include "base/buffer.h" +#include "base/conf.h" +#include "base/macros.h" +#include "base/mutex.h" + class TraceLog { + public: + enum WriteMode { + kBlocking = base::UnixSocket::Mode::kBlocking, + kNonblocking = base::UnixSocket::Mode::kNonblocking, + }; + bool Init(const char *msg_id, WriteMode mode); + void Log(base::LogMessage::Severity severity, const char *fmt, + va_list ap); + TraceLog(); + ~TraceLog(); + + private:+ void LogInternal(base::LogMessage::Severity severity, const char *fmt,+ va_list ap);+ static constexpr const uint32_t kMaxSequenceId = uint32_t{0x7fffffff};+ base::LogMessage::HostName fqdn_{""}; + base::LogMessage::AppName app_name_{""}; + base::LogMessage::ProcId proc_id_{""}; + base::LogMessage::MsgId msg_id_{""}; + uint32_t sequence_id_; + base::UnixClientSocket* log_socket_; + base::Buffer<512> buffer_; + base::Mutex* mutex_; + + DELETE_COPY_AND_MOVE_OPERATORS(TraceLog); + }; +} +#endif + #ifdef __cplusplus extern "C" { #endif@@ -129,7 +168,9 @@ extern void logtrace_trace(const char *file, unsigned line, unsigned category,__attribute__((format(printf, 4, 5))); extern bool is_logtrace_enabled(unsigned category);-extern void logtrace_output(const char *file, unsigned line, unsigned priority, +extern void trace_output(const char *file, unsigned line, unsigned priority, + unsigned category, const char *format, va_list ap); +extern void log_output(const char *file, unsigned line, unsigned priority, unsigned category, const char *format, va_list ap);/* LOG API. Use same levels as syslog */@@ -169,13 +210,14 @@ extern void logtrace_output(const char *file, unsigned line, unsigned priority,logtrace_trace(__FILE__, __LINE__, CAT_TRACE8, (format), ##args) #ifdef __cplusplus + class Trace { public: Trace() {} ~Trace() { if (!trace_leave_called && is_logtrace_enabled(CAT_TRACE_LEAVE)) { va_list ap{};- logtrace_output(file_, 0, LOG_DEBUG, CAT_TRACE_LEAVE, function_, ap); + trace_output(file_, 0, LOG_DEBUG, CAT_TRACE_LEAVE, function_, ap);} } void trace(const char *file, const char *function, unsigned line, @@ -185,7 +227,7 @@ class Trace { file_ = file; function_ = function; va_start(ap, format); - logtrace_output(file, line, LOG_DEBUG, category, format, ap); + trace_output(file, line, LOG_DEBUG, category, format, ap); va_end(ap); } } @@ -196,7 +238,7 @@ class Trace { if (is_logtrace_enabled(category)) { va_start(ap, format); trace_leave_called = true; - logtrace_output(file, line, LOG_DEBUG, category, format, ap); + trace_output(file, line, LOG_DEBUG, category, format, ap); va_end(ap); } } diff --git a/src/mds/mds_log.cc b/src/mds/mds_log.cc index 5acae55..69fb372 100644 --- a/src/mds/mds_log.cc +++ b/src/mds/mds_log.cc @@ -35,6 +35,7 @@ #include "base/buffer.h" #include "base/conf.h" #include "base/log_message.h" +#include "base/logtrace.h" #include "base/macros.h" #include "base/mutex.h" #include "base/ncsgl_defs.h" @@ -44,106 +45,9 @@ #include "dtm/common/osaflog_protocol.h" #include "mds/mds_dt2c.h" #include "mds/mds_papi.h" - -class MdsLog { - public: - static bool Init(); - static void Log(base::LogMessage::Severity severity, const char *fmt, - va_list ap); - - private:- MdsLog(const std::string &fqdn, const char *app_name, uint32_t proc_id,- const char *socket_name);- void LogInternal(base::LogMessage::Severity severity, const char *fmt,- va_list ap);- static constexpr const uint32_t kMaxSequenceId = uint32_t{0x7fffffff};- static MdsLog *instance_; - const base::LogMessage::HostName fqdn_; - const base::LogMessage::AppName app_name_; - const base::LogMessage::ProcId proc_id_; - uint32_t sequence_id_; - base::UnixClientSocket log_socket_; - base::Buffer<512> buffer_; - base::Mutex mutex_; - - DELETE_COPY_AND_MOVE_OPERATORS(MdsLog); -}; - int gl_mds_log_level = 3; -MdsLog *MdsLog::instance_ = nullptr; --MdsLog::MdsLog(const std::string &fqdn, const char *app_name, uint32_t proc_id,- const char *socket_name) - : fqdn_{base::LogMessage::HostName{fqdn}}, - app_name_{base::LogMessage::AppName{app_name}}, - proc_id_{base::LogMessage::ProcId{std::to_string(proc_id)}}, - sequence_id_{1}, - log_socket_{socket_name, base::UnixSocket::kNonblocking}, - buffer_{}, - mutex_{} {} - -/***************************************************** - Function NAME: get_process_name() - Returns : <process_name>[<pid> or <tipc_port_ref>] -*****************************************************/ -bool MdsLog::Init() { - if (instance_ != nullptr) return false; - char app_name[MDS_MAX_PROCESS_NAME_LEN]; - char pid_path[1024]; - uint32_t process_id = getpid(); - char *token, *saveptr; - char *pid_name = nullptr; - - memset(app_name, 0, MDS_MAX_PROCESS_NAME_LEN); - memset(pid_path, 0, 1024); -- snprintf(pid_path, sizeof(pid_path), "/proc/%" PRIu32 "/cmdline", process_id);- FILE *f = fopen(pid_path, "r"); - if (f != nullptr) { - size_t size; - size = fread(pid_path, sizeof(char), 1024, f); - if (size > 0) { - if ('\n' == pid_path[size - 1]) pid_path[size - 1] = '\0'; - } - fclose(f); - } else { - pid_path[0] = '\0'; - } - token = strtok_r(pid_path, "/", &saveptr); - while (token != nullptr) { - pid_name = token; - token = strtok_r(nullptr, "/", &saveptr); - } - if (snprintf(app_name, sizeof(app_name), "%s", pid_name) < 0) { - app_name[0] = '\0'; - } - base::Conf::InitFullyQualifiedDomainName(); - const std::string &fqdn = base::Conf::FullyQualifiedDomainName(); - instance_ =- new MdsLog{fqdn, app_name, process_id, Osaflog::kServerSocketPath};- return instance_ != nullptr; -}-void MdsLog::Log(base::LogMessage::Severity severity, const char *fmt,- va_list ap) { - if (instance_ != nullptr) instance_->LogInternal(severity, fmt, ap); -} --void MdsLog::LogInternal(base::LogMessage::Severity severity, const char *fmt,- va_list ap) { - base::Lock lock(mutex_); - uint32_t id = sequence_id_; - sequence_id_ = id < kMaxSequenceId ? id + 1 : 1; - buffer_.clear(); - base::LogMessage::Write(- base::LogMessage::Facility::kLocal1, severity, base::ReadRealtimeClock(),- fqdn_, app_name_, proc_id_, base::LogMessage::MsgId{"mds.log"}, - {{base::LogMessage::SdName{"meta"}, - {base::LogMessage::Parameter{base::LogMessage::SdName{"sequenceId"}, - std::to_string(id)}}}}, - fmt, ap, &buffer_); - log_socket_.Send(buffer_.data(), buffer_.size()); -} +static TraceLog gl_mds_log; /******************************************************************************* * Funtion Name : mds_log_init@@ -154,7 +58,9 @@ void MdsLog::LogInternal(base::LogMessage::Severity severity, const char *fmt,* *******************************************************************************/ uint32_t mds_log_init(const char *) { - if (!MdsLog::Init()) return NCSCC_RC_FAILURE; + if (!gl_mds_log.Init("mds.log", TraceLog::kNonblocking)) { + return NCSCC_RC_FAILURE; + } tzset();log_mds_notify("BEGIN MDS LOGGING| ARCHW=%x|64bit=%zu\n", MDS_SELF_ARCHWORD,MDS_WORD_SIZE_TYPE); @@ -173,7 +79,7 @@ void log_mds_critical(const char *fmt, ...) { if (gl_mds_log_level < NCSMDS_LC_CRITICAL) return; va_list ap; va_start(ap, fmt); - MdsLog::Log(base::LogMessage::Severity::kCrit, fmt, ap); + gl_mds_log.Log(base::LogMessage::Severity::kCrit, fmt, ap); va_end(ap); } @@ -189,7 +95,7 @@ void log_mds_err(const char *fmt, ...) { if (gl_mds_log_level < NCSMDS_LC_ERR) return; va_list ap; va_start(ap, fmt); - MdsLog::Log(base::LogMessage::Severity::kErr, fmt, ap); + gl_mds_log.Log(base::LogMessage::Severity::kErr, fmt, ap); va_end(ap); } @@ -205,7 +111,7 @@ void log_mds_notify(const char *fmt, ...) { if (gl_mds_log_level < NCSMDS_LC_NOTIFY) return; va_list ap; va_start(ap, fmt); - MdsLog::Log(base::LogMessage::Severity::kNotice, fmt, ap); + gl_mds_log.Log(base::LogMessage::Severity::kNotice, fmt, ap); va_end(ap); } @@ -221,7 +127,7 @@ void log_mds_info(const char *fmt, ...) { if (gl_mds_log_level < NCSMDS_LC_INFO) return; va_list ap; va_start(ap, fmt); - MdsLog::Log(base::LogMessage::Severity::kInfo, fmt, ap); + gl_mds_log.Log(base::LogMessage::Severity::kInfo, fmt, ap); va_end(ap); } @@ -238,6 +144,6 @@ void log_mds_dbg(const char *fmt, ...) { if (gl_mds_log_level < NCSMDS_LC_DBG) return; va_list ap; va_start(ap, fmt); - MdsLog::Log(base::LogMessage::Severity::kDebug, fmt, ap); + gl_mds_log.Log(base::LogMessage::Severity::kDebug, fmt, ap); va_end(ap); }
------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot _______________________________________________ Opensaf-devel mailing list Opensaf-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/opensaf-devel
