Hi Minh,
ack, code review only. Some comments below.
/Thanks HansN
On 04/12/2018 01:12 AM, Minh Chau wrote:
Unify 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];
[HansN] instead of static global use unnamed namespaces instead. Also
try to avoid globals, why change Log and Init from
static members? (An alternative is to use a singleton instead, if 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"
+
[HansN] perhaps move the TraceLog class to a separate file? The extern
"C++" is not needed.
+#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
