Hi!

This code has degraded in one aspect: it does not return so quickly (or 
at all) when term_fd_ has been signalled. Could you try to fix this in 
some way? Other comments marked [AndersW] below.

regards,

Anders Widell


On 10/11/2016 04:57 PM, Hans Nordeback wrote:
>   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            
> |  41 +++++----
>   3 files changed, 27 insertions(+), 21 deletions(-)
>
>
> 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, 1);
>   }
>   
>   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
> @@ -25,15 +25,16 @@
>   #include "./configmake.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},
> -      pkgpiddir_{base::GetEnv<std::string>("pkgpiddir", PKGPIDDIR)},
> -      proc_path_{"/proc/"},
> -      mds_log_file_{base::GetEnv<std::string>("pkglogdir", PKGLOGDIR)
> -          + "/mds.log"},
> -      old_mds_log_file_{mds_log_file_ + ".old"},
> -      use_tipc_{base::GetEnv<std::string>("MDS_TRANSPORT", "TCP") == "TIPC"} 
> {
> +pkgpiddir_{base::GetEnv<std::string>("pkgpiddir", PKGPIDDIR)},
> +proc_path_{"/proc/"},
> +mds_log_file_{base::GetEnv<std::string>("pkglogdir", PKGLOGDIR)
> +              + "/mds.log"},
> +old_mds_log_file_{mds_log_file_ + ".old"},
> +use_tipc_{base::GetEnv<std::string>("MDS_TRANSPORT", "TCP") == "TIPC"} {
[AndersW] Indentation has been messed up here.
>   }
>   
>   TransportMonitor::~TransportMonitor() {
> @@ -43,15 +44,17 @@ pid_t TransportMonitor::WaitForDaemon(co
>                                         int64_t seconds_to_wait) {
>     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;
> +  int rc = file_notify.WaitForFileCreation(pidfile, seconds_to_wait * 1000);
[AndersW] Possibly use kMillisPerSec from osaf_time.h instead of writing 
1000 explicitly.
> +
> +  if (rc == base::FileNotify::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) {
> @@ -65,6 +68,7 @@ bool TransportMonitor::IsDir(const std::
>   }
>   
>   void TransportMonitor::RotateMdsLogs(pid_t pid_to_watch) {
> +  base::FileNotify file_notify;
>     std::string pid_path{proc_path_ + std::to_string(pid_to_watch)};
>     for (;;) {
>       if (FileSize(mds_log_file_) > kMaxFileSize) {
> @@ -72,10 +76,11 @@ void TransportMonitor::RotateMdsLogs(pid
>         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;
> -      }
> +      int rc =
> +          file_notify.WaitForFileDeletion(pid_path,
> +                                          kLogRotationIntervalInSeconds * 
> 1000);
> +      if (rc == base::FileNotify::kOK)
> +        return;
>       } else {
>         if (Sleep(kLogRotationIntervalInSeconds))
>           return;


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