Command line utility to run the HostResolver. ...

I don't actually want this issue anymore. I just want to test my gmail filter.

[cbentzelold, 2009-11-21 04:25:10]

I wrote this to learn the ropes of the Chromium codebase, but a discussion on
chromium-dev lead me to believe that this could be of potential use to others.

I listed you as a reviewer since svn blame listed you as a main contributor to
host_resolver.[h|cc]. Please re-route as necessary.

[cbentzelold, 2009-11-21 04:27:13]

Also - the lint errors in hresolv.cc are due to including "build/build_config.h"
prior to ws2tcpip.h/netdb.h which need to be conditionally included based on
platform using macros that build_config.h specifies.

There may be a workaround for this - but another consideration is to change
cpplint.py to whitelist certain files.

[eroman, 2009-11-21 05:28:25]

This is a lot of code.

I am a bit skeptical as to how useful this is to have checked into the
repository, since it doesn't seem any more illustrative then the unit-tests.

Maybe I am wrong; adding wtc to see what he thinks.

http://codereview.chromium.org/428004/diff/1/4
File net/base/host_resolver.h (right):

http://codereview.chromium.org/428004/diff/1/4#newcode119
Line 119: // If |out_req| is non-NULL, then |*out_req| will be filled with a
handle to
Good spots!

Can you move the comment fixes to a separate changelist?

(That will keep the changelog descriptions more concise, and also expedite
getting them fixed in).

http://codereview.chromium.org/428004/diff/1/3
File net/tools/hresolv/hresolv.cc (right):

