OpenBSD patches for net, split from CR #8275005

OpenBSD patches for net, split from CR #8275005

net/base/host_resolver_proc.cc:
  - define AI_ADDRCONFIG as 0

net/base/listen_socket.cc:
  - Shuffle and add missing headers since the order
    of including these matters.

net/base/net_util.h:
  - Missing header for all POSIX platforms. Needed on
    OpenBSD and it won't hurt others.

net/dns/dns_config_service_posix.h:
  - Missing header for OpenBSD. The order matters here too

net/dns/dns_config_service_posix.cc:
  - Use res_init() on OpenBSD and disable RES_ROTATE
    if it is not defined

net/net.gyp:
  - Do not use libresolv on OpenBSD since it's not there,
    the resolver is in libc.
  - Include base/platform_mime_util_linux.cc in the OpenBSD builds.

BUG=
TEST=


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

[wtc, 2011-10-18 21:50:25]

robert.nagy: thank you for the CL.  Please read my review
comments on Patch Set 2.

cbentzel,mmenke,szym: just FYI, since most of the files in
this CL are related to DNS.  Most of the changes in this file
have to do with the fact that traditionally Unix socket headers
are not self-contained and must be included in a specific
order:

#include <sys/types.h>
#include <sys/socket.h>
#include <netinet/in.h>

In current versions of Unix, <sys/socket.h> no longer
requires <sys/types.h>, for example,
http://pubs.opengroup.org/onlinepubs/7908799/xns/recvfrom.html

I don't know why OpenBSD still requires that.

http://codereview.chromium.org/8336024/diff/9/crypto/encryptor.cc
File crypto/encryptor.cc (right):

http://codereview.chromium.org/8336024/diff/9/crypto/encryptor.cc#newcode24
crypto/encryptor.cc:24: #define bswap_64(x) swap64(x)

Just curious: are swap16, swap32, and swap64 compiler built-in
functions?  I'm just wondering why you don't need to include
a header for these functions.

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

http://codereview.chromium.org/8336024/diff/9/net/base/dnsrr_resolver.cc#newc...
net/base/dnsrr_resolver.cc:8: #include <netinet/in.h>

Just add this to line 12 below.

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

http://codereview.chromium.org/8336024/diff/9/net/base/host_resolver_proc.cc#...
net/base/host_resolver_proc.cc:17: #define AI_ADDRCONFIG 0

With this AI_ADDRCONFIG macro definition, you should remove
    || defined(OS_OPENBSD)
from line 151.

I suggest you move
  #include <sys/types.h>
to line 21 below.

Why does OpenBSD need <sys/types.h> in this file, but
a different header <netinet/in.h> in net/base/dnsrr_resolver.cc?

http://codereview.chromium.org/8336024/diff/9/net/dns/dns_config_service_posi...
File net/dns/dns_config_service_posix.cc (right):

http://codereview.chromium.org/8336024/diff/9/net/dns/dns_config_service_posi...
net/dns/dns_config_service_posix.cc:34: extern struct __res_state _res;

Is the _res global variable not declared in any system header?

http://codereview.chromium.org/8336024/diff/9/net/dns/dns_config_service_posi...
net/dns/dns_config_service_posix.cc:134: #endif

If RES_ROTATE is defined as a macro on all platforms that have
it, this would be a better way to handle it:

#if defined(RES_ROTATE)
  dns_config->rotate = res.options & RES_ROTATE;
#endif

http://codereview.chromium.org/8336024/diff/9/net/dns/dns_config_service_posix.h
File net/dns/dns_config_service_posix.h (right):

http://codereview.chromium.org/8336024/diff/9/net/dns/dns_config_service_posi...
net/dns/dns_config_service_posix.h:15: #endif

Note: it may be better to use a forward declaration of
struct __res_state in this header, and move the system
header inclusions to .cc files.

I recommend removing the #if defined(OS_OPENBSD).

Is <sys/socket.h> necessary?  From what I found in old Unix
man pages, these headers should be included:

#include <sys/types.h>
#include <netinet/in.h>
#include <arpa/nameser.h>
#include <resolv.h>

http://download.oracle.com/docs/cd/E19109-01/tsolaris8/835-8004/6ruu29hft/ind...
http://publib.boulder.ibm.com/infocenter/pseries/v5r3/index.jsp?topic=/com.ib...
http://nixdoc.net/man-pages/Tru64/res_mkquery.3.html

