[Linux] Make FilePathWatcher somewhat more symlink aware.

[Linux] Make FilePathWatcher somewhat more symlink aware.

This is still broken in a variety of ways.

BUG=91561
TEST=updated FilePathWatcher tests and manual testing

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=99657

[Craig, 2011-08-30 05:01:15]

Mattias would you mind taking a look at this please? It's not a full solution to
the symlink issue (I'm working on something better) but I'm hoping to get
something like this in the tree so I can land my resolv.conf watching changes
before M15. What's vaguely appealing about the option here is that it should be
relatively low risk as the code is mostly unchanged for the non-symlink case.

Essentially this change adds an additional watch for the directory in which the
target of symlink lives. The loop that checks for the fired watch now has to
iterate over all the watch entries rather than breaking out early btw. as it's
possible that the newly watched directory is already being watched i.e. we'd get
the same watch descriptor back for two separate watch entries.

If you think this is too horrid to go into the tree, yell soon please so I can
devote some extra time trying to build the "right" solution in time -  that
which will try to flatten/expand the path in a manner consistent with how path
resolution works and watch all the entries along the path but it's tricky and
unfinished at this point.

Thank you!!

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

A couple of comments. I can see that this version solves your resolv.conf use
case, and I'm happy to commit this for now. But please do add comments that
describe the current limitations, and put TODOs that state what needs to be done
to resolve them.

http://codereview.chromium.org/7583034/diff/2001/base/files/file_path_watcher...
File base/files/file_path_watcher_browsertest.cc (right):

http://codereview.chromium.org/7583034/diff/2001/base/files/file_path_watcher...
base/files/file_path_watcher_browsertest.cc:556: }
You may want to add another test that deletes the target file.

http://codereview.chromium.org/7583034/diff/2001/base/files/file_path_watcher...
File base/files/file_path_watcher_linux.cc (right):

http://codereview.chromium.org/7583034/diff/2001/base/files/file_path_watcher...
base/files/file_path_watcher_linux.cc:333: if (fired_watch ==
watch_entry->watch_) {
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?

http://codereview.chromium.org/7583034/diff/2001/base/files/file_path_watcher...
base/files/file_path_watcher_linux.cc:342: (watch_entry->subdir_.empty() &&
(child == watch_entry->linkname_)) ||
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.

http://codereview.chromium.org/7583034/diff/2001/base/files/file_path_watcher...
base/files/file_path_watcher_linux.cc:444: file_util::IsLink(path)) {
We're not passing IN_DONT_FOLLOW, so this only kicks in if the target doesn't
exist?

http://codereview.chromium.org/7583034/diff/2001/base/files/file_path_watcher...
base/files/file_path_watcher_linux.cc:451:
g_inotify_reader.Get().AddWatch(link.DirName(), this);
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]

Thanks for the comments! I'll add some extra comments as requested and an extra
"delete" test a bit later but I'm pressing send now to respond to the questions
you have raised.

http://codereview.chromium.org/7583034/diff/2001/base/files/file_path_watcher...
File base/files/file_path_watcher_browsertest.cc (right):

http://codereview.chromium.org/7583034/diff/2001/base/files/file_path_watcher...
base/files/file_path_watcher_browsertest.cc:556: }
On 2011/08/30 09:53:06, Mattias Nissler wrote:
> You may want to add another test that deletes the target file.

Will do.

http://codereview.chromium.org/7583034/diff/2001/base/files/file_path_watcher...
File base/files/file_path_watcher_linux.cc (right):

http://codereview.chromium.org/7583034/diff/2001/base/files/file_path_watcher...
base/files/file_path_watcher_linux.cc:333: if (fired_watch ==
watch_entry->watch_) {
On 2011/08/30 09:53:06, Mattias Nissler wrote:
> 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?

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.

http://codereview.chromium.org/7583034/diff/2001/base/files/file_path_watcher...
base/files/file_path_watcher_linux.cc:342: (watch_entry->subdir_.empty() &&
(child == watch_entry->linkname_)) ||
On 2011/08/30 09:53:06, Mattias Nissler wrote:
> 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.

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?

http://codereview.chromium.org/7583034/diff/2001/base/files/file_path_watcher...
base/files/file_path_watcher_linux.cc:444: file_util::IsLink(path)) {
On 2011/08/30 09:53:06, Mattias Nissler wrote:
> We're not passing IN_DONT_FOLLOW, so this only kicks in if the target doesn't
> exist?

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)

