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
