[net/dns] Watch configd notifications to detect DnsConfig changes on Mac.

[net/dns] Watch configd notifications to detect DnsConfig changes on Mac.

libresolv communicates with configd via the BSD libnotify mechanism. Using the
same interface, DnsConfigService can receive signal at the exact moment new
configuration is available to libresolv, then read it using res_ninit.

BUG=121126
TEST=Manually poke at system configuration while DnsConfigService is running.


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

[szym, 2012-04-16 22:25:09]

Mark, I think rsleevi's comment
http://code.google.com/p/chromium/issues/detail?id=121126#c6 refers to you. My
opinion (see #c8) is that configd is the better interface, but my experience
with OS X was built over the last month. Could you take a look at this
implementation and check if there's anything not Mac-proper here?

(In my manual tests, this implementation does exactly what is intended.)

[Mark Mentovai, 2012-04-17 16:27:43]

http://codereview.chromium.org/9969190/diff/3007/net/dns/dns_config_service_p...
File net/dns/dns_config_service_posix.cc (right):

http://codereview.chromium.org/9969190/diff/3007/net/dns/dns_config_service_p...
net/dns/dns_config_service_posix.cc:120: // TODO(szym): Use
dns_configuration_notify_key() from dnsinfo.h?
Yes. Don’t hard-code.

(Unless you have a good reason to, in which case the comment should say what the
good reason is.)

http://codereview.chromium.org/9969190/diff/3007/net/dns/dns_config_service_p...
net/dns/dns_config_service_posix.cc:121: // TODO(szym): Remove |config_watcher_|
and use |notify_watcher_| exclusively.
…implying that both should just be different implementations of the same
interface?

http://codereview.chromium.org/9969190/diff/3007/net/dns/dns_config_service_p...
net/dns/dns_config_service_posix.cc:123: "dns_configuration";
If you were keeping this, which you aren’t, it’d be preferable to not break up
the string like this, since the whole thing would fit on one line. The
continuation line would be bumped over by a 4-space indent. But you’re not
keeping this, because you’re using dns_configuration_notify_key().

http://codereview.chromium.org/9969190/diff/3007/net/dns/notify_watcher_mac.cc
File net/dns/notify_watcher_mac.cc (right):

http://codereview.chromium.org/9969190/diff/3007/net/dns/notify_watcher_mac.c...
net/dns/notify_watcher_mac.cc:18: DCHECK(key);
#include "base/logging.h".

http://codereview.chromium.org/9969190/diff/3007/net/dns/notify_watcher_mac.c...
net/dns/notify_watcher_mac.cc:42: callback_.Reset();
Would you need to reset this even if notify_fd_ <= 0?

http://codereview.chromium.org/9969190/diff/3007/net/dns/notify_watcher_mac.c...
net/dns/notify_watcher_mac.cc:43: watcher_.StopWatchingFileDescriptor();
Is this safe if you got to Cancel() because WatchFileDescriptor failed in
Watch(…)?

http://codereview.chromium.org/9969190/diff/3007/net/dns/notify_watcher_mac.c...
net/dns/notify_watcher_mac.cc:47: void
NotifyWatcherMac::OnFileCanReadWithoutBlocking(int /* fd */) {
No /* */ necessary

http://codereview.chromium.org/9969190/diff/3007/net/dns/notify_watcher_mac.c...
net/dns/notify_watcher_mac.cc:49: int status = read(notify_fd_, &token,
sizeof(token));
Always write HANDLE_EINTR around read. #include "base/eintr_wrapper.h".

http://codereview.chromium.org/9969190/diff/3007/net/dns/notify_watcher_mac.c...
net/dns/notify_watcher_mac.cc:55: // Ignoring |token| read to avoid possible
endianness mismatch:
read -> value

http://codereview.chromium.org/9969190/diff/3007/net/dns/notify_watcher_mac.c...
net/dns/notify_watcher_mac.cc:61: 
No blank line at EOF.

http://codereview.chromium.org/9969190/diff/3007/net/dns/notify_watcher_mac.h
File net/dns/notify_watcher_mac.h (right):

http://codereview.chromium.org/9969190/diff/3007/net/dns/notify_watcher_mac.h...
net/dns/notify_watcher_mac.h:4: 
You need #include guards.

http://codereview.chromium.org/9969190/diff/3007/net/dns/notify_watcher_mac.h...
net/dns/notify_watcher_mac.h:10: // Receives notifications from notify_register
and passes result to a Callback.
This doesn’t explain what notify_register is, and that word doesn’t appear
elsewhere in the file.

http://codereview.chromium.org/9969190/diff/3007/net/dns/notify_watcher_mac.h...
net/dns/notify_watcher_mac.h:16: // When deleted, automatically cancels.
Blank line before this. That helps the reader “see” this as something new and
not just garbage following the constructor on the preceding line.

http://codereview.chromium.org/9969190/diff/3007/net/dns/notify_watcher_mac.h...
net/dns/notify_watcher_mac.h:20: // will pass true to |callback| on
notification, and false on error.
This comment is a little bit confusing to me: is the argument to the callback
always the same as the return value from this function? Does the callback even
get called on error?

http://codereview.chromium.org/9969190/diff/3007/net/dns/notify_watcher_mac.h...
net/dns/notify_watcher_mac.h:29: virtual void OnFileCanReadWithoutBlocking(int
/* fd */) OVERRIDE;
The /* fd */ shouldn’t be necessary, because we don’t ever build with
-Wunused-argument. Same on the next line.

http://codereview.chromium.org/9969190/diff/3007/net/dns/notify_watcher_mac.h...
net/dns/notify_watcher_mac.h:36: };
DISALLOW_COPY_AND_ASSIGN.

http://codereview.chromium.org/9969190/diff/3007/net/dns/notify_watcher_mac.h...
net/dns/notify_watcher_mac.h:39: 
Get rid of the blank line at the end of the file.

[szym, 2012-04-17 19:39:31]

Thanks for the thorough review.

http://codereview.chromium.org/9969190/diff/3007/net/dns/dns_config_service_p...
File net/dns/dns_config_service_posix.cc (right):

http://codereview.chromium.org/9969190/diff/3007/net/dns/dns_config_service_p...
net/dns/dns_config_service_posix.cc:120: // TODO(szym): Use
dns_configuration_notify_key() from dnsinfo.h?
On 2012/04/17 16:27:43, Mark Mentovai wrote:
> Yes. Don’t hard-code.
> 
> (Unless you have a good reason to, in which case the comment should say what
the
> good reason is.)

#include <dnsinfo.h> does not work on my Mac. It seems there is no dnsinfo.h on
my Mac at all. Wasn't sure if importing this constant warrants a third_party
dep.

http://codereview.chromium.org/9969190/diff/3007/net/dns/dns_config_service_p...
net/dns/dns_config_service_posix.cc:121: // TODO(szym): Remove |config_watcher_|
and use |notify_watcher_| exclusively.
On 2012/04/17 16:27:43, Mark Mentovai wrote:
> …implying that both should just be different implementations of the same
> interface?

That makes sense. I'm quite confident NotifyWatcherMac is sufficient and there's
no need for FilePathWatcher on resolv.conf so I'll unify the interfaces.

http://codereview.chromium.org/9969190/diff/3007/net/dns/notify_watcher_mac.cc
File net/dns/notify_watcher_mac.cc (right):

http://codereview.chromium.org/9969190/diff/3007/net/dns/notify_watcher_mac.c...
net/dns/notify_watcher_mac.cc:42: callback_.Reset();
On 2012/04/17 16:27:43, Mark Mentovai wrote:
> Would you need to reset this even if notify_fd_ <= 0?

This is set in line 34, only after status == NOTIFY_STATUS_OK. I'd add a
DCHECK_GE(notify_fd_, 0); up there.

http://codereview.chromium.org/9969190/diff/3007/net/dns/notify_watcher_mac.c...
net/dns/notify_watcher_mac.cc:43: watcher_.StopWatchingFileDescriptor();
On 2012/04/17 16:27:43, Mark Mentovai wrote:
> Is this safe if you got to Cancel() because WatchFileDescriptor failed in
> Watch(…)?

This is always safe to call, see message_pump_libevent.h

http://codereview.chromium.org/9969190/diff/3007/net/dns/notify_watcher_mac.c...
net/dns/notify_watcher_mac.cc:61: 
On 2012/04/17 16:27:43, Mark Mentovai wrote:
> No blank line at EOF.

As before. Lint complained about that.

http://codereview.chromium.org/9969190/diff/3007/net/dns/notify_watcher_mac.h
File net/dns/notify_watcher_mac.h (right):

http://codereview.chromium.org/9969190/diff/3007/net/dns/notify_watcher_mac.h...
net/dns/notify_watcher_mac.h:4: 
On 2012/04/17 16:27:43, Mark Mentovai wrote:
> You need #include guards.

Oops.

http://codereview.chromium.org/9969190/diff/3007/net/dns/notify_watcher_mac.h...
net/dns/notify_watcher_mac.h:20: // will pass true to |callback| on
notification, and false on error.
On 2012/04/17 16:27:43, Mark Mentovai wrote:
> This comment is a little bit confusing to me: is the argument to the callback
> always the same as the return value from this function? Does the callback even
> get called on error?

The callback is never called synchronously from Watch(), but if in
OnFileCanReadWithoutBlocking the read fails, I take it as an error, and signal
it through the callback.

http://codereview.chromium.org/9969190/diff/3007/net/dns/notify_watcher_mac.h...
net/dns/notify_watcher_mac.h:39: 
On 2012/04/17 16:27:43, Mark Mentovai wrote:
> Get rid of the blank line at the end of the file.

Ok, although if you look at Patch Set 1, you'll see that Lint complains about
that. Is Lint buggy?

[Mark Mentovai, 2012-04-17 19:47:39]

http://codereview.chromium.org/9969190/diff/3007/net/dns/dns_config_service_p...
File net/dns/dns_config_service_posix.cc (right):

http://codereview.chromium.org/9969190/diff/3007/net/dns/dns_config_service_p...
net/dns/dns_config_service_posix.cc:120: // TODO(szym): Use
dns_configuration_notify_key() from dnsinfo.h?
szym wrote:
> On 2012/04/17 16:27:43, Mark Mentovai wrote:
> > Yes. Don’t hard-code.
> > 
> > (Unless you have a good reason to, in which case the comment should say what
> the
> > good reason is.)
> 
> #include <dnsinfo.h> does not work on my Mac. It seems there is no dnsinfo.h
on
> my Mac at all. Wasn't sure if importing this constant warrants a third_party
> dep.

I see. I don’t have that header either.

Where’d you find dnsinfo.h and dns_configuration_notify_key(), then? Just by
poking around in the configd project?

http://codereview.chromium.org/9969190/diff/3007/net/dns/notify_watcher_mac.h
File net/dns/notify_watcher_mac.h (right):

http://codereview.chromium.org/9969190/diff/3007/net/dns/notify_watcher_mac.h...
net/dns/notify_watcher_mac.h:39: 
szym wrote:
> On 2012/04/17 16:27:43, Mark Mentovai wrote:
> > Get rid of the blank line at the end of the file.
> 
> Ok, although if you look at Patch Set 1, you'll see that Lint complains about
> that. Is Lint buggy?

The lint on Rietveld is buggy for added files. Patch set 1 was correct. If the
file truly didn’t end with a newline, the patch would have said something like
“\No newline at end of file”. That would be a problem. lint wants to force you
to put TWO newlines at the end of a file, which is one line-ender and one blank
line, and this is wrong.

[Mark Mentovai, 2012-04-17 19:53:54]

I think this is nearly perfect now.

http://codereview.chromium.org/9969190/diff/8001/net/dns/dns_config_service_p...
File net/dns/dns_config_service_posix.cc (right):

http://codereview.chromium.org/9969190/diff/8001/net/dns/dns_config_service_p...
net/dns/dns_config_service_posix.cc:89: // Normally defined in dnsinfo.h in
configd.
You can do this:

extern "C" {

// From 10.7.3 configd-395.10/dnsinfo/dnsinfo.h
const char* dns_configuration_notify_key();

}  // extern "C"

Then you can call dns_configuration_notify_key() without hardcoding anything and
without <dnsinfo.h> being exposed. I like treating the Apple call as the
interface better than matching what they’ve hard-coded, even though both are
technically private.

http://codereview.chromium.org/9969190/diff/8001/net/dns/dns_config_service_p...
net/dns/dns_config_service_posix.cc:90: const char* kDnsNotifyKey =
If you’re sticking with a const char*, make it static or put it in an anonymous
namespace. There’s no reason to expose it beyond this file (even if in a
namespace called “internal”.)

http://codereview.chromium.org/9969190/diff/8001/net/dns/dns_config_service_p...
net/dns/dns_config_service_posix.cc:100: const FilePath::CharType*
kFilePathConfig = FILE_PATH_LITERAL(_PATH_RESCONF);
This, too, can be static or in an anonymous namespace.

http://codereview.chromium.org/9969190/diff/8001/net/dns/file_path_watcher_wr...
File net/dns/file_path_watcher_wrapper.h (right):

http://codereview.chromium.org/9969190/diff/8001/net/dns/file_path_watcher_wr...
net/dns/file_path_watcher_wrapper.h:5: #ifndef
NET_DNS_FILE_PATH_WATCHER_WRAPPER_H_
Ha, nice catch.

http://codereview.chromium.org/9969190/diff/8001/net/dns/notify_watcher_mac.h
File net/dns/notify_watcher_mac.h (right):

http://codereview.chromium.org/9969190/diff/8001/net/dns/notify_watcher_mac.h...
net/dns/notify_watcher_mac.h:5: #ifndef NET_DNS_NOTIFY_WATCHER_MAC_
This should have _H_ at the end. Also on lines 6 and 48.

[szym, 2012-04-17 20:14:31]

All done, except...

http://codereview.chromium.org/9969190/diff/8001/net/dns/dns_config_service_p...
File net/dns/dns_config_service_posix.cc (right):

http://codereview.chromium.org/9969190/diff/8001/net/dns/dns_config_service_p...
net/dns/dns_config_service_posix.cc:89: // Normally defined in dnsinfo.h in
configd.
On 2012/04/17 19:53:54, Mark Mentovai wrote:
> You can do this:
> 
> extern "C" {
> 
> // From 10.7.3 configd-395.10/dnsinfo/dnsinfo.h
> const char* dns_configuration_notify_key();
> 
> }  // extern "C"
> 
> Then you can call dns_configuration_notify_key() without hardcoding anything
and
> without <dnsinfo.h> being exposed. I like treating the Apple call as the
> interface better than matching what they’ve hard-coded, even though both are
> technically private.

I wish this worked. Unfortunately, it won't link ("ld: symbol(s) not found").

[Mark Mentovai, 2012-04-17 20:22:20]

LGTM

http://codereview.chromium.org/9969190/diff/8001/net/dns/dns_config_service_p...
File net/dns/dns_config_service_posix.cc (right):

http://codereview.chromium.org/9969190/diff/8001/net/dns/dns_config_service_p...
net/dns/dns_config_service_posix.cc:89: // Normally defined in dnsinfo.h in
configd.
szym wrote:
> On 2012/04/17 19:53:54, Mark Mentovai wrote:
> > You can do this:
> > 
> > extern "C" {
> > 
> > // From 10.7.3 configd-395.10/dnsinfo/dnsinfo.h
> > const char* dns_configuration_notify_key();
> > 
> > }  // extern "C"
> > 
> > Then you can call dns_configuration_notify_key() without hardcoding anything
> and
> > without <dnsinfo.h> being exposed. I like treating the Apple call as the
> > interface better than matching what they’ve hard-coded, even though both are
> > technically private.
> 
> I wish this worked. Unfortunately, it won't link ("ld: symbol(s) not found").

I see, it’s marked “private_extern” in 10.5, which appears to be a bug based on
the AVAILABLE_MAC_OS_X_VERSION_10_4_AND_LATER in its declaration, but that’s
life.

[commit-bot: I haz the power, 2012-04-17 20:34:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/szym@chromium.org/9969190/13001

[commit-bot: I haz the power, 2012-04-17 20:52:24]

Try job failure for 9969190-13001 (retry) (retry) on android for steps "Compile,
build".
It's a second try, previously, steps "Compile, build" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...

[commit-bot: I haz the power, 2012-04-17 20:52:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/szym@chromium.org/9969190/13004

[commit-bot: I haz the power, 2012-04-17 23:45:15]

Change committed as 132698