Clean up the Linux FileWatcher implementation.

base/files/file_path_watcher_linux.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/files/file_path_watcher.h"
 
 #include <errno.h>
 #include <string.h>
 #include <sys/inotify.h>
 #include <sys/ioctl.h>
@@ skipping 31 matching lines @@
 // http://crbug.com/38174
 class InotifyReader {
  public:
   typedef int Watch;  // Watch descriptor used by AddWatch and RemoveWatch.
   static const Watch kInvalidWatch = -1;
 
   // Watch directory |path| for changes. |watcher| will be notified on each
   // change. Returns kInvalidWatch on failure.
   Watch AddWatch(const FilePath& path, FilePathWatcherImpl* watcher);
 
-  // Remove |watch|. Returns true on success.
-  bool RemoveWatch(Watch watch, FilePathWatcherImpl* watcher);
+  // Remove |watch| if it's valid.
+  void RemoveWatch(Watch watch, FilePathWatcherImpl* watcher);
 
   // Callback for InotifyReaderTask.
   void OnInotifyEvent(const inotify_event* event);
 
  private:
   friend struct DefaultLazyInstanceTraits<InotifyReader>;
 
   typedef std::set<FilePathWatcherImpl*> WatcherSet;
 
   InotifyReader();
@@ skipping 26 matching lines @@
   FilePathWatcherImpl();
 
   // Called for each event coming from the watch. |fired_watch| identifies the
   // watch that fired, |child| indicates what has changed, and is relative to
   // the currently watched path for |fired_watch|. The flag |created| is true if
   // the object appears.
   void OnFilePathChanged(InotifyReader::Watch fired_watch,
                          const FilePath::StringType& child,
                          bool created);
 
+ protected:
+  virtual ~FilePathWatcherImpl() {}
+
+ private:
   // Start watching |path| for changes and notify |delegate| on each change.
   // Returns true if watch for |path| has been added successfully.
   virtual bool Watch(const FilePath& path,
                      bool recursive,
                      const FilePathWatcher::Callback& callback) OVERRIDE;
 
   // Cancel the watch. This unregisters the instance with InotifyReader.
   virtual void Cancel() OVERRIDE;
 
+  // Cleans up and stops observing the message_loop() thread.
+  virtual void CancelOnMessageLoopThread() OVERRIDE;
+
   // Deletion of the FilePathWatcher will call Cancel() to dispose of this
   // object in the right thread. This also observes destruction of the required
   // cleanup thread, in case it quits before Cancel() is called.
   virtual void WillDestroyCurrentMessageLoop() OVERRIDE;
 
- protected:
-  virtual ~FilePathWatcherImpl() {}
-
- private:
-  // Cleans up and stops observing the |message_loop_| thread.
-  virtual void CancelOnMessageLoopThread() OVERRIDE;
-
   // Inotify watches are installed for all directory components of |target_|. A
   // WatchEntry instance holds the watch descriptor for a component and the
   // subdirectory for that identifies the next component. If a symbolic link
   // is being watched, the target of the link is also kept.
   struct WatchEntry {
-    WatchEntry(InotifyReader::Watch watch, const FilePath::StringType& subdir)
-        : watch_(watch),
-          subdir_(subdir) {}
+    explicit WatchEntry(const FilePath::StringType& dirname)
+        : watch(InotifyReader::kInvalidWatch),
+          subdir(dirname) {}
 
-    InotifyReader::Watch watch_;
-    FilePath::StringType subdir_;
-    FilePath::StringType linkname_;
+    InotifyReader::Watch watch;
+    FilePath::StringType subdir;
+    FilePath::StringType linkname;
   };
   typedef std::vector<WatchEntry> WatchVector;
 
   // Reconfigure to watch for the most specific parent directory of |target_|
   // that exists. Updates |watched_path_|. Returns true on success.
   bool UpdateWatches() WARN_UNUSED_RESULT;
 
+  // If |path| is a symlink to a non-existent target, attempt to add a watch to
+  // the link target's parent directory. Returns true and update |watch_entry|
+  // on success.
+  bool AddWatchForBrokenSymlink(const FilePath& path, WatchEntry* watch_entry);
+
+  bool HasValidWatchVector() const;
+
   // Callback to notify upon changes.
   FilePathWatcher::Callback callback_;
 
   // The file or directory we're supposed to watch.
   FilePath target_;
 
   // The vector of watches and next component names for all path components,
   // starting at the root directory. The last entry corresponds to the watch for
-  // |target_| and always stores an empty next component name in |subdir_|.
+  // |target_| and always stores an empty next component name in |subdir|.
   WatchVector watches_;
 
   DISALLOW_COPY_AND_ASSIGN(FilePathWatcherImpl);
 };
 
 void InotifyReaderCallback(InotifyReader* reader, int inotify_fd,
                            int shutdown_fd) {
   // Make sure the file descriptors are good for use with select().
   CHECK_LE(0, inotify_fd);
   CHECK_GT(FD_SETSIZE, inotify_fd);
@@ skipping 58 matching lines @@
     : thread_("inotify_reader"),
       inotify_fd_(inotify_init()),
       valid_(false) {
   if (inotify_fd_ < 0)
     PLOG(ERROR) << "inotify_init() failed";
 
   shutdown_pipe_[0] = -1;
   shutdown_pipe_[1] = -1;
   if (inotify_fd_ >= 0 && pipe(shutdown_pipe_) == 0 && thread_.Start()) {
     thread_.message_loop()->PostTask(
-        FROM_HERE, Bind(&InotifyReaderCallback, this, inotify_fd_,
-                              shutdown_pipe_[0]));
+        FROM_HERE,
+        Bind(&InotifyReaderCallback, this, inotify_fd_, shutdown_pipe_[0]));
     valid_ = true;
   }
 }
 
 InotifyReader::~InotifyReader() {
   if (valid_) {
     // Write to the self-pipe so that the select call in InotifyReaderTask
     // returns.
     ssize_t ret = HANDLE_EINTR(write(shutdown_pipe_[1], "", 1));
     DPCHECK(ret > 0);
@@ skipping 21 matching lines @@
                                   IN_ONLYDIR);
 
   if (watch == kInvalidWatch)
     return kInvalidWatch;
 
   watchers_[watch].insert(watcher);
 
   return watch;
 }
 
