DnsConfigService and a posix implementation

net/dns/dns_config_service.h

+// 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.
+
+#ifndef NET_DNS_CONFIG_SERVICE_H_
+#define NET_DNS_CONFIG_SERVICE_H_
+#pragma once
+
+#include <list>
+#include <map>
+#include <string>
+#include <vector>
+
+#include "net/base/net_api.h"
+#include "net/base/net_util.h"
+
+namespace net {
+
+class IPEndPoint;
+
+// DnsConfig stores configuration of the system resolver.
+// To be used with AsyncHostResolver.

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

Nit: Remove the comment about "To be used with AsyncHostResolver" - other
consumers could exist.

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

Done.

[cbentzel, 2011/08/05 13:01:25]

It doesn't look like you uploaded a patch with this fix. Typically "Done" is
written after that case.

+// TODO(szym): determine the next query for a given host according to settings.

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

What is this TODO about?

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

Options |ndots| and |search| determine a sequence of FQDNs that should be
queried, until first success. I suppose this logic could be implemented in
DnsConfig. 

+struct DnsConfig {
+  // resolv.conf stuff -- standard on POSIX/glibc systems

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

Nit: I'd remove this comment as it's intended to be platform neutral.

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

Done.

+  // win stores a subset of those in the registry
+
+  DnsConfig()
+    : ndots(1),
+      timeout(5),   // see resolv.h RES_TIMEOUT
+      attempts(2),  // see resolv.h RES_DFLRETRY
+      rotate(false),
+      edns0(false) {}
+
+  // list of name server addresses

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

General issue throughout this CL - the comments don't follow the style guide,
which prefers capitalization on the first word, punctuation at the end, and
generally looking like English sentences.

See some examples like
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Comments

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

Done.

+  std::vector<IPEndPoint> nameservers;
+  // suffix search list, overrides |domain|,
+  // used on first lookup when #dots in given name < |ndots|
+  std::vector<std::string> search;
+  // use only if |search| is empty
+  std::string domain;
+
+  // options; see man resolv.conf
+  // min. number of dots before global resolution precedes |search|
+  int ndots;
+  // in seconds between retransmissions, see res_state.retrans
+  int timeout;
+  // max. number of retries, see res_state.retry
+  int attempts;
+  // round robin in |nameservers|
+  bool rotate;
+  // enable EDNS0
+  bool edns0;
+
+  // /etc/hosts stuff
+  // NOTE: HostCache stores these in reverse order: hostname -> AddressList
+  std::map<IPAddressNumber, std::list<std::string>  > hosts;

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

This isn't filled in right now - I'd remove until it is.

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

Done.

+
+  // so that we only notify on real change
+  bool operator==(const DnsConfig& d) const {

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

Style guide explicitly is against using operator== or other operator
overloading.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Operator_Overl...

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

Done.

+    return (nameservers == d.nameservers) &&
+           (search == d.search) &&
+           (domain == d.domain) &&
+           (ndots == d.ndots) &&
+           (timeout == d.timeout) &&
+           (attempts == d.attempts) &&
+           (rotate == d.rotate) &&
+           (edns0 == d.edns0);
+  }
+
+  bool operator!=(const DnsConfig& d) const {
+    return !(*this == d);
+  }
+};
+
+// DnsConfigService fetches system DNS configuration as DnsConfig

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

Nit: period at the end.

Maybe clearer to document as 

// Service for watching when the DNS settings have changed.

This would make it a closer match to the comment in proxy_config_service.h

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

Done.

+// Depending on the platform, watches files in /etc/ or win registry.
+class DnsConfigService {
+ public:
+  // Callback interface for the client. The delegate is called on the same

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

You mentioned using an observer approach before - I think this is probably
appropriate over a delegate, if only to make this consistent with
ProxyConfigService. 

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

Are we talking about just a name change? Delegate -> Observer?

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

No - actually do an AddObserver/RemoveObserver on DnsConfigService and support
multiple observers.

+  // thread as Watch(). Delegate must outlive the service.
+  class Delegate {
+   public:
+    virtual ~Delegate() {}
+
+    // Called only when |dns_config| is different from the last check.
+    virtual void OnConfigChanged(const DnsConfig& dns_config) = 0;
+
+    // Called when config could not be accessed/parsed.
+    virtual void OnConfigError() = 0;

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

CONCERN: Does this actually need to be escalated at all? Could you remove this
and do restarts of watches if /etc/resolv.conf gets deleted, or is malformed?
It's a much simpler API if OnConfigChanged is only called when 

a) The new configuration is valid
b) The DnsConfig is different from the last time this was called [so it doesn't
get triggered if someone just adds a comment to /etc/resolv.conf for example]

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

The a) and b) conditions for OnConfigChanged are already provided. I'll wrap up
restarts when FilePathWatcher signal OnFilePathError. Should such restarts have
a bit of a back-off between them?

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

Yeah. 

+  };
+
+  // Creates the platform-specific DnsConfigService.
+  static DnsConfigService* CreateSystemService();
+
+  DnsConfigService() {}
+  virtual ~DnsConfigService() {}
+
+  // Starts watching system configuration for changes. Also attempts to read the
+  // configuration immediately and pass the result to |delegate|.
+  // NOTE: Call at most once. The current thread must have an IO loop
+  // (for base::files::FilePathWatcher).
+  virtual bool Watch(Delegate* delegate) = 0;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(DnsConfigService);
+};
+
+}  // namespace net
+
+#endif  // NET_DNS_CONFIG_SERVICE_H_
+