Hi!

Here are my initial review comments:

* Avoid using preprocessor macros: TRANSPORTD_CONFIG_FILE should be a string constant. Also, according to naming conventions (Google C++ style guide) it should be named kTransportdConfigFile.

* LogServer::no_of_backups and LogServer:kmax_file_size should be instance variables (i.e. not declared static). Also, according to naming convention the names should end with an underscore character. Remove the initial "k" from kmax_file_size (since it is not a constant).

* Instead of using SIGUSR2 to trigger re-loading of the config file, you should add a new option to the osaflog command for this purpose. The osaflog command currently has a --flush option which sends a message to the osaftransportd service, and you can add a new similar option. Instead of re-reading the config file, the option could take the new log file size and number of backup files as arguments and send them in the message to the osaftransportd service. The options could be named --max-backups and --max-file-size. It should be possible to give either both of them in the same command, or just one of them (in which case the other setting is left unchanged).

* Please comment out (insert hash characters before) the two options in transportd.conf, since they have the default values.

regards,
Anders Widell

On 12/27/2017 12:25 PM, syam-talluri wrote:
---
  opensaf.spec.in                            |  2 +
  src/dtm/Makefile.am                        |  3 +
  src/dtm/transport/log_server.cc            | 95 ++++++++++++++++++++++++++++--
  src/dtm/transport/log_server.h             | 10 +++-
  src/dtm/transport/log_writer.cc            |  6 +-
  src/dtm/transport/log_writer.h             |  4 +-
  src/dtm/transport/main.cc                  |  4 ++
  src/dtm/transport/osaf-transport.in        |  1 +
  src/dtm/transport/tests/log_writer_test.cc |  2 +-
  src/dtm/transport/transportd.conf          | 13 ++++
  10 files changed, 129 insertions(+), 11 deletions(-)
  create mode 100644 src/dtm/transport/transportd.conf

diff --git a/opensaf.spec.in b/opensaf.spec.in
index db4b5be..452d1c8 100644
--- a/opensaf.spec.in
+++ b/opensaf.spec.in
@@ -1397,6 +1397,7 @@ fi
  %config(noreplace) %{_pkgsysconfdir}/nodeinit.conf.controller
  %config(noreplace) %{_pkgsysconfdir}/nodeinit.conf.payload
  %config(noreplace) %{_pkgsysconfdir}/dtmd.conf
+%config(noreplace) %{_pkgsysconfdir}/transportd.conf
  %{_pkglibdir}/osafrded
  %{_pkgclcclidir}/osaf-rded
  %{_pkglibdir}/osaffmd
@@ -1423,6 +1424,7 @@ fi
  %dir %{_pkgsysconfdir}
  %config(noreplace) %{_pkgsysconfdir}/nodeinit.conf.payload
  %config(noreplace) %{_pkgsysconfdir}/dtmd.conf
+%config(noreplace) %{_pkgsysconfdir}/transportd.conf
  %{_pkglibdir}/osafdtmd
  %{_pkglibdir}/osaftransportd
  %{_pkgclcclidir}/osaf-dtm
diff --git a/src/dtm/Makefile.am b/src/dtm/Makefile.am
index f3ba720..822249c 100644
--- a/src/dtm/Makefile.am
+++ b/src/dtm/Makefile.am
@@ -57,6 +57,9 @@ nodist_pkgclccli_SCRIPTS += \
  dist_pkgsysconf_DATA += \
        src/dtm/dtmnd/dtmd.conf
+dist_pkgsysconf_DATA += \
+       src/dtm/transport/transportd.conf
+
  bin_osaftransportd_CXXFLAGS = $(AM_CXXFLAGS)
bin_osaftransportd_CPPFLAGS = \
diff --git a/src/dtm/transport/log_server.cc b/src/dtm/transport/log_server.cc
index 2d6c961..780feb1 100644
--- a/src/dtm/transport/log_server.cc
+++ b/src/dtm/transport/log_server.cc
@@ -18,21 +18,28 @@
#include "dtm/transport/log_server.h"
  #include <cstring>
+#include <signal.h>
+#include <syslog.h>
  #include "base/osaf_poll.h"
  #include "base/time.h"
  #include "dtm/common/osaflog_protocol.h"
  #include "osaf/configmake.h"
+#define TRANSPORTD_CONFIG_FILE PKGSYSCONFDIR "/transportd.conf"
+
+size_t LogServer::no_of_backups = 9;
+size_t LogServer::kmax_file_size = 5000 * 1024;
+
  const Osaflog::ClientAddressConstantPrefix LogServer::address_header_{};