http://codereview.chromium.org/7583034/diff/2001/base/files/file_path_watcher...
base/files/file_path_watcher_linux.cc:451:
g_inotify_reader.Get().AddWatch(link.DirName(), this);
On 2011/08/30 09:53:06, Mattias Nissler wrote:
> 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...

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]

http://codereview.chromium.org/7583034/diff/2001/base/files/file_path_watcher...
File base/files/file_path_watcher_linux.cc (right):

http://codereview.chromium.org/7583034/diff/2001/base/files/file_path_watcher...
base/files/file_path_watcher_linux.cc:333: if (fired_watch ==
watch_entry->watch_) {
On 2011/08/30 16:00:16, Craig wrote:
> On 2011/08/30 09:53:06, Mattias Nissler wrote:
> > 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?
> 
> 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.

I see. Fair enough.

http://codereview.chromium.org/7583034/diff/2001/base/files/file_path_watcher...
base/files/file_path_watcher_linux.cc:342: (watch_entry->subdir_.empty() &&
(child == watch_entry->linkname_)) ||
On 2011/08/30 16:00:16, Craig wrote:
> On 2011/08/30 09:53:06, Mattias Nissler wrote:
> > 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.
> 
> 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?

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?

http://codereview.chromium.org/7583034/diff/2001/base/files/file_path_watcher...
base/files/file_path_watcher_linux.cc:444: file_util::IsLink(path)) {
On 2011/08/30 16:00:16, Craig wrote:
> On 2011/08/30 09:53:06, Mattias Nissler wrote:
> > We're not passing IN_DONT_FOLLOW, so this only kicks in if the target
doesn't
> > exist?
> 
> 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.

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

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

http://codereview.chromium.org/7583034/diff/2001/base/files/file_path_watcher...
base/files/file_path_watcher_linux.cc:451:
g_inotify_reader.Get().AddWatch(link.DirName(), this);
On 2011/08/30 16:00:16, Craig wrote:
> On 2011/08/30 09:53:06, Mattias Nissler wrote:
> > 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...
> 
> 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.

Sounds good.

[Craig, 2011-09-01 19:04:07]

Sorry for the delay in responding ... I've been fighting other fires. I'll try
to patch this up tomorrow. Comments inline below.

http://codereview.chromium.org/7583034/diff/2001/base/files/file_path_watcher...
File base/files/file_path_watcher_linux.cc (right):

http://codereview.chromium.org/7583034/diff/2001/base/files/file_path_watcher...
base/files/file_path_watcher_linux.cc:342: (watch_entry->subdir_.empty() &&
(child == watch_entry->linkname_)) ||
On 2011/08/30 16:38:26, Mattias Nissler wrote:
> On 2011/08/30 16:00:16, Craig wrote:
> > On 2011/08/30 09:53:06, Mattias Nissler wrote:
> > > 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.
> > 
> > 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?
> 
> 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?

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

http://codereview.chromium.org/7583034/diff/2001/base/files/file_path_watcher...
base/files/file_path_watcher_linux.cc:444: file_util::IsLink(path)) {
On 2011/08/30 16:38:26, Mattias Nissler wrote:
> On 2011/08/30 16:00:16, Craig wrote:
> > On 2011/08/30 09:53:06, Mattias Nissler wrote:
> > > We're not passing IN_DONT_FOLLOW, so this only kicks in if the target
> doesn't
> > > exist?
> > 
> > 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.
> 
> How can we be sure there is no symlink on the path?

Yep, I missed that :(

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

[Craig, 2011-09-02 07:23:41]

I've added some extra tests and tweaked the determination of whether the path
etc. has changed now. Please take another look. Thank you!

[Mattias Nissler (ping if slow), 2011-09-02 11:34:32]

LGTM if you address my question regarding the root dir. In the best case, the
code you have just works and we're just adding a redundant watch (please add a
comment then). If not, please fix and I'll take another look.

http://codereview.chromium.org/7583034/diff/9001/base/files/file_path_watcher...
File base/files/file_path_watcher_browsertest.cc (right):

http://codereview.chromium.org/7583034/diff/9001/base/files/file_path_watcher...
base/files/file_path_watcher_browsertest.cc:518:
ASSERT_TRUE(WriteFile(test_file(), "content"));
do you need the WriteFile()? I guess it shouldn't make a difference.

http://codereview.chromium.org/7583034/diff/9001/base/files/file_path_watcher...
File base/files/file_path_watcher_linux.cc (right):

http://codereview.chromium.org/7583034/diff/9001/base/files/file_path_watcher...
base/files/file_path_watcher_linux.cc:457:
g_inotify_reader.Get().AddWatch(link.DirName(), this);
What if link is "/", i.e. a symbolic link pointing to the file system root? I
think FilePath("/").DirName() == FilePath("/"), but will we get confused then?

[Craig, 2011-09-03 07:46:05]

On 2011/09/02 11:34:32, Mattias Nissler wrote:
> LGTM if you address my question regarding the root dir. In the best case, the
> code you have just works and we're just adding a redundant watch (please add a
> comment then). If not, please fix and I'll take another look.

