Getting service process on Mac to handle having things moved/changed underneath it.

base/file_path_component_watcher_mac.cc

+// Copyright (c) 2010 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/file_path_component_watcher.h"
+
+#include <CoreFoundation/CoreFoundation.h>
+#include <fcntl.h>
+#include <sys/event.h>
+#include <sys/param.h>
+#include <vector>

[Mark Mentovai, 2011/03/11 20:13:52]

Blank line before. C++ system headers are in a separate section than C system
headers.

+
+#include "base/file_util.h"
+#include "base/mac/scoped_cftyperef.h"
+
+namespace base {
+
+namespace {
+
+// Mac-specific file watcher implementation based on kqueue.

[Mark Mentovai, 2011/03/11 20:13:52]

This sounds like it’s going to eat up a lot of file descriptors. :)

+class FilePathComponentWatcherImpl
+    : public FilePathComponentWatcher::PlatformDelegate {
+ public:
+  FilePathComponentWatcherImpl(FilePathComponentWatcher* watcher)

[Mark Mentovai, 2011/03/11 20:13:52]

explicit

+      : watcher_(watcher) {}
+  virtual ~FilePathComponentWatcherImpl() {}

[Mark Mentovai, 2011/03/11 20:13:52]

Not needed?

[Scott Byer, 2011/03/11 20:25:02]

What about closing the file descriptors on exit, if they're still open?
Otherwise, a failure setting up the kernel event watchers could leak FDs.

+
+  void OnFilePathChanged(CFFileDescriptorRef fd);
+
+  // FilePathComponentWatcher::PlatformDelegate overrides.
+  virtual bool Watch(const FilePath& path,
+                     FilePathComponentWatcher::Delegate* delegate);
+  virtual void Cancel();
+
+ private:
+  FilePathComponentWatcher* watcher_;
+  base::mac::ScopedCFTypeRef<CFFileDescriptorRef> kqueue_fd_;
+  std::vector<int> file_descriptors_;
+  scoped_refptr<FilePathComponentWatcher::Delegate> delegate_;
+  FilePath path_;
+
+  DISALLOW_COPY_AND_ASSIGN(FilePathComponentWatcherImpl);
+};
+
+void CloseFile(int fd) {
+  if(close(fd) != 0) {

[Mark Mentovai, 2011/03/11 20:13:52]

if<space>(close…

+    PLOG(ERROR) << "close";
+  }
+}
+
+void CloseFileDescriptors(std::vector<int>& descriptors) {

[Mark Mentovai, 2011/03/11 20:13:52]

It’s uncommon (and therefore unexpected) to pass non-const references. This’ll
work with a pointer instead.

+  std::for_each(descriptors.begin(), descriptors.end(), CloseFile);
+  descriptors.clear();
+}
+
+bool OpenFileDescriptorsForPath(FilePath path, std::vector<int>* descriptors) {

[Mark Mentovai, 2011/03/11 20:13:52]

Start this function with CloseFileDescriptors(*descriptors);
descriptors->clear(); to ensure you’re working with clean state.

+  FilePath normalized_path;
+  if (!file_util::NormalizeFilePath(path, &normalized_path)) {
+    return false;
+  }
+
+  std::vector<FilePath::StringType> components;
+  normalized_path.GetComponents(&components);
+
+  if (components.size() < 1) {
+    return false;
+  }
+
+  FilePath built_path(components[0]);
+  std::vector<int> local_descriptors;
+  for(std::vector<FilePath::StringType>::iterator i = components.begin() + 1;
+      i != components.end(); ++i) {
+    built_path = built_path.Append(*i);
+    int fd = open(built_path.value().c_str(), O_RDONLY);
+    if (fd == -1) {
+      PLOG(ERROR) << "open " << built_path.value();
+      CloseFileDescriptors(*descriptors);
+      descriptors->clear();
+      return false;
+    } else {

[Mark Mentovai, 2011/03/11 20:13:52]

No else needed after a return.

+      descriptors->push_back(fd);
+    }
+  }
+  return true;
+}
+
+void PathComponentWatcherTrampoline(CFFileDescriptorRef fd,
+                                    CFOptionFlags callBackTypes,

[Mark Mentovai, 2011/03/11 20:13:52]

Match our style: callback_types.

+                                    void *info) {
+  FilePathComponentWatcherImpl *impl =
+      reinterpret_cast<FilePathComponentWatcherImpl*>(info);
+  impl->OnFilePathChanged(fd);
+}
+
+// FilePathComponentWatcherImpl implementation:
+
+void FilePathComponentWatcherImpl::OnFilePathChanged(CFFileDescriptorRef fd) {

[Mark Mentovai, 2011/03/11 20:13:52]

I would find this class implementation easier to read if OnFilePathChanged
followed Watch. I know that this will make the method definition order not match
the order that things were declared in, but I think it’s a net win.

(If you wind up making this Mac-specific and combine things, you’ll get rid of
the platform delegate, and then you can even have things in the right temporal
order in both the declaration and implementations.)

+  int native_fd = CFFileDescriptorGetNativeDescriptor(fd);
+  struct kevent kev;
+  struct timespec timeout = {0, 0};
+  int status = kevent(native_fd, NULL, 0, &kev, 1, &timeout);
+  FilePath new_path;

[Mark Mentovai, 2011/03/11 20:13:52]

Don’t declare this ’til you use it, which looks like it’d be right before the
fcntl.

+  if (status < 0) {
+    PLOG(ERROR) << "kevent";
+    if (delegate_) {
+      delegate_->OnError(watcher_);
+    }
+  } else if (delegate_) {

[Mark Mentovai, 2011/03/11 20:13:52]

When will there not be a delegate?

[Scott Byer, 2011/03/11 20:25:02]

If delegate_ is NULL, I would think we'd want to DCHECK (CHECK?) in Watch, since
that effectively turns this into a no-op - save the FDs.

+    char path[MAXPATHLEN];

[Mark Mentovai, 2011/03/11 20:13:52]

Anything involving MAXPATHLEN is a hack. I think we’ve discussed this before.
This doesn’t make me happy, but evidently it’s the way to do this. Bad, bad
interface.

+    int path_fd = file_descriptors_[file_descriptors_.size() - 1];

[Mark Mentovai, 2011/03/11 20:13:52]

You can just use file_descriptors_.back().

+    if (fcntl(path_fd, F_GETPATH, path) != -1) {
+      new_path = FilePath(path);
+    }
+    delegate_->OnFilePathComponentsChanged(watcher_, path_, new_path);

[Mark Mentovai, 2011/03/11 20:13:52]

new_path may be empty here, which I’m assuming is your signal that it’s been
deleted. You should document that in the delegate declaration in the header.

+  }
+}
+
+bool FilePathComponentWatcherImpl::Watch(
+    const FilePath& path, FilePathComponentWatcher::Delegate* delegate) {
+  delegate_ = delegate;
+  path_ = path;
+  int kq = kqueue();
+  if (kq == -1) {
+    PLOG(ERROR) << "kqueue";
+    return false;
+  }
+  if (!OpenFileDescriptorsForPath(path, &file_descriptors_)) {
+    LOG(ERROR) << "FileDescriptorsForPath: " << path.value();

[Mark Mentovai, 2011/03/11 20:13:52]

OpenFileDescriptorsForPath?

+    return false;
+  }
+
+  size_t count = file_descriptors_.size();
+
+  scoped_ptr<struct kevent> changes(reinterpret_cast<struct kevent*>(

[Mark Mentovai, 2011/03/11 20:13:52]

scoped_ptr is intended to be used with objects allocated by |operator new|, and
frees them with |delete|.

calloc ≠ new.

You should use std::vector<struct kevent> changes(count). When you need to call
EV_SET, use &changes[i]. When you need to pass the whole thing to kevent, use
&changes[0]. (Ensure that count > 0, obviously.)

+      calloc(count, sizeof(struct kevent))));
+
+  for(size_t i = 0; i != count; ++i) {

[Mark Mentovai, 2011/03/11 20:13:52]

for<space>(size_t…

+    EV_SET(&(changes.get())[i], file_descriptors_[i], EVFILT_VNODE,
+           (EV_ADD | EV_CLEAR | EV_ENABLE),
+           (NOTE_RENAME | NOTE_DELETE | NOTE_ATTRIB | NOTE_REVOKE), 0, NULL);
+  }
+
+  if (HANDLE_EINTR(kevent(kq, changes.get(), count, NULL, 0, NULL)) == -1) {

[Mark Mentovai, 2011/03/11 20:13:52]

This confused me for a second, because I saw the NULL last argument, which
usually means “wait forever,” and I was going to ask when you expected this to
return. It wasn’t until I realized that you were asking for 0 changes that I
realized it would return immediately, and you’re just using this to get your
changes into the kqueue but not actually wait.

Might be worth a comment.

+    PLOG(ERROR) << "kevent";
+    return false;
+  }
+
+  CFFileDescriptorContext context;
+  memset(&context, 0, sizeof(context));

[Mark Mentovai, 2011/03/11 20:13:52]

Or just CFFileDescriptorContext context = {0};

+  context.info = this;
+
+  kqueue_fd_.reset(
+      CFFileDescriptorCreate(kCFAllocatorDefault, kq, true,
+                             PathComponentWatcherTrampoline, &context));
+  CFFileDescriptorEnableCallBacks(kqueue_fd_, kCFFileDescriptorReadCallBack);
+  base::mac::ScopedCFTypeRef<CFRunLoopSourceRef> source(
+      CFFileDescriptorCreateRunLoopSource(kCFAllocatorDefault, kqueue_fd_, 0));
+  CFRunLoopAddSource(CFRunLoopGetMain(),

[Mark Mentovai, 2011/03/11 20:13:52]

The run loop source leaks because nobody ever removes it from the run loop, even
after the kqueue it’s monitoring is gone.

[Mark Mentovai, 2011/03/11 20:13:52]

I usually use CFRunLoopGetCurrent. Do you have a reason to prefer one over
another in this case?

+                     source,

[Mark Mentovai, 2011/03/11 20:13:52]

This didn’t all fit on one line?

+                     kCFRunLoopDefaultMode);
+
+  return true;
+}
+
+void FilePathComponentWatcherImpl::Cancel() {
+  if (kqueue_fd_.get()) {
+    CFFileDescriptorInvalidate(kqueue_fd_);
+    kqueue_fd_.reset();
+  }
+  CloseFileDescriptors(file_descriptors_);
+}
+
+}  // namespace
+
+FilePathComponentWatcher::FilePathComponentWatcher() {
+  impl_ = new FilePathComponentWatcherImpl(this);
+}
+
+}  // namespace base