Linux: Monitor resolv.conf for changes and use that to reload resolver.

Linux: Monitor resolv.conf for changes and use that to reload resolver.

BUG=67734
TEST=manual testing by poking at resolv.conf

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=99666

[Craig, 2011-04-27 15:20:43]

willchan: would you mind taking a look at this please? It's not ready/finished
yet but I'm keen to see if I'm vaguely on the right track before continuing. I
can split this into multiple patches if desired.

Suggested "review" order:

network_change_notifier: I've added a DNSObserver modelled on the existing
IPAddressObserver and added some filewatcher code into the existing
NetworkChangeNotifierLinux::Thread. I'm only watching resolv.conf at the moment
but I should probably watch /etc/hosts too.

dns_reloader: this replaces the existing dns_reload_timer and is based on that
code. I've made it take responsibility for actually doing the
res_ninit/res_nclose stuff now which means we can hopefully kill the low_level
peeking into the resolver state that host_resolver_impl and dnsrr_resolver did
before trying to res_nclose. It uses a generation number that increments each
time resolv.conf changes in a similar fashion to what rojer9 did in
http://codereview.chromium.org/6033006/

host_resolver_impl and dnsrr_resolver: (not finished) here we should  observe
DNS changes and take appropriate actions such as flushing the cache etc. There
are also some changes to work with the new dns_reloader. I haven't looked in too
much detail at dnsrr_resolver yet.

(90% of the redness in the try jobs seems not to be me btw. but I do need to
update suppressions for the long-lived worker thread though)

Thank you!

[Craig, 2011-07-12 17:25:39]

This should be ready for review now. PTAL or suggest someone who can review pls.
Thank you.


On 2011/04/27 15:20:43, Craig wrote:
> willchan: would you mind taking a look at this please? It's not ready/finished
> yet but I'm keen to see if I'm vaguely on the right track before continuing. I
> can split this into multiple patches if desired.
> 
> Suggested "review" order:
> 
> network_change_notifier: I've added a DNSObserver modelled on the existing
> IPAddressObserver and added some filewatcher code into the existing
> NetworkChangeNotifierLinux::Thread. I'm only watching resolv.conf at the
moment
> but I should probably watch /etc/hosts too.
> 
> dns_reloader: this replaces the existing dns_reload_timer and is based on that
> code. I've made it take responsibility for actually doing the
> res_ninit/res_nclose stuff now which means we can hopefully kill the low_level
> peeking into the resolver state that host_resolver_impl and dnsrr_resolver did
> before trying to res_nclose. It uses a generation number that increments each
> time resolv.conf changes in a similar fashion to what rojer9 did in
> http://codereview.chromium.org/6033006/
> 
> host_resolver_impl and dnsrr_resolver: (not finished) here we should  observe
> DNS changes and take appropriate actions such as flushing the cache etc. There
> are also some changes to work with the new dns_reloader. I haven't looked in
too
> much detail at dnsrr_resolver yet.
> 
> (90% of the redness in the try jobs seems not to be me btw. but I do need to
> update suppressions for the long-lived worker thread though)
> 
> Thank you!

[willchan no longer on Chromium, 2011-07-13 12:10:36]

Sorry for missing this earlier, I need to take another look. But for now, let's
add agl and eroman. I'm also fine with eroman sending approval for this
changelist, but I'll try to take another look if I get a chance.

http://codereview.chromium.org/6903061/diff/17001/net/base/dns_reloader.h
File net/base/dns_reloader.h (right):

http://codereview.chromium.org/6903061/diff/17001/net/base/dns_reloader.h#new...
net/base/dns_reloader.h:1: // Copyright (c) 2009 The Chromium Authors. All
rights reserved.
Wrong copyright date

http://codereview.chromium.org/6903061/diff/17001/net/base/network_change_not...
File net/base/network_change_notifier.h (right):

