Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7518028:
DnsConfigService and a posix implementation (Closed)
|
Created:
9 years, 4 months ago by szym Modified:
9 years, 4 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org Visibility:
Public. |
DescriptionDnsConfigService and a posix implementation
Contributed by: Szymon Jakubczak <szym@chromium.org>;
BUG=90881
TEST=./net_unittests --gtest_filter="DnsConfigServiceTest*"
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=97282
Patch Set 1 #Patch Set 2 : fixed lint #
Total comments: 20
Patch Set 3 : added unit test, fixed some issues around ref-counting (and a sigsegv) #Patch Set 4 : fixed lint #Patch Set 5 : missing defaults for DnsConfig; complete unittest #Patch Set 6 : delinted #
Total comments: 47
Patch Set 7 : Changed Delegate -> Observer(List); internal handling of Watch failures. #
Total comments: 14
Patch Set 8 : Just a comment. Ready to review. #Patch Set 9 : Mocked-out res_init and FilePathWatcher for increased coverage and less flakiness. #Patch Set 10 : Addressing lint errors. #
Total comments: 54
Patch Set 11 : Deflaked test. Addressed memory leaks and comments. #
Total comments: 1
Patch Set 12 : Fixes for mac and clang. #Patch Set 13 : consistency #Patch Set 14 : addressing review comment #Patch Set 15 : Replaced res_nclose with CloseResState in mocks/tests. #Patch Set 16 : delinted #Patch Set 17 : delinted more #Patch Set 18 : delinted even more #Patch Set 19 : CloseResState applies to OS_LINUX only #
Messages
Total messages: 20 (0 generated)
This is roughly what I expected - great! Talib - does this look like it would work for you? I'd expect AsyncHostResolver to be a DnsConfigService::Delegate here. Let me know when you want me to do a full code review. I'm trying to think through how to do unit tests. Given that you are using res_ninit we don't have to test parsing. Probably converting a __res_state to DnsConfig object could be tested. http://codereview.chromium.org/7518028/diff/2001/net/dns/dns_config_service.h File net/dns/dns_config_service.h (right): http://codereview.chromium.org/7518028/diff/2001/net/dns/dns_config_service.h... net/dns/dns_config_service.h:23: struct NET_API DnsConfig { I don't know if these will need NET_API - these are likely net stack internal things which don't need to be exposed to consumers of the network library. http://codereview.chromium.org/7518028/diff/2001/net/dns/dns_config_service.h... net/dns/dns_config_service.h:24: public: Nit: public by default on structs. http://codereview.chromium.org/7518028/diff/2001/net/dns/dns_config_service.h... net/dns/dns_config_service.h:26: // win stores a subset of those in the registry I'd want a default constructor due to the int/bool/POD types - make sure they are initialized to something reasonable. http://codereview.chromium.org/7518028/diff/2001/net/dns/dns_config_service.h... net/dns/dns_config_service.h:52: // so that we only notify on real change Another style thing - we tend to avoid operator overloading in Chrome. http://codereview.chromium.org/7518028/diff/2001/net/dns/dns_config_service.h... net/dns/dns_config_service.h:69: // Reads the current config now, separate for testability This probably doesn't need to be defined here - implementation specific thing for the POSIX implementation,. http://codereview.chromium.org/7518028/diff/2001/net/dns/dns_config_service.h... net/dns/dns_config_service.h:89: virtual void OnConfigError() = 0; Does this need to be exposed? I'd remove it until needed. http://codereview.chromium.org/7518028/diff/2001/net/dns/dns_config_service.h... net/dns/dns_config_service.h:100: virtual bool Watch(Delegate* delegate) = 0; What does the return value indicate? When would this fail? http://codereview.chromium.org/7518028/diff/2001/net/dns/dns_config_service_p... File net/dns/dns_config_service_posix.cc (right): http://codereview.chromium.org/7518028/diff/2001/net/dns/dns_config_service_p... net/dns/dns_config_service_posix.cc:30: if (res_ninit(&res) != 0) Assuming that it's a newly constructed DnsConfig object and not having to clear is fine, given how it's used. http://codereview.chromium.org/7518028/diff/2001/net/dns/dns_config_service_p... net/dns/dns_config_service_posix.cc:58: class DnsConfigServicePosix : public DnsConfigService, Is this compiling? Doesn't it need to be derived from RefCounted to handle the NewRunnableMethod call? http://codereview.chromium.org/7518028/diff/2001/net/dns/dns_config_service_p... net/dns/dns_config_service_posix.cc:94: Should probably DCHECK that delegate_ is NULL.
Thanks for the pre-review. I'm getting accustomed to the testing framework. I haven't tested if this actually makes sense and matches /etc/resolv.conf yet. To test converting __res_state to DnsConfig, I plan to add static DnsConfigReaderPosix::ConfigFromResolver(DnsConfig*) that will be testable. Might also unittest equality testing, but the rest of the component depends on correctness of FilePathWatcher so I think that would be it for unittests. How does that sounds? http://codereview.chromium.org/7518028/diff/2001/net/dns/dns_config_service.h File net/dns/dns_config_service.h (right): http://codereview.chromium.org/7518028/diff/2001/net/dns/dns_config_service.h... net/dns/dns_config_service.h:23: struct NET_API DnsConfig { On 2011/08/01 19:52:26, cbentzel wrote: > I don't know if these will need NET_API - these are likely net stack internal > things which don't need to be exposed to consumers of the network library. Done. http://codereview.chromium.org/7518028/diff/2001/net/dns/dns_config_service.h... net/dns/dns_config_service.h:24: public: On 2011/08/01 19:52:26, cbentzel wrote: > Nit: public by default on structs. Done. http://codereview.chromium.org/7518028/diff/2001/net/dns/dns_config_service.h... net/dns/dns_config_service.h:26: // win stores a subset of those in the registry On 2011/08/01 19:52:26, cbentzel wrote: > I'd want a default constructor due to the int/bool/POD types - make sure they > are initialized to something reasonable. Done. http://codereview.chromium.org/7518028/diff/2001/net/dns/dns_config_service.h... net/dns/dns_config_service.h:52: // so that we only notify on real change On 2011/08/01 19:52:26, cbentzel wrote: > Another style thing - we tend to avoid operator overloading in Chrome. This is inspired by ProxyConfig. There's a dozen other classes in net with operator== overloaded. Would .equals() be preferred? I wasn't sure if keeping track of last_config and avoiding OnConfigChanged when no real change occurred was actually necessary, but I suspected that AsyncHostResolver might have to flush some cache out in response and that could be a waste. http://codereview.chromium.org/7518028/diff/2001/net/dns/dns_config_service.h... net/dns/dns_config_service.h:69: // Reads the current config now, separate for testability On 2011/08/01 19:52:26, cbentzel wrote: > This probably doesn't need to be defined here - implementation specific thing > for the POSIX implementation,. I planned a Windows implementation that reads the registry settings and writes them to the DnsConfig. Is there no need to have a synchronous "read-on-demand" utility? How will AsyncHostResolver be initialized? http://codereview.chromium.org/7518028/diff/2001/net/dns/dns_config_service.h... net/dns/dns_config_service.h:89: virtual void OnConfigError() = 0; On 2011/08/01 19:52:26, cbentzel wrote: > Does this need to be exposed? I'd remove it until needed. Not sure how to handle FilePathWatcher::Delegate::OnFilePathError otherwise. This is a hard failure: FPWatcher stops watching afterwards. http://codereview.chromium.org/7518028/diff/2001/net/dns/dns_config_service.h... net/dns/dns_config_service.h:100: virtual bool Watch(Delegate* delegate) = 0; On 2011/08/01 19:52:26, cbentzel wrote: > What does the return value indicate? When would this fail? When FilePathWatcher::Watch fails, possibly when the registry key watch fails to installs as well. We could postpone the error to be delivered asynchronously via Delegate::OnConfigError. http://codereview.chromium.org/7518028/diff/2001/net/dns/dns_config_service_p... File net/dns/dns_config_service_posix.cc (right): http://codereview.chromium.org/7518028/diff/2001/net/dns/dns_config_service_p... net/dns/dns_config_service_posix.cc:30: if (res_ninit(&res) != 0) On 2011/08/01 19:52:26, cbentzel wrote: > Assuming that it's a newly constructed DnsConfig object and not having to clear > is fine, given how it's used. I'm not sure this is a safe general assumption. It seems some user could want to re-use a DnsConfig it owns. Alternatively, we put DnsConfig in namespace {}. Can it still be unit-tested in that case? http://codereview.chromium.org/7518028/diff/2001/net/dns/dns_config_service_p... net/dns/dns_config_service_posix.cc:58: class DnsConfigServicePosix : public DnsConfigService, On 2011/08/01 19:52:26, cbentzel wrote: > Is this compiling? Doesn't it need to be derived from RefCounted to handle the > NewRunnableMethod call? It is compiling. Is that an error? Is it safe to add RefCountedThreadSafe<DnsConfigServicePosix> to inheritance list? I forgot whether in the case of duplicate definition order of classes in this list matters. http://codereview.chromium.org/7518028/diff/2001/net/dns/dns_config_service_p... net/dns/dns_config_service_posix.cc:94: On 2011/08/01 19:52:26, cbentzel wrote: > Should probably DCHECK that delegate_ is NULL. Done.
The biggest thing I am worried about for testing is lifetime issues especially during shutdown - this is something eric roman has had to deal with at length with HostResolverImpl. It seems like this is ready for a more detailed review at this point. Should I proceed?
Chris, Before the full review, I'd like to ensure this code works. So I'd like to write a small test program that simply starts DnsConfigService and prints out the DnsConfig it obtains. I imagine that means adding a new target to net.gyp, but I don't want it there permanently. Any recommendation on how to perform such "manual" tests of the net library? --Szymon On Mon, Aug 1, 2011 at 5:11 PM, <cbentzel@chromium.org> wrote: > The biggest thing I am worried about for testing is lifetime issues > especially > during shutdown - this is something eric roman has had to deal with at > length > with HostResolverImpl. > > It seems like this is ready for a more detailed review at this point. Should > I > proceed? > > http://codereview.chromium.org/7518028/ >
Why don't you have AsyncHostResolver create a DnsConfigService, and act as a delegate? You could add a new constructor which doesn't take a dns_server argument for example. You may need to amend CreateGlobalHostResolver in chrome/browser/io_thread.cc to use this - perhaps when --dns-server is specified, but without any arguments for the server name. This would also get you most of the way to using it as well.
On 2011/08/01 19:52:25, cbentzel wrote: > This is roughly what I expected - great! > > Talib - does this look like it would work for you? I'd expect AsyncHostResolver > to be a DnsConfigService::Delegate here. I took a quick look, I've yet no idea how I will use this (or maybe not use at all since I will run out of time), but since it gives me list of DNS servers as IPEndPoints, I should be fine. > > Let me know when you want me to do a full code review. > > I'm trying to think through how to do unit tests. Given that you are using > res_ninit we don't have to test parsing. Probably converting a __res_state to > DnsConfig object could be tested. > > http://codereview.chromium.org/7518028/diff/2001/net/dns/dns_config_service.h > File net/dns/dns_config_service.h (right): > > http://codereview.chromium.org/7518028/diff/2001/net/dns/dns_config_service.h... > net/dns/dns_config_service.h:23: struct NET_API DnsConfig { > I don't know if these will need NET_API - these are likely net stack internal > things which don't need to be exposed to consumers of the network library. > > http://codereview.chromium.org/7518028/diff/2001/net/dns/dns_config_service.h... > net/dns/dns_config_service.h:24: public: > Nit: public by default on structs. > > http://codereview.chromium.org/7518028/diff/2001/net/dns/dns_config_service.h... > net/dns/dns_config_service.h:26: // win stores a subset of those in the registry > I'd want a default constructor due to the int/bool/POD types - make sure they > are initialized to something reasonable. > > http://codereview.chromium.org/7518028/diff/2001/net/dns/dns_config_service.h... > net/dns/dns_config_service.h:52: // so that we only notify on real change > Another style thing - we tend to avoid operator overloading in Chrome. > > http://codereview.chromium.org/7518028/diff/2001/net/dns/dns_config_service.h... > net/dns/dns_config_service.h:69: // Reads the current config now, separate for > testability > This probably doesn't need to be defined here - implementation specific thing > for the POSIX implementation,. > > http://codereview.chromium.org/7518028/diff/2001/net/dns/dns_config_service.h... > net/dns/dns_config_service.h:89: virtual void OnConfigError() = 0; > Does this need to be exposed? I'd remove it until needed. > > http://codereview.chromium.org/7518028/diff/2001/net/dns/dns_config_service.h... > net/dns/dns_config_service.h:100: virtual bool Watch(Delegate* delegate) = 0; > What does the return value indicate? When would this fail? > > http://codereview.chromium.org/7518028/diff/2001/net/dns/dns_config_service_p... > File net/dns/dns_config_service_posix.cc (right): > > http://codereview.chromium.org/7518028/diff/2001/net/dns/dns_config_service_p... > net/dns/dns_config_service_posix.cc:30: if (res_ninit(&res) != 0) > Assuming that it's a newly constructed DnsConfig object and not having to clear > is fine, given how it's used. > > http://codereview.chromium.org/7518028/diff/2001/net/dns/dns_config_service_p... > net/dns/dns_config_service_posix.cc:58: class DnsConfigServicePosix : public > DnsConfigService, > Is this compiling? Doesn't it need to be derived from RefCounted to handle the > NewRunnableMethod call? > > http://codereview.chromium.org/7518028/diff/2001/net/dns/dns_config_service_p... > net/dns/dns_config_service_posix.cc:94: > Should probably DCHECK that delegate_ is NULL.
Added a sketch of a unittest. Currently, DnsConfigServicePosix must be destructed on the same thread as DnsConfigServicePosix::Watch was called. This does not seem like the right thing to require.
This is looking pretty good. Please don't be concerned about the number of comments - this is typical. In fact, the style generally seems to match Chrome's style, and there are lots of good things like OVERRIDE. http://codereview.chromium.org/7518028/diff/12006/net/dns/dns_config_service.h File net/dns/dns_config_service.h (right): http://codereview.chromium.org/7518028/diff/12006/net/dns/dns_config_service.... net/dns/dns_config_service.h:22: // To be used with AsyncHostResolver. Nit: Remove the comment about "To be used with AsyncHostResolver" - other consumers could exist. http://codereview.chromium.org/7518028/diff/12006/net/dns/dns_config_service.... net/dns/dns_config_service.h:23: // TODO(szym): determine the next query for a given host according to settings. What is this TODO about? http://codereview.chromium.org/7518028/diff/12006/net/dns/dns_config_service.... net/dns/dns_config_service.h:25: // resolv.conf stuff -- standard on POSIX/glibc systems Nit: I'd remove this comment as it's intended to be platform neutral. http://codereview.chromium.org/7518028/diff/12006/net/dns/dns_config_service.... net/dns/dns_config_service.h:35: // list of name server addresses 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 http://codereview.chromium.org/7518028/diff/12006/net/dns/dns_config_service.... net/dns/dns_config_service.h:57: std::map<IPAddressNumber, std::list<std::string> > hosts; This isn't filled in right now - I'd remove until it is. http://codereview.chromium.org/7518028/diff/12006/net/dns/dns_config_service.... net/dns/dns_config_service.h:60: bool operator==(const DnsConfig& d) const { Style guide explicitly is against using operator== or other operator overloading. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Operator_Overl... http://codereview.chromium.org/7518028/diff/12006/net/dns/dns_config_service.... net/dns/dns_config_service.h:76: // DnsConfigService fetches system DNS configuration as DnsConfig 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 http://codereview.chromium.org/7518028/diff/12006/net/dns/dns_config_service.... net/dns/dns_config_service.h:80: // Callback interface for the client. The delegate is called on the same You mentioned using an observer approach before - I think this is probably appropriate over a delegate, if only to make this consistent with ProxyConfigService. http://codereview.chromium.org/7518028/diff/12006/net/dns/dns_config_service.... net/dns/dns_config_service.h:90: virtual void OnConfigError() = 0; 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] http://codereview.chromium.org/7518028/diff/12006/net/dns/dns_config_service_... File net/dns/dns_config_service_posix.cc (right): http://codereview.chromium.org/7518028/diff/12006/net/dns/dns_config_service_... net/dns/dns_config_service_posix.cc:33: if (ipe.FromSockAddr( 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. http://codereview.chromium.org/7518028/diff/12006/net/dns/dns_config_service_... net/dns/dns_config_service_posix.cc:38: NOTREACHED() << "could not convert nsaddr"; 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. http://codereview.chromium.org/7518028/diff/12006/net/dns/dns_config_service_... net/dns/dns_config_service_posix.cc:47: dns_config->domain = res.defdname; // use only if |search| is empty 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. http://codereview.chromium.org/7518028/diff/12006/net/dns/dns_config_service_... net/dns/dns_config_service_posix.cc:49: dns_config->timeout = res.retrans; 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. http://codereview.chromium.org/7518028/diff/12006/net/dns/dns_config_service_... net/dns/dns_config_service_posix.cc:52: dns_config->edns0 = res.options & RES_USE_EDNS0; 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. http://codereview.chromium.org/7518028/diff/12006/net/dns/dns_config_service_... net/dns/dns_config_service_posix.cc:58: // However, it might not be thread-safe on OpenBSD. 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. http://codereview.chromium.org/7518028/diff/12006/net/dns/dns_config_service_... net/dns/dns_config_service_posix.cc:93: delegate_->OnConfigError(); Yeah, I don't think you need to escalate this to the caller. It should be handled internally as much as possible. http://codereview.chromium.org/7518028/diff/12006/net/dns/dns_config_service_... net/dns/dns_config_service_posix.cc:110: message_loop_->PostTask(FROM_HERE, base::Bind( If it wasn't successful, I think we should just skip posting a task back to the message_loop_. http://codereview.chromium.org/7518028/diff/12006/net/dns/dns_config_service_... net/dns/dns_config_service_posix.cc:145: }; DISALLOW_COPY_AND_ASSIGN http://codereview.chromium.org/7518028/diff/12006/net/dns/dns_config_service_... net/dns/dns_config_service_posix.cc:166: } Nit: newline between here and // namespace net.
http://codereview.chromium.org/7518028/diff/12006/net/dns/dns_config_service_... File net/dns/dns_config_service_posix.cc (right): http://codereview.chromium.org/7518028/diff/12006/net/dns/dns_config_service_... net/dns/dns_config_service_posix.cc:153: FilePath path(FILE_PATH_LITERAL("/etc/resolv.conf")); 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
Thanks for all the comments. Next, I'll ensure to have only one res_ninit job at a time so that they are properly ordered. http://codereview.chromium.org/7518028/diff/12006/net/dns/dns_config_service.h File net/dns/dns_config_service.h (right): http://codereview.chromium.org/7518028/diff/12006/net/dns/dns_config_service.... net/dns/dns_config_service.h:22: // To be used with AsyncHostResolver. On 2011/08/04 18:09:27, cbentzel wrote: > Nit: Remove the comment about "To be used with AsyncHostResolver" - other > consumers could exist. Done. http://codereview.chromium.org/7518028/diff/12006/net/dns/dns_config_service.... net/dns/dns_config_service.h:23: // TODO(szym): determine the next query for a given host according to settings. On 2011/08/04 18:09:27, cbentzel wrote: > What is this TODO about? 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. http://codereview.chromium.org/7518028/diff/12006/net/dns/dns_config_service.... net/dns/dns_config_service.h:25: // resolv.conf stuff -- standard on POSIX/glibc systems On 2011/08/04 18:09:27, cbentzel wrote: > Nit: I'd remove this comment as it's intended to be platform neutral. Done. http://codereview.chromium.org/7518028/diff/12006/net/dns/dns_config_service.... net/dns/dns_config_service.h:35: // list of name server addresses On 2011/08/04 18:09:27, cbentzel wrote: > 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 Done. http://codereview.chromium.org/7518028/diff/12006/net/dns/dns_config_service.... net/dns/dns_config_service.h:57: std::map<IPAddressNumber, std::list<std::string> > hosts; On 2011/08/04 18:09:27, cbentzel wrote: > This isn't filled in right now - I'd remove until it is. Done. http://codereview.chromium.org/7518028/diff/12006/net/dns/dns_config_service.... net/dns/dns_config_service.h:60: bool operator==(const DnsConfig& d) const { On 2011/08/04 18:09:27, cbentzel wrote: > Style guide explicitly is against using operator== or other operator > overloading. > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Operator_Overl... Done. http://codereview.chromium.org/7518028/diff/12006/net/dns/dns_config_service.... net/dns/dns_config_service.h:76: // DnsConfigService fetches system DNS configuration as DnsConfig On 2011/08/04 18:09:27, cbentzel wrote: > 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 Done. http://codereview.chromium.org/7518028/diff/12006/net/dns/dns_config_service.... net/dns/dns_config_service.h:80: // Callback interface for the client. The delegate is called on the same On 2011/08/04 18:09:27, cbentzel wrote: > You mentioned using an observer approach before - I think this is probably > appropriate over a delegate, if only to make this consistent with > ProxyConfigService. Are we talking about just a name change? Delegate -> Observer? http://codereview.chromium.org/7518028/diff/12006/net/dns/dns_config_service.... net/dns/dns_config_service.h:90: virtual void OnConfigError() = 0; On 2011/08/04 18:09:27, cbentzel wrote: > 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] 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? http://codereview.chromium.org/7518028/diff/12006/net/dns/dns_config_service_... File net/dns/dns_config_service_posix.cc (right): http://codereview.chromium.org/7518028/diff/12006/net/dns/dns_config_service_... net/dns/dns_config_service_posix.cc:33: if (ipe.FromSockAddr( On 2011/08/04 18:09:27, cbentzel wrote: > 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. I'll look into this. http://codereview.chromium.org/7518028/diff/12006/net/dns/dns_config_service_... net/dns/dns_config_service_posix.cc:38: NOTREACHED() << "could not convert nsaddr"; On 2011/08/04 18:09:27, cbentzel wrote: > 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. Right. I was swayed by IPEndPoint::FromSockAddr which cannot return false. http://codereview.chromium.org/7518028/diff/12006/net/dns/dns_config_service_... net/dns/dns_config_service_posix.cc:47: dns_config->domain = res.defdname; // use only if |search| is empty On 2011/08/04 18:09:27, cbentzel wrote: > 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. 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? http://codereview.chromium.org/7518028/diff/12006/net/dns/dns_config_service_... net/dns/dns_config_service_posix.cc:49: dns_config->timeout = res.retrans; On 2011/08/04 18:09:27, cbentzel wrote: > 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. I can't think of a way to do that without parsing /etc/resolv.conf ourselves. http://codereview.chromium.org/7518028/diff/12006/net/dns/dns_config_service_... net/dns/dns_config_service_posix.cc:52: dns_config->edns0 = res.options & RES_USE_EDNS0; On 2011/08/04 18:09:27, cbentzel wrote: > 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. I'm not sure I understand. Should I remove those options? http://codereview.chromium.org/7518028/diff/12006/net/dns/dns_config_service_... net/dns/dns_config_service_posix.cc:58: // However, it might not be thread-safe on OpenBSD. On 2011/08/04 18:09:27, cbentzel wrote: > 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. Done. http://codereview.chromium.org/7518028/diff/12006/net/dns/dns_config_service_... net/dns/dns_config_service_posix.cc:93: delegate_->OnConfigError(); On 2011/08/04 18:09:27, cbentzel wrote: > Yeah, I don't think you need to escalate this to the caller. It should be > handled internally as much as possible. Done. http://codereview.chromium.org/7518028/diff/12006/net/dns/dns_config_service_... net/dns/dns_config_service_posix.cc:110: message_loop_->PostTask(FROM_HERE, base::Bind( On 2011/08/04 18:09:27, cbentzel wrote: > If it wasn't successful, I think we should just skip posting a task back to the > message_loop_. Can I have a stupid while(!the_thing_to_do); or should there be some pause, max retry count, etc? http://codereview.chromium.org/7518028/diff/12006/net/dns/dns_config_service_... net/dns/dns_config_service_posix.cc:145: }; On 2011/08/04 18:09:27, cbentzel wrote: > DISALLOW_COPY_AND_ASSIGN Done. http://codereview.chromium.org/7518028/diff/12006/net/dns/dns_config_service_... net/dns/dns_config_service_posix.cc:153: FilePath path(FILE_PATH_LITERAL("/etc/resolv.conf")); On 2011/08/04 18:24:56, cbentzel wrote: > 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 I'll add a #ifndef _PATH_RESCONF #define _PATH_RESCONF "..." in this file. http://codereview.chromium.org/7518028/diff/12006/net/dns/dns_config_service_... net/dns/dns_config_service_posix.cc:166: } On 2011/08/04 18:09:27, cbentzel wrote: > Nit: newline between here and // namespace net. Done.
http://codereview.chromium.org/7518028/diff/12006/net/dns/dns_config_service.h File net/dns/dns_config_service.h (right): http://codereview.chromium.org/7518028/diff/12006/net/dns/dns_config_service.... net/dns/dns_config_service.h:80: // Callback interface for the client. The delegate is called on the same On 2011/08/04 18:55:10, szym wrote: > On 2011/08/04 18:09:27, cbentzel wrote: > > You mentioned using an observer approach before - I think this is probably > > appropriate over a delegate, if only to make this consistent with > > ProxyConfigService. > > Are we talking about just a name change? Delegate -> Observer? No - actually do an AddObserver/RemoveObserver on DnsConfigService and support multiple observers. http://codereview.chromium.org/7518028/diff/12006/net/dns/dns_config_service.... net/dns/dns_config_service.h:90: virtual void OnConfigError() = 0; On 2011/08/04 18:55:10, szym wrote: > On 2011/08/04 18:09:27, cbentzel wrote: > > 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] > > 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? Yeah. http://codereview.chromium.org/7518028/diff/12006/net/dns/dns_config_service_... File net/dns/dns_config_service_posix.cc (right): http://codereview.chromium.org/7518028/diff/12006/net/dns/dns_config_service_... net/dns/dns_config_service_posix.cc:47: dns_config->domain = res.defdname; // use only if |search| is empty On 2011/08/04 18:55:10, szym wrote: > On 2011/08/04 18:09:27, cbentzel wrote: > > 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. > > 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? That seems good. http://codereview.chromium.org/7518028/diff/12006/net/dns/dns_config_service_... net/dns/dns_config_service_posix.cc:49: dns_config->timeout = res.retrans; On 2011/08/04 18:55:10, szym wrote: > On 2011/08/04 18:09:27, cbentzel wrote: > > 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. > > I can't think of a way to do that without parsing /etc/resolv.conf ourselves. Yeah, agree. http://codereview.chromium.org/7518028/diff/12006/net/dns/dns_config_service_... net/dns/dns_config_service_posix.cc:52: dns_config->edns0 = res.options & RES_USE_EDNS0; On 2011/08/04 18:55:10, szym wrote: > On 2011/08/04 18:09:27, cbentzel wrote: > > 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. > > I'm not sure I understand. Should I remove those options? No, keep them. I was musing about what we need to do if edns0 is set in the AsyncHostResolver. http://codereview.chromium.org/7518028/diff/12006/net/dns/dns_config_service_... net/dns/dns_config_service_posix.cc:110: message_loop_->PostTask(FROM_HERE, base::Bind( On 2011/08/04 18:55:10, szym wrote: > On 2011/08/04 18:09:27, cbentzel wrote: > > If it wasn't successful, I think we should just skip posting a task back to > the > > message_loop_. > > Can I have a stupid while(!the_thing_to_do); or should there be some pause, max > retry count, etc? 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.
http://codereview.chromium.org/7518028/diff/12006/net/dns/dns_config_service.h File net/dns/dns_config_service.h (right): http://codereview.chromium.org/7518028/diff/12006/net/dns/dns_config_service.... net/dns/dns_config_service.h:22: // To be used with AsyncHostResolver. On 2011/08/04 18:55:10, szym wrote: > On 2011/08/04 18:09:27, cbentzel wrote: > > Nit: Remove the comment about "To be used with AsyncHostResolver" - other > > consumers could exist. > > Done. It doesn't look like you uploaded a patch with this fix. Typically "Done" is written after that case.
On 2011/08/05 13:01:25, cbentzel wrote: > http://codereview.chromium.org/7518028/diff/12006/net/dns/dns_config_service.h > File net/dns/dns_config_service.h (right): > > http://codereview.chromium.org/7518028/diff/12006/net/dns/dns_config_service.... > net/dns/dns_config_service.h:22: // To be used with AsyncHostResolver. > On 2011/08/04 18:55:10, szym wrote: > > On 2011/08/04 18:09:27, cbentzel wrote: > > > Nit: Remove the comment about "To be used with AsyncHostResolver" - other > > > consumers could exist. > > > > Done. > > It doesn't look like you uploaded a patch with this fix. Typically "Done" is > written after that case. Will do from now on. I'm still not handling WorkerPool::PostTask error. AFAICT it can't fail on Linux, but it could on Mac. I'm still looking into this.
http://codereview.chromium.org/7518028/diff/18001/net/dns/dns_config_service.h File net/dns/dns_config_service.h (right): http://codereview.chromium.org/7518028/diff/18001/net/dns/dns_config_service.... net/dns/dns_config_service.h:41: // Min. number of dots before global resolution precedes |search|. May as well spell out Minimum http://codereview.chromium.org/7518028/diff/18001/net/dns/dns_config_service.... net/dns/dns_config_service.h:44: int timeout; Probably better to use a base::TimeDelta from base/time.h here instead - and do the conversion in ProxyConfigServiceLinux. That way you don't need to worry about unit conversion issues. http://codereview.chromium.org/7518028/diff/18001/net/dns/dns_config_service.... net/dns/dns_config_service.h:97: protected: Why protected? In general I prefer public/private. Derived classes can always use dns_config() if needed. http://codereview.chromium.org/7518028/diff/18001/net/dns/dns_config_service_... File net/dns/dns_config_service_posix.cc (right): http://codereview.chromium.org/7518028/diff/18001/net/dns/dns_config_service_... net/dns/dns_config_service_posix.cc:48: // TODO(szym): this might not compile on mac. Can you run try jobs to confirm? May also want to comment that this is adding IPV6 server addresses. Do you know if there is a way via res_init to interleave ordering of IPv6 and IPv4 nameservers? In this case, it seems like the IPv6 ones will always be processed last. http://codereview.chromium.org/7518028/diff/18001/net/dns/dns_config_service_... net/dns/dns_config_service_posix.cc:105: class WatcherDelegate : public FilePathWatcher::Delegate { Might be clearer to make this a non-nested class - especially since this is only in this .cc file. I had a hard time when reading understanding which parts were on the WatcherDelegate and which parts on the DnsConfigServicePosix. http://codereview.chromium.org/7518028/diff/18001/net/dns/dns_config_service_... net/dns/dns_config_service_posix.cc:109: service_(service) {} You need to initialize reading_(false) and read_pending_(false) Could be arbitrary data at this point. http://codereview.chromium.org/7518028/diff/18001/net/dns/dns_config_service_... File net/dns/dns_config_service_posix_unittest.cc (right): http://codereview.chromium.org/7518028/diff/18001/net/dns/dns_config_service_... net/dns/dns_config_service_posix_unittest.cc:117: ASSERT_EQ(res_ninit(&res), 0); This seems flaky and won't get code coverage of regression cases well. One thought is to isolate out the function which does __res_state to DNSConfig translation to make sure that's caught. To test the overall system, I'd suggest being able to overload the file location name [so not /etc/resolv.conf] and add a function pointer or callback so res_ninit isn't called, but some unit-test specific thing which generates a predefined __res_state structure.
http://codereview.chromium.org/7518028/diff/18001/net/dns/dns_config_service.h File net/dns/dns_config_service.h (right): http://codereview.chromium.org/7518028/diff/18001/net/dns/dns_config_service.... net/dns/dns_config_service.h:41: // Min. number of dots before global resolution precedes |search|. On 2011/08/05 19:11:39, cbentzel wrote: > May as well spell out Minimum Done. http://codereview.chromium.org/7518028/diff/18001/net/dns/dns_config_service.... net/dns/dns_config_service.h:44: int timeout; On 2011/08/05 19:11:39, cbentzel wrote: > Probably better to use a base::TimeDelta from base/time.h here instead - and do > the conversion in ProxyConfigServiceLinux. That way you don't need to worry > about unit conversion issues. Done. http://codereview.chromium.org/7518028/diff/18001/net/dns/dns_config_service.... net/dns/dns_config_service.h:97: protected: On 2011/08/05 19:11:39, cbentzel wrote: > Why protected? In general I prefer public/private. Derived classes can always > use dns_config() if needed. The derived class needs to set dns_config_ but I don't want a public setter. Is a protected setter better than a protected field? In the end I removed any non-private access from dns_config_. http://codereview.chromium.org/7518028/diff/18001/net/dns/dns_config_service_... File net/dns/dns_config_service_posix.cc (right): http://codereview.chromium.org/7518028/diff/18001/net/dns/dns_config_service_... net/dns/dns_config_service_posix.cc:48: // TODO(szym): this might not compile on mac. On 2011/08/05 19:11:39, cbentzel wrote: > Can you run try jobs to confirm? > > May also want to comment that this is adding IPV6 server addresses. > > Do you know if there is a way via res_init to interleave ordering of IPv6 and > IPv4 nameservers? In this case, it seems like the IPv6 ones will always be > processed last. glibc uses funny code to determine the ordering of IPv6 vs. IPv4. I elaborated in a comment. http://codereview.chromium.org/7518028/diff/18001/net/dns/dns_config_service_... net/dns/dns_config_service_posix.cc:105: class WatcherDelegate : public FilePathWatcher::Delegate { On 2011/08/05 19:11:39, cbentzel wrote: > Might be clearer to make this a non-nested class - especially since this is only > in this .cc file. I had a hard time when reading understanding which parts were > on the WatcherDelegate and which parts on the DnsConfigServicePosix. Done. http://codereview.chromium.org/7518028/diff/18001/net/dns/dns_config_service_... net/dns/dns_config_service_posix.cc:109: service_(service) {} On 2011/08/05 19:11:39, cbentzel wrote: > You need to initialize reading_(false) and read_pending_(false) > > Could be arbitrary data at this point. Done. Good catch. I wrongly assumed bool() == false. http://codereview.chromium.org/7518028/diff/18001/net/dns/dns_config_service_... File net/dns/dns_config_service_posix_unittest.cc (right): http://codereview.chromium.org/7518028/diff/18001/net/dns/dns_config_service_... net/dns/dns_config_service_posix_unittest.cc:117: ASSERT_EQ(res_ninit(&res), 0); On 2011/08/05 19:11:39, cbentzel wrote: > This seems flaky and won't get code coverage of regression cases well. > > One thought is to isolate out the function which does __res_state to DNSConfig > translation to make sure that's caught. > > To test the overall system, I'd suggest being able to overload the file location > name [so not /etc/resolv.conf] and add a function pointer or callback so > res_ninit isn't called, but some unit-test specific thing which generates a > predefined __res_state structure. Done.
Overall this looks very good. You're dealing very well with tricky threading and lifetime issues, and testing many of these. Most of my concerns are therefore nits and style concerns. http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service.h File net/dns/dns_config_service.h (right): http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service.... net/dns/dns_config_service.h:5: #ifndef NET_DNS_CONFIG_SERVICE_H_ This needs to be NET_DNS_DNS_CONFIG_SERVICE_H for the header guards. presubmit should catch this. http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service.... net/dns/dns_config_service.h:16: #include "net/base/net_util.h" Do you need the net_util.h include? http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service.... net/dns/dns_config_service.h:23: struct DnsConfig { This will need to be NET_EXPORT_PRIVATE for unit tests, I believe. http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service.... net/dns/dns_config_service.h:53: bool Equals(const DnsConfig& d) const { Nit: methods are ordered before variables. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service.... net/dns/dns_config_service.h:53: bool Equals(const DnsConfig& d) const { I'm also not sure if this should be inlined, as it's doing non-trivial work. http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service.... net/dns/dns_config_service.h:70: class DnsConfigService { This will also need to be NET_EXPORT_PRIVATE for unit tests. You should do a try job on the linux_shared bot. http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service_... File net/dns/dns_config_service_posix.cc (right): http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service_... net/dns/dns_config_service_posix.cc:104: // Retry Watch in 100ms or so. 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. http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service_... net/dns/dns_config_service_posix.cc:104: // Retry Watch in 100ms or so. Add a DCHECK(message_loop_->BelongsToCurrentThread()) http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service_... net/dns/dns_config_service_posix.cc:123: virtual ~WatcherDelegate() {} 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. http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service_... net/dns/dns_config_service_posix.cc:132: base::WorkerPool::PostTask(FROM_HERE, base::Bind( Perhaps do a NOTREACHED() if this ever returns false - if the implementation of POSIX version of PostTask ever changes, we want to know. http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service_... net/dns/dns_config_service_posix.cc:174: if (!service_) Good, glad this check is here. http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service_... net/dns/dns_config_service_posix.cc:196: // The watcher must be destroyed on the same thread that called Watch. 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. http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service_... net/dns/dns_config_service_posix.cc:204: DCHECK(resolver_lib_.get()); May as well add DCHECK(watcher_factory_.get()) as well. http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service_... net/dns/dns_config_service_posix.cc:222: watcher_delegate_->ScheduleWatch(); 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? http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service_... File net/dns/dns_config_service_posix.h (right): http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service_... net/dns/dns_config_service_posix.h:20: struct ResolverLib { struct's are intended for buckets-of-bits only. Once you add behavioral stuff like virtuals it should be a class. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Structs_vs._Cl... http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service_... net/dns/dns_config_service_posix.h:20: struct ResolverLib { You may also want to make this listed as NonThreadSafe to help check that it's only called from one worker-thread. It's a little complicated in this case because you will have to do a DetachFromThread each time you post to the worker thread, and again each time that you come back from the worker thread. http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service_... net/dns/dns_config_service_posix.h:27: } DISALLOW_COPY_AND_ASSIGN http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service_... net/dns/dns_config_service_posix.h:31: class FilePathWatcherShim { I'm wondering if these should be nested classes of DnsConfigServicePosix, since the names are fairly generic and I'm not sure if they should be part of the net namespace. http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service_... net/dns/dns_config_service_posix.h:40: scoped_ptr<base::files::FilePathWatcher> watcher_; DISALLOW_COPY_AND_ASSIGN http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service_... net/dns/dns_config_service_posix.h:43: struct FilePathWatcherFactory { Ditto here. http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service_... net/dns/dns_config_service_posix.h:52: class DnsConfigServicePosix : public DnsConfigService { Could this inherit from NonThreadSafe [from base/threading/non_thread_safe] and add appropriate DCHECK's in the functions? This is intended to be used on only one thread. http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service_... net/dns/dns_config_service_posix.h:57: virtual void AddObserver(Observer* observer) OVERRIDE { Don't inline virtual methods. There's a number of rules about when inline'ing is generally OK in Chrome, see http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts for details. I'm not going to mention each one in this CL, so please do a clean up pass. http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service_... net/dns/dns_config_service_posix.h:57: virtual void AddObserver(Observer* observer) OVERRIDE { Thanks for adding OVERRIDE. http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service_... net/dns/dns_config_service_posix.h:59: if (have_config_) { Good idea, cool. http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service_... net/dns/dns_config_service_posix.h:110: Nit: extra new line http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service_... File net/dns/dns_config_service_posix_unittest.cc (right): http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service_... net/dns/dns_config_service_posix_unittest.cc:108: Nit: extra line. http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service_... net/dns/dns_config_service_posix_unittest.cc:129: virtual int ninit(res_state res) OVERRIDE { Does this need an nclose()? http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service_... net/dns/dns_config_service_posix_unittest.cc:250: : res_lib_(new MockResolverLib(this)), Does this get leaked in favor of the one created in SetUp()? http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service_... net/dns/dns_config_service_posix_unittest.cc:349: EXPECT_TRUE(watch_called_) << "Must call FilePathWatcher::Watch()."; I need to think through this some more, but I'm worried that there's potentially flake here where we process the call to watch and post the quit task, another thread gets scheduled for a while, we come back to this thread and handle the delayed callback before actually quitting the message loop. It might be better to set up a series of expectations in that case. But let me think through it some more. http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service_... net/dns/dns_config_service_posix_unittest.cc:353: message_loop_->Run(); // until Watch, takes 100ms You may want to make it clear that there isn't a sleep going on here, or other timing things - those are notoriously flaky for tests, and this is a more principled approach.
Hopefully all races are gone, but trybot might still catch some. I haven't "valground" it yet. http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service.h File net/dns/dns_config_service.h (right): http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service.... net/dns/dns_config_service.h:5: #ifndef NET_DNS_CONFIG_SERVICE_H_ On 2011/08/15 18:16:34, cbentzel wrote: > This needs to be > > NET_DNS_DNS_CONFIG_SERVICE_H for the header guards. presubmit should catch this. > Done. http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service.... net/dns/dns_config_service.h:16: #include "net/base/net_util.h" On 2011/08/15 18:16:34, cbentzel wrote: > Do you need the net_util.h include? Done. http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service.... net/dns/dns_config_service.h:23: struct DnsConfig { On 2011/08/15 18:16:34, cbentzel wrote: > This will need to be NET_EXPORT_PRIVATE for unit tests, I believe. Done. http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service.... net/dns/dns_config_service.h:53: bool Equals(const DnsConfig& d) const { On 2011/08/15 18:16:34, cbentzel wrote: > Nit: methods are ordered before variables. > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order Done. http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service.... net/dns/dns_config_service.h:70: class DnsConfigService { On 2011/08/15 18:16:34, cbentzel wrote: > This will also need to be NET_EXPORT_PRIVATE for unit tests. You should do a try > job on the linux_shared bot. Done. http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service_... File net/dns/dns_config_service_posix.cc (right): http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service_... net/dns/dns_config_service_posix.cc:104: // Retry Watch in 100ms or so. On 2011/08/15 18:16:34, cbentzel wrote: > Add a DCHECK(message_loop_->BelongsToCurrentThread()) Done. http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service_... net/dns/dns_config_service_posix.cc:104: // Retry Watch in 100ms or so. On 2011/08/15 18:16:34, cbentzel wrote: > 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. Looking at _linux, indeed it cannot fail. However, in _mac (which is covered by _posix), it could. http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service_... net/dns/dns_config_service_posix.cc:123: virtual ~WatcherDelegate() {} On 2011/08/15 18:16:34, cbentzel wrote: > 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. 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. http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service_... net/dns/dns_config_service_posix.cc:132: base::WorkerPool::PostTask(FROM_HERE, base::Bind( On 2011/08/15 18:16:34, cbentzel wrote: > Perhaps do a NOTREACHED() if this ever returns false - if the implementation of > POSIX version of PostTask ever changes, we want to know. Done. http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service_... net/dns/dns_config_service_posix.cc:196: // The watcher must be destroyed on the same thread that called Watch. On 2011/08/15 18:16:34, cbentzel wrote: > 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. Done. http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service_... net/dns/dns_config_service_posix.cc:204: DCHECK(resolver_lib_.get()); On 2011/08/15 18:16:34, cbentzel wrote: > May as well add DCHECK(watcher_factory_.get()) as well. Done. http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service_... net/dns/dns_config_service_posix.cc:222: watcher_delegate_->ScheduleWatch(); On 2011/08/15 18:16:34, cbentzel wrote: > 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? Added a comment to WatcherDelegate::ScheduleWatch to explain that this is to avoid having to add RefCountedBase to DnsConfigServicePosix. http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service_... File net/dns/dns_config_service_posix.h (right): http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service_... net/dns/dns_config_service_posix.h:20: struct ResolverLib { On 2011/08/15 18:16:34, cbentzel wrote: > struct's are intended for buckets-of-bits only. Once you add behavioral stuff > like virtuals it should be a class. > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Structs_vs._Cl... Done. http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service_... net/dns/dns_config_service_posix.h:27: } On 2011/08/15 18:16:34, cbentzel wrote: > DISALLOW_COPY_AND_ASSIGN Done. http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service_... net/dns/dns_config_service_posix.h:31: class FilePathWatcherShim { On 2011/08/15 18:16:34, cbentzel wrote: > I'm wondering if these should be nested classes of DnsConfigServicePosix, since > the names are fairly generic and I'm not sure if they should be part of the net > namespace. Done. http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service_... net/dns/dns_config_service_posix.h:40: scoped_ptr<base::files::FilePathWatcher> watcher_; On 2011/08/15 18:16:34, cbentzel wrote: > DISALLOW_COPY_AND_ASSIGN Done. http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service_... net/dns/dns_config_service_posix.h:43: struct FilePathWatcherFactory { On 2011/08/15 18:16:34, cbentzel wrote: > Ditto here. Done. http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service_... net/dns/dns_config_service_posix.h:52: class DnsConfigServicePosix : public DnsConfigService { On 2011/08/15 18:16:34, cbentzel wrote: > Could this inherit from NonThreadSafe [from base/threading/non_thread_safe] and > add appropriate DCHECK's in the functions? This is intended to be used on only > one thread. Done. http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service_... net/dns/dns_config_service_posix.h:57: virtual void AddObserver(Observer* observer) OVERRIDE { On 2011/08/15 18:16:34, cbentzel wrote: > Don't inline virtual methods. There's a number of rules about when inline'ing is > generally OK in Chrome, see > http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts for details. > I'm not going to mention each one in this CL, so please do a clean up pass. Done. Although only in .h files. http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service_... net/dns/dns_config_service_posix.h:110: On 2011/08/15 18:16:34, cbentzel wrote: > Nit: extra new line Done. http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service_... File net/dns/dns_config_service_posix_unittest.cc (right): http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service_... net/dns/dns_config_service_posix_unittest.cc:108: On 2011/08/15 18:16:34, cbentzel wrote: > Nit: extra line. Done. http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service_... net/dns/dns_config_service_posix_unittest.cc:129: virtual int ninit(res_state res) OVERRIDE { On 2011/08/15 18:16:34, cbentzel wrote: > Does this need an nclose()? Done. There were a few missing nclose calls. http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service_... net/dns/dns_config_service_posix_unittest.cc:250: : res_lib_(new MockResolverLib(this)), On 2011/08/15 18:16:34, cbentzel wrote: > Does this get leaked in favor of the one created in SetUp()? Good catch. Done. http://codereview.chromium.org/7518028/diff/25002/net/dns/dns_config_service_... net/dns/dns_config_service_posix_unittest.cc:353: message_loop_->Run(); // until Watch, takes 100ms On 2011/08/15 18:16:34, cbentzel wrote: > You may want to make it clear that there isn't a sleep going on here, or other > timing things - those are notoriously flaky for tests, and this is a more > principled approach. Done.
LGTM I'll patch and run a try job. http://codereview.chromium.org/7518028/diff/25004/net/dns/dns_config_service_... File net/dns/dns_config_service_posix.cc (right): http://codereview.chromium.org/7518028/diff/25004/net/dns/dns_config_service_... net/dns/dns_config_service_posix.cc:171: void DnsConfigServicePosix::OnConfigRead(const DnsConfig& config) { Maybe DCHECK(CalledOnValidThread()). It's not a public method, but I sometimes find these very useful as both documentation and sanity checking.
The commit queue went berserk retrying too often for a seemingly flaky test. Builder is linux, revision is 97176, job name was 7518028-36007 (retry) (retry) (retry) (retry).
Change committed as 97282 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
