Hi Anders,

good comments! I'll update and send out a new version. I'll keep the 
dirname and basename for now, (the c++ version requires some more work,

boost filesystem path may be used but it requires building).

/Thanks  HansN


On 10/14/2016 10:27 AM, Anders Widell wrote:
>
> Ack with comments (marked [AndersW] below):
>
> regards,
> Anders Widell
>
> On 10/11/2016 04:57 PM, Hans Nordeback wrote:
>>   osaf/libs/core/cplusplus/base/Makefile.am    |    2 +
>>   osaf/libs/core/cplusplus/base/file_notify.cc |  163 
>> +++++++++++++++++++++++++++
>>   osaf/libs/core/cplusplus/base/file_notify.h  |   84 +++++++++++++
>>   3 files changed, 249 insertions(+), 0 deletions(-)
>>
>>
>> diff --git a/osaf/libs/core/cplusplus/base/Makefile.am 
>> b/osaf/libs/core/cplusplus/base/Makefile.am
>> --- a/osaf/libs/core/cplusplus/base/Makefile.am
>> +++ b/osaf/libs/core/cplusplus/base/Makefile.am
>> @@ -23,6 +23,7 @@ DEFAULT_INCLUDES =
>>   SUBDIRS = tests
>>   
>>   noinst_HEADERS = \
>> +    file_notify.h \
>>      getenv.h \
>>      macros.h \
>>      process.h \
>> @@ -38,5 +39,6 @@ libbase_la_CPPFLAGS = \
>>   libbase_la_LDFLAGS = -static
>>   
>>   libbase_la_SOURCES = \
>> +    file_notify.cc \
>>      getenv.cc \
>>      process.cc
>> diff --git a/osaf/libs/core/cplusplus/base/file_notify.cc 
>> b/osaf/libs/core/cplusplus/base/file_notify.cc
>> new file mode 100644
>> --- /dev/null
>> +++ b/osaf/libs/core/cplusplus/base/file_notify.cc
>> @@ -0,0 +1,163 @@
>> +/*      -*- OpenSAF  -*-
>> + *
>> + * (C) Copyright 2016 The OpenSAF Foundation
>> + *
>> + * This program is distributed in the hope that it will be useful, but
>> + * WITHOUT ANY WARRANTY; without even the implied warranty of 
>> MERCHANTABILITY
>> + * or FITNESS FOR A PARTICULAR PURPOSE. This file and program are licensed
>> + * under the GNU Lesser General Public License Version 2.1, February 1999.
>> + * The complete license can be accessed from the following location:
>> + *http://opensource.org/licenses/lgpl-license.php
>> + * See the Copying file included with the OpenSAF distribution for full
>> + * licensing terms.
>> + *
>> + * Author(s): Ericsson AB
>> + *
>> + */
>> +
>> +#include "base/file_notify.h"
>> +#include <libgen.h>
>> +#include <unistd.h>
>> +#include <sys/stat.h>
>> +#include <cstring>
>> +#include "osaf_time.h"
>> +#include "osaf_poll.h"
>> +#include "logtrace.h"
>> +
>> +namespace base {
>> +
>> +//
> [AndersW] Why empty comments? Remove.
>> +
>> +FileNotify::FileNotify() {
>> +  if ((inotify_fd_ = inotify_init()) == -1) {
>> +    LOG_NO("inotify_init failed: %s", strerror(errno));
>> +  }
>> +}
>> +
>> +//
>> +
>> +FileNotify::~FileNotify() {
> [AndersW] Should close(inotify_fd_);
>> +  if (inotify_fd_ != -1) {
>> +    if (inotify_rm_watch(inotify_fd_, inotify_wd_) == -1) {
> [AndersW] inotify_wd_ can be -1 here. Not necessary to call 
> inotify_rm_watch since close(inotify_fd_) will take care of it.
>> +      LOG_NO("inotify_rm_watch: %s", strerror(errno));
>> +    }
>> +  }
>> +}
>> +
>> +//
>> +
>> +inline bool FileNotify::FileExists(const std::string& file_name) {
>> +  struct stat buffer;
>> +  return (stat(file_name.c_str(), &buffer) == 0);
> [AndersW] The "return" statement does not need parentheses around its 
> expression.
>> +}
>> +
>> +//
>> +
>> +void FileNotify::SplitFileName(const std::string &file_name) {
>> +  //
>> +  char *tmp1 = strdup(file_name.c_str());
>> +  char *tmp2 = strdup(file_name.c_str());
>> +  file_path_ = dirname(tmp1);
>> +  file_name_ = basename(tmp2);
>> +  free(tmp1);
>> +  free(tmp2);
> [AndersW] Should use std::string methods to do this:
>
> std::string::size_type last_slash = file_name.find_last_of('/');
> file_name_ = file_name.substr(last_slash != std::string::npos ? 
> last_slash + 1 : last_slash);
> file_path_ = file_name.substr(0, last_slash);
>> +}
>> +
>> +//
>> +
>> +int FileNotify::WaitForFileCreation(const std::string &file_name, int 
>> timeout) {
>> +  SplitFileName(file_name);
>> +
>> +  //
>> +  if ((inotify_wd_ =
>> +       inotify_add_watch(inotify_fd_, file_path_.c_str(), IN_CREATE)) == 
>> -1) {
>> +    LOG_NO("inotify_add_watch failed: %s", strerror(errno));
>> +    return kError;
>> +  }
>> +
>> +  if (FileExists(file_name)) {
>> +    TRACE("File already created: %s", file_name.c_str());
> [AndersW] Forgot inotify_rm_watch() here
>> +    return kOK;
>> +  }
>> +
>> +  return ProcessEvents(timeout);
> [AndersW] Forgot inotify_rm_watch() here
>> +}
>> +
>> +//
>> +
>> +int FileNotify::WaitForFileDeletion(const std::string &file_name, int 
>> timeout) {
>> +  //
>> +  SplitFileName(file_name);
>> +
>> +  //
>> +  if ((inotify_wd_ =
>> +       inotify_add_watch(inotify_fd_, file_path_.c_str(), IN_DELETE)) == 
>> -1) {
> [AndersW] Wouldn't it be better to watch IN_DELETE_SELF on the full 
> file name path, instead of IN_DELETE on the directory?
>> +    LOG_NO("inotify_add_watch failed: %s", strerror(errno));
>> +    return kError;
>> +  }
>> +
>> +  if (!FileExists(file_name)) {
>> +    TRACE("File already deleted: %s", file_name.c_str());
> [AndersW] Forgot inotify_rm_watch() here
>> +    return kOK;
>> +  }
>> +  return ProcessEvents(timeout);
> [AndersW] Forgot inotify_rm_watch() here
>> +}
>> +
>> +int FileNotify::ProcessEvents(int timeout) {
> [AndersW] return type should be FileNotify::FileNotifyErrors
>> +  //
>> +  timespec start_time;
>> +  timespec time_left_ts;
>> +  timespec timeout_ts;
>> +
>> +  osaf_millis_to_timespec(timeout, &timeout_ts);
>> +  osaf_clock_gettime(CLOCK_MONOTONIC, &start_time);
> [AndersW] Can use the C++ variants of these functions, declared in 
> base/time.h
>> +
>> +  while (true) {
>> +    struct timespec current_time;
>> +    struct timespec elapsed_time = {.tv_sec = 0, .tv_nsec = 0};
>> +
>> +    TRACE("remaining timeout: %d", timeout);
>> +    int rc = osaf_poll_one_fd(inotify_fd_, timeout);
>> +
>> +    if (rc > 0) {
>> +      ssize_t num_read = read(inotify_fd_, buf_, kBufferSize);
>> +
>> +      if (num_read == 0) {
>> +        LOG_WA("read returned zero");
>> +        return kError;
>> +      } else if (num_read == -1) {
> [AndersW] Should handle EINTR.
>> +        LOG_WA("read returned -1");
> [AndersW] Maybe log errno value?
>> +        return kError;
>> +      } else {
>> +        for (char *p = buf_; p < buf_ + num_read;) {
>> +          inotify_event *event = reinterpret_cast<inotify_event*> (p);
> [AndersW] Should handle the IN_IGNORED flag.
>> +          if (file_name_ == event->name) {
>> +            TRACE("file name: %s created", file_name_.c_str());
>> +            return kOK;
>> +          }
>> +          p += sizeof (inotify_event) + event->len;
>> +        }
>> +        // calculate remaining timeout
>> +        osaf_clock_gettime(CLOCK_MONOTONIC, &current_time);
>> +
>> +        if (osaf_timespec_compare(&current_time, &start_time) >= 0) {
>> +          osaf_timespec_subtract(&current_time, &start_time,
>> +                                 &elapsed_time);
>> +        }
>> +        if (osaf_timespec_compare(&elapsed_time, &timeout_ts) >= 0) {
>> +          return kTimeOut;
>> +        }
>> +        osaf_timespec_subtract(&timeout_ts, &elapsed_time, &time_left_ts);
>> +
>> +        timeout = osaf_timespec_to_millis(&time_left_ts);
>> +      }
>> +    } else if (rc == 0) {
>> +      TRACE("timeout");
>> +      return kTimeOut;
>> +    } else {
>> +      LOG_WA("poll error %s", strerror(errno));
>> +      return kError;
>> +    }
>> +  }
>> +}
>> +}  // namespace base
>> diff --git a/osaf/libs/core/cplusplus/base/file_notify.h 
>> b/osaf/libs/core/cplusplus/base/file_notify.h
>> new file mode 100644
>> --- /dev/null
>> +++ b/osaf/libs/core/cplusplus/base/file_notify.h
>> @@ -0,0 +1,84 @@
>> +/*      -*- OpenSAF  -*-
>> + *
>> + * (C) Copyright 2016 The OpenSAF Foundation
>> + *
>> + * This program is distributed in the hope that it will be useful, but
>> + * WITHOUT ANY WARRANTY; without even the implied warranty of 
>> MERCHANTABILITY
>> + * or FITNESS FOR A PARTICULAR PURPOSE. This file and program are licensed
>> + * under the GNU Lesser General Public License Version 2.1, February 1999.
>> + * The complete license can be accessed from the following location:
>> + *http://opensource.org/licenses/lgpl-license.php
>> + * See the Copying file included with the OpenSAF distribution for full
>> + * licensing terms.
>> + *
>> + * Author(s): Ericsson AB
>> + *
>> + */
>> +
>> +#include <limits.h>
>> +#include <sys/inotify.h>
>> +#include <string>
>> +#include "base/macros.h"
>> +
>> +#ifndef OSAF_LIBS_CORE_CPLUSPLUS_BASE_FILE_NOTIFY_H_
>> +#define OSAF_LIBS_CORE_CPLUSPLUS_BASE_FILE_NOTIFY_H_
>> +
>> +namespace base {
>> +
>> +class FileNotify {
>> +  DELETE_COPY_AND_MOVE_OPERATORS(FileNotify);
> [AndersW] This line should be the very last line in the class 
> definition according to Google style guide.
>> +
>> + public:
>> +  enum FileNotifyErrors {
> [AndersW] enum class maybe?
>> +    kOK = 0,
>> +    kTimeOut,
>> +    kError,
>> +  };
>> +
>> +  FileNotify();
>> +  virtual ~FileNotify();
>> +
>> +  /**
>> +   * @brief Wait for a file to be created with a timeout.
>> +   *
>> +   * @a file name in format /path/file.
>> +   * This function blocks until the file has been created or until the @a 
>> timeout expires,
>> +   * whichever happens first.
>> +   *
>> +   */
>> +  int WaitForFileCreation(const std::string &file_name, int timeout);
>> +
>> +  /**
>> +   * @brief Wait for a file to be deleted.
>> +   *
>> +   * @a file name in format /path/file.
>> +   * This function blocks until the file has been deleted or until the @a 
>> timeout expires,
>> +   * whichever happens first.
>> +   *
>> +   */
>> +  int WaitForFileDeletion(const std::string &file_name, int timeout);
>> +
>> + private:
>> +  enum {
>> +    kBufferSize = 10 * (sizeof (inotify_event) + NAME_MAX + 1)
>> +  };
> [AndersW] Why enum? Should be static const.
>> +
>> +  char buf_[kBufferSize] __attribute__((aligned(8))) = {};
> [Should be moved down below all method declarations.
>> +
>> +  int ProcessEvents(int timeout);
>> +  inline bool FileExists(const std::string& file_name);
> [AndersW] Shouldn't this method be defined in the header file if it is 
> inline?
>> +  void SplitFileName(const std::string &file_name);
>> +
>> +  std::string file_path_;
>> +  std::string file_name_;
>> +
>> +  int inotify_fd_{-1};
>> +  int inotify_wd_{-1};
>> +};
>> +
>> +}  // namespace base
>> +
>> +#endif  // OSAF_LIBS_CORE_CPLUSPLUS_BASE_FILE_NOTIFY_H_
>> +
>> +
>> +
>

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