DnsConfigService and a posix implementation

net/dns/dns_config_service_posix.cc

+// 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 <resolv.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/scoped_ptr.h"
+#include "base/threading/worker_pool.h"
+#include "net/base/ip_endpoint.h"
+#include "net/base/net_util.h"
+#include "net/dns/dns_config_service.h"
+
+namespace net {
+
+namespace {
+
+using base::files::FilePathWatcher;
+
+// fills in |dns_config| from |res|
+void ConfigFromResolver(const struct __res_state& res, DnsConfig* dns_config) {
+  CHECK(dns_config != NULL);
+  DCHECK(res.options & RES_INIT);
+  dns_config->nameservers.clear();
+  for (int i = 0; i < res.nscount; ++i) {
+    IPEndPoint ipe;
+    if (ipe.FromSockAddr(

[cbentzel, 2011/08/04 18:09:27]

You may want to handle IPv6 DNS servers here - or at least add a TODO. Looking
at glibc, there is an _u._ext which contains sockaddr_in6 for the IPv6
addresses. 

[szym, 2011/08/04 18:55:10]

I'll look into this.

+        reinterpret_cast<const struct sockaddr*>(&res.nsaddr_list[i]),
+        sizeof res.nsaddr_list[i])) {
+      dns_config->nameservers.push_back(ipe);
+    } else {
+      NOTREACHED() << "could not convert nsaddr";

[cbentzel, 2011/08/04 18:09:27]

I think this function should return a boolean, and be false in this case.
Putting NOTREACHED for IO from external libraries doesn't seem like a good idea.

[szym, 2011/08/04 18:55:10]

Right. I was swayed by IPEndPoint::FromSockAddr which cannot 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->domain = res.defdname;  // use only if |search| is empty

[cbentzel, 2011/08/04 18:09:27]

This should be conditional on dns_config->search.empty(). Looking at glibc,
defdname is used to store the actual strings that the dnsrch ones point to.

[szym, 2011/08/04 18:55:10]

Even though defdname stores all the strings in dnsrch, for all practical
purposes it is the first name in dnsrch (null-terminated) and that's how it will
be stored in DnsConfig. It is redundant, so how about I remove domain completely
and we use search[0] instead?

[cbentzel, 2011/08/04 20:01:21]

That seems good. 

+  dns_config->ndots = res.ndots;
+  dns_config->timeout = res.retrans;

[cbentzel, 2011/08/04 18:09:27]

I wish there was a way to distinguish whether these were unspecified and just
the default, or explicitly specified. We'd like to use our own retransmit/retry
logic but should maybe obey if they are explicitly specified. 

[szym, 2011/08/04 18:55:10]

I can't think of a way to do that without parsing /etc/resolv.conf ourselves.

[cbentzel, 2011/08/04 20:01:21]

Yeah, agree. 

+  dns_config->attempts = res.retry;
+  dns_config->rotate = res.options & RES_ROTATE;
+  dns_config->edns0 = res.options & RES_USE_EDNS0;

[cbentzel, 2011/08/04 18:09:27]

It's probably good to pull in all of these things. In the AsyncHostResolver
case, we may want to always fall back to the default host resolver if EDNS0 is
desired [since we don't currently support it] or any of the other configurations
are set.

[szym, 2011/08/04 18:55:10]

I'm not sure I understand. Should I remove those options?

[cbentzel, 2011/08/04 20:01:21]

No, keep them. I was musing about what we need to do if edns0 is set in the
AsyncHostResolver.

+}
+
+// fills in |dns_config| using res_ninit, returns true on success
+bool ReadConfig(DnsConfig* dns_config) {
+  // res_ninit is available on Linux (glibc 2+), Mac OS X, and Android.
+  // However, it might not be thread-safe on OpenBSD.

[cbentzel, 2011/08/04 18:09:27]

I think it would be good to limit to only one res_init at a time, for a couple
of reasons:

- Concerns about whether res_ninit is thread-safe on all systems
- If there are very rapid changes, only one event will fire on the caller. You
will need to remember that a file change happened while a res_init call is
pending on a worker thread to reissue, though.
- If 2 file changes come quickly, we'll have res_init called on two different
threads. The provided DnsConfig's may come out-of-order so the last one doesn't
match the most recent change to the filesystem.

[szym, 2011/08/04 18:55:10]

Done.

+  struct __res_state res;
+  if (res_ninit(&res) != 0)
+    return false;
+
+  ConfigFromResolver(res, dns_config);
+  return true;
+}
+
+class DnsConfigServicePosix : public DnsConfigService {
+ public:
+  DnsConfigServicePosix() {}
+  virtual ~DnsConfigServicePosix() {
+    // we must be destroyed on the same thread that called Watch
+    if (watcher_delegate_.get())
+      watcher_delegate_->Cancel();
+  }
+  virtual bool Watch(DnsConfigService::Delegate* delegate) OVERRIDE;
+
+ private:
+  // FilePathWatcher::Delegate is refcounted, so we separate it from the Service
+  // We'll also use it on the WorkerPool
+  class WatcherDelegate : public FilePathWatcher::Delegate {
+   public:
+    explicit WatcherDelegate(DnsConfigService::Delegate* delegate)
+      : message_loop_(base::MessageLoopProxy::CreateForCurrentThread()),
+        delegate_(delegate) {}
+
+    // FilePathWatcher::Delegate interface
+    virtual void OnFilePathChanged(const FilePath& path) OVERRIDE {
+      DCHECK(message_loop_->BelongsToCurrentThread());
+      if (!delegate_)
+        return;
+      if (!base::WorkerPool::PostTask(FROM_HERE, base::Bind(
+          &WatcherDelegate::DoRead, this), false)) {
+        delegate_->OnConfigError();

[cbentzel, 2011/08/04 18:09:27]

Yeah, I don't think you need to escalate this to the caller. It should be
handled internally as much as possible. 

[szym, 2011/08/04 18:55:10]

Done.

+      }
+    }
+
+    virtual void OnFilePathError(const FilePath& path) OVERRIDE {
+      DCHECK(message_loop_->BelongsToCurrentThread());
+      if (!delegate_)
+        return;
+      delegate_->OnConfigError();
+    }
+
+    // called on the worker thread
+    // reads DnsConfig and posts OnResultAvailable to |message_loop_|
+    void DoRead() {
+      // on the WorkerPool
+      DnsConfig config;
+      bool success = ReadConfig(&config);
+      message_loop_->PostTask(FROM_HERE, base::Bind(

[cbentzel, 2011/08/04 18:09:27]

If it wasn't successful, I think we should just skip posting a task back to the
message_loop_. 

[szym, 2011/08/04 18:55:10]

Can I have a stupid while(!the_thing_to_do); or should there be some pause, max
retry count, etc?

[cbentzel, 2011/08/04 20:01:21]

Well, if someone made a typo in resolv.conf and then fixes it, there should be
another OnFilePathChanged call which will trigger another worker. So I think you
can just do nothing if it fails.

+          &WatcherDelegate::OnResultAvailable, this, config, success));
+    }
+
+    // called on the delegate's thread
+    // communicates result from DnsConfigReader to the delegate
+    void OnResultAvailable(const DnsConfig &config, bool success) {
+      DCHECK(message_loop_->BelongsToCurrentThread());
+      if (!delegate_)
+        return;
+      if (!success) {
+        delegate_->OnConfigError();
+      } else if (config != last_config_) {
+        delegate_->OnConfigChanged(config);
+        last_config_ = config;
+      }
+    }
+
+    void Cancel() {
+      DCHECK(message_loop_->BelongsToCurrentThread());
+      delegate_ = NULL;
+    }
+
+   private:
+    virtual ~WatcherDelegate() {}
+
+    // message loop for the thread on which Watch is called
+    // this should be an IO loop
+    scoped_refptr<base::MessageLoopProxy> message_loop_;
+    DnsConfigService::Delegate* delegate_;
+    DnsConfig last_config_;
+  };
+
+  scoped_ptr<FilePathWatcher> resolv_file_watcher_;
+  scoped_refptr<WatcherDelegate> watcher_delegate_;
+};

[cbentzel, 2011/08/04 18:09:27]

DISALLOW_COPY_AND_ASSIGN

[szym, 2011/08/04 18:55:10]

Done.

+
+bool DnsConfigServicePosix::Watch(DnsConfigService::Delegate* delegate) {
+  DCHECK(!resolv_file_watcher_.get());
+  DCHECK(!watcher_delegate_.get());
+  DCHECK(MessageLoopForIO::current());
+
+  // start FilePathWatcher
+  FilePath path(FILE_PATH_LITERAL("/etc/resolv.conf"));

[cbentzel, 2011/08/04 18:24:56]

Is _PATH_RESCONF defined correctly in the different resolv.h's? If so, it may
make sense to use that instead of an explicit /etc/resolv.conf

[szym, 2011/08/04 18:55:10]

I'll add a #ifndef _PATH_RESCONF #define _PATH_RESCONF "..." in this file.

+  watcher_delegate_ = new WatcherDelegate(delegate);
+  resolv_file_watcher_.reset(new FilePathWatcher());
+  bool result = resolv_file_watcher_->Watch(path, watcher_delegate_);
+  // make the initial read
+  watcher_delegate_->OnFilePathChanged(path);
+  return result;
+}
+
+}  // namespace
+
+DnsConfigService* DnsConfigService::CreateSystemService() {
+  return new DnsConfigServicePosix();
+}

[cbentzel, 2011/08/04 18:09:27]

Nit: newline between here and // namespace net.

[szym, 2011/08/04 18:55:10]

Done.

+}  // namespace net
+