http://codereview.chromium.org/8336024/diff/9/net/net.gyp
File net/net.gyp (right):

http://codereview.chromium.org/8336024/diff/9/net/net.gyp#newcode919
net/net.gyp:919: ],

Nit: it may be better to move the OS=="openbsd" code from
lines 732-736 to here, next to the other OS == "foo" blocks.

[Robert Nagy, 2011-10-19 07:10:02]

http://codereview.chromium.org/8336024/diff/9/crypto/encryptor.cc
File crypto/encryptor.cc (right):

http://codereview.chromium.org/8336024/diff/9/crypto/encryptor.cc#newcode24
crypto/encryptor.cc:24: #define bswap_64(x) swap64(x)
On 2011/10/18 21:50:26, wtc wrote:
> 
> Just curious: are swap16, swap32, and swap64 compiler built-in
> functions?  I'm just wondering why you don't need to include
> a header for these functions.

They are defined in sys/endian.h so I just included that to be on the safe side
of things.

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

http://codereview.chromium.org/8336024/diff/9/net/base/dnsrr_resolver.cc#newc...
net/base/dnsrr_resolver.cc:8: #include <netinet/in.h>
On 2011/10/18 21:50:26, wtc wrote:
> 
> Just add this to line 12 below.

Done.

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

http://codereview.chromium.org/8336024/diff/9/net/base/host_resolver_proc.cc#...
net/base/host_resolver_proc.cc:17: #define AI_ADDRCONFIG 0
On 2011/10/18 21:50:26, wtc wrote:
> 
> With this AI_ADDRCONFIG macro definition, you should remove
>     || defined(OS_OPENBSD)
> from line 151.
> 
> I suggest you move
>   #include <sys/types.h>
> to line 21 below.
> 
> Why does OpenBSD need <sys/types.h> in this file, but
> a different header <netinet/in.h> in net/base/dnsrr_resolver.cc?

Actually neither sys/types.h nor resolv.h is needed in this file, so i just
removed them. But in any case, we should try this on the trybots.

http://codereview.chromium.org/8336024/diff/9/net/dns/dns_config_service_posi...
File net/dns/dns_config_service_posix.cc (right):

http://codereview.chromium.org/8336024/diff/9/net/dns/dns_config_service_posi...
net/dns/dns_config_service_posix.cc:34: extern struct __res_state _res;
On 2011/10/18 21:50:26, wtc wrote:
> 
> Is the _res global variable not declared in any system header?
It is in resolv.h so I removed the declaration from here.

http://codereview.chromium.org/8336024/diff/9/net/dns/dns_config_service_posi...
net/dns/dns_config_service_posix.cc:134: #endif
On 2011/10/18 21:50:26, wtc wrote:
> 
> If RES_ROTATE is defined as a macro on all platforms that have
> it, this would be a better way to handle it:
> 
> #if defined(RES_ROTATE)
>   dns_config->rotate = res.options & RES_ROTATE;
> #endif

Done.

http://codereview.chromium.org/8336024/diff/9/net/dns/dns_config_service_posix.h
File net/dns/dns_config_service_posix.h (right):

http://codereview.chromium.org/8336024/diff/9/net/dns/dns_config_service_posi...
net/dns/dns_config_service_posix.h:15: #endif
On 2011/10/18 21:50:26, wtc wrote:
> 
> Note: it may be better to use a forward declaration of
> struct __res_state in this header, and move the system
> header inclusions to .cc files.
> 
> I recommend removing the #if defined(OS_OPENBSD).
> 
> Is <sys/socket.h> necessary?  From what I found in old Unix
> man pages, these headers should be included:
> 
> #include <sys/types.h>
> #include <netinet/in.h>
> #include <arpa/nameser.h>
> #include <resolv.h>
> 
>
http://download.oracle.com/docs/cd/E19109-01/tsolaris8/835-8004/6ruu29hft/ind...
>
http://publib.boulder.ibm.com/infocenter/pseries/v5r3/index.jsp?topic=/com.ib...
> http://nixdoc.net/man-pages/Tru64/res_mkquery.3.html

Yes it makes sense to include these headers everywhere. Which i've done in the
new patchset, but I kept them in the header file.

[cbentzel, 2011-10-19 19:35:18]

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