http://codereview.chromium.org/428004/diff/1/3#newcode72
Line 72: if (ai_flags & flag_name.flag) {
nit: might be possible there are values not enumerated by kAddrInfoFlagNames.
would be robust if anything is left over, and if so print that as  say: " |
0x10000001"

http://codereview.chromium.org/428004/diff/1/3#newcode201
Line 201: 
omit empty line

http://codereview.chromium.org/428004/diff/1/3#newcode207
Line 207: t.ToInternalValue(), event_kind.c_str(), id,
Please don't use ToInternvalValue(), those units are undefined.

Instead, subtract Time() from t to get a TimeDelta, and then call
InMilliseconds() or something on it.

http://codereview.chromium.org/428004/diff/1/3#newcode214
Line 214: explicit ResolveTask(net::HostResolver* resolver,
No explicit for ctors with more than 1 argument.

http://codereview.chromium.org/428004/diff/1/3#newcode235
Line 235: printf("Error resolving %s: %d\n", host_.c_str(), err);
Not necessarily an error. May have completed with synchronous success (net::OK).

http://codereview.chromium.org/428004/diff/1/3#newcode240
Line 240: 
remove empty line

http://codereview.chromium.org/428004/diff/1/3#newcode255
Line 255: 
ditto.

http://codereview.chromium.org/428004/diff/1/3#newcode256
Line 256: explicit ResolverInvoker(net::HostResolver* resolver) :
no explicit keyword.

also, the formatting is off. see
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr...

http://codereview.chromium.org/428004/diff/1/3#newcode273
Line 273: base::TimeDelta::FromMilliseconds(hosts_and_times[i].time_in_ms);
continued lines indent by 4.

http://codereview.chromium.org/428004/diff/1/3#newcode294
Line 294: 
remove this line

http://codereview.chromium.org/428004/diff/1/3#newcode303
Line 303: if (err) {
we usually try not to rely on fact that OK is 0.
how about if (err != OK)

http://codereview.chromium.org/428004/diff/1/3#newcode321
Line 321: resolve_thread_.message_loop()->PostTask(FROM_HERE, task);
I don't quite get why this async branch is using a resolve thread.

When you operate the HostResolver in async mode (by specifying a non-NULL
callback during Resolve(), it will already be spinning up a task in the worker
pool, and returning immediately to the caller (to be notified later of
completion).

So deferring further the call to Resolve() seems unnecessarily complicated.
Perhaps I am not understanding this.

http://codereview.chromium.org/428004/diff/1/3#newcode334
Line 334: 
remove line

http://codereview.chromium.org/428004/diff/1/3#newcode339
Line 339: address_list_(new net::AddressList()) {
why make address_list a pointer?

http://codereview.chromium.org/428004/diff/1/3#newcode347
Line 347: if (err) {
same comment as earlier -- favor doing if (err != OK) instead of if (err).

http://codereview.chromium.org/428004/diff/1/3#newcode361
Line 361: 
please remove empty line.

http://codereview.chromium.org/428004/diff/1/3#newcode375
Line 375: CommandLineOptions() :
see earlier comment about formatting initializer lists.

http://codereview.chromium.org/428004/diff/1/3#newcode535
Line 535: PlatformThread::Sleep(100);
Instead of a sleep loop, what about just doing:

    MessageLoop::PostDelayedTask()

to enqueue a task for each of the entries, to fire at the right time?

[cbentzelold, 2009-11-21 14:51:01]

On 2009/11/21 05:28:25, eroman wrote:
> This is a lot of code.

Yes - thanks for taking the time looking at it. I will fix the issues you
pointed out - but most of them seem like things that should have been caught and
repaired automatically (like the formatting issues). I'll try to add new rules
to cpplint.py and an emacs mode.

I'll also break out the comment fixes to a separate CL as you suggested.

> I am a bit skeptical as to how useful this is to have checked into the
> repository, since it doesn't seem any more illustrative then the unit-tests.

The main advantage is I can try it out on real domains and see how it reacts -
it's not isolated from real DNS servers like the unittests correctly are. For
example, I ran this with tcpdump/wireshark on windows to see which DNS queries
were actually issued when multiple requests for the same host were invoked and
they were not picked up by the cache. It has the potential to help debug strange
problems which can then be formalized as a unit test to prevent regressions.

That being said - I also have some reservations of how useful it will turn out
to be. Here's a discussion about it on chromium-dev:
http://groups.google.com/group/chromium-dev/browse_thread/thread/e48c8ea620f6...

[cbentzelold, 2009-11-21 14:51:25]

http://codereview.chromium.org/428004/diff/1/3
File net/tools/hresolv/hresolv.cc (right):

http://codereview.chromium.org/428004/diff/1/3#newcode321
Line 321: resolve_thread_.message_loop()->PostTask(FROM_HERE, task);
On 2009/11/21 05:28:25, eroman wrote:
> I don't quite get why this async branch is using a resolve thread.
> 

The reason this is done is that ResolveAsync is called from the main thread for
the program. The HostResolver::Resolve code must be called from the thread
containing the message loop for the HostResolver - because the worker threads
post back to that thread when they are done and expect a message loop. If
HostResolver::Resolve is called from the main thread then the completion
notifications will never be processed.
> When you operate the HostResolver in async mode (by specifying a non-NULL
> callback during Resolve(), it will already be spinning up a task in the worker
> pool, and returning immediately to the caller (to be notified later of
> completion).
> 
> So deferring further the call to Resolve() seems unnecessarily complicated.
> Perhaps I am not understanding this.

[cbentzelold, 2009-11-23 19:05:59]

Thanks again for doing the review - sorry that so many of the issues were style
problems.

http://codereview.chromium.org/428004/diff/1/3
File net/tools/hresolv/hresolv.cc (right):

http://codereview.chromium.org/428004/diff/1/3#newcode72
net/tools/hresolv/hresolv.cc:72: if (ai_flags & flag_name.flag) {
On 2009/11/21 05:28:25, eroman wrote:
> nit: might be possible there are values not enumerated by kAddrInfoFlagNames.
> would be robust if anything is left over, and if so print that as  say: " |
> 0x10000001"

Done.

http://codereview.chromium.org/428004/diff/1/3#newcode201
net/tools/hresolv/hresolv.cc:201: 
On 2009/11/21 05:28:25, eroman wrote:
> omit empty line

Done.

http://codereview.chromium.org/428004/diff/1/3#newcode207
net/tools/hresolv/hresolv.cc:207: t.ToInternalValue(), event_kind.c_str(), id,
On 2009/11/21 05:28:25, eroman wrote:
> Please don't use ToInternvalValue(), those units are undefined.
> 
> Instead, subtract Time() from t to get a TimeDelta, and then call
> InMilliseconds() or something on it.

Done.

http://codereview.chromium.org/428004/diff/1/3#newcode214
net/tools/hresolv/hresolv.cc:214: explicit ResolveTask(net::HostResolver*
resolver,
On 2009/11/21 05:28:25, eroman wrote:
> No explicit for ctors with more than 1 argument.

I always find this rule odd as I prefer to just explicit-by-default so I don't
mistakenly miss it when necessary.

Still, fixed.

http://codereview.chromium.org/428004/diff/1/3#newcode235
net/tools/hresolv/hresolv.cc:235: printf("Error resolving %s: %d\n",
host_.c_str(), err);
On 2009/11/21 05:28:25, eroman wrote:
> Not necessarily an error. May have completed with synchronous success
(net::OK).

http://codereview.chromium.org/428004/diff/1/3#newcode240
net/tools/hresolv/hresolv.cc:240: 
On 2009/11/21 05:28:25, eroman wrote:
> remove empty line

Done.

http://codereview.chromium.org/428004/diff/1/3#newcode255
net/tools/hresolv/hresolv.cc:255: 
On 2009/11/21 05:28:25, eroman wrote:
> ditto.

Done.

http://codereview.chromium.org/428004/diff/1/3#newcode256
net/tools/hresolv/hresolv.cc:256: explicit ResolverInvoker(net::HostResolver*
resolver) :
On 2009/11/21 05:28:25, eroman wrote:
> no explicit keyword.
> 
> also, the formatting is off. see
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr...

Why no explicit for a single argument constructor?

Also, fixed formatting.

http://codereview.chromium.org/428004/diff/1/3#newcode273
net/tools/hresolv/hresolv.cc:273:
base::TimeDelta::FromMilliseconds(hosts_and_times[i].time_in_ms);
On 2009/11/21 05:28:25, eroman wrote:
> continued lines indent by 4.

Done.

http://codereview.chromium.org/428004/diff/1/3#newcode294
net/tools/hresolv/hresolv.cc:294: 
On 2009/11/21 05:28:25, eroman wrote:
> remove this line

Done.

http://codereview.chromium.org/428004/diff/1/3#newcode303
net/tools/hresolv/hresolv.cc:303: if (err) {
On 2009/11/21 05:28:25, eroman wrote:
> we usually try not to rely on fact that OK is 0.
> how about if (err != OK)

Done.

http://codereview.chromium.org/428004/diff/1/3#newcode334
net/tools/hresolv/hresolv.cc:334: 
On 2009/11/21 05:28:25, eroman wrote:
> remove line

Done.

http://codereview.chromium.org/428004/diff/1/3#newcode339
net/tools/hresolv/hresolv.cc:339: address_list_(new net::AddressList()) {
On 2009/11/21 05:28:25, eroman wrote:
> why make address_list a pointer?

Good point - fixed.

http://codereview.chromium.org/428004/diff/1/3#newcode347
net/tools/hresolv/hresolv.cc:347: if (err) {
On 2009/11/21 05:28:25, eroman wrote:
> same comment as earlier -- favor doing if (err != OK) instead of if (err).

Done.

http://codereview.chromium.org/428004/diff/1/3#newcode361
net/tools/hresolv/hresolv.cc:361: 
On 2009/11/21 05:28:25, eroman wrote:
> please remove empty line.

Done.

http://codereview.chromium.org/428004/diff/1/3#newcode375
net/tools/hresolv/hresolv.cc:375: CommandLineOptions() :
On 2009/11/21 05:28:25, eroman wrote:
> see earlier comment about formatting initializer lists.

Done.

http://codereview.chromium.org/428004/diff/1/3#newcode535
net/tools/hresolv/hresolv.cc:535: PlatformThread::Sleep(100);
On 2009/11/21 05:28:25, eroman wrote:
> Instead of a sleep loop, what about just doing:
> 
>     MessageLoop::PostDelayedTask() 
> to enqueue a task for each of the entries, to fire at the right time?

We'll still want to wait for the async requests to finish up - the Task will
simply be a request to issue the async resolve on the HostResolver's message
loop, but we really want to wait for the worker threads to finish their jobs.

I considered using PostDelayedTask to schedule the HostResolver::Resolve calls
but decided against it because it complicated the sync codepath compared to the
sleep loop IMO.

[cbentzelold, 2009-11-23 19:11:12]

http://codereview.chromium.org/428004/diff/1/3
File net/tools/hresolv/hresolv.cc (right):

http://codereview.chromium.org/428004/diff/1/3#newcode235
net/tools/hresolv/hresolv.cc:235: printf("Error resolving %s: %d\n",
host_.c_str(), err);
On 2009/11/23 19:06:00, cbentzel wrote:
> On 2009/11/21 05:28:25, eroman wrote:
> > Not necessarily an error. May have completed with synchronous success
> (net::OK).
> 
> 

Done.

[wtc, 2009-11-24 20:00:48]

LGTM.  Some suggested changes below.

http://codereview.chromium.org/428004/diff/6007/6008
File net/net.gyp (right):

http://codereview.chromium.org/428004/diff/6007/6008#newcode852
net/net.gyp:852: 'target_name': 'hresolv',
It seems that you need an 'msvs_guid' entry for
hresolv.  I don't know how it's generated though.

Is 'hresolv' a Unix command?

http://codereview.chromium.org/428004/diff/6007/6009
File net/tools/hresolv/hresolv.cc (right):

http://codereview.chromium.org/428004/diff/6007/6009#newcode1
net/tools/hresolv/hresolv.cc:1: // Copyright (c) 2006-2009 The Chromium Authors.
All rights reserved.
Nit: the copyright year should be just 2009.

http://codereview.chromium.org/428004/diff/6007/6009#newcode20
net/tools/hresolv/hresolv.cc:20: #include "build/build_config.h"
Nit: add a blank line below.

http://codereview.chromium.org/428004/diff/6007/6009#newcode44
net/tools/hresolv/hresolv.cc:44: unsigned int flag;
Nit: 'flag' should have the 'int' type because the
ai_flags field of struct addrinfo is an 'int':
http://www.opengroup.org/onlinepubs/009695399/basedefs/netdb.h.html
http://msdn.microsoft.com/en-us/library/ms737530(VS.85).aspx

http://codereview.chromium.org/428004/diff/6007/6009#newcode55
net/tools/hresolv/hresolv.cc:55: #if defined(OS_LINUX)
We probably don't need these Linux extensions.

Nit: __USE_GNU is a glibc internal macro.  We probably
should test _GNU_SOURCE instead.  See the comments in
/usr/include/features.h.

http://codereview.chromium.org/428004/diff/6007/6009#newcode64
net/tools/hresolv/hresolv.cc:64: {AI_NUMERICSERV, "AI_NUMERICSERV"},
We should not need ifdefs for AI_NUMERICSERV.  Is it
not available on Mac OS X?

http://codereview.chromium.org/428004/diff/6007/6009#newcode68
net/tools/hresolv/hresolv.cc:68: std::string FormatAddrinfoFlags(unsigned int
ai_flags) {
Nit: declare ai_flags as 'int' to match the type of the
ai_flags field of struct addrinfo, unless you need it to
match the %x format you use below.

http://codereview.chromium.org/428004/diff/6007/6009#newcode89
net/tools/hresolv/hresolv.cc:89: const char* GetNameOfFlag(unsigned int
num_flag_names,
Nit: reverse the order of num_flag_names and flag_names.
The (array_ptr, array_size) ordering is more common.

http://codereview.chromium.org/428004/diff/6007/6009#newcode265
net/tools/hresolv/hresolv.cc:265: int time_in_ms;
Please document what this field means.

http://codereview.chromium.org/428004/diff/6007/6009#newcode304
net/tools/hresolv/hresolv.cc:304: AutoLock l(lock_outstanding_requests_);
Nit: use "lock" instead of "l".

http://codereview.chromium.org/428004/diff/6007/6009#newcode342
net/tools/hresolv/hresolv.cc:342: outstanding_requests_.find(request);
Nit: indent a continuation line by 4.

http://codereview.chromium.org/428004/diff/6007/6009#newcode401
net/tools/hresolv/hresolv.cc:401: const char* kVerbose = "verbose";
Our convention is to use hyphen '-' (instead of underscore '_')
to break the words in a command line option.  See examples
in base/base_switches.cc and chrome/common/chrome_switches.cc.

http://codereview.chromium.org/428004/diff/6007/6009#newcode479
net/tools/hresolv/hresolv.cc:479: case 1:
cases with curly braces should be formatted like this:
  case 1: {
    ...
    break;
  }

See the style guide:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Loops_and_Swit...

http://codereview.chromium.org/428004/diff/6007/6009#newcode548
net/tools/hresolv/hresolv.cc:548: CommandLine::Reset();
Add
  return 0;
at the end of the main() function.

[cbentzelold, 2009-11-24 20:05:52]

Thanks for looking at it - I'll fix up the nits that you mentioned and ping this
thread back when done.

http://codereview.chromium.org/428004/diff/6007/6008
File net/net.gyp (right):

http://codereview.chromium.org/428004/diff/6007/6008#newcode852
net/net.gyp:852: 'target_name': 'hresolv',
On 2009/11/24 20:00:48, wtc wrote:
> It seems that you need an 'msvs_guid' entry for
> hresolv.  I don't know how it's generated though.
> 
> Is 'hresolv' a Unix command?

Probably - I did build this on Windows but I'll discover how.

I made up the name hresolv - and it does follow the unix convention of
short-but-obscure command names. I'm fine with changing it to host_resolve or
something more obvious.

http://codereview.chromium.org/428004/diff/6007/6009
File net/tools/hresolv/hresolv.cc (right):

http://codereview.chromium.org/428004/diff/6007/6009#newcode64
net/tools/hresolv/hresolv.cc:64: {AI_NUMERICSERV, "AI_NUMERICSERV"},
On 2009/11/24 20:00:48, wtc wrote:
> We should not need ifdefs for AI_NUMERICSERV.  Is it
> not available on Mac OS X?

It's not available on OSX.

[wtc, 2009-11-24 20:13:55]

Re: hresolv: the reason I asked is that I remember using
an hresolv command or function before.  I was not complaining.
The name you chose sounds very Unix-y.

http://codereview.chromium.org/428004/diff/6007/6009
File net/tools/hresolv/hresolv.cc (right):

http://codereview.chromium.org/428004/diff/6007/6009#newcode64
net/tools/hresolv/hresolv.cc:64: {AI_NUMERICSERV, "AI_NUMERICSERV"},
On 2009/11/24 20:05:52, cbentzel wrote:
>
> It's not available on OSX.

Assuming these flags are defined as macros, a better way
to deal with the flags that aren't universally available
is to test them directly:

#if defined(AI_NUMERICSERV)
  {AI_NUMERICSERV, "AI_NUMERICSERV"},
#endif

[cbentzelold, 2009-11-24 21:46:10]

http://codereview.chromium.org/428004/diff/6007/6008
File net/net.gyp (right):

http://codereview.chromium.org/428004/diff/6007/6008#newcode852
net/net.gyp:852: 'target_name': 'hresolv',
On 2009/11/24 20:05:52, cbentzel wrote:
> On 2009/11/24 20:00:48, wtc wrote:
> > It seems that you need an 'msvs_guid' entry for
> > hresolv.  I don't know how it's generated though.
> > 
> > Is 'hresolv' a Unix command?
> 
> Probably - I did build this on Windows but I'll discover how.
> 
> I made up the name hresolv - and it does follow the unix convention of
> short-but-obscure command names. I'm fine with changing it to host_resolve or
> something more obvious.

It turns out that msvs_guid is being deprecated and new targets should not
include it.

Also - the email thread for this code review is now the top hit on Google for
hresolv, so I don't think it's a name of a tool (or at least a commonly used
one).

http://codereview.chromium.org/428004/diff/6007/6009
File net/tools/hresolv/hresolv.cc (right):

http://codereview.chromium.org/428004/diff/6007/6009#newcode1
net/tools/hresolv/hresolv.cc:1: // Copyright (c) 2006-2009 The Chromium Authors.
All rights reserved.
On 2009/11/24 20:00:48, wtc wrote:
> Nit: the copyright year should be just 2009.

Done.

http://codereview.chromium.org/428004/diff/6007/6009#newcode20
net/tools/hresolv/hresolv.cc:20: #include "build/build_config.h"
On 2009/11/24 20:00:48, wtc wrote:
> Nit: add a blank line below.

Done.

http://codereview.chromium.org/428004/diff/6007/6009#newcode44
net/tools/hresolv/hresolv.cc:44: unsigned int flag;
On 2009/11/24 20:00:48, wtc wrote:
> Nit: 'flag' should have the 'int' type because the
> ai_flags field of struct addrinfo is an 'int':
> http://www.opengroup.org/onlinepubs/009695399/basedefs/netdb.h.html
> http://msdn.microsoft.com/en-us/library/ms737530%28VS.85%29.aspx

Done.

http://codereview.chromium.org/428004/diff/6007/6009#newcode55
net/tools/hresolv/hresolv.cc:55: #if defined(OS_LINUX)
On 2009/11/24 20:00:48, wtc wrote:
> We probably don't need these Linux extensions.

You're right - I removed them.

> 
> Nit: __USE_GNU is a glibc internal macro.  We probably
> should test _GNU_SOURCE instead.  See the comments in
> /usr/include/features.h.

I will do this in the future.

http://codereview.chromium.org/428004/diff/6007/6009#newcode64
net/tools/hresolv/hresolv.cc:64: {AI_NUMERICSERV, "AI_NUMERICSERV"},
On 2009/11/24 20:13:55, wtc wrote:
> On 2009/11/24 20:05:52, cbentzel wrote:
> >
> > It's not available on OSX.
> 
> Assuming these flags are defined as macros, a better way
> to deal with the flags that aren't universally available
> is to test them directly:
> 
> #if defined(AI_NUMERICSERV)
>   {AI_NUMERICSERV, "AI_NUMERICSERV"},
> #endif
> 

Yes - they are macros on Linux and Windows. However, I'm just going to remove
this as it's not currently used. The MSDN documentation indicates that it's
present only on Vista and above - and not supported in any case. 

http://msdn.microsoft.com/en-us/library/ms738520(VS.85).aspx

http://codereview.chromium.org/428004/diff/6007/6009#newcode68
net/tools/hresolv/hresolv.cc:68: std::string FormatAddrinfoFlags(unsigned int
ai_flags) {
On 2009/11/24 20:00:48, wtc wrote:
> Nit: declare ai_flags as 'int' to match the type of the
> ai_flags field of struct addrinfo, unless you need it to
> match the %x format you use below.

Done.

http://codereview.chromium.org/428004/diff/6007/6009#newcode89
net/tools/hresolv/hresolv.cc:89: const char* GetNameOfFlag(unsigned int
num_flag_names,
On 2009/11/24 20:00:48, wtc wrote:
> Nit: reverse the order of num_flag_names and flag_names.
> The (array_ptr, array_size) ordering is more common.

Done.

http://codereview.chromium.org/428004/diff/6007/6009#newcode265
net/tools/hresolv/hresolv.cc:265: int time_in_ms;
On 2009/11/24 20:00:48, wtc wrote:
> Please document what this field means.

Added documentation here - and to the rest of the file.

http://codereview.chromium.org/428004/diff/6007/6009#newcode304
net/tools/hresolv/hresolv.cc:304: AutoLock l(lock_outstanding_requests_);
On 2009/11/24 20:00:48, wtc wrote:
> Nit: use "lock" instead of "l".

Done.

http://codereview.chromium.org/428004/diff/6007/6009#newcode342
net/tools/hresolv/hresolv.cc:342: outstanding_requests_.find(request);
On 2009/11/24 20:00:48, wtc wrote:
> Nit: indent a continuation line by 4.

I've got to spend time configuring VS2008/emacs to auto-indent correctly.

Done.

http://codereview.chromium.org/428004/diff/6007/6009#newcode401
net/tools/hresolv/hresolv.cc:401: const char* kVerbose = "verbose";
On 2009/11/24 20:00:48, wtc wrote:
> Our convention is to use hyphen '-' (instead of underscore '_')
> to break the words in a command line option.  See examples
> in base/base_switches.cc and chrome/common/chrome_switches.cc.

Done.

http://codereview.chromium.org/428004/diff/6007/6009#newcode479
net/tools/hresolv/hresolv.cc:479: case 1:
On 2009/11/24 20:00:48, wtc wrote:
> cases with curly braces should be formatted like this:
>   case 1: {
>     ...
>     break;
>   }
> 
> See the style guide:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Loops_and_Swit...

Done.

http://codereview.chromium.org/428004/diff/6007/6009#newcode548
net/tools/hresolv/hresolv.cc:548: CommandLine::Reset();
On 2009/11/24 20:00:48, wtc wrote:
> Add
>   return 0;
> at the end of the main() function.

D'oh. Done.

[cbentzelold, 2009-11-24 21:55:04]

On 2009/11/24 21:46:10, cbentzel wrote:
> http://codereview.chromium.org/428004/diff/6007/6008
> File net/net.gyp (right):
> 
> http://codereview.chromium.org/428004/diff/6007/6008#newcode852
> net/net.gyp:852: 'target_name': 'hresolv',
> On 2009/11/24 20:05:52, cbentzel wrote:
> > On 2009/11/24 20:00:48, wtc wrote:
> > > It seems that you need an 'msvs_guid' entry for
> > > hresolv.  I don't know how it's generated though.
> > > 
> > > Is 'hresolv' a Unix command?
> > 
> > Probably - I did build this on Windows but I'll discover how.
> > 
> > I made up the name hresolv - and it does follow the unix convention of
> > short-but-obscure command names. I'm fine with changing it to host_resolve
or
> > something more obvious.
> 
> It turns out that msvs_guid is being deprecated and new targets should not
> include it.
> 
> Also - the email thread for this code review is now the top hit on Google for
> hresolv, so I don't think it's a name of a tool (or at least a commonly used
> one).
> 
> http://codereview.chromium.org/428004/diff/6007/6009
> File net/tools/hresolv/hresolv.cc (right):
> 
> http://codereview.chromium.org/428004/diff/6007/6009#newcode1
> net/tools/hresolv/hresolv.cc:1: // Copyright (c) 2006-2009 The Chromium
Authors.
> All rights reserved.
> On 2009/11/24 20:00:48, wtc wrote:
> > Nit: the copyright year should be just 2009.
> 
> Done.
> 
> http://codereview.chromium.org/428004/diff/6007/6009#newcode20
> net/tools/hresolv/hresolv.cc:20: #include "build/build_config.h"
> On 2009/11/24 20:00:48, wtc wrote:
> > Nit: add a blank line below.
> 
> Done.
> 
> http://codereview.chromium.org/428004/diff/6007/6009#newcode44
> net/tools/hresolv/hresolv.cc:44: unsigned int flag;
> On 2009/11/24 20:00:48, wtc wrote:
> > Nit: 'flag' should have the 'int' type because the
> > ai_flags field of struct addrinfo is an 'int':
> > http://www.opengroup.org/onlinepubs/009695399/basedefs/netdb.h.html
> > http://msdn.microsoft.com/en-us/library/ms737530%2528VS.85%2529.aspx
> 
> Done.
> 
> http://codereview.chromium.org/428004/diff/6007/6009#newcode55
> net/tools/hresolv/hresolv.cc:55: #if defined(OS_LINUX)
> On 2009/11/24 20:00:48, wtc wrote:
> > We probably don't need these Linux extensions.
> 
> You're right - I removed them.
> 
> > 
> > Nit: __USE_GNU is a glibc internal macro.  We probably
> > should test _GNU_SOURCE instead.  See the comments in
> > /usr/include/features.h.
> 
> I will do this in the future.
> 
> http://codereview.chromium.org/428004/diff/6007/6009#newcode64
> net/tools/hresolv/hresolv.cc:64: {AI_NUMERICSERV, "AI_NUMERICSERV"},
> On 2009/11/24 20:13:55, wtc wrote:
> > On 2009/11/24 20:05:52, cbentzel wrote:
> > >
> > > It's not available on OSX.
> > 
> > Assuming these flags are defined as macros, a better way
> > to deal with the flags that aren't universally available
> > is to test them directly:
> > 
> > #if defined(AI_NUMERICSERV)
> >   {AI_NUMERICSERV, "AI_NUMERICSERV"},
> > #endif
> > 
> 
> Yes - they are macros on Linux and Windows. However, I'm just going to remove
> this as it's not currently used. The MSDN documentation indicates that it's
> present only on Vista and above - and not supported in any case. 
> 
> http://msdn.microsoft.com/en-us/library/ms738520%28VS.85%29.aspx
> 
> http://codereview.chromium.org/428004/diff/6007/6009#newcode68
> net/tools/hresolv/hresolv.cc:68: std::string FormatAddrinfoFlags(unsigned int
> ai_flags) {
> On 2009/11/24 20:00:48, wtc wrote:
> > Nit: declare ai_flags as 'int' to match the type of the
> > ai_flags field of struct addrinfo, unless you need it to
> > match the %x format you use below.
> 
> Done.
> 
> http://codereview.chromium.org/428004/diff/6007/6009#newcode89
> net/tools/hresolv/hresolv.cc:89: const char* GetNameOfFlag(unsigned int
> num_flag_names,
> On 2009/11/24 20:00:48, wtc wrote:
> > Nit: reverse the order of num_flag_names and flag_names.
> > The (array_ptr, array_size) ordering is more common.
> 
> Done.
> 
> http://codereview.chromium.org/428004/diff/6007/6009#newcode265
> net/tools/hresolv/hresolv.cc:265: int time_in_ms;
> On 2009/11/24 20:00:48, wtc wrote:
> > Please document what this field means.
> 
> Added documentation here - and to the rest of the file.
> 
> http://codereview.chromium.org/428004/diff/6007/6009#newcode304
> net/tools/hresolv/hresolv.cc:304: AutoLock l(lock_outstanding_requests_);
> On 2009/11/24 20:00:48, wtc wrote:
> > Nit: use "lock" instead of "l".
> 
> Done.
> 
> http://codereview.chromium.org/428004/diff/6007/6009#newcode342
> net/tools/hresolv/hresolv.cc:342: outstanding_requests_.find(request);
> On 2009/11/24 20:00:48, wtc wrote:
> > Nit: indent a continuation line by 4.
> 
> I've got to spend time configuring VS2008/emacs to auto-indent correctly.
> 
> Done.
> 
> http://codereview.chromium.org/428004/diff/6007/6009#newcode401
> net/tools/hresolv/hresolv.cc:401: const char* kVerbose = "verbose";
> On 2009/11/24 20:00:48, wtc wrote:
> > Our convention is to use hyphen '-' (instead of underscore '_')
> > to break the words in a command line option.  See examples
> > in base/base_switches.cc and chrome/common/chrome_switches.cc.
> 
> Done.
> 
> http://codereview.chromium.org/428004/diff/6007/6009#newcode479
> net/tools/hresolv/hresolv.cc:479: case 1:
> On 2009/11/24 20:00:48, wtc wrote:
> > cases with curly braces should be formatted like this:
> >   case 1: {
> >     ...
> >     break;
> >   }
> > 
> > See the style guide:
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Loops_and_Swit...
> 
> Done.
> 
> http://codereview.chromium.org/428004/diff/6007/6009#newcode548
> net/tools/hresolv/hresolv.cc:548: CommandLine::Reset();
> On 2009/11/24 20:00:48, wtc wrote:
> > Add
> >   return 0;
> > at the end of the main() function.
> 
> D'oh. Done.

[wtc, 2009-11-24 23:24:41]

LGTM.  Remember to update the Description of your CL
to reflect the new command-line options (which now
contain '-').

http://codereview.chromium.org/428004/diff/14001/12003
File net/tools/hresolv/hresolv.cc (right):

http://codereview.chromium.org/428004/diff/14001/12003#newcode319
net/tools/hresolv/hresolv.cc:319: PlatformThread::Sleep(100);
We should use a condition variable or some other
synchronization object instead of this Sleep loop.

[cbentzelold, 2009-11-29 04:42:22]

I added a ConditionVariable for synchronization rather than using a sleep loop. 

I'll change the CL description. Also - this will need to be patched and
submitted by a committer.

On 2009/11/24 23:24:41, wtc wrote:
> LGTM.  Remember to update the Description of your CL
> to reflect the new command-line options (which now
> contain '-').
> 
> http://codereview.chromium.org/428004/diff/14001/12003
> File net/tools/hresolv/hresolv.cc (right):
> 
> http://codereview.chromium.org/428004/diff/14001/12003#newcode319
> net/tools/hresolv/hresolv.cc:319: PlatformThread::Sleep(100);
> We should use a condition variable or some other
> synchronization object instead of this Sleep loop.

[eroman, 2009-11-30 19:57:18]

I still think this would be a bunch simpler without using threads/locks.

The way it could work is

(1) Post delayed tasks to the message loop to schedule the start of the
resolves.

(2) Each time a resolve completes, decrement the outstanding requests counter
(no synchronization needed, since it will all be on the current message loop).

(3) Once outstanding requests reaches 0, we are done.


So basically in the main function, after posting the delayed tasks, you would
call:

MessageLoop::current()->Run() to enter the message loop, which will infinite
spin.

And then on completion of all the requests, you would call

MesssageLoop::current()->Quit() to break out of the Run().

[wtc, 2009-11-30 20:04:32]

Chris,

I can commit your CL for you when it's ready.

I have some comments below for Patch Set 8.  However, while
reviewing your use of threads in more detail and discussing
it with eroman, I found that you don't need to create
resolve_thread_ for the async case.  You can just run a
message loop on the main thread to receive the completion
callbacks of HostResolver::Resolve.  Then you don't need
a lock or a condition variable because the manipulation of
outstanding_requests_ occurs on the main thread only.  So
my comments below (except for the comment on msvs_guid) are
moot ...

http://codereview.chromium.org/428004/diff/19001/19002
File net/net.gyp (right):

http://codereview.chromium.org/428004/diff/19001/19002#newcode852
net/net.gyp:852: 'msvs_guid': 'FF1BAC48-D326-4CB4-96DA-8B03DE23ED6E',
maruel said we don't need to add a msvs_guid.  Why did
you add one, and how did you generate it?

http://codereview.chromium.org/428004/diff/19001/19003
File net/tools/hresolv/hresolv.cc (right):

http://codereview.chromium.org/428004/diff/19001/19003#newcode322
net/tools/hresolv/hresolv.cc:322: while (remaining_hosts_ > 0) {
There are two bugs.

1. There is no Release() call to match the Acquire() call.

2. You need to test remaining_hosts_ > 0 while holding
lock_outstanding_requests_.

Something like this should work:

  {
    AutoLock lock(lock_outstanding_requests_);
    while (remaining_hosts_ > 0)
      cv_hosts_.Wait();
  }

You may want to consider using a condition variable only
when 'async' is true.  Then you don't need to change the
ResolveSync() method, and you don't need the
remaining_hosts_ member variable (you can just test
outstanding_requests_.size() != 0 in the while loop).

http://codereview.chromium.org/428004/diff/19001/19003#newcode381
net/tools/hresolv/hresolv.cc:381: void DecRemainingHosts() {
Please document that this must be called while
holding lock_outstanding_requests_.

http://codereview.chromium.org/428004/diff/19001/19003#newcode423
net/tools/hresolv/hresolv.cc:423: Lock lock_outstanding_requests_;
Please document what member variables
lock_outstanding_requests_ protects.  Before it was obvious
by naming convention (lock_outstanding_requests_ protects
outstanding_requests_).  Now that the lock also protects
remaining_hosts_, we need the help of a comment.  Please
also document what cv_hosts_ is used for.

[cbentzelold, 2009-11-30 20:10:39]

You can hold off on the patch as I'm going to incorporate eroman's
suggestion. I don't see any obvious problems with it, and it will greatly
simplify the code.

On Mon, Nov 30, 2009 at 3:04 PM, <wtc@chromium.org> wrote:

> Chris,
>
> I can commit your CL for you when it's ready.
>
> I have some comments below for Patch Set 8.  However, while
> reviewing your use of threads in more detail and discussing
> it with eroman, I found that you don't need to create
> resolve_thread_ for the async case.  You can just run a
> message loop on the main thread to receive the completion
> callbacks of HostResolver::Resolve.  Then you don't need
> a lock or a condition variable because the manipulation of
> outstanding_requests_ occurs on the main thread only.  So
> my comments below (except for the comment on msvs_guid) are
> moot ...
>
>
> http://codereview.chromium.org/428004/diff/19001/19002
>
> File net/net.gyp (right):
>
> http://codereview.chromium.org/428004/diff/19001/19002#newcode852
> net/net.gyp:852: 'msvs_guid': 'FF1BAC48-D326-4CB4-96DA-8B03DE23ED6E',
> maruel said we don't need to add a msvs_guid.  Why did
> you add one, and how did you generate it?
>
> http://codereview.chromium.org/428004/diff/19001/19003
>
> File net/tools/hresolv/hresolv.cc (right):
>
> http://codereview.chromium.org/428004/diff/19001/19003#newcode322
> net/tools/hresolv/hresolv.cc:322: while (remaining_hosts_ > 0) {
> There are two bugs.
>
> 1. There is no Release() call to match the Acquire() call.
>
> 2. You need to test remaining_hosts_ > 0 while holding
> lock_outstanding_requests_.
>
> Something like this should work:
>
>  {
>    AutoLock lock(lock_outstanding_requests_);
>    while (remaining_hosts_ > 0)
>      cv_hosts_.Wait();
>  }
>
> You may want to consider using a condition variable only
> when 'async' is true.  Then you don't need to change the
> ResolveSync() method, and you don't need the
> remaining_hosts_ member variable (you can just test
> outstanding_requests_.size() != 0 in the while loop).
>
> http://codereview.chromium.org/428004/diff/19001/19003#newcode381
> net/tools/hresolv/hresolv.cc:381: void DecRemainingHosts() {
> Please document that this must be called while
> holding lock_outstanding_requests_.
>
> http://codereview.chromium.org/428004/diff/19001/19003#newcode423
> net/tools/hresolv/hresolv.cc:423: Lock lock_outstanding_requests_;
> Please document what member variables
> lock_outstanding_requests_ protects.  Before it was obvious
> by naming convention (lock_outstanding_requests_ protects
> outstanding_requests_).  Now that the lock also protects
> remaining_hosts_, we need the help of a comment.  Please
> also document what cv_hosts_ is used for.
>
>
> http://codereview.chromium.org/428004
>

[cbentzelold, 2009-11-30 20:56:27]

http://codereview.chromium.org/428004/diff/6007/6008
File net/net.gyp (right):

http://codereview.chromium.org/428004/diff/6007/6008#newcode852
net/net.gyp:852: 'target_name': 'hresolv',
On 2009/11/24 21:46:10, cbentzel wrote:
> On 2009/11/24 20:05:52, cbentzel wrote:
> > On 2009/11/24 20:00:48, wtc wrote:
> > > It seems that you need an 'msvs_guid' entry for
> > > hresolv.  I don't know how it's generated though.
> > > 
> > > Is 'hresolv' a Unix command?
> > 
> > Probably - I did build this on Windows but I'll discover how.
> > 
> > I made up the name hresolv - and it does follow the unix convention of
> > short-but-obscure command names. I'm fine with changing it to host_resolve
or
> > something more obvious.
> 
> It turns out that msvs_guid is being deprecated and new targets should not
> include it.
> 
> Also - the email thread for this code review is now the top hit on Google for
> hresolv, so I don't think it's a name of a tool (or at least a commonly used
> one).
> 

Opinion changed regarding msvs_guid so I introduced a new one. FYI - it can be
done in VS by using Tools > Create GUID.

http://codereview.chromium.org/428004/diff/19001/19002#newcode852
net/net.gyp:852: 'target_name': 'hresolv',
On 2009/11/30 20:04:32, wtc wrote:
> maruel said we don't need to add a msvs_guid.  Why did
> you add one, and how did you generate it?

Actually, there were follow-ups on that thread which states that msvs_guid's
must be added for the time being. Unfortunately, the group's archive of this
thread does not seem to include these additional messages. I did validate that
chromium-dev@googlegroups.com was either in the to or cc list on the additional
messages.

To create one, I simply did Tools > Create GUID from within VS 2008 and
copy-and-pasted the value into the GYP.

I created with

[cbentzelold, 2009-12-01 20:22:46]

I changed this to use PostDelayedTask and a MessageLoop::Run as Eric suggested,
and the code simplified somewhat.

One aspect which is a bit trickier than I would like is the need for an
AsyncContext object. The HostResolver::Resolve will invoke a
net::CompletionCallback when the request is finished processing on an async
request. I couldn't find an easy way to have the callbacks take options bound at
creation time and some options bound at invocation time. What I'd really like is
something like

class Foo {
 public:
   void OnDone(int err, int foo, char* bar);
};

Foo* foo = new Foo();
net::CompletionCallback* callback = NewCallback(foo, &Foo::OnDone,
                                                27, "arg");

And the callback would be invoked like

callback->Run(net::OK) 

with the 27 and "arg" values passed in since they are stored with the callback
object. If there is a way to do this, it would simplify the code a bit.

[eroman, 2009-12-04 10:38:21]

first off, apologies for the delay in reviewing this -- i briefly went on
vacation and didnt check emails.

> One aspect which is a bit trickier than I would like is the need for an
> AsyncContext object. 

Yeah, that tends to be a bit of a nuisance with completion callbacks.

You can simplify things further if you merge the AsyncContext and ResolveTask
classes into a single "AsyncRequest" class.
This would package all of the request's state, and would own the callback,
address list, etc.. (avoiding needing to manually mange their lifetimes).

Here is about how that would look (just free-writing this in browser, don't
expect it to actually work :)


class AsyncRequest : public base::RefCounted<AsyncRequest> {
 public:
   AsyncRequest(const std::string& host, HostResolver* resolver,
ResolverInvoker* invoker)
    : host_(host),
      resolver_(resolver),
      invoker_(invoker), 
      ALLOW_THIS_IN_INITIALIZER_LIST(io_callback_(this,
&AsyncRequest::OnResolveComplete)) {
  }

  void Start() {
    // Balanced in OnResolveComplete().
    this->AddRef();

    int rv = resolver_->Resolve(..., &io_completion_, ...);
    if (rv != ERR_IO_PENDING) {
      // Completed synchronously.
      OnResolveComplete(rv);
    } 
  }

 private:
  void OnResolveComplete(int result) {
    invoker_->OnResolved(result, address_list_, host_);
    this->Release();  // Might delete |this|.
  }

  std::string host_;
  scoped_refptr<HostResolver> resolver_;
  ResolverInvoker* invoker_;  // Must remain valid throughout our lifetime.

  AddressList address_list_;
  CompletionCallbackImpl<AsyncRequest> io_callback_;
};



Now in place of where you posted a ResolveTask, you would instead post a
runnable method to AsyncRequest:



  scoped_refptr<AsyncRequest> async_req = new AsyncRequest(host, resolover_,
this);
  PostDelayedTask(FROM_HERE,
      NewRunnableMethod(async_req, &AsyncRequest::Start), delay);

http://codereview.chromium.org/428004/diff/20006/20008
File net/tools/hresolv/hresolv.cc (right):

http://codereview.chromium.org/428004/diff/20006/20008#newcode17
net/tools/hresolv/hresolv.cc:17: // Future ideas: Specifying whether the lookup
is speculative
Another future idea could be to let you specify the address family.

[cbentzelold, 2009-12-08 20:35:11]

On 2009/12/04 10:38:21, eroman wrote:
> first off, apologies for the delay in reviewing this -- i briefly went on
> vacation and didnt check emails.

No problem - as you can see I'm not that hasty with this reply either.

> 
> class AsyncRequest : public base::RefCounted<AsyncRequest> {
>  public:
>    AsyncRequest(const std::string& host, HostResolver* resolver,
> ResolverInvoker* invoker)
>     : host_(host),
>       resolver_(resolver),
>       invoker_(invoker), 
>       ALLOW_THIS_IN_INITIALIZER_LIST(io_callback_(this,
> &AsyncRequest::OnResolveComplete)) {
>   }
> 
>   void Start() {
>     // Balanced in OnResolveComplete().
>     this->AddRef();
> 
>     int rv = resolver_->Resolve(..., &io_completion_, ...);
>     if (rv != ERR_IO_PENDING) {
>       // Completed synchronously.
>       OnResolveComplete(rv);
>     } 
>   }
> 
>  private:
>   void OnResolveComplete(int result) {
>     invoker_->OnResolved(result, address_list_, host_);
>     this->Release();  // Might delete |this|.
>   }
> 
>   std::string host_;
>   scoped_refptr<HostResolver> resolver_;
>   ResolverInvoker* invoker_;  // Must remain valid throughout our lifetime.
> 
>   AddressList address_list_;
>   CompletionCallbackImpl<AsyncRequest> io_callback_;
> };
> 

This worked almost as-is, and greatly simplified the code again. Love it. 

(The only change was that I needed to AddRef before a NewRunnableObject was
created since NewRunnableObject does an AddRef/Release inside of
RunnableMethodTraits::RetainCallee in non-NDEBUG mode).

[eroman, 2009-12-09 23:00:31]

LGTM

http://codereview.chromium.org/428004/diff/27001/28002
File net/tools/hresolv/hresolv.cc (right):

http://codereview.chromium.org/428004/diff/27001/28002#newcode98
net/tools/hresolv/hresolv.cc:98: {AF_UNSPEC, "AF_UNSPEC"},
style-nit: I'm not sure on what the indentation should be here (2 spaces or 4
spaces). I don't much care either way (as long as you are consistent throughout,
which you are), but was curious if you knew offhand which way "correct"  :)

http://codereview.chromium.org/428004/diff/27001/28002#newcode170
net/tools/hresolv/hresolv.cc:170: for (const struct addrinfo* it =
address_list.head();
style-nit: I suggest using a more descriptive name than |it|. Perhaps
|cur_address|

http://codereview.chromium.org/428004/diff/27001/28002#newcode171
net/tools/hresolv/hresolv.cc:171: it != NULL;
minor-style-nit: Omit the "!= NULL"

http://codereview.chromium.org/428004/diff/27001/28002#newcode188
net/tools/hresolv/hresolv.cc:188: address_list_(),
nit: omit default constructor calls.

http://codereview.chromium.org/428004/diff/27001/28002#newcode210
net/tools/hresolv/hresolv.cc:210: 
nit: please delete this blank line.

http://codereview.chromium.org/428004/diff/27001/28002#newcode269
net/tools/hresolv/hresolv.cc:269: resolve_request->AddRef();
nit: I recommend documenting that it is balanced by ::OnResolveComplete().

http://codereview.chromium.org/428004/diff/27001/28002#newcode300
net/tools/hresolv/hresolv.cc:300: void DelayedResolve::OnResolveComplete(int
result) {
nit: this definition could be arranged higher up in the file (i.e. nearer the
declaration of DelayedResolve, or inlined inside of DelayedResolve.

http://codereview.chromium.org/428004/diff/27001/28002#newcode450
net/tools/hresolv/hresolv.cc:450: new net::HostResolverImpl(NULL,
options.cache_size, options.cache_ttl));
Depending who checks in first, someone will have some merging to do :)

I have a soon to come CL that changes the constructor:
http://codereview.chromium.org/464084/show

[eroman, 2009-12-10 02:39:14]

Committed as r34225

[cbentzelold, 2012-10-18 15:36:45]

http://codereview.chromium.org/428004/diff/20006/net/tools/hresolv/hresolv.cc
File net/tools/hresolv/hresolv.cc (right):

http://codereview.chromium.org/428004/diff/20006/net/tools/hresolv/hresolv.cc...
net/tools/hresolv/hresolv.cc:17: // Future ideas: Specifying whether the lookup
is speculative
On 2009/12/04 10:38:21, eroman wrote:
> Another future idea could be to let you specify the address family.

Done.