Add support for almost-recursive watches in Linux DirectoryWatcher

Add support for almost-recursive watches in Linux DirectoryWatcher

After this patch DirectoryWatcher when asked for recursive watch will scan the subtree and add inotify watches for each subfolder, but further changes to the tree structure won't trigger adding/removing watches.

Support for really recursive watches is planned. This is just to divide the work, because the task is not easy.

http://crbug.com/8968

[j.dinata, 2009-04-22 19:24:13]

Hi Pawel, 
This is a simpler version where watcher doesn't handle inotify events.

[Paweł Hajdan Jr., 2009-04-22 20:21:16]

Janwar, please also take a look at lint issues (each file has a "lint" column)
and fix all of them (I verified that it really makes sense). Please also don't
change the issue title or description. This will be a commit message (the final
commit message will also credit you).

I'm also adding more reviewers:
Evan, please look at the code.
Aaron, please just say if you're ok with this approach ("almost-recursive
DirectoryWatcher on Linux").

http://codereview.chromium.org/92032/diff/1/3
File base/directory_watcher_inotify.cc (right):

http://codereview.chromium.org/92032/diff/1/3#newcode46
Line 46: // Remove |watch| for |delegate|.
instead of |delegate|, this comment should say now |watcher|

http://codereview.chromium.org/92032/diff/1/3#newcode47
Line 47: // Returns true on success.
nit: this should fit on the previous line. If it doesn't, ignore.

http://codereview.chromium.org/92032/diff/1/3#newcode91
Line 91: void HandleInotifyEvent (inotify_event* event);
I would prefer something more like this:
// Called for each event coming from one of our watches.
void HandleInotifyEvent(const inotify_event* event);

http://codereview.chromium.org/92032/diff/1/3#newcode93
Line 93: // On recursive mode, watchSubtree will not have any affect on return
value.
I would make the comment more like this:
Start watching |path| for changes and notify |delegate| on each change. If
|recursive| is true, watch entire subtree.
Returns true if watch for |path| has been added successfuly. Watches required
for |recursive| are added on a background thread and have no effect on the
return value.

http://codereview.chromium.org/92032/diff/1/3#newcode102
Line 102: bool getInode(const FilePath path, ino_t* inode) const;
This belongs rather in base/file_util (POSIX-only, of course).
This should be const FilePath& path.

http://codereview.chromium.org/92032/diff/1/3#newcode302
Line 302: //DLOG(WARNING) << "Add watch: " << watch << " abs " << path.value();
Please remove the DLOGs added for debugging when submitting.

http://codereview.chromium.org/92032/diff/1/3#newcode309
Line 309: //DLOG(WARNING) << "remove watch: " << watch;
Here too.

http://codereview.chromium.org/92032/diff/1/3#newcode318
Line 318: watchers_[watch].erase(watchers_[watch].find(watcher));
Now it's not multiset, so if possible use erase(watcher).

http://codereview.chromium.org/92032/diff/1/3#newcode346
Line 346: DirectoryWatcherImpl* watcher = *i;
nit: no need for temp variable here. Just use (*i)-> below.

http://codereview.chromium.org/92032/diff/1/3#newcode375
Line 375: DCHECK(event->len);
This part with DCHECK and above is not needed now.

http://codereview.chromium.org/92032/diff/1/3#newcode384
Line 384: NOTIMPLEMENTED();
Good idea!

http://codereview.chromium.org/92032/diff/1/3#newcode393
Line 393: DLOG(WARNING) << "Failed to get inode: " << path.value() << " " <<
strerror(errno);
No need for this DLOG here.

http://codereview.chromium.org/92032/diff/1/3#newcode400
Line 400: void DirectoryWatcherImpl::traverseTree (InotifyReader::Watch watch,
WatchSet& paths) {
I don't like this method, and the next one, at least at this point. I think that
you just need to keep track of all added watches, and then make sure that you
remove them on destruction. These watches can be kept on a set, or even a list.

http://codereview.chromium.org/92032/diff/1/3#newcode471
Line 471: if(!file_util::PathExists(path))
I would just let InotifyReader's AddWatch fail for non-existent path. This check
seems redundant to me.

http://codereview.chromium.org/92032/diff/1/3#newcode474
Line 474: FilePath absolute_path(path);
This part with absolute paths is not needed.

http://codereview.chromium.org/92032/diff/1/3#newcode496
Line 496: DLOG(WARNING) << "Failed to get inode: " << root_path_.value() << " "
<< strerror(errno);
No need for this DLOG.

http://codereview.chromium.org/92032/diff/1/3#newcode502
Line 502: DCHECK(watchSubtree(root_watch_, root_path_));
Oops. DCHECK is a macro, so watchSubtree won't be executed in release mode. It
should run on another thread anyway. Remember to guarantee that this thread
can't outlive DirectoryWatcherImpl. This means you'd have to keep a base::Thread
as a member variable, and then DirectoryWatcherImpl's dtor will ensure that the
thread terminates.

http://codereview.chromium.org/92032/diff/1/3#newcode511
Line 511: DCHECK(recursive_);
That's good.

http://codereview.chromium.org/92032/diff/1/2
File base/directory_watcher_unittest.cc (right):

http://codereview.chromium.org/92032/diff/1/2#newcode169
Line 169: sleep(1);
No, that's very bad. Tests which depend on that are flaky (unreliable) and very
often fail on buildbots. I'll think more  what's the best way to solve that.

http://codereview.chromium.org/92032/diff/1/2#newcode271
Line 271: TEST_F(DirectoryWatcherTest, MultipleWatchersDifferentFilesRecursive)
{
I would rather just throw some recursive watches to the test above (so that some
watches would be recursive, and some not).

http://codereview.chromium.org/92032/diff/1/2#newcode305
Line 305: TEST_F(DirectoryWatcherTest, MoveDirectoryAcrossWatchRecursive) {
I'll think more if this test should be included now, but I generally don't see a
big need (it doesn't hurt too).

[Aaron Boodman, 2009-04-23 17:53:22]

On 2009/04/22 20:21:16, Paweł Hajdan Jr. wrote:
> Aaron, please just say if you're ok with this approach ("almost-recursive
> DirectoryWatcher on Linux").

It's fine. Thanks!