-bool InotifyReader::RemoveWatch(Watch watch,
-                                FilePathWatcherImpl* watcher) {
-  if (!valid_)
-    return false;
+void InotifyReader::RemoveWatch(Watch watch, FilePathWatcherImpl* watcher) {
+  if (!valid_ || (watch == kInvalidWatch))
+    return;
 
   AutoLock auto_lock(lock_);
 
   watchers_[watch].erase(watcher);
 
   if (watchers_[watch].empty()) {
     watchers_.erase(watch);
-    return (inotify_rm_watch(inotify_fd_, watch) == 0);
+    inotify_rm_watch(inotify_fd_, watch);
   }
-
-  return true;
 }
 
 void InotifyReader::OnInotifyEvent(const inotify_event* event) {
   if (event->mask & IN_IGNORED)
     return;
 
-  FilePath::StringType child(event->len ? event->name : FILE_PATH_LITERAL(""));

[Mattias Nissler (ping if slow), 2014/04/25 08:38:24]

Dropping FILE_PATH_LITERAL adds inconsistency: Either we use
FilePath::StringType with FILE_PATH_LITERAL, or we accept this code is only ever
going to be useful on Linux, in which case we should use std::string instead of
FilePath::StringType and drop FILE_PATH_LITERAL. I'm leaning towards the former,
since that is more consistent with the rest of the code base.

[Lei Zhang, 2014/04/25 08:49:32]

Ok, done.

+  FilePath::StringType child(event->len ? event->name : "");
   AutoLock auto_lock(lock_);
 
   for (WatcherSet::iterator watcher = watchers_[event->wd].begin();
        watcher != watchers_[event->wd].end();
        ++watcher) {
     (*watcher)->OnFilePathChanged(event->wd,
                                   child,
                                   event->mask & (IN_CREATE | IN_MOVED_TO));
   }
 }
 
 FilePathWatcherImpl::FilePathWatcherImpl() {
 }
 
 void FilePathWatcherImpl::OnFilePathChanged(InotifyReader::Watch fired_watch,
                                             const FilePath::StringType& child,
                                             bool created) {
   if (!message_loop()->BelongsToCurrentThread()) {
-    // Switch to message_loop_ to access watches_ safely.
-    message_loop()->PostTask(FROM_HERE,
-        Bind(&FilePathWatcherImpl::OnFilePathChanged,
-                   this,
-                   fired_watch,
-                   child,
-                   created));
+    // Switch to message_loop() to access |watches_| safely.
+    message_loop()->PostTask(
+        FROM_HERE,
+        Bind(&FilePathWatcherImpl::OnFilePathChanged, this,
+             fired_watch, child, created));
+    return;
+  }
+
+  // Check to see if CancelOnMessageLoopThread() has already been called.
+  // Happens when code flow reaches here from the PostTask() above.

[Mattias Nissler (ping if slow), 2014/04/25 08:38:24]

nit: s/Happens/May happen/

[Lei Zhang, 2014/04/25 08:49:32]

Done.

+  if (watches_.empty()) {
+    DCHECK(target_.empty());
     return;
   }
 
   DCHECK(MessageLoopForIO::current());
+  DCHECK(HasValidWatchVector());
 
   // Find the entry in |watches_| that corresponds to |fired_watch|.
-  WatchVector::const_iterator watch_entry(watches_.begin());
-  for ( ; watch_entry != watches_.end(); ++watch_entry) {
-    if (fired_watch == watch_entry->watch_) {
-      // Check whether a path component of |target_| changed.
-      bool change_on_target_path = child.empty() ||
-          ((child == watch_entry->subdir_) && watch_entry->linkname_.empty()) ||
-          (child == watch_entry->linkname_);
+  for (size_t i = 0; i < watches_.size(); ++i) {

[Mattias Nissler (ping if slow), 2014/04/25 08:38:24]

So you just prefer indexing over iterators? Or is there a good reason for the
conversion(s) that I'm missing?

[Lei Zhang, 2014/04/25 08:49:32]

I do, it makes the for-loop line easier to read in some cases. e.g. line 387 on
the left.

[Mattias Nissler (ping if slow), 2014/04/25 09:06:57]

I don't see the advantage to be honest, and I personally do prefer iterators as
these do work for all STL containers equally well. However, I don't think it's
worth fighting over, and I don't convert code from on way to the other when I
touch it. Do what you wish.

+    const WatchEntry& watch_entry = watches_[i];
+    if (fired_watch != watch_entry.watch)
+      continue;
 
-      // Check whether the change references |target_| or a direct child.
-      DCHECK(watch_entry->subdir_.empty() ||
-          (watch_entry + 1) != watches_.end());
-      bool target_changed =
-          (watch_entry->subdir_.empty() && (child == watch_entry->linkname_)) ||
-          (watch_entry->subdir_.empty() && watch_entry->linkname_.empty()) ||
-          (watch_entry->subdir_ == child && (watch_entry + 1)->subdir_.empty());
+    // Check whether a path component of |target_| changed.
+    bool change_on_target_path =
+        child.empty() ||
+        (child == watch_entry.linkname) ||
+        (child == watch_entry.subdir);
 
-      // Update watches if a directory component of the |target_| path
-      // (dis)appears. Note that we don't add the additional restriction
-      // of checking the event mask to see if it is for a directory here
-      // as changes to symlinks on the target path will not have
-      // IN_ISDIR set in the event masks. As a result we may sometimes
-      // call UpdateWatches() unnecessarily.
-      if (change_on_target_path && !UpdateWatches()) {
-        callback_.Run(target_, true /* error */);
-        return;
-      }
+    // Check if the change references |target_| or a direct child of |target_|.
+    bool is_direct_child = watch_entry.subdir.empty();

[Mattias Nissler (ping if slow), 2014/04/25 08:38:24]

is_direct_child is a misleading name. If watch_entry.subdir.empty(), then that
means there's a change in the last entry in the watch chain, which refers to the
target. Hence, a better name here could be |is_watch_for_target|.

[Lei Zhang, 2014/04/25 08:49:32]

Done.

+    bool target_changed =
+        (is_direct_child && (child == watch_entry.linkname)) ||
+        (is_direct_child && watch_entry.linkname.empty()) ||
+        (watch_entry.subdir == child && watches_[i + 1].subdir.empty());
 
-      // Report the following events:
-      //  - The target or a direct child of the target got changed (in case the
-      //    watched path refers to a directory).
-      //  - One of the parent directories got moved or deleted, since the target
-      //    disappears in this case.
-      //  - One of the parent directories appears. The event corresponding to
-      //    the target appearing might have been missed in this case, so
-      //    recheck.
-      if (target_changed ||
-          (change_on_target_path && !created) ||
-          (change_on_target_path && PathExists(target_))) {
-        callback_.Run(target_, false);
-        return;
-      }
+    // Update watches if a directory component of the |target_| path
+    // (dis)appears. Note that we don't add the additional restriction of
+    // checking the event mask to see if it is for a directory here as changes
+    // to symlinks on the target path will not have IN_ISDIR set in the event
+    // masks. As a result we may sometimes call UpdateWatches() unnecessarily.
+    if (change_on_target_path && !UpdateWatches()) {
+      callback_.Run(target_, true /* error */);
+      return;
+    }
+
+    // Report the following events:
+    //  - The target or a direct child of the target got changed (in case the
+    //    watched path refers to a directory).
+    //  - One of the parent directories got moved or deleted, since the target
+    //    disappears in this case.
+    //  - One of the parent directories appears. The event corresponding to
+    //    the target appearing might have been missed in this case, so recheck.
+    if (target_changed ||
+        (change_on_target_path && !created) ||
+        (change_on_target_path && PathExists(target_))) {
+      callback_.Run(target_, false /* error */);
+      return;
     }
   }
 }
 
 bool FilePathWatcherImpl::Watch(const FilePath& path,
                                 bool recursive,
                                 const FilePathWatcher::Callback& callback) {
   DCHECK(target_.empty());
   DCHECK(MessageLoopForIO::current());
   if (recursive) {
     // Recursive watch is not supported on this platform.
     NOTIMPLEMENTED();
     return false;
   }
 
   set_message_loop(MessageLoopProxy::current().get());
   callback_ = callback;
   target_ = path;
   MessageLoop::current()->AddDestructionObserver(this);
 
   std::vector<FilePath::StringType> comps;
   target_.GetComponents(&comps);
   DCHECK(!comps.empty());
-  std::vector<FilePath::StringType>::const_iterator comp = comps.begin();
-  for (++comp; comp != comps.end(); ++comp)
-    watches_.push_back(WatchEntry(InotifyReader::kInvalidWatch, *comp));
-
-  watches_.push_back(WatchEntry(InotifyReader::kInvalidWatch,
-                                FilePath::StringType()));
+  for (size_t i = 1; i < comps.size(); ++i)
+    watches_.push_back(WatchEntry(comps[i]));
+  watches_.push_back(WatchEntry(FilePath::StringType()));
   return UpdateWatches();
 }
 
 void FilePathWatcherImpl::Cancel() {
   if (callback_.is_null()) {
-    // Watch was never called, or the |message_loop_| thread is already gone.
+    // Watch was never called, or the message_loop() thread is already gone.
     set_cancelled();
     return;
   }
 
-  // Switch to the message_loop_ if necessary so we can access |watches_|.
+  // Switch to the message_loop() if necessary so we can access |watches_|.
   if (!message_loop()->BelongsToCurrentThread()) {
     message_loop()->PostTask(FROM_HERE,
                              Bind(&FilePathWatcher::CancelWatch,
-                                        make_scoped_refptr(this)));
+                                  make_scoped_refptr(this)));
   } else {
     CancelOnMessageLoopThread();
   }
 }
 
 void FilePathWatcherImpl::CancelOnMessageLoopThread() {
-  if (!is_cancelled())
-    set_cancelled();
+  set_cancelled();
 
   if (!callback_.is_null()) {
     MessageLoop::current()->RemoveDestructionObserver(this);
     callback_.Reset();
   }
 
-  for (WatchVector::iterator watch_entry(watches_.begin());
-       watch_entry != watches_.end(); ++watch_entry) {
-    if (watch_entry->watch_ != InotifyReader::kInvalidWatch)
-      g_inotify_reader.Get().RemoveWatch(watch_entry->watch_, this);
-  }
+  for (size_t i = 0; i < watches_.size(); ++i)
+    g_inotify_reader.Get().RemoveWatch(watches_[i].watch, this);
   watches_.clear();
   target_.clear();
 }
 
 void FilePathWatcherImpl::WillDestroyCurrentMessageLoop() {
   CancelOnMessageLoopThread();
 }
 
 bool FilePathWatcherImpl::UpdateWatches() {
-  // Ensure this runs on the |message_loop_| exclusively in order to avoid
+  // Ensure this runs on the message_loop() exclusively in order to avoid
   // concurrency issues.
   DCHECK(message_loop()->BelongsToCurrentThread());
+  DCHECK(HasValidWatchVector());
 
   // Walk the list of watches and update them as we go.
-  FilePath path(FILE_PATH_LITERAL("/"));
+  FilePath path("/");
   bool path_valid = true;
-  for (WatchVector::iterator watch_entry(watches_.begin());
-       watch_entry != watches_.end(); ++watch_entry) {
-    InotifyReader::Watch old_watch = watch_entry->watch_;
+  for (size_t i = 0; i < watches_.size(); ++i) {
+    WatchEntry& watch_entry = watches_[i];
+    InotifyReader::Watch old_watch = watch_entry.watch;
+    watch_entry.watch = InotifyReader::kInvalidWatch;
+    watch_entry.linkname.clear();
     if (path_valid) {
-      watch_entry->watch_ = g_inotify_reader.Get().AddWatch(path, this);
-      if ((watch_entry->watch_ == InotifyReader::kInvalidWatch) &&
-          base::IsLink(path)) {
-        FilePath link;
-        if (ReadSymbolicLink(path, &link)) {
-          if (!link.IsAbsolute())
-            link = path.DirName().Append(link);
-          // Try watching symlink target directory. If the link target is "/",
-          // then we shouldn't get here in normal situations and if we do, we'd
-          // watch "/" for changes to a component "/" which is harmless so no
-          // special treatment of this case is required.
-          watch_entry->watch_ =
-              g_inotify_reader.Get().AddWatch(link.DirName(), this);
-          if (watch_entry->watch_ != InotifyReader::kInvalidWatch) {
-            watch_entry->linkname_ = link.BaseName().value();
-          } else {
-            DPLOG(WARNING) << "Watch failed for "  << link.DirName().value();
-            // TODO(craig) Symlinks only work if the parent directory
-            // for the target exist. Ideally we should make sure we've
-            // watched all the components of the symlink path for
-            // changes. See crbug.com/91561 for details.
-          }
-        }
+      watch_entry.watch = g_inotify_reader.Get().AddWatch(path, this);
+      if (watch_entry.watch == InotifyReader::kInvalidWatch) {

[Mattias Nissler (ping if slow), 2014/04/25 08:38:24]

I would prefer to leave the base::IsLink() check here instead of moving it into
AddWatchForBrokenSymlink. Otherwise, one may get the impression from the
function name that the condition here indicates we're dealing with a broken
symlink, which is not true.

[Lei Zhang, 2014/04/25 08:49:32]

Done.

+        path_valid = AddWatchForBrokenSymlink(path, &watch_entry);
       }
-      if (watch_entry->watch_ == InotifyReader::kInvalidWatch) {
-        path_valid = false;
-      }
-    } else {
-      watch_entry->watch_ = InotifyReader::kInvalidWatch;
     }
-    if (old_watch != InotifyReader::kInvalidWatch &&
-        old_watch != watch_entry->watch_) {
+    if (old_watch != watch_entry.watch)
       g_inotify_reader.Get().RemoveWatch(old_watch, this);
-    }
-    path = path.Append(watch_entry->subdir_);
+    path = path.Append(watch_entry.subdir);
   }
 
   return true;
 }
 
+bool FilePathWatcherImpl::AddWatchForBrokenSymlink(const FilePath& path,
+                                                   WatchEntry* watch_entry) {
+  DCHECK_EQ(InotifyReader::kInvalidWatch, watch_entry->watch);
+  if (!IsLink(path))
+    return false;
+
+  FilePath link;
+  if (!ReadSymbolicLink(path, &link))
+    return false;
+
+  if (!link.IsAbsolute())
+    link = path.DirName().Append(link);
+
+  // Try watching symlink target directory. If the link target is "/", then we
+  // shouldn't get here in normal situations and if we do, we'd watch "/" for
+  // changes to a component "/" which is harmless so no special treatment of
+  // this case is required.
+  InotifyReader::Watch watch =
+      g_inotify_reader.Get().AddWatch(link.DirName(), this);
+  if (watch == InotifyReader::kInvalidWatch) {
+    // TODO(craig) Symlinks only work if the parent directory for the target
+    // exist. Ideally we should make sure we've watched all the components of
+    // the symlink path for changes. See crbug.com/91561 for details.
+    DPLOG(WARNING) << "Watch failed for "  << link.DirName().value();
+    return false;
+  }
+  watch_entry->watch = watch;
+  watch_entry->linkname = link.BaseName().value();
+  return true;
+}
+
+bool FilePathWatcherImpl::HasValidWatchVector() const {
+  if (watches_.empty())
+    return false;
+  for (size_t i = 0; i < watches_.size() - 1; ++i) {
+    if (watches_[i].subdir.empty())
+      return false;
+  }
+  return watches_[watches_.size() - 1].subdir.empty();
+}
+
 }  // namespace
 
 FilePathWatcher::FilePathWatcher() {
   impl_ = new FilePathWatcherImpl();
 }
 
 }  // namespace base