Ok Thanks , ACK from me.
-AVM
On 11/7/2016 8:00 PM, Hans Nordeback wrote:
>
> 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