http://codereview.chromium.org/6903061/diff/17001/net/base/network_change_not...
net/base/network_change_notifier.h:53: class DNSObserver {
Please make this part of NET_API.

[agl, 2011-07-13 22:17:48]

LGTM I think

http://codereview.chromium.org/6903061/diff/17001/net/base/dns_reloader.cc
File net/base/dns_reloader.cc (right):

http://codereview.chromium.org/6903061/diff/17001/net/base/dns_reloader.cc#ne...
net/base/dns_reloader.cc:1: // Copyright (c) 2010 The Chromium Authors. All
rights reserved.
wrong year.

http://codereview.chromium.org/6903061/diff/17001/net/base/dns_reloader.cc#ne...
net/base/dns_reloader.cc:42: virtual void OnDNSChanged() {
needs a comment to say which thread it runs on. (An assert is a perfectly good
comment.)

http://codereview.chromium.org/6903061/diff/17001/net/base/dns_reloader.cc#ne...
net/base/dns_reloader.cc:72: int resolver_generation_;
it appears that this is read/written across different threads so I think it
should be guarded by a lock. There's always some case there some CPU won't see
updates/will write crap without a lock. You could look at the atomic stuff in
base/, but I don't immediately see a good candidate wrapper around subtle::.

http://codereview.chromium.org/6903061/diff/17001/net/base/dns_reloader.cc#ne...
net/base/dns_reloader.cc:110: void DnsReloaderMaybeReload() {
Needs a comment to say that it runs on any DNS worker thread.

[willchan no longer on Chromium, 2011-07-14 15:42:32]

http://codereview.chromium.org/6903061/diff/17001/net/base/dns_reloader.cc
File net/base/dns_reloader.cc (right):

http://codereview.chromium.org/6903061/diff/17001/net/base/dns_reloader.cc#ne...
net/base/dns_reloader.cc:77: // multiple times. Initialize the
ThreadLocalStorage slot only once.
Really? Which test does this?

http://codereview.chromium.org/6903061/diff/17001/net/base/dns_reloader.cc#ne...
net/base/dns_reloader.cc:88: static base::ThreadLocalStorage::Slot tls_index_ ;
We generally use ThreadLocalPointer instead of ThreadLocalStorage, which is used
for implementing ThreadLocalPointer.

http://codereview.chromium.org/6903061/diff/17001/net/base/dns_reloader.h
File net/base/dns_reloader.h (right):

http://codereview.chromium.org/6903061/diff/17001/net/base/dns_reloader.h#new...
net/base/dns_reloader.h:15: void DnsReloaderInit();
This should probably be called EnsureDnsReloaderInit() or something, like how we
do EnsureWinsockInit().

http://codereview.chromium.org/6903061/diff/17001/net/base/dnsrr_resolver.cc
File net/base/dnsrr_resolver.cc (right):

http://codereview.chromium.org/6903061/diff/17001/net/base/dnsrr_resolver.cc#...
net/base/dnsrr_resolver.cc:196: #if !defined(OS_MACOSX) && !defined(OS_OPENBSD)
It looks like this code will always be executed in the non MACOSX/OPENBSD case,
so perhaps we can move it outside of this if (r) and change the previous
#if/#endif block to #if/#else/#endif?

[Craig, 2011-07-14 17:23:30]

Thanks for the comments! This is just a quick reply to respond to two of the
issues raised but I'll take a closer look at everything in the next day or two
and fix things up appropriately.

http://codereview.chromium.org/6903061/diff/17001/net/base/dns_reloader.cc
File net/base/dns_reloader.cc (right):

http://codereview.chromium.org/6903061/diff/17001/net/base/dns_reloader.cc#ne...
net/base/dns_reloader.cc:72: int resolver_generation_;
On 2011/07/13 22:17:48, agl wrote:
> it appears that this is read/written across different threads so I think it
> should be guarded by a lock. There's always some case there some CPU won't see
> updates/will write crap without a lock. You could look at the atomic stuff in
> base/, but I don't immediately see a good candidate wrapper around subtle::.

I assumed that it was thread-safe based on the assumption that access to the int
should be atomic and that it is only written to from one thread (and read from
several other threads) but I'll throw a lock at the problem to be sure.

http://codereview.chromium.org/6903061/diff/17001/net/base/dns_reloader.cc#ne...
net/base/dns_reloader.cc:77: // multiple times. Initialize the
ThreadLocalStorage slot only once.
On 2011/07/14 15:42:32, willchan wrote:
> Really? Which test does this?

I copied the code blindly from dns_reload_timer :( The comment/code comes from
r33594 from Pawel - back then the net test suite was being tweaked with a call
to test_suite.EnforceTestIsolation() that seemed to fiddle with LazyInstance in
interesting ways. Test isolation seems to have been backed out in r56229 but
this snippet was left in place. I'll stare at this in more detail but it's
probably not necessary anymore and I shouldn't just have copied it without
making sure I knew what it did.

[Craig, 2011-07-18 16:18:12]

Responses to all issues raised are tagged on below. I've also tweaked the
valgrind suppressions and tossed a NOTREACHED into the destructor. Please take
another look. Thank you!

http://codereview.chromium.org/6903061/diff/17001/net/base/dns_reloader.cc
File net/base/dns_reloader.cc (right):

http://codereview.chromium.org/6903061/diff/17001/net/base/dns_reloader.cc#ne...
net/base/dns_reloader.cc:1: // Copyright (c) 2010 The Chromium Authors. All
rights reserved.
On 2011/07/13 22:17:48, agl wrote:
> wrong year.

Done.

http://codereview.chromium.org/6903061/diff/17001/net/base/dns_reloader.cc#ne...
net/base/dns_reloader.cc:42: virtual void OnDNSChanged() {
On 2011/07/13 22:17:48, agl wrote:
> needs a comment to say which thread it runs on. (An assert is a perfectly good
> comment.)

Done with a DCHECK but it's a touch ugly.

http://codereview.chromium.org/6903061/diff/17001/net/base/dns_reloader.cc#ne...
net/base/dns_reloader.cc:72: int resolver_generation_;
On 2011/07/14 17:23:30, Craig wrote:
> On 2011/07/13 22:17:48, agl wrote:
> > it appears that this is read/written across different threads so I think it
> > should be guarded by a lock. There's always some case there some CPU won't
see
> > updates/will write crap without a lock. You could look at the atomic stuff
in
> > base/, but I don't immediately see a good candidate wrapper around subtle::.
> 
> I assumed that it was thread-safe based on the assumption that access to the
int
> should be atomic and that it is only written to from one thread (and read from
> several other threads) but I'll throw a lock at the problem to be sure.

Done.

http://codereview.chromium.org/6903061/diff/17001/net/base/dns_reloader.cc#ne...
net/base/dns_reloader.cc:77: // multiple times. Initialize the
ThreadLocalStorage slot only once.
On 2011/07/14 17:23:30, Craig wrote:
> On 2011/07/14 15:42:32, willchan wrote:
> > Really? Which test does this?
> 
> I copied the code blindly from dns_reload_timer :( The comment/code comes from
> r33594 from Pawel - back then the net test suite was being tweaked with a call
> to test_suite.EnforceTestIsolation() that seemed to fiddle with LazyInstance
in
> interesting ways. Test isolation seems to have been backed out in r56229 but
> this snippet was left in place. I'll stare at this in more detail but it's
> probably not necessary anymore and I shouldn't just have copied it without
> making sure I knew what it did.

Done.

http://codereview.chromium.org/6903061/diff/17001/net/base/dns_reloader.cc#ne...
net/base/dns_reloader.cc:88: static base::ThreadLocalStorage::Slot tls_index_ ;
On 2011/07/14 15:42:32, willchan wrote:
> We generally use ThreadLocalPointer instead of ThreadLocalStorage, which is
used
> for implementing ThreadLocalPointer.

With ThreadLocalStorage, I  can hook into the SlotReturnFunction and use that to
do some cleanups but I can't see how I'd do that with a ThreadLocalPointer so I
haven't changed this for now.

http://codereview.chromium.org/6903061/diff/17001/net/base/dns_reloader.cc#ne...
net/base/dns_reloader.cc:110: void DnsReloaderMaybeReload() {
On 2011/07/13 22:17:48, agl wrote:
> Needs a comment to say that it runs on any DNS worker thread.

Done.

http://codereview.chromium.org/6903061/diff/17001/net/base/dns_reloader.h
File net/base/dns_reloader.h (right):

http://codereview.chromium.org/6903061/diff/17001/net/base/dns_reloader.h#new...
net/base/dns_reloader.h:1: // Copyright (c) 2009 The Chromium Authors. All
rights reserved.
On 2011/07/13 12:10:37, willchan wrote:
> Wrong copyright date

Done.

http://codereview.chromium.org/6903061/diff/17001/net/base/dns_reloader.h#new...
net/base/dns_reloader.h:15: void DnsReloaderInit();
On 2011/07/14 15:42:32, willchan wrote:
> This should probably be called EnsureDnsReloaderInit() or something, like how
we
> do EnsureWinsockInit().

Done.

http://codereview.chromium.org/6903061/diff/17001/net/base/dnsrr_resolver.cc
File net/base/dnsrr_resolver.cc (right):

http://codereview.chromium.org/6903061/diff/17001/net/base/dnsrr_resolver.cc#...
net/base/dnsrr_resolver.cc:196: #if !defined(OS_MACOSX) && !defined(OS_OPENBSD)
On 2011/07/14 15:42:32, willchan wrote:
> It looks like this code will always be executed in the non MACOSX/OPENBSD
case,
> so perhaps we can move it outside of this if (r) and change the previous
> #if/#endif block to #if/#else/#endif?

Done.

http://codereview.chromium.org/6903061/diff/17001/net/base/network_change_not...
File net/base/network_change_notifier.h (right):

http://codereview.chromium.org/6903061/diff/17001/net/base/network_change_not...
net/base/network_change_notifier.h:53: class DNSObserver {
On 2011/07/13 12:10:37, willchan wrote:
> Please make this part of NET_API.

Done.

[willchan no longer on Chromium, 2011-07-19 15:45:34]

LGTM, I think it'd be cool if eroman reviewed this too, but it's fine to commit
with just my LGTM.

http://codereview.chromium.org/6903061/diff/35002/net/base/dns_reloader.h
File net/base/dns_reloader.h (right):

http://codereview.chromium.org/6903061/diff/35002/net/base/dns_reloader.h#new...
net/base/dns_reloader.h:14: // Call on the IO thread before calling
DnsReloaderMaybeReload() anywhere.
Please call this "network" thread. net/ does not have a concept of the chrome IO
thread.

http://codereview.chromium.org/6903061/diff/35002/net/base/dns_reloader.h#new...
net/base/dns_reloader.h:22: #endif
I am anal and like to terminate #endifs with the condition in comments.

http://codereview.chromium.org/6903061/diff/35002/net/base/network_change_not...
File net/base/network_change_notifier_linux.cc (right):

http://codereview.chromium.org/6903061/diff/35002/net/base/network_change_not...
net/base/network_change_notifier_linux.cc:107:
&NetworkChangeNotifierLinux::Thread::NotifyObserversOfDNSChange,
Unless you *need* the new callback system, please use the old one for now. We're
still working out some critical things and want to make sure it's stable first.
In case things change, I'd like to minimize the number of existing consumers
we'd have to fix.

It looks like you might need it for base::Unretained() which is fine. Just
checking.

http://codereview.chromium.org/6903061/diff/35002/net/net.gyp
File net/net.gyp (right):

http://codereview.chromium.org/6903061/diff/35002/net/net.gyp#newcode78
net/net.gyp:78: 'base/dns_reloader.cc',
Resort

[Craig, 2011-07-25 18:00:55]

On 2011/07/19 15:45:34, willchan wrote:
> LGTM, I think it'd be cool if eroman reviewed this too, but it's fine to
commit
> with just my LGTM.

Awesome, thank you. Sorry for the delay in responding ... its been a crazy week.

>http://codereview.chromium.org/6903061/diff/35002/net/base/network_change_notifier_linux.cc#newcode107
> net/base/network_change_notifier_linux.cc:107:
> &NetworkChangeNotifierLinux::Thread::NotifyObserversOfDNSChange,
> Unless you *need* the new callback system, please use the old one for now.
We're
> still working out some critical things and want to make sure it's stable
first.
> In case things change, I'd like to minimize the number of existing consumers
> we'd have to fix.
> 
> It looks like you might need it for base::Unretained() which is fine. Just
> checking.

I used the new callback system because neither the old nor the new callback
system was familiar to me and there was a lot of excitement around the new stuff
:) NetworkChangeNotifierLinux::Thread isn't refcounted (and since it contains
the filepathwatchers and fpw delegate that doesn't seem like a good plan) so I
used unretained as suggested by bind_helpers.h. It looks like I should be able
to use the old callback system for that too though so I'll take a stab at that
and roll in the other cleanups you suggested too...

[eroman, 2011-07-25 20:13:02]

General design LGTM. I have some implementation nits:

http://codereview.chromium.org/6903061/diff/35002/net/base/dns_reloader.cc
File net/base/dns_reloader.cc (right):

http://codereview.chromium.org/6903061/diff/35002/net/base/dns_reloader.cc#ne...
net/base/dns_reloader.cc:33: // OpenBSD does not have thread-safe
res_ninit/res_nclose so we can't do
Actually, it looks like since these calls are protected by locks,
res_ninit/res_nclose will never be run in parallel. Although maybe the
threadsafety issues has to do with running getaddrinfo() at the same time?

http://codereview.chromium.org/6903061/diff/35002/net/base/dns_reloader.cc#ne...
net/base/dns_reloader.cc:46: DCHECK(MessageLoop::current()->thread_name() ==
"Chrome_IOThread");
Please avoid referencing chrome thread names (since this code might run in other
projects alltogether, or even things like the chrome "service process" which may
name threads differently).

If you still want an assertion, you can test the messageloop's type is an IO
thread (which at least catches incorrect runs on worker threads and/or ui
threads).

http://codereview.chromium.org/6903061/diff/35002/net/base/dns_reloader.cc#ne...
net/base/dns_reloader.cc:82: DnsReloader() : resolver_generation_(0) {
style-nit: please declare methods above data members. In this case, the ctor and
dtor should be at the top of this privae section.

http://codereview.chromium.org/6903061/diff/35002/net/base/host_resolver_proc.cc
File net/base/host_resolver_proc.cc (right):

http://codereview.chromium.org/6903061/diff/35002/net/base/host_resolver_proc...
net/base/host_resolver_proc.cc:199: #if defined(OS_POSIX) && !defined(OS_MACOSX)
&& !defined(OS_OPENBSD) && \
nit: I wander if it would be cleaner to internalize the platform checks into
DnsReloaderMaybeReload() itself -- i.e. define it on all platforms, but have it
be a no-op on platforms where it isn't necessary (or isn't supported).

http://codereview.chromium.org/6903061/diff/35002/net/base/network_change_not...
File net/base/network_change_notifier_linux.cc (right):

http://codereview.chromium.org/6903061/diff/35002/net/base/network_change_not...
net/base/network_change_notifier_linux.cc:112: LOG(ERROR) << "Failed to setup
watch for /etc/resolv.conf";
I wander if we need to fail harder than logging an error.

Before on getaddrinfo() failures we would eventually run res_ninit(), whereas
now we will not unless we have observed a file change.

Fine for now, but we may want to follow-up with some histograms to verify people
aren't hitting this condition.

http://codereview.chromium.org/6903061/diff/35002/tools/valgrind/memcheck/sup...
File tools/valgrind/memcheck/suppressions.txt (right):

http://codereview.chromium.org/6903061/diff/35002/tools/valgrind/memcheck/sup...
tools/valgrind/memcheck/suppressions.txt:4847:
fun:_ZN12_GLOBAL__N_111DnsReloader11MaybeReloadEv
good catch on updating these!

[Craig, 2011-07-26 15:15:31]

"quick" response to the openbsd issue below. Thanks for the comments! I'll
cleanup the other issues too but I'm still a little swamped with other issues
for the next couple of days.

http://codereview.chromium.org/6903061/diff/35002/net/base/dns_reloader.cc
File net/base/dns_reloader.cc (right):

http://codereview.chromium.org/6903061/diff/35002/net/base/dns_reloader.cc#ne...
net/base/dns_reloader.cc:33: // OpenBSD does not have thread-safe
res_ninit/res_nclose so we can't do
On 2011/07/25 20:13:03, eroman wrote:
> Actually, it looks like since these calls are protected by locks,
> res_ninit/res_nclose will never be run in parallel. Although maybe the
> threadsafety issues has to do with running getaddrinfo() at the same time?

Ah, interesting!

I'm not that clued up on *BSD so treat these comments with caution - the
original comment there comes from pvalchev in
http://codereview.chromium.org/556068 btw.

We didn't have locks back then but it seems as if openbsd doesn't have
res_ninit/res_nclose at all, only res_init/close. The openbsd resolver does seem
to stat resolv.conf internally after 30 seconds and I think it calls init
internally as required so I'd speculate that things may work reasonably on
openbsd even without any DnsReloader support - see
http://openbsd.monkey.org/tech/200503/msg00114.html

It's not exactly clear how badly their resolver is affected by thread safety
issues though -  the comment in rev 1.28 of 
http://www.openbsd.org/cgi-bin/cvsweb/src/lib/libc/net/res_init.c says "thread
safer libc (note: safer, not safe)". There's an open bug for this stuff
crbug.com/33481 so I'm tempted to leave this as is rather than enabling this
code on openbsd (and, for example, using res_init/close instead of ninit/nclose
for them). If I did enable it, we'd still need filepathwatcher support too of
course since I've killed the timer stuff.

[eroman, 2011-07-26 16:53:13]

http://codereview.chromium.org/6903061/diff/35002/net/base/dns_reloader.cc
File net/base/dns_reloader.cc (right):

http://codereview.chromium.org/6903061/diff/35002/net/base/dns_reloader.cc#ne...
net/base/dns_reloader.cc:33: // OpenBSD does not have thread-safe
res_ninit/res_nclose so we can't do
On 2011/07/26 15:15:32, Craig wrote:
> On 2011/07/25 20:13:03, eroman wrote:
> > Actually, it looks like since these calls are protected by locks,
> > res_ninit/res_nclose will never be run in parallel. Although maybe the
> > threadsafety issues has to do with running getaddrinfo() at the same time?
> 
> Ah, interesting!
> 
> I'm not that clued up on *BSD so treat these comments with caution - the
> original comment there comes from pvalchev in
> http://codereview.chromium.org/556068 btw.
> 
> We didn't have locks back then but it seems as if openbsd doesn't have
> res_ninit/res_nclose at all, only res_init/close. The openbsd resolver does
seem
> to stat resolv.conf internally after 30 seconds and I think it calls init
> internally as required so I'd speculate that things may work reasonably on
> openbsd even without any DnsReloader support - see
> http://openbsd.monkey.org/tech/200503/msg00114.html
> 
> It's not exactly clear how badly their resolver is affected by thread safety
> issues though -  the comment in rev 1.28 of 
> http://www.openbsd.org/cgi-bin/cvsweb/src/lib/libc/net/res_init.c says "thread
> safer libc (note: safer, not safe)". There's an open bug for this stuff
> crbug.com/33481 so I'm tempted to leave this as is rather than enabling this
> code on openbsd (and, for example, using res_init/close instead of
ninit/nclose
> for them). If I did enable it, we'd still need filepathwatcher support too of
> course since I've killed the timer stuff.

Agreed. I definitely am not suggesting you enable BSD support as part of this
CL. I just figured I would ask these questions while I had an expert on the line
:)

[Craig, 2011-08-02 04:58:35]

Still working on this btw. but I am temporarily bogged down with RL issues. I'm
chasing what looks like a bug in FilePathWatcher which has me worried - full
details will follow but it looks like it doesn't play well with symlinks and
that may be a show stopper given the crazy things people do with resolv.conf on
various distros :(

[cbentzel, 2011-08-02 12:03:47]

Adding szym to the cc list - he's working on a similar change to watch
/etc/resolv.conf [and /etc/hosts] to update the AsyncHostResolver with a new DNS
configuration each time. Perhaps these could be combined.

[Craig, 2011-08-03 19:30:58]

Updated with review fixes (it's rebased too so there is extra noise when
compared to the previous patchset, sorry).

In addition to the comments below I've taking the mildly paranoid step of
disabling the delegate callback in the NetworkChangeNotifierLinux::Thread
cleanup to avoid potential issues. In practice, the messageloop destruction
wipes all of the watches in FPW so the callback won't fire anyway but it seemed
unreasonable to rely on that rather than being a bit more defensive about the
shutdown safety.

I'm going to stare at crbug.com/91561 now but will probably only get to
experiment with that later in the week.

http://codereview.chromium.org/6903061/diff/35002/net/base/dns_reloader.cc
File net/base/dns_reloader.cc (right):

http://codereview.chromium.org/6903061/diff/35002/net/base/dns_reloader.cc#ne...
net/base/dns_reloader.cc:46: DCHECK(MessageLoop::current()->thread_name() ==
"Chrome_IOThread");
On 2011/07/25 20:13:03, eroman wrote:
> Please avoid referencing chrome thread names (since this code might run in
other
> projects alltogether, or even things like the chrome "service process" which
may
> name threads differently).
> 
> If you still want an assertion, you can test the messageloop's type is an IO
> thread (which at least catches incorrect runs on worker threads and/or ui
> threads).

Done.

http://codereview.chromium.org/6903061/diff/35002/net/base/dns_reloader.cc#ne...
net/base/dns_reloader.cc:82: DnsReloader() : resolver_generation_(0) {
On 2011/07/25 20:13:03, eroman wrote:
> style-nit: please declare methods above data members. In this case, the ctor
and
> dtor should be at the top of this privae section.

Done.

http://codereview.chromium.org/6903061/diff/35002/net/base/dns_reloader.h
File net/base/dns_reloader.h (right):

http://codereview.chromium.org/6903061/diff/35002/net/base/dns_reloader.h#new...
net/base/dns_reloader.h:14: // Call on the IO thread before calling
DnsReloaderMaybeReload() anywhere.
On 2011/07/19 15:45:34, willchan wrote:
> Please call this "network" thread. net/ does not have a concept of the chrome
IO
> thread.

Done.

http://codereview.chromium.org/6903061/diff/35002/net/base/dns_reloader.h#new...
net/base/dns_reloader.h:22: #endif
On 2011/07/19 15:45:34, willchan wrote:
> I am anal and like to terminate #endifs with the condition in comments.

Done.

http://codereview.chromium.org/6903061/diff/35002/net/base/host_resolver_proc.cc
File net/base/host_resolver_proc.cc (right):

http://codereview.chromium.org/6903061/diff/35002/net/base/host_resolver_proc...
net/base/host_resolver_proc.cc:199: #if defined(OS_POSIX) && !defined(OS_MACOSX)
&& !defined(OS_OPENBSD) && \
On 2011/07/25 20:13:03, eroman wrote:
> nit: I wander if it would be cleaner to internalize the platform checks into
> DnsReloaderMaybeReload() itself -- i.e. define it on all platforms, but have
it
> be a no-op on platforms where it isn't necessary (or isn't supported).

I'll do this but I'm going to chase the symlink stuff around first and report
back on that.

http://codereview.chromium.org/6903061/diff/35002/net/base/network_change_not...
File net/base/network_change_notifier_linux.cc (right):

http://codereview.chromium.org/6903061/diff/35002/net/base/network_change_not...
net/base/network_change_notifier_linux.cc:107:
&NetworkChangeNotifierLinux::Thread::NotifyObserversOfDNSChange,
On 2011/07/19 15:45:34, willchan wrote:
> Unless you *need* the new callback system, please use the old one for now.
We're
> still working out some critical things and want to make sure it's stable
first.
> In case things change, I'd like to minimize the number of existing consumers
> we'd have to fix.
> 
> It looks like you might need it for base::Unretained() which is fine. Just
> checking.

Converted to use callback_old.

http://codereview.chromium.org/6903061/diff/35002/net/base/network_change_not...
net/base/network_change_notifier_linux.cc:112: LOG(ERROR) << "Failed to setup
watch for /etc/resolv.conf";
On 2011/07/25 20:13:03, eroman wrote:
> I wander if we need to fail harder than logging an error.
> 
> Before on getaddrinfo() failures we would eventually run res_ninit(), whereas
> now we will not unless we have observed a file change.

Yes, very true.

> Fine for now, but we may want to follow-up with some histograms to verify
people
> aren't hitting this condition.

OK. I glanced at the internals of the linux file_path_watcher and it never fails
when adding watches so the LOG code here won't be triggered at the moment but
arguably it could if the implementation of FPW changes. I filed crbug.com/91132
as I initially thought returning true was wrong but have marked that as invalid
now.

This however made me think of another problem scenario - symlinks. 
FilePathWatcher doesn't seem to notice changes to symlinks if the change happens
directly to the target (crbug.com/91561) so I have normalized the path here but
I'm still a bit worried about this hack - I can imagine strange scenarios in
which the /etc/resolv.conf symlink is changed to point to some other target
rather than the (already) normalized target being updated which would cause even
the latest code here to break. The right fix IMHO would be for FilePathWatcher
to deal with symlinks correctly so I'm going to chase that around a bit as time
permits.

http://codereview.chromium.org/6903061/diff/35002/net/net.gyp
File net/net.gyp (right):

http://codereview.chromium.org/6903061/diff/35002/net/net.gyp#newcode78
net/net.gyp:78: 'base/dns_reloader.cc',
On 2011/07/19 15:45:34, willchan wrote:
> Resort

Done.

[eroman, 2011-08-11 01:57:55]

LGTM.

Sorry for keeping you waiting, after coming back from vacation this was buried
deep in my inbox ;)

http://codereview.chromium.org/6903061/diff/46001/net/base/dns_reloader.cc
File net/base/dns_reloader.cc (right):

http://codereview.chromium.org/6903061/diff/46001/net/base/dns_reloader.cc#ne...
net/base/dns_reloader.cc:45: virtual void OnDNSChanged() {
nit: Could you add the OVERRIDE macro here?

http://codereview.chromium.org/6903061/diff/46001/net/base/dns_reloader.cc#ne...
net/base/dns_reloader.cc:46: DCHECK(MessageLoop::current()->type() ==
MessageLoop::TYPE_IO);
nit: DCHECK_EQ

http://codereview.chromium.org/6903061/diff/46001/net/base/dns_reloader.cc#ne...
net/base/dns_reloader.cc:85:
net::NetworkChangeNotifier::RemoveDNSObserver(this);
Please remove this (if it is notreached).

Plus it might not be safe to call this around the time the NetworkChangeNotifier
is destroyed (since that code doesn't use locks).

http://codereview.chromium.org/6903061/diff/46001/net/base/network_change_not...
File net/base/network_change_notifier_linux.cc (right):

http://codereview.chromium.org/6903061/diff/46001/net/base/network_change_not...
net/base/network_change_notifier_linux.cc:33: callback_.reset(callback);
nit: I suggest moving this into the initializer list.

http://codereview.chromium.org/6903061/diff/46001/net/base/network_change_not...
net/base/network_change_notifier_linux.cc:37: callback_enabled = false;
Not sure the bool is necessary, could do callback_.reset(), and test for if
(callback_.get() when running.

http://codereview.chromium.org/6903061/diff/46001/net/base/network_change_not...
net/base/network_change_notifier_linux.cc:40: virtual void
OnFilePathChanged(const FilePath& path);
nit: Can you add OVERRIDE annotation?

http://codereview.chromium.org/6903061/diff/46001/net/base/network_change_not...
net/base/network_change_notifier_linux.cc:41: private:
[optional]: I suggest hooking OnFilePathError() and printing an error message
(could help with debugging should the watching go awry).

http://codereview.chromium.org/6903061/diff/46001/net/base/network_change_not...
net/base/network_change_notifier_linux.cc:150:
file_watcher_delegate_->DisableCallback();
I think it would be sufficient to reset the file watchers instead (since they
cleanup on destruction).

  resolve_file_watcher_.reset();
  hosts_file_watcher_.reset();

[Craig, 2011-08-15 17:10:56]

Thanks for the review! I've done the necessary cleanups. Sorry for the sloppy
code I was cranking out and the delay in responding - I've been chasing other
problems around.

In patch set 12 I have also reverted the NormalizeFilePath hack for the symlink
issue as ideally I'd like to fix that properly at the filepathwatcher level
(hopefully with a suitably fixed up version of
http://codereview.chromium.org/7583034/).

From a quick glance at szym's DnsConfigService
(http://codereview.chromium.org/7518028/), it doesn't look like it is dependent
on the changes to NetworkChangeNotifier here so I think holding off committing
this until the filepathwatcher fix is in should be ok (szym's code will be
subject to the same symlink issues in filepathwatcher btw. until that is fixed).

Comments?

http://codereview.chromium.org/6903061/diff/46001/net/base/dns_reloader.cc
File net/base/dns_reloader.cc (right):

http://codereview.chromium.org/6903061/diff/46001/net/base/dns_reloader.cc#ne...
net/base/dns_reloader.cc:45: virtual void OnDNSChanged() {
On 2011/08/11 01:57:55, eroman wrote:
> nit: Could you add the OVERRIDE macro here?

Done.

http://codereview.chromium.org/6903061/diff/46001/net/base/dns_reloader.cc#ne...
net/base/dns_reloader.cc:46: DCHECK(MessageLoop::current()->type() ==
MessageLoop::TYPE_IO);
On 2011/08/11 01:57:55, eroman wrote:
> nit: DCHECK_EQ

Done.

http://codereview.chromium.org/6903061/diff/46001/net/base/dns_reloader.cc#ne...
net/base/dns_reloader.cc:85:
net::NetworkChangeNotifier::RemoveDNSObserver(this);
On 2011/08/11 01:57:55, eroman wrote:
> Please remove this (if it is notreached).
> 
> Plus it might not be safe to call this around the time the
NetworkChangeNotifier
> is destroyed (since that code doesn't use locks).

Done.

http://codereview.chromium.org/6903061/diff/46001/net/base/network_change_not...
File net/base/network_change_notifier_linux.cc (right):

http://codereview.chromium.org/6903061/diff/46001/net/base/network_change_not...
net/base/network_change_notifier_linux.cc:33: callback_.reset(callback);
On 2011/08/11 01:57:55, eroman wrote:
> nit: I suggest moving this into the initializer list.

Done.

http://codereview.chromium.org/6903061/diff/46001/net/base/network_change_not...
net/base/network_change_notifier_linux.cc:37: callback_enabled = false;
On 2011/08/11 01:57:55, eroman wrote:
> Not sure the bool is necessary, could do callback_.reset(), and test for if
> (callback_.get() when running.

Done.

http://codereview.chromium.org/6903061/diff/46001/net/base/network_change_not...
net/base/network_change_notifier_linux.cc:40: virtual void
OnFilePathChanged(const FilePath& path);
On 2011/08/11 01:57:55, eroman wrote:
> nit: Can you add OVERRIDE annotation?

Done.

http://codereview.chromium.org/6903061/diff/46001/net/base/network_change_not...
net/base/network_change_notifier_linux.cc:41: private:
On 2011/08/11 01:57:55, eroman wrote:
> [optional]: I suggest hooking OnFilePathError() and printing an error message
> (could help with debugging should the watching go awry).

Done. At the moment FilePathWatcher on linux won't actually ever call
OnFilePathError btw.

http://codereview.chromium.org/6903061/diff/46001/net/base/network_change_not...
net/base/network_change_notifier_linux.cc:150:
file_watcher_delegate_->DisableCallback();
On 2011/08/11 01:57:55, eroman wrote:
> I think it would be sufficient to reset the file watchers instead (since they
> cleanup on destruction).
> 
>   resolve_file_watcher_.reset();
>   hosts_file_watcher_.reset();

Done. Much cleaner. I've killed the DisableCallback stuff.

[Craig, 2011-09-05 16:14:11]

> From a quick glance at szym's DnsConfigService
> (http://codereview.chromium.org/7518028/), it doesn't look like it is
dependent
> on the changes to NetworkChangeNotifier here so I think holding off committing
> this until the filepathwatcher fix is in should be ok (szym's code will be
> subject to the same symlink issues in filepathwatcher btw. until that is
fixed).

I've just landed an interim filepathwatcher fix now and plan on landing this a
bit later.

(when I rebased now I had to make a trivial fix/change from NET_API ->
NET_EXPORT as that changed in trunk but otherwise the code is unchanged so I'm
assuming I'm still good to commit ... yell if anyone disagrees pls)