Thank you. I'll test the root dir scenario later to make sure it's ok but I'm
hitting send now 'cos there's an unexpected situation wrt. the DeleteLink
question and I want to make sure you're ok with that.

http://codereview.chromium.org/7583034/diff/9001/base/files/file_path_watcher...
File base/files/file_path_watcher_browsertest.cc (right):

http://codereview.chromium.org/7583034/diff/9001/base/files/file_path_watcher...
base/files/file_path_watcher_browsertest.cc:518:
ASSERT_TRUE(WriteFile(test_file(), "content"));
On 2011/09/02 11:34:32, Mattias Nissler wrote:
> do you need the WriteFile()? I guess it shouldn't make a difference.

I put it in initially because I wanted the test to start with the watched
content in place rather than a dangling link. I tried it out now though and
unfortunately the kernel (presumably) doesn't seem to notify about the dangling
link being deleted:

(with extra debug output to see the watches being added etc.)

[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from FilePathWatcherTest
[ RUN      ] FilePathWatcherTest.DeleteLink
[3956:3957:0903/092108:69624606905:INFO:file_path_watcher_linux.cc(264)] Adding
watch for / retval: 1
[3956:3957:0903/092108:69624607198:INFO:file_path_watcher_linux.cc(264)] Adding
watch for /tmp retval: 2
[3956:3957:0903/092108:69624607259:INFO:file_path_watcher_linux.cc(264)] Adding
watch for /tmp/.org.chromium.Chromium.sBahFI retval: 3
[3956:3957:0903/092108:69624607338:INFO:file_path_watcher_linux.cc(264)] Adding
watch for /tmp/.org.chromium.Chromium.sBahFI/FilePathWatcherTest.lnk retval: -1:
No such file or directory
[3956:3957:0903/092108:69624607407:INFO:file_path_watcher_linux.cc(457)]
Readlink returns /tmp/.org.chromium.Chromium.sBahFI/FilePathWatcherTest
[3956:3957:0903/092108:69624607446:INFO:file_path_watcher_linux.cc(460)] Should
be watching dir /tmp/.org.chromium.Chromium.sBahFI for link
[3956:3957:0903/092108:69624607490:INFO:file_path_watcher_linux.cc(264)] Adding
watch for /tmp/.org.chromium.Chromium.sBahFI retval: 3
[0903/092153:ERROR:out_of_proc_test_runner.cc(388)] Test timeout (45000 ms)
exceeded for FilePathWatcherTest.DeleteLink

Note that there is no call to FilePathWatcherImpl::OnFilePathChanged in there
(which I was logging) :(

Bleargh. I'm not sure there's much we can do about this as presumably inotify
isn't working for this scenario and a quick glance at the man page doesn't yield
any obvious clues. On the plus side of things, the situation of watching a
dangling symlink is a bit strange. Both before and after the link is deleted,
you don't have an actual target to watch and if the target does exist,
notifications do happen.

http://codereview.chromium.org/7583034/diff/9001/base/files/file_path_watcher...
File base/files/file_path_watcher_linux.cc (right):

http://codereview.chromium.org/7583034/diff/9001/base/files/file_path_watcher...
base/files/file_path_watcher_linux.cc:457:
g_inotify_reader.Get().AddWatch(link.DirName(), this);
On 2011/09/02 11:34:32, Mattias Nissler wrote:
> What if link is "/", i.e. a symbolic link pointing to the file system root? I
> think FilePath("/").DirName() == FilePath("/"), but will we get confused then?

It's unlikely we'd get here under normal conditions - say for example we had
/foo/rootlink pointing to / and were watching /foo/rootlink or
/foo/rootlink/bar. Then adding the watch for /tmp/rootlink should work (since
the / directory exists and we aren't using IN_DONT_FOLLOW). If adding the watch
failed (say a permissions issue or something) then we could attempt to do
FilePath("/").DirName but that's guaranteed to return FilePath("/") according to
the comments in file_path.h. Adding the watch for / is harmless AFAICT but I'll
test it out too just in case a bit later.

[Craig, 2011-09-04 19:42:24]

Comments updated in patch 4 to address issues raised.

[Mattias Nissler (ping if slow), 2011-09-05 09:07:54]

LGTM