Ack with comments. regards,
Anders Widell On 10/22/2016 02:54 PM, Hans Nordeback wrote: > osaf/libs/core/common/daemon.c > | 26 ++++ > 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 > | 59 +++++++-- > osaf/services/infrastructure/dtms/transport/transport_monitor.h > | 2 + > 5 files changed, 77 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 __fifofile[NAME_MAX]; [AndersW] Remove the leading underscore characters. Names starting with two underscore characters are reserved in all scopes. Names starting with one underscore are reserved in the global namespace. See e.g. http://stackoverflow.com/questions/228783/what-are-the-rules-about-using-an-underscore-in-a-c-identifier > static char __pidfile[NAME_MAX]; > static char __tracefile[NAME_MAX]; > static char __runas_username[UT_NAMESIZE]; > @@ -118,6 +119,28 @@ static int __create_pidfile(const char* > return rc; > } > > +static void __create_fifofile(const char* fifofile) [AndersW] Remove the leading underscore characters. > +{ > + mode_t mask; > + > + /* Lets remove any such file if it already exists */ > + unlink(fifofile); [AndersW] I would suggest not to remove the FIFO here. There is a potential race between osaftransportd and osafdtmd: problems will happen if the FIFO already exists and osaftransportd has already opened the old FIFO (which is removed here). Instead, we should remove the FIFO in daemon_exit(). > + mask = umask(0); > + > + if (mkfifo(fifofile, 0666) == -1) { [AndersW] If we don't call unlink() above, then EEXIST is okay here. > + syslog(LOG_WARNING, "mkfifo failed: %s %s", fifofile, > strerror(errno)); > + umask(mask); > + return; > + } > + > + if (open(fifofile, O_NONBLOCK|O_RDONLY) == -1) { [AndersW] EINTR should be handled. > + syslog(LOG_WARNING, "open fifo failed: %s %s", fifofile, > strerror(errno)); > + } > + > + umask(mask); > + return; > +} > + > static int level2mask(const char *level) > { > if (strncmp("notice", level, 6) == 0) { > @@ -133,6 +156,7 @@ static int level2mask(const char *level) > static void __set_default_options(const char *progname) > { > /* Set the default option values */ > + snprintf(__fifofile, sizeof(__fifofile), PKGLOCALSTATEDIR "/%s.fifo", > progname); > snprintf(__pidfile, sizeof(__pidfile), PKGPIDDIR "/%s.pid", progname); > snprintf(__tracefile, sizeof(__tracefile), PKGLOGDIR "/%s", progname); > if (strlen(__runas_username) == 0) { > @@ -320,6 +344,8 @@ void daemonize(int argc, char *argv[]) > if (__create_pidfile(__pidfile) != 0) > exit(EXIT_FAILURE); > > + __create_fifofile(__fifofile); > + > /* Cancel certain signals */ > signal(SIGCHLD, SIG_DFL); /* A child process dies */ > signal(SIGTSTP, SIG_IGN); /* Various TTY signals */ > 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,18 @@ > #endif > #include "osaf/services/infrastructure/dtms/transport/transport_monitor.h" > #include <sys/stat.h> > +#include <sys/types.h> > +#include <sys/stat.h> [AndersW] ./transport_monitor.cc:24: "sys/stat.h" already included at ./transport_monitor.cc:22 [build/include] [4] > +#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 +48,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, seconds_to_wait * > kMillisPerSec, > + user_fds); > + > + 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 +77,39 @@ 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)}; > + [AndersW] You could use WaitForFileCreation() here to wait for the FIFO to be created. But only if pid_to_watch != pid_t{0}. > + if ((fifo_fd = open(fifo_file_.c_str(), O_WRONLY|O_NONBLOCK)) == -1) { [AndersW] Only open the FIFO if pid_to_watch != pid_t{0} [AndersW] EINTR should be handled. [AndersW] There is a race here: what if osafdtmd has not yet opened the FIFO for writing? open() will fail with ENXIO in that case. Solution is to either use O_RDWR instead of O_WRONLY (need to test if monitoring still works if we do this), or use a loop and retry for a limited amount of time when we get ENXIO. > + LOG_WA("open fifo file failed"); [AndersW] Shouldn't this be treated as a fatal error - i.e. we ought to return here? > + } 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, > + kLogRotationIntervalInSeconds * kMillisPerSec, > + user_fds); > + if (rc != base::FileNotify::FileNotifyErrors::kTimeOut) { > + close(fifo_fd); > + return; > + } else { > + TRACE("Timeout received, continuing..."); > } > } else { > - if (Sleep(kLogRotationIntervalInSeconds)) > + if (Sleep(kLogRotationIntervalInSeconds)) { > + close(fifo_fd); [AndersW] Remove close(), since we didn't open the FIFO in this case (if you follow the comments above). > 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_; ------------------------------------------------------------------------------ The Command Line: Reinvented for Modern Developers Did the resurgence of CLI tooling catch you by surprise? Reconnect with the command line and become more productive. Learn the new .NET and ASP.NET CLI. Get your free copy! http://sdm.link/telerik _______________________________________________ Opensaf-devel mailing list Opensaf-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/opensaf-devel
