Hi Mahesh,
one more comment below/Thanks HansN
On 11/07/2016 08:16 AM, A V Mahesh wrote:
> Hi Hans N,
>
> ACK with following minor comments .
>
> -AVM
>
>
> On 10/27/2016 8:55 PM, Hans Nordeback wrote:
>> osaf/libs/core/common/daemon.c | 38 ++++++
>> osaf/services/infrastructure/dtms/transport/tests/Makefile.am | 3 +-
>> osaf/services/infrastructure/dtms/transport/tests/transport_monitor_test.cc
>> | 4 +-
>> osaf/services/infrastructure/dtms/transport/transport_monitor.cc |
>> 62 +++++++--
>> osaf/services/infrastructure/dtms/transport/transport_monitor.h | 2 +
>> 5 files changed, 92 insertions(+), 17 deletions(-)
>>
>>
>> diff --git a/osaf/libs/core/common/daemon.c
>> b/osaf/libs/core/common/daemon.c
>> --- a/osaf/libs/core/common/daemon.c
>> +++ b/osaf/libs/core/common/daemon.c
>> @@ -56,6 +56,7 @@ static const char* internal_version_id_;
>> extern void __gcov_flush(void) __attribute__((weak));
>> +static char fifo_file[NAME_MAX];
> [AVM] can we change the _POSIX_NAME_MAX instead of NAME_MAX
> including existing
[HansN] NAME_MAX is specified in LSB 5 and is 255 _POSIX_NAME_MAX is 14,
I suggest that we keep NAME_MAX
>
>> static char __pidfile[NAME_MAX];
>> static char __tracefile[NAME_MAX];
>> static char __runas_username[UT_NAMESIZE];
>> @@ -118,6 +119,37 @@ static int __create_pidfile(const char*
>> return rc;
>> }
>> +static void create_fifofile(const char* fifofile)
>> +{
>> + mode_t mask;
>> +
>> + mask = umask(0);
>> +
>> + if (mkfifo(fifofile, 0666) == -1) {
>> + if (errno == EEXIST ) {
>> + syslog(LOG_INFO, "mkfifo already exists: %s %s",
>> fifofile, strerror(errno));
>> + } else {
>> + syslog(LOG_WARNING, "mkfifo failed: %s %s", fifofile,
>> strerror(errno));
>> + umask(mask);
>> + return;
>> + }
>> + }
>> +
>> + int fd = -1;
>> +
>> + do {
>> + fd = open(fifofile, O_NONBLOCK|O_RDONLY);
>> +
>> + } while (fd == -1 && errno == EINTR);
>> +
>> + if (fd == -1) {
>> + syslog(LOG_WARNING, "open fifo failed: %s %s", fifofile,
>> strerror(errno));
>> + }
>> +
>> + umask(mask);
>> + return;
>> +}
> [AVM] can we optimize the mkfifo & open in to single `open` with
> option `a` (Open the file for writing only. If the file does not
> exist, create a new empty file)
>
>
>> +
>> static int level2mask(const char *level)
>> {
>> if (strncmp("notice", level, 6) == 0) {
>> @@ -133,6 +165,7 @@ static int level2mask(const char *level)
>> static void __set_default_options(const char *progname)
>> {
>> /* Set the default option values */
>> + snprintf(fifo_file, sizeof(fifo_file), PKGLOCALSTATEDIR
>> "/%s.fifo", progname);
>> snprintf(__pidfile, sizeof(__pidfile), PKGPIDDIR "/%s.pid",
>> progname);
>> snprintf(__tracefile, sizeof(__tracefile), PKGLOGDIR "/%s",
>> progname);
>> if (strlen(__runas_username) == 0) {
>> @@ -316,6 +349,8 @@ void daemonize(int argc, char *argv[])
>> exit(EXIT_FAILURE);
>> }
>> + create_fifofile(fifo_file);
>> +
>> /* Create the process PID file */
>> if (__create_pidfile(__pidfile) != 0)
>> exit(EXIT_FAILURE);
>> @@ -425,6 +460,9 @@ void daemon_exit(void)
>> {
>> syslog(LOG_NOTICE, "exiting for shutdown");
>> + /* Lets remove any such file if it already exists */
>> + unlink(fifo_file);
>> +
>> if (__gcov_flush) {
>> __gcov_flush();
>> }
>> diff --git
>> a/osaf/services/infrastructure/dtms/transport/tests/Makefile.am
>> b/osaf/services/infrastructure/dtms/transport/tests/Makefile.am
>> --- a/osaf/services/infrastructure/dtms/transport/tests/Makefile.am
>> +++ b/osaf/services/infrastructure/dtms/transport/tests/Makefile.am
>> @@ -42,4 +42,5 @@ transport_test_SOURCES = \
>> transport_test_LDADD = \
>> $(GTEST_DIR)/lib/libgtest.la \
>> - $(GTEST_DIR)/lib/libgtest_main.la
>> + $(GTEST_DIR)/lib/libgtest_main.la \
>> + $(top_builddir)/osaf/libs/core/libopensaf_core.la
>> diff --git
>> a/osaf/services/infrastructure/dtms/transport/tests/transport_monitor_test.cc
>>
>> b/osaf/services/infrastructure/dtms/transport/tests/transport_monitor_test.cc
>>
>>
>> ---
>> a/osaf/services/infrastructure/dtms/transport/tests/transport_monitor_test.cc
>> +++
>> b/osaf/services/infrastructure/dtms/transport/tests/transport_monitor_test.cc
>> @@ -82,14 +82,14 @@ class TransportMonitorTest : public ::te
>> TEST_F(TransportMonitorTest, WaitForNonexistentDaemonName) {
>> pid_t pid = monitor_->WaitForDaemon("name_does_not_exist", 17);
>> EXPECT_EQ(pid, pid_t{-1});
>> - EXPECT_EQ(mock_osaf_poll.invocations, 17);
>> + EXPECT_EQ(mock_osaf_poll.invocations, 0);
>> }
>> TEST_F(TransportMonitorTest, WaitForNonexistentDaemonPid) {
>> CreatePidFile("pid_does_not_exist", 1234567890, false);
>> pid_t pid = monitor_->WaitForDaemon("pid_does_not_exist", 1);
>> EXPECT_EQ(pid, pid_t{-1});
>> - EXPECT_EQ(mock_osaf_poll.invocations, 1);
>> + EXPECT_EQ(mock_osaf_poll.invocations, 0);
>> }
>> TEST_F(TransportMonitorTest, WaitForExistingPid) {
>> diff --git
>> a/osaf/services/infrastructure/dtms/transport/transport_monitor.cc
>> b/osaf/services/infrastructure/dtms/transport/transport_monitor.cc
>> --- a/osaf/services/infrastructure/dtms/transport/transport_monitor.cc
>> +++ b/osaf/services/infrastructure/dtms/transport/transport_monitor.cc
>> @@ -20,11 +20,17 @@
>> #endif
>> #include
>> "osaf/services/infrastructure/dtms/transport/transport_monitor.h"
>> #include <sys/stat.h>
>> +#include <sys/types.h>
>> +#include <fcntl.h>
>> #include <cstdio>
>> #include <fstream>
>> -#include "./configmake.h"
>> +#include <vector>
>> +#include "logtrace.h"
>> +#include "osaf_time.h"
>> #include "osaf/libs/core/common/include/osaf_poll.h"
>> #include "osaf/libs/core/cplusplus/base/getenv.h"
>> +#include "osaf/libs/core/cplusplus/base/file_notify.h"
>> +
>> TransportMonitor::TransportMonitor(int term_fd)
>> : term_fd_{term_fd},
>> @@ -41,21 +47,26 @@ TransportMonitor::~TransportMonitor() {
>> pid_t TransportMonitor::WaitForDaemon(const std::string&
>> daemon_name,
>> int64_t seconds_to_wait) {
>> + std::vector<int> user_fds {term_fd_};
>> std::string pidfile = pkgpiddir_ + "/" + daemon_name + ".pid";
>> pid_t pid = pid_t{-1};
>> - for (int64_t count = 0; count != seconds_to_wait; ++count) {
>> - if (!(std::ifstream{pidfile} >> pid).fail()
>> - && IsDir(proc_path_ + std::to_string(pid)))
>> - break;
>> - pid = pid_t{-1};
>> - if (Sleep(1))
>> - return pid_t{-1};
>> + base::FileNotify file_notify;
>> + base::FileNotify::FileNotifyErrors rc =
>> + file_notify.WaitForFileCreation(pidfile, user_fds,
>> + seconds_to_wait * kMillisPerSec);
>> +
>> + if (rc == base::FileNotify::FileNotifyErrors::kOK) {
>> + if (!(std::ifstream{pidfile} >> pid).fail()) {
>> + if (IsDir(proc_path_ + std::to_string(pid))) {
>> + return pid;
>> + }
>> + }
>> }
>> - return pid;
>> + return pid_t{-1};
>> }
>> bool TransportMonitor::Sleep(int64_t seconds_to_wait) {
>> - return osaf_poll_one_fd(term_fd_, seconds_to_wait * 1000) != 0;
>> + return osaf_poll_one_fd(term_fd_, seconds_to_wait * kMillisPerSec)
>> != 0;
>> }
>> bool TransportMonitor::IsDir(const std::string& path) {
>> @@ -65,20 +76,43 @@ bool TransportMonitor::IsDir(const std::
>> }
>> void TransportMonitor::RotateMdsLogs(pid_t pid_to_watch) {
>> + int fifo_fd{-1};
>> + std::vector<int> user_fds {term_fd_};
>> + base::FileNotify file_notify;
>> std::string pid_path{proc_path_ + std::to_string(pid_to_watch)};
>> +
>> + if (pid_to_watch != pid_t{0}) {
>> + do {
>> + fifo_fd = open(fifo_file_.c_str(), O_WRONLY|O_NONBLOCK);
>> + } while (fifo_fd == -1 && errno == EINTR);
>> +
>> + if (fifo_fd == -1) {
>> + LOG_WA("open fifo file failed");
>> + } else {
>> + user_fds.emplace_back(fifo_fd);
>> + }
>> + }
>> +
>> for (;;) {
>> if (FileSize(mds_log_file_) > kMaxFileSize) {
>> unlink(old_mds_log_file());
>> rename(mds_log_file(), old_mds_log_file());
>> }
>> if (pid_to_watch != pid_t{0}) {
>> - for (int64_t i = 0; i != kLogRotationIntervalInSeconds; ++i) {
>> - if (!IsDir(pid_path) || Sleep(1))
>> - return;
>> + base::FileNotify::FileNotifyErrors rc =
>> + file_notify.WaitForFileDeletion(
>> + pid_path,
>> + user_fds, kLogRotationIntervalInSeconds * kMillisPerSec);
>> + if (rc != base::FileNotify::FileNotifyErrors::kTimeOut) {
>> + close(fifo_fd);
>> + return;
>> + } else {
>> + TRACE("Timeout received, continuing...");
>> }
>> } else {
>> - if (Sleep(kLogRotationIntervalInSeconds))
>> + if (Sleep(kLogRotationIntervalInSeconds)) {
>> return;
>> + }
>> }
>> }
>> }
>> diff --git
>> a/osaf/services/infrastructure/dtms/transport/transport_monitor.h
>> b/osaf/services/infrastructure/dtms/transport/transport_monitor.h
>> --- a/osaf/services/infrastructure/dtms/transport/transport_monitor.h
>> +++ b/osaf/services/infrastructure/dtms/transport/transport_monitor.h
>> @@ -21,6 +21,7 @@
>> #include <unistd.h>
>> #include <cstdint>
>> #include <string>
>> +#include "./configmake.h"
>> #include "osaf/libs/core/cplusplus/base/macros.h"
>> // This class is responsible for monitoring the osafdtmd process
>> and rotating
>> @@ -80,6 +81,7 @@ class TransportMonitor {
>> static uint64_t FileSize(const std::string& path);
>> int term_fd_;
>> + const std::string fifo_file_ {PKGLOCALSTATEDIR "/osafdtmd.fifo"};
>> const std::string pkgpiddir_;
>> const std::string proc_path_;
>> const std::string mds_log_file_;
>
------------------------------------------------------------------------------
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today. http://sdm.link/xeonphi
_______________________________________________
Opensaf-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
