DnsConfigService and a posix implementation

net/dns/dns_config_service_posix.cc

Index: net/dns/dns_config_service_posix.cc
diff --git a/net/dns/dns_config_service_posix.cc b/net/dns/dns_config_service_posix.cc
new file mode 100644
index 0000000000000000000000000000000000000000..9ab2497503515e9f9081b617615afa0891578745
--- /dev/null
+++ b/net/dns/dns_config_service_posix.cc
@@ -0,0 +1,231 @@
+// Copyright (c) 2011 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 "net/dns/dns_config_service_posix.h"
+
+#include "base/bind.h"
+#include "base/compiler_specific.h"
+#include "base/file_path.h"
+#include "base/files/file_path_watcher.h"
+#include "base/memory/ref_counted.h"
+#include "base/message_loop.h"
+#include "base/message_loop_proxy.h"
+#include "base/observer_list.h"
+#include "base/scoped_ptr.h"
+#include "base/threading/worker_pool.h"
+#include "net/base/ip_endpoint.h"
+#include "net/base/net_util.h"
+
+#ifndef _PATH_RESCONF  // Normally defined in <resolv.h>
+#define _PATH_RESCONF "/etc/resolv.conf"
+#endif
+
+namespace net {
+
+using base::files::FilePathWatcher;
+
+FilePathWatcherShim::FilePathWatcherShim() : watcher_(new FilePathWatcher()) {}
+
+bool FilePathWatcherShim::Watch(const FilePath& path,
+                                FilePathWatcher::Delegate* delegate) {
+  return watcher_->Watch(path, delegate);
+}
+
+bool ConvertResToConfig(const struct __res_state& res, DnsConfig* dns_config) {
+  CHECK(dns_config != NULL);
+  DCHECK(res.options & RES_INIT);
+
+  dns_config->nameservers.clear();
+
+#if OS_LINUX
+  // Initially, glibc stores IPv6 in _ext.nsaddrs and IPv4 in nsaddr_list.
+  // Next (res_send.c::__libc_res_nsend), it copies nsaddr_list after nsaddrs.
+  // If RES_ROTATE is enabled, the list is shifted left after each res_send.
+  // However, if nsaddr_list changes, it will refill nsaddr_list (IPv4) but
+  // leave the IPv6 entries in nsaddr in the same (shifted) order.
+
+  // Put IPv6 addresses ahead of IPv4.
+  for (int i = 0; i < res._u._ext.nscount6; ++i) {
+    IPEndPoint ipe;
+    if (ipe.FromSockAddr(
+        reinterpret_cast<const struct sockaddr*>(res._u._ext.nsaddrs[i]),
+        sizeof *res._u._ext.nsaddrs[i])) {
+      dns_config->nameservers.push_back(ipe);
+    } else {
+      return false;
+    }
+  }
+#endif
+
+  for (int i = 0; i < res.nscount; ++i) {
+    IPEndPoint ipe;
+    if (ipe.FromSockAddr(
+        reinterpret_cast<const struct sockaddr*>(&res.nsaddr_list[i]),
+        sizeof res.nsaddr_list[i])) {
+      dns_config->nameservers.push_back(ipe);
+    } else {
+      return false;
+    }
+  }
+
+  dns_config->search.clear();
+  for (int i = 0; (i < MAXDNSRCH) && res.dnsrch[i]; ++i) {
+    dns_config->search.push_back(std::string(res.dnsrch[i]));
+  }
+
+  dns_config->ndots = res.ndots;
+  dns_config->timeout = base::TimeDelta::FromSeconds(res.retrans);
+  dns_config->attempts = res.retry;
+  dns_config->rotate = res.options & RES_ROTATE;
+  dns_config->edns0 = res.options & RES_USE_EDNS0;
+
+  return true;
+}
+
+// FilePathWatcher::Delegate is refcounted, so we separate it from the Service
+// It also hosts callbacks on the WorkerPool.
+class WatcherDelegate : public FilePathWatcher::Delegate {
+ public:
+  // Takes ownership of |lib|.
+  WatcherDelegate(DnsConfigServicePosix* service, ResolverLib* lib)
+    : service_(service),
+      resolver_lib_(lib),
+      message_loop_(base::MessageLoopProxy::CreateForCurrentThread()),
+      reading_(false),
+      read_pending_(false) {}
+
+  void Cancel() {
+    DCHECK(message_loop_->BelongsToCurrentThread());
+    service_ = NULL;
+  }
+
+  void ScheduleWatch() {
+    // Retry Watch in 100ms or so.

[cbentzel, 2011/08/15 18:16:34]

Should this do a bit of an exponential backoff? 

Do you know the cases that would require a reschedule? Looking at the Linux
implementation it looks like Watch should always return true. 

I'd also rename this to RetryWatch or RescheduleWatch to indicate that it's not
for the first watch, or that this isn't a polling loop.

[cbentzel, 2011/08/15 18:16:34]

Add a DCHECK(message_loop_->BelongsToCurrentThread())

[szym, 2011/08/15 22:02:01]

Done.

[szym, 2011/08/15 22:02:01]

Looking at _linux, indeed it cannot fail. However, in _mac (which is covered by
_posix), it could. 

+    message_loop_->PostDelayedTask(
+        FROM_HERE, base::Bind(&WatcherDelegate::StartWatch, this), 100);
+  }
+
+  // FilePathWatcher::Delegate interface
+  virtual void OnFilePathChanged(const FilePath& path) OVERRIDE {
+    DCHECK(message_loop_->BelongsToCurrentThread());
+    if (!service_)
+      return;
+    ScheduleRead();
+  }
+
+  virtual void OnFilePathError(const FilePath& path) OVERRIDE {
+    DCHECK(message_loop_->BelongsToCurrentThread());
+    StartWatch();
+  }
+
+ private:
+  virtual ~WatcherDelegate() {}

[cbentzel, 2011/08/15 18:16:34]

Is it possible for the FilePathWatcher to outlive this delegate? I couldn't
think of a way (the watcher is owned by the DnsConfigServicePosix, which also
maintains a reference to this watcher) but want to make sure.

[szym, 2011/08/15 22:02:01]

It's possible for the WatcherDelegate to outlive both the FilePathWatcher and
DnsConfigServicePosix, but not vice-versa. When Service is destructed, the link
from the Delegate to the Service is severed. The Service cleans up the Watcher,
but the Delegate is ref-counted. 

I think the lifetime issues are covered.

+
+  // Unless already scheduled, post DoRead to WorkerPool.
+  void ScheduleRead() {
+    if (reading_) {
+      // Mark that we need to re-read after DoRead posts results.
+      read_pending_ = true;
+    } else {
+      // This can't fail on posix. See worker_pool_posix.cc.
+      base::WorkerPool::PostTask(FROM_HERE, base::Bind(

[cbentzel, 2011/08/15 18:16:34]

Perhaps do a NOTREACHED() if this ever returns false - if the implementation of
POSIX version of PostTask ever changes, we want to know.

[szym, 2011/08/15 22:02:01]

Done.

+          &WatcherDelegate::DoRead, this), false);
+      reading_ = true;
+      read_pending_ = false;
+    }
+  }
+
+  // Reads DnsConfig and posts OnResultAvailable to |message_loop_|.
+  // Must be called on the worker thread.
+  void DoRead() {
+    DnsConfig config;
+    struct __res_state res;
+    bool success = false;
+    if (resolver_lib_->ninit(&res) == 0) {
+      success = ConvertResToConfig(res, &config);
+      resolver_lib_->nclose(&res);
+    }
+    // If this fails, the loop is gone, so there is no point retrying.
+    message_loop_->PostTask(FROM_HERE, base::Bind(
+        &WatcherDelegate::OnResultAvailable, this, config, success));
+  }
+
+  // Communicates result to the service. Must be called on the the same thread
+  // that constructed WatcherDelegate.
+  void OnResultAvailable(const DnsConfig &config, bool success) {
+    DCHECK(message_loop_->BelongsToCurrentThread());
+    if (!service_)
+      return;
+    reading_ = false;
+    if (read_pending_) {
+      // Discard this result and re-schedule.
+      ScheduleRead();
+      return;
+    }
+    if (!success) {
+      VLOG(1) << "Failed to read DnsConfig";
+    } else {
+      service_->OnConfigRead(config);
+    }
+  }
+
+  void StartWatch() {
+    if (!service_)

[cbentzel, 2011/08/15 18:16:34]

Good, glad this check is here.

+      return;
+    service_->StartWatch();
+  }
+
+  DnsConfigServicePosix* service_;
+  scoped_ptr<ResolverLib> resolver_lib_;
+  // Message loop for the thread on which Watch is called (of TYPE_IO).
+  scoped_refptr<base::MessageLoopProxy> message_loop_;
+  // True after DoRead before OnResultsAvailable.
+  bool reading_;
+  // True after OnFilePathChanged fires while |reading_| is true.
+  bool read_pending_;
+};
+
+DnsConfigServicePosix::DnsConfigServicePosix()
+  : have_config_(false),
+    resolver_lib_(new ResolverLib()),
+    watcher_factory_(new FilePathWatcherFactory()) {
+}
+
+DnsConfigServicePosix::~DnsConfigServicePosix() {
+  // The watcher must be destroyed on the same thread that called Watch.

[cbentzel, 2011/08/15 18:16:34]

This comment doesn't seem too useful - I'd get rid of it. If you make it
NonThreadSafe the base destructor will have a DCHECK validating that it's
destroyed on the same thread it's created on.

[szym, 2011/08/15 22:02:01]

Done.

+  if (watcher_delegate_.get())
+    watcher_delegate_->Cancel();
+}
+
+void DnsConfigServicePosix::Watch() {
+  DCHECK(!watcher_delegate_.get());
+  DCHECK(!resolv_file_watcher_.get());
+  DCHECK(resolver_lib_.get());

[cbentzel, 2011/08/15 18:16:34]

May as well add DCHECK(watcher_factory_.get()) as well. 

[szym, 2011/08/15 22:02:01]

Done.

+
+  watcher_delegate_ = new WatcherDelegate(this, resolver_lib_.release());
+  StartWatch();
+}
+
+void DnsConfigServicePosix::StartWatch() {
+  DCHECK(watcher_delegate_.get());
+
+  FilePath path(FILE_PATH_LITERAL(_PATH_RESCONF));
+
+  // FilePathWatcher allows only one Watch per lifetime, so we need a new one.
+  resolv_file_watcher_.reset(watcher_factory_->CreateFilePathWatcher());
+  if (resolv_file_watcher_->Watch(path, watcher_delegate_.get())) {
+    // Make the initial read after watch is installed.
+    watcher_delegate_->OnFilePathChanged(path);
+  } else {
+    VLOG(1) << "Watch failed, scheduling restart";
+    watcher_delegate_->ScheduleWatch();

[cbentzel, 2011/08/15 18:16:34]

It's a little strange to me to have this on the WatcherDelegate rather than the
DnsConfigServicePosix. Is this done to prevent refcounting on the service?

[szym, 2011/08/15 22:02:01]

Added a comment to WatcherDelegate::ScheduleWatch to explain that this is to
avoid having to add RefCountedBase to DnsConfigServicePosix.

+  }
+}
+
+DnsConfigService* DnsConfigService::CreateSystemService() {
+  return new DnsConfigServicePosix();
+}
+
+}  // namespace net
+