[Linux] Make FilePathWatcher somewhat more symlink aware.

base/files/file_path_watcher_linux.cc

Index: base/files/file_path_watcher_linux.cc
diff --git a/base/files/file_path_watcher_linux.cc b/base/files/file_path_watcher_linux.cc
index 0fb10e4590e448dadb90f23e5492e153efce0297..4f682e849dca73b003af1f4b9b3cb3049afce6c3 100644
--- a/base/files/file_path_watcher_linux.cc
+++ b/base/files/file_path_watcher_linux.cc
@@ -119,7 +119,8 @@ class FilePathWatcherImpl : public FilePathWatcher::PlatformDelegate,
 
   // 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.
+  // 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),
@@ -127,6 +128,7 @@ class FilePathWatcherImpl : public FilePathWatcher::PlatformDelegate,
 
     InotifyReader::Watch watch_;
     FilePath::StringType subdir_;
+    FilePath::StringType linkname_;
   };
   typedef std::vector<WatchEntry> WatchVector;
 
@@ -328,41 +330,43 @@ void FilePathWatcherImpl::OnFilePathChanged(
   // 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_)
-      break;
-  }
-
-  // If this notification is from a previous generation of watches or the watch
-  // has been cancelled (|watches_| is empty then), bail out.
-  if (watch_entry == watches_.end())
-    return;
-
-  // Check whether a path component of |target_| changed.
-  bool change_on_target_path = child.empty() || child == watch_entry->subdir_;
-
-  // 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() ||
-      (watch_entry->subdir_ == child && (++watch_entry)->subdir_.empty());
+    if (fired_watch == watch_entry->watch_) {

[Mattias Nissler (ping if slow), 2011/08/30 09:53:06]

What is the reason you don't break this loop? I cannot see a condition that
makes two of the watches fire at the same time?

[Craig, 2011/08/30 16:00:16]

This covers the situation where the symlink is targeting a file in the same
subdirectory as the symlink itself. In that case I'd have watched the same
subdirectory twice - once for the symlink and once for the target of the
symlink. Even though the directory is watched twice, we get the same watch
descriptor back from the kernel so it's necessary to loop through all the watch
entries to make sure we've checked both options as only one of them will match.

This isn't a wonderful situation and I'd like to do better in the final
solution.

[Mattias Nissler (ping if slow), 2011/08/30 16:38:26]

I see. Fair enough.

+      // Check whether a path component of |target_| changed.
+      bool change_on_target_path = child.empty() ||
+          ((child == watch_entry->subdir_) && watch_entry->linkname_.empty());
+
+      // 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_)) ||

[Mattias Nissler (ping if slow), 2011/08/30 09:53:06]

What if the link occurs somewhere in the middle of the path? This check here is
wrong then, it should go into change_on_target path.

[Craig, 2011/08/30 16:00:16]

The watch for a symlink's target directory is only added if the symlink is on
the end of a path at the moment. Symlinks in the middle of a path are "ignored"
right now so I _think_ the existing change here is correct?

[Mattias Nissler (ping if slow), 2011/08/30 16:38:26]

I don't understand where in the code we reject symlinks that are part of the
path? What prevents client code to setup a watch to something that contains a
symlink somewhere along the path?

[Craig, 2011/09/01 19:04:08]

Nothing stops the client from calling us with a symlink somewhere along the path
but I've been foolishly only considering the case where the symlinked part of
the path existed (and so AddWatch will succeed etc. and my extra code won't run)
and that's why I've been claiming that symlinks on the path rather than on the
end of the path don't really matter.

However the part that I missed is that this is going to behave differently if
the symlink in the middle of the path is dangling and in that special case, bad
things will probably happen.

Let me mull it over a bit and write a test or two for it...

+          (watch_entry->subdir_.empty() && watch_entry->linkname_.empty()) ||
+          (watch_entry->subdir_ == child && (watch_entry + 1)->subdir_.empty());
+
+      // Update watches if a directory component of the |target_| path
+      // (dis)appears.
+      if (is_directory && change_on_target_path && !UpdateWatches()) {
+        delegate_->OnFilePathError(target_);
+        return;
+      }
 
-  // Update watches if a directory component of the |target_| path (dis)appears.
-  if (is_directory && change_on_target_path && !UpdateWatches()) {
-    delegate_->OnFilePathError(target_);
-    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 && file_util::PathExists(target_))) {
+        delegate_->OnFilePathChanged(target_);
+        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 && file_util::PathExists(target_))) {
-    delegate_->OnFilePathChanged(target_);
-  }
 }
 
 bool FilePathWatcherImpl::Watch(const FilePath& path,
@@ -436,6 +440,18 @@ bool FilePathWatcherImpl::UpdateWatches() {
     InotifyReader::Watch old_watch = watch_entry->watch_;
     if (path_valid) {
       watch_entry->watch_ = g_inotify_reader.Get().AddWatch(path, this);
+      if ((watch_entry->watch_ == InotifyReader::kInvalidWatch) &&
+          file_util::IsLink(path)) {

[Mattias Nissler (ping if slow), 2011/08/30 09:53:06]

We're not passing IN_DONT_FOLLOW, so this only kicks in if the target doesn't
exist?

[Craig, 2011/08/30 16:00:16]

AddWatch will fail when called with a filename or a symlink to a filename
instead of a directory name (since AddWatch uses IN_ONLYDIR). Since everything
on the path is a dir except for possibly the end of the filename, this means the
code here normally only runs for the tail of the pathname being watched i.e. if
the full pathname is a symlink.

(in the case of a non-existant pathname being watched, the code here shouldn't
run as IsLink will fail. In the case where the target of the symlink doesn't
exist, IsLink should succeed and we'll watch for the creation of the symlink
target)

[Mattias Nissler (ping if slow), 2011/08/30 16:38:26]

How can we be sure there is no symlink on the path?

[Craig, 2011/09/01 19:04:08]

Yep, I missed that :(

+        FilePath link;
+        file_util::ReadSymbolicLink(path, &link);
+        if (!link.IsAbsolute())
+          link = path.DirName().Append(link);
+        // Try watching symlink target directory.
+        watch_entry->watch_ =
+            g_inotify_reader.Get().AddWatch(link.DirName(), this);

[Mattias Nissler (ping if slow), 2011/08/30 09:53:06]

So we fail here if the immediate parent directory doesn't exist, correct? The
only way to handle this would probably be to create a new chain of watches from
/ to the link target though...

[Craig, 2011/08/30 16:00:16]

Yes. In the final solution I'd like to add watches for everything along the
"expanded" path. The solution here is rather limited but it does solve the
common resolv.conf symlink scenario. I'll post something on the bug later about 
my thinking on how the full solution would work.

[Mattias Nissler (ping if slow), 2011/08/30 16:38:26]

Sounds good.

+        if (watch_entry->watch_ != InotifyReader::kInvalidWatch)
+          watch_entry->linkname_ = link.BaseName().value();
+      }
       if (watch_entry->watch_ == InotifyReader::kInvalidWatch) {
         path_valid = false;
       }