http://codereview.chromium.org/8336024/diff/7001/net/dns/dns_config_service_p...
net/dns/dns_config_service_posix.cc:34: if ((res_init() == 0) && (_res.options &
RES_INIT)) {
We intentionally called res_ninit rather than res_init here to prevent mutation
of the global _res structure. This was primarily for unit tests, which may not
be as big of a concern for the OPENBSD configuration.

http://codereview.chromium.org/8336024/diff/7001/net/dns/dns_config_service_p...
File net/dns/dns_config_service_posix.h (right):

http://codereview.chromium.org/8336024/diff/7001/net/dns/dns_config_service_p...
net/dns/dns_config_service_posix.h:11: #include <sys/types.h>
Should this includes be nested within OPENBSD conditionals?

[Robert Nagy, 2011-10-19 19:49:34]

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

http://codereview.chromium.org/8336024/diff/7001/net/dns/dns_config_service_p...
net/dns/dns_config_service_posix.cc:34: if ((res_init() == 0) && (_res.options &
RES_INIT)) {
On 2011/10/19 19:35:18, cbentzel wrote:
> We intentionally called res_ninit rather than res_init here to prevent
mutation
> of the global _res structure. This was primarily for unit tests, which may not
> be as big of a concern for the OPENBSD configuration.
Which I want to use on OpenBSD too. Unittests are important.

http://codereview.chromium.org/8336024/diff/7001/net/dns/dns_config_service_p...
File net/dns/dns_config_service_posix.h (right):

http://codereview.chromium.org/8336024/diff/7001/net/dns/dns_config_service_p...
net/dns/dns_config_service_posix.h:11: #include <sys/types.h>
On 2011/10/19 19:35:18, cbentzel wrote:
> Should this includes be nested within OPENBSD conditionals?

I think it is totally fine on every posix system, and the less ifdefs we have,
the better.

[wtc, 2011-10-19 22:34:27]

Review comments on Patch Set 3.

cbentzel: I replied to your question below.

http://codereview.chromium.org/8336024/diff/7001/net/dns/dns_config_service_p...
File net/dns/dns_config_service_posix.h (right):

http://codereview.chromium.org/8336024/diff/7001/net/dns/dns_config_service_p...
net/dns/dns_config_service_posix.h:9: #include "build/build_config.h"
Please remove this.  I believe this is not necessary now.

http://codereview.chromium.org/8336024/diff/7001/net/dns/dns_config_service_p...
net/dns/dns_config_service_posix.h:11: #include <sys/types.h>
On 2011/10/19 19:35:18, cbentzel wrote:
> Should this includes be nested within OPENBSD conditionals?

<sys/types.h> used to be required on Unix in the early 1990s.
It is no longer required on most Unix platforms today, but
still required by OpenBSD.  It is harmless to include
<sys/types.h>.

http://codereview.chromium.org/8336024/diff/7001/net/dns/dns_config_service_p...
net/dns/dns_config_service_posix.h:13: #include <arpa/nameser.h>
Don't add <arpa/nameser.h> unless it's required by OpenBSD.

Note: in this file, we should include only the headers
required for the definition of struct __res.  All the other
system headers should be moved to dns_config_service_posix.cc.
So you need to read the compiler errors of
dns_config_service_posix.cc carefully and figure out which
system headers should be included by
dns_config_service_posix.h and which should be included by
dns_config_service_posix.cc.

[Robert Nagy, 2011-10-20 07:37:43]

http://codereview.chromium.org/8336024/diff/7001/net/dns/dns_config_service_p...
File net/dns/dns_config_service_posix.h (right):

http://codereview.chromium.org/8336024/diff/7001/net/dns/dns_config_service_p...
net/dns/dns_config_service_posix.h:9: #include "build/build_config.h"
On 2011/10/19 22:34:27, wtc wrote:
> Please remove this.  I believe this is not necessary now.

Done.

http://codereview.chromium.org/8336024/diff/7001/net/dns/dns_config_service_p...
net/dns/dns_config_service_posix.h:13: #include <arpa/nameser.h>
On 2011/10/19 22:34:27, wtc wrote:
> Don't add <arpa/nameser.h> unless it's required by OpenBSD.
> 
> Note: in this file, we should include only the headers
> required for the definition of struct __res.  All the other
> system headers should be moved to dns_config_service_posix.cc.
> So you need to read the compiler errors of
> dns_config_service_posix.cc carefully and figure out which
> system headers should be included by
> dns_config_service_posix.h and which should be included by
> dns_config_service_posix.cc.

I've removed nameser.h, but the others has to stay. __res is in resolv.h that
needs netinet/in.h and that needs sys/types.h.

[cbentzel, 2011-10-20 13:23:22]

dns changes look good. At some point we may end up just parsing /etc/resolv.conf
directly rather than relying on res_ninit to deal with it, so I'm not as
concerned about the use of res_init for OpenBSD.

[wtc, 2011-10-20 14:01:46]

Review comments on Patch Set 4:

Very close now.  Two mandatory changes, and two optional
suggested changes.

Mandatory changes:

1. Please update the CL's commit message to reflect the
current changes to the following files (I included my
proposed wording):
   net/base/host_resolver_proc.cc:
   - define AI_ADDRCONFIG as 0
   net/dns/dns_config_service_posix.cc:
   - Use res_init() on OpenBSD and disable RES_ROTATE 

2. Remove #include "build/build_config.h" from
net/dns/dns_config_service_posix.h (also noted below).

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

http://codereview.chromium.org/8336024/diff/10004/net/base/dnsrr_resolver.cc#...
net/base/dnsrr_resolver.cc:194: if (res_ninit(&_res) != 0)

Hmm... If res_ninit(&_res) is equivalent to res_init(),
perhaps we should just use res_init() for OS_MACOSX as well.

http://codereview.chromium.org/8336024/diff/10004/net/dns/dns_config_service_...
File net/dns/dns_config_service_posix.cc (right):

http://codereview.chromium.org/8336024/diff/10004/net/dns/dns_config_service_...
net/dns/dns_config_service_posix.cc:49: #endif

It may be better to apply #if defined(OS_OPENBSD) to the entire
function, like this:

  void DoWork() OVERRIDE {
#if defined(OS_OPENBSD)
    if ((res_init() == 0) && (_res.options & RES_INIT)) {
      success_ = ConvertResToConfig(_res, &dns_config_);
    } else {
      XXX no comment here.  I assume OpenBSD doesn't use
      XXX glibc.
      success_ = false;
    }
#else  // defined(OS_OPENBSD)
    ... the original code ...
#endif  // defined(OS_OPENBSD)
  }

http://codereview.chromium.org/8336024/diff/10004/net/dns/dns_config_service_...
File net/dns/dns_config_service_posix.h (right):

http://codereview.chromium.org/8336024/diff/10004/net/dns/dns_config_service_...
net/dns/dns_config_service_posix.h:9: #include "build/build_config.h"

Please remove this line.

[Robert Nagy, 2011-10-20 17:40:58]

On 2011/10/20 14:01:46, wtc wrote:
> Review comments on Patch Set 4:
> 
> Very close now.  Two mandatory changes, and two optional
> suggested changes.
> 
> Mandatory changes:
> 
> 1. Please update the CL's commit message to reflect the
> current changes to the following files (I included my
> proposed wording):
>    net/base/host_resolver_proc.cc:
>    - define AI_ADDRCONFIG as 0
>    net/dns/dns_config_service_posix.cc:
>    - Use res_init() on OpenBSD and disable RES_ROTATE 
> 
> 2. Remove #include "build/build_config.h" from
> net/dns/dns_config_service_posix.h (also noted below).
> 
> http://codereview.chromium.org/8336024/diff/10004/net/base/dnsrr_resolver.cc
> File net/base/dnsrr_resolver.cc (right):
> 
>
http://codereview.chromium.org/8336024/diff/10004/net/base/dnsrr_resolver.cc#...
> net/base/dnsrr_resolver.cc:194: if (res_ninit(&_res) != 0)
> 
> Hmm... If res_ninit(&_res) is equivalent to res_init(),
> perhaps we should just use res_init() for OS_MACOSX as well.
> 
>
http://codereview.chromium.org/8336024/diff/10004/net/dns/dns_config_service_...
> File net/dns/dns_config_service_posix.cc (right):
> 
>
http://codereview.chromium.org/8336024/diff/10004/net/dns/dns_config_service_...
> net/dns/dns_config_service_posix.cc:49: #endif
> 
> It may be better to apply #if defined(OS_OPENBSD) to the entire
> function, like this:
> 
>   void DoWork() OVERRIDE {
> #if defined(OS_OPENBSD)
>     if ((res_init() == 0) && (_res.options & RES_INIT)) {
>       success_ = ConvertResToConfig(_res, &dns_config_);
>     } else {
>       XXX no comment here.  I assume OpenBSD doesn't use
>       XXX glibc.
>       success_ = false;
>     }
> #else  // defined(OS_OPENBSD)
>     ... the original code ...
> #endif  // defined(OS_OPENBSD)
>   }
> 
>
http://codereview.chromium.org/8336024/diff/10004/net/dns/dns_config_service_...
> File net/dns/dns_config_service_posix.h (right):
> 
>
http://codereview.chromium.org/8336024/diff/10004/net/dns/dns_config_service_...
> net/dns/dns_config_service_posix.h:9: #include "build/build_config.h"
> 
> Please remove this line.

I've replied to your suggestions and did the mandatory changes.

[Robert Nagy, 2011-10-20 17:44:44]

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

http://codereview.chromium.org/8336024/diff/10004/net/base/dnsrr_resolver.cc#...
net/base/dnsrr_resolver.cc:194: if (res_ninit(&_res) != 0)
On 2011/10/20 14:01:46, wtc wrote:
> 
> Hmm... If res_ninit(&_res) is equivalent to res_init(),
> perhaps we should just use res_init() for OS_MACOSX as well.
THere might be differences so I would rather stay on the safe side at this
point.

http://codereview.chromium.org/8336024/diff/10004/net/dns/dns_config_service_...
File net/dns/dns_config_service_posix.cc (right):

http://codereview.chromium.org/8336024/diff/10004/net/dns/dns_config_service_...
net/dns/dns_config_service_posix.cc:49: #endif
On 2011/10/20 14:01:46, wtc wrote:
> 
> It may be better to apply #if defined(OS_OPENBSD) to the entire
> function, like this:
> 
>   void DoWork() OVERRIDE {
> #if defined(OS_OPENBSD)
>     if ((res_init() == 0) && (_res.options & RES_INIT)) {
>       success_ = ConvertResToConfig(_res, &dns_config_);
>     } else {
>       XXX no comment here.  I assume OpenBSD doesn't use
>       XXX glibc.
>       success_ = false;
>     }
> #else  // defined(OS_OPENBSD)
>     ... the original code ...
> #endif  // defined(OS_OPENBSD)
>   }

It is fine like this, but I extended the comment in the else { } block, to
reflect reality.

http://codereview.chromium.org/8336024/diff/10004/net/dns/dns_config_service_...
File net/dns/dns_config_service_posix.h (right):

http://codereview.chromium.org/8336024/diff/10004/net/dns/dns_config_service_...
net/dns/dns_config_service_posix.h:9: #include "build/build_config.h"
On 2011/10/20 14:01:46, wtc wrote:
> 
> Please remove this line.

Done.

[wtc, 2011-10-20 19:33:35]

http://codereview.chromium.org/8336024/diff/15001/net/dns/dns_config_service_...
File net/dns/dns_config_service_posix.cc (right):

http://codereview.chromium.org/8336024/diff/15001/net/dns/dns_config_service_...
net/dns/dns_config_service_posix.cc:43: // this behaviour is the same as of
res_init

Please change the comment as follows:
  // Note: res_ninit in glibc always returns 0 and sets RES_INIT.
  // res_init behaves the same way.

The first line should remain a sentence.  Just add the second line.

[Robert Nagy, 2011-10-20 19:42:54]

http://codereview.chromium.org/8336024/diff/15001/net/dns/dns_config_service_...
File net/dns/dns_config_service_posix.cc (right):

http://codereview.chromium.org/8336024/diff/15001/net/dns/dns_config_service_...
net/dns/dns_config_service_posix.cc:43: // this behaviour is the same as of
res_init
On 2011/10/20 19:33:36, wtc wrote:
> 
> Please change the comment as follows:
>   // Note: res_ninit in glibc always returns 0 and sets RES_INIT.
>   // res_init behaves the same way.
> 
> The first line should remain a sentence.  Just add the second line.

Done.

[wtc, 2011-10-20 19:58:44]

Patch Set 6 LGTM.  Do you need me to submit this to the
commit queue for you?

[Robert Nagy, 2011-10-20 20:18:02]

On 2011/10/20 19:58:44, wtc wrote:
> Patch Set 6 LGTM.  Do you need me to submit this to the
> commit queue for you?
Yes please, since I don't have access to that.

[commit-bot: I haz the power, 2011-10-20 20:46:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/robert.nagy@gmail.com/8336024/15003

[commit-bot: I haz the power, 2011-10-20 22:39:27]

Change committed as 106604