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, ¤t_time); >> + >> + if (osaf_timespec_compare(¤t_time, &start_time) >= 0) { >> + osaf_timespec_subtract(¤t_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