LogServer::LogServer(int term_fd)
      : term_fd_{term_fd},
        log_socket_{Osaflog::kServerSocketPath, base::UnixSocket::kNonblocking},
        log_streams_{},
-      current_stream_{new LogStream{"mds.log", 1}},
+      current_stream_{new LogStream{"mds.log", 1, LogServer::kmax_file_size}},
        no_of_log_streams_{1} {
    log_streams_["mds.log"] = current_stream_;
-}
+  }
LogServer::~LogServer() {
    for (const auto& s : log_streams_) delete s.second;
@@ -40,6 +47,12 @@ LogServer::~LogServer() {
void LogServer::Run() {
    struct pollfd pfd[2] = {{term_fd_, POLLIN, 0}, {log_socket_.fd(), POLLIN, 
0}};
+
+  /* Initialize a signal handler for loading new configuration from 
transportd.conf */
+  if ((signal(SIGUSR2, usr2_sig_handler)) == SIG_ERR) {
+      syslog(LOG_ERR,"signal USR2 registration failed: %s", strerror(errno));
+  }
+
    do {
      for (int i = 0; i < 256; ++i) {
        char* buffer = current_stream_->current_buffer_position();
@@ -88,6 +101,12 @@ void LogServer::Run() {
    } while ((pfd[0].revents & POLLIN) == 0);
  }
+void LogServer::usr2_sig_handler(int sig) {
+   syslog(LOG_ERR, "Recived the SIGUSR2 Signal");
+   ReadConfig(TRANSPORTD_CONFIG_FILE);
+   signal(SIGUSR2, usr2_sig_handler);
+}
+
  LogServer::LogStream* LogServer::GetStream(const char* msg_id,
                                             size_t msg_id_size) {
    if (msg_id_size == current_stream_->log_name_size() &&
@@ -99,7 +118,8 @@ LogServer::LogStream* LogServer::GetStream(const char* 
msg_id,
    if (iter != log_streams_.end()) return iter->second;
    if (no_of_log_streams_ >= kMaxNoOfStreams) return nullptr;
    if (!ValidateLogName(msg_id, msg_id_size)) return nullptr;
-  LogStream* stream = new LogStream{log_name, 9};
+
+  LogStream* stream = new LogStream{log_name, LogServer::no_of_backups, 
LogServer::kmax_file_size};
    auto result = log_streams_.insert(
        std::map<std::string, LogStream*>::value_type{log_name, stream});
    if (!result.second) osaf_abort(msg_id_size);
@@ -107,6 +127,71 @@ LogServer::LogStream* LogServer::GetStream(const char* 
msg_id,
    return stream;
  }
+bool LogServer::ReadConfig(const char *transport_config_file) {
+  FILE *transport_conf_file;
+  char line[256];
+  size_t maxFileSize=0;
+  size_t noOfBackupFiles=0;
+  int i, n, comment_line, tag_len = 0;
+
+  /* Open transportd.conf config file. */
+  transport_conf_file = fopen(transport_config_file, "r");
+  if (transport_conf_file == nullptr) {
+
+    syslog(LOG_ERR,"Not able to read transportd.conf: %s", strerror(errno));
+    return false;
+  }
+
+
+  /* Read file. */
+  while (fgets(line, 256, transport_conf_file)) {
+    /* If blank line, skip it - and set tag back to 0. */
+    if (strcmp(line, "\n") == 0) {
+      continue;
+    }
+
+    /* If a comment line, skip it. */
+    n = strlen(line);
+    comment_line = 0;
+    for (i = 0; i < n; i++) {
+      if ((line[i] == ' ') || (line[i] == '\t'))
+        continue;
+      else if (line[i] == '#') {
+        comment_line = 1;
+        break;
+      } else
+        break;
+    }
+    if (comment_line) continue;
+    line[n - 1] = 0;
+
+    if (strncmp(line, "TRANSPORT_MAX_LOG_FILESIZE=",
+                  strlen("TRANSPORT_MAX_LOG_FILESIZE=")) == 0) {
+      tag_len = strlen("TRANSPORT_MAX_LOG_FILESIZE=");
+      maxFileSize = atoi(&line[tag_len]);
+
+      if (maxFileSize > 1) {
+        LogServer::kmax_file_size = maxFileSize * 1024 * 1024;
+      }
+    }
+
+    if (strncmp(line, "TRANSPORT_NO_OF_BACKUP_LOG_FILES=",
+                  strlen("TRANSPORT_NO_OF_BACKUP_LOG_FILES=")) == 0) {
+      tag_len = strlen("TRANSPORT_NO_OF_BACKUP_LOG_FILES=");
+      noOfBackupFiles = atoi(&line[tag_len]);
+
+      if (noOfBackupFiles > 1) {
+        LogServer::no_of_backups = noOfBackupFiles;
+      }
+    }
+  }
+
+  /* Close file. */
+  fclose(transport_conf_file);
+
+  return true;
+}
+
  bool LogServer::ValidateLogName(const char* msg_id, size_t msg_id_size) {
    if (msg_id_size < 1 || msg_id_size > LogStream::kMaxLogNameSize) return 
false;
    if (msg_id[0] == '.') return false;
@@ -152,8 +237,8 @@ std::string LogServer::ExecuteCommand(const std::string& 
command) {
  }
LogServer::LogStream::LogStream(const std::string& log_name,
-                                size_t no_of_backups)
-    : log_name_{log_name}, last_flush_{}, log_writer_{log_name, no_of_backups} 
{
+                                size_t no_of_backups, size_t kmax_file_size)
+    : log_name_{log_name}, last_flush_{}, log_writer_{log_name, no_of_backups, 
kmax_file_size} {
    if (log_name.size() > kMaxLogNameSize) osaf_abort(log_name.size());
  }
diff --git a/src/dtm/transport/log_server.h b/src/dtm/transport/log_server.h
index c988b61..95ea980 100644
--- a/src/dtm/transport/log_server.h
+++ b/src/dtm/transport/log_server.h
@@ -42,12 +42,14 @@ class LogServer {
    // process has received the SIGTERM signal, which is indicated by the caller
    // by making the term_fd (provided in the constructor) readable.
    void Run();
+  // To read Transportd.conf
+  static bool ReadConfig(const char *transport_config_file);
private:
    class LogStream {
     public:
      static constexpr size_t kMaxLogNameSize = 32;
-    LogStream(const std::string& log_name, size_t no_of_backups);
+    LogStream(const std::string& log_name, size_t no_of_backups, size_t 
kMaxFileSize);
size_t log_name_size() const { return log_name_.size(); }
      const char* log_name_data() const { return log_name_.data(); }
@@ -79,10 +81,16 @@ class LogServer {
    static bool ValidateLogName(const char* msg_id, size_t msg_id_size);
    void ExecuteCommand(const char* command, size_t size,
                        const struct sockaddr_un& addr, socklen_t addrlen);
+  // Signal handler for reading config file transportd.conf
+  static void usr2_sig_handler(int sig);
    static bool ValidateAddress(const struct sockaddr_un& addr,
                                socklen_t addrlen);
    std::string ExecuteCommand(const std::string& command);
    int term_fd_;
+  // Configuration for LogServer
+  static size_t no_of_backups;
+  static size_t kmax_file_size;
+
    base::UnixServerSocket log_socket_;
    std::map<std::string, LogStream*> log_streams_;
    LogStream* current_stream_;
diff --git a/src/dtm/transport/log_writer.cc b/src/dtm/transport/log_writer.cc
index 8f1d6a7..329ad77 100644
--- a/src/dtm/transport/log_writer.cc
+++ b/src/dtm/transport/log_writer.cc
@@ -17,6 +17,7 @@
   */
#include "dtm/transport/log_writer.h"
+#include "dtm/transport/log_server.h"
  #include <fcntl.h>
  #include <sys/stat.h>
  #include <unistd.h>
@@ -25,13 +26,14 @@
  #include "base/getenv.h"
  #include "osaf/configmake.h"
-LogWriter::LogWriter(const std::string& log_name, size_t no_of_backups)
+LogWriter::LogWriter(const std::string& log_name, size_t no_of_backups, size_t 
kmax_file_size)
      : log_file_{base::GetEnv<std::string>("pkglogdir", PKGLOGDIR) + "/" +
                  log_name},
        fd_{-1},
        current_file_size_{0},
        current_buffer_size_{0},
        no_of_backups_{no_of_backups},
+      kmax_file_size_{kmax_file_size},
        buffer_{new char[kBufferSize]} {}
LogWriter::~LogWriter() {
@@ -95,7 +97,7 @@ void LogWriter::Flush() {
    if (size == 0) return;
    if (fd_ < 0) Open();
    if (fd_ < 0) return;
-  if (current_file_size_ >= kMaxFileSize) {
+  if (current_file_size_ >= kmax_file_size_) {
      RotateLog();
      if (fd_ < 0) Open();
      if (fd_ < 0) return;
diff --git a/src/dtm/transport/log_writer.h b/src/dtm/transport/log_writer.h
index 89d7b37..eb38cfa 100644
--- a/src/dtm/transport/log_writer.h
+++ b/src/dtm/transport/log_writer.h
@@ -29,7 +29,7 @@ class LogWriter {
   public:
    constexpr static const size_t kMaxMessageSize = 2 * size_t{1024};
- LogWriter(const std::string& log_name, size_t no_of_backups);
+  LogWriter(const std::string& log_name, size_t no_of_backups, size_t 
kmax_file_size);
    virtual ~LogWriter();
char* current_buffer_position() { return buffer_ + current_buffer_size_; }
@@ -43,7 +43,6 @@ class LogWriter {
private:
    constexpr static const size_t kBufferSize = 128 * size_t{1024};
-  constexpr static const size_t kMaxFileSize = 5000 * size_t{1024};
    void Open();
    void Close();
    void RotateLog();
@@ -55,6 +54,7 @@ class LogWriter {
    size_t current_file_size_;
    size_t current_buffer_size_;
    size_t no_of_backups_;
+  size_t kmax_file_size_;
    char* buffer_;
DELETE_COPY_AND_MOVE_OPERATORS(LogWriter);
diff --git a/src/dtm/transport/main.cc b/src/dtm/transport/main.cc
index cf091c8..0d1fedf 100644
--- a/src/dtm/transport/main.cc
+++ b/src/dtm/transport/main.cc
@@ -26,6 +26,9 @@
  #include "dtm/transport/log_server.h"
  #include "dtm/transport/transport_monitor.h"
+
+#define TRANSPORTD_CONFIG_FILE PKGSYSCONFDIR "/transportd.conf"
+
  constexpr static const int kDaemonStartWaitTimeInSeconds = 15;
enum Termination { kExit, kDaemonExit, kReboot };
@@ -82,6 +85,7 @@ Result MainFunction(int term_fd) {
      pthread_attr_destroy(&attr);
      return Result{kExit, "pthread_attr_setinheritsched() failed", result};
    }
+  LogServer::ReadConfig(TRANSPORTD_CONFIG_FILE);
    LogServer log_server{term_fd};
    pthread_t thread_id;
    result =
diff --git a/src/dtm/transport/osaf-transport.in 
b/src/dtm/transport/osaf-transport.in
index 99a2563..a30ac63 100644
--- a/src/dtm/transport/osaf-transport.in
+++ b/src/dtm/transport/osaf-transport.in
@@ -24,6 +24,7 @@ if [ ! -r $osafdirfile ]; then
        exit 6
  else
        . $osafdirfile
+       . $pkgsysconfdir/transportd.conf
        . $pkgsysconfdir/nid.conf
  fi
diff --git a/src/dtm/transport/tests/log_writer_test.cc b/src/dtm/transport/tests/log_writer_test.cc
index f57681c..e96831e 100644
--- a/src/dtm/transport/tests/log_writer_test.cc
+++ b/src/dtm/transport/tests/log_writer_test.cc
@@ -54,7 +54,7 @@ TEST_F(LogWriterTest, ExistingFileShouldBeAppended) {
    std::ofstream ostr(tmpdir_ + std::string("/mds.log"));
    ostr << first_line << std::endl;
    ostr.close();
-  LogWriter* log_writer = new LogWriter{"mds.log", 1};
+  LogWriter* log_writer = new LogWriter{"mds.log", 1, 5000 * 1024};
    memcpy(log_writer->current_buffer_position(), second_line,
           sizeof(second_line) - 1);
    log_writer->current_buffer_position()[sizeof(second_line) - 1] = '\n';
diff --git a/src/dtm/transport/transportd.conf 
b/src/dtm/transport/transportd.conf
new file mode 100644
index 0000000..48b334f
--- /dev/null
+++ b/src/dtm/transport/transportd.conf
@@ -0,0 +1,13 @@
+# This file contains configuration for the Transportd service
+
+#
+# TRANSPORT_MAX_LOG_FILESIZE: The  maximum size of the log file. The size 
value should
+# be in MB's i.e if you give 6 then it is treated as 6 MB. By default value 
will be
+# 5 MB
+TRANSPORT_MAX_LOG_FILESIZE=5
+
+#
+# TRANSPORT_NO_OF_BACKUP_LOG_FILES: Number of backup files to maintain. Log 
rotation will
+# be done based on this value. Default value will be 9
+# i.e totally 10 log files will be maintain.
+TRANSPORT_NO_OF_BACKUP_LOG_FILES=9


------------------------------------------------------------------------------
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

Reply via email to