code review 101220044: net: separate DNS transport from DNS query-response int...

net: separate DNS transport from DNS query-response interaction

Before fixing issue 6579 this CL separates DNS transport from
DNS message interaction to make it easier to add builtin DNS
resolver control logic.

Update issue 6579

[mikio, 2014-06-12 15:46:43]

Hello golang-codereviews@googlegroups.com (cc:
golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[axaxs, 2014-06-13 00:18:18]

Also - have you run any mem benchmarks to see how this change affects?

https://codereview.appspot.com/101220044/diff/40001/src/pkg/net/dnsclient_uni...
File src/pkg/net/dnsclient_unix.go (right):

https://codereview.appspot.com/101220044/diff/40001/src/pkg/net/dnsclient_uni...
src/pkg/net/dnsclient_unix.go:51: b := make([]byte, 512) // see RFC 1035
This is 'correct' behavior per RFC, but can one be sure that all resolvers
respect these settings?  We were using 2000B buffer before which would handle it
if resolver sent message over 512 limit.  Just a question for your thoughts.

https://codereview.appspot.com/101220044/diff/40001/src/pkg/net/dnsclient_uni...
src/pkg/net/dnsclient_unix.go:147: for _, network := range []string{"udp",
"tcp"} {
I don't like this behavior.  If a dns query times out, or if ids don't match it
will retry same server on tcp which I don't feel is correct behavior.
For now let's reserve tcp for proven truncates only.  This would involve
removing this loop and adding a bit more code below likely.

[mikio, 2014-06-13 01:51:41]

> Also - have you run any mem benchmarks to see how this change affects?

slightly. the following is the result of tryOneName(cfg, "com.", dnsTypeALL)
benchmark run.

benchmark                 old ns/op     new ns/op     delta      
BenchmarkDNSTransport     130379229     124166302     -4.77%     
benchmark                 old allocs     new allocs     delta      
BenchmarkDNSTransport     256            255            -0.39%     
benchmark                 old bytes     new bytes     delta      
BenchmarkDNSTransport     16587         15672         -5.52%

https://codereview.appspot.com/101220044/diff/40001/src/pkg/net/dnsclient_uni...
File src/pkg/net/dnsclient_unix.go (right):

https://codereview.appspot.com/101220044/diff/40001/src/pkg/net/dnsclient_uni...
src/pkg/net/dnsclient_unix.go:51: b := make([]byte, 512) // see RFC 1035
please let me know if you have a nice solution for preventing "1st fragment
piggyback attack" even if it doesn't much matter on resolver side. also, for
now--fortunately, we have no edns0 support. one of the motivation of this cl is
to explicit that there're several matters to be attended for using udp as a dns
transport.

https://codereview.appspot.com/101220044/diff/40001/src/pkg/net/dnsclient_uni...
src/pkg/net/dnsclient_unix.go:147: for _, network := range []string{"udp",
"tcp"} {
nice catch! thx.

[axaxs, 2014-06-13 04:08:18]

Not sure I follow udp fragmentation attack at a dns client level.  If you have
any literature on it would be interested to read - it seems such attacks are
normally targeted at a recursive resolver. No matter to the code, 512 is fine by
me unless we get complaints of noncompliant resolvers...

Thanks

https://codereview.appspot.com/101220044/diff/60001/src/pkg/net/dnsclient_uni...
File src/pkg/net/dnsclient_unix.go (right):

https://codereview.appspot.com/101220044/diff/60001/src/pkg/net/dnsclient_uni...
src/pkg/net/dnsclient_unix.go:151: if nerr, ok := err.(Error); ok &&
nerr.Timeout() {
Why are we checking for timeouts each err case?  Can we return the error and
move these checks and coercion to DNSError to tryOneName?  Functionally should
be the same, but would save many loc(and rid break statements).
The 'for attempt' loop doesn't belong in this function anyhow, this is part of
what other change I submitted fixes.  So there's no benefit in checking error
type repeatedly when we may do once later.  Thoughts?

[mikio, 2014-06-13 04:35:40]

> Not sure I follow udp fragmentation attack at a dns client level.  If you have
> any literature on it would be interested to read - it seems such attacks are
> normally targeted at a recursive resolver.

but indirectly, resolvers that prefer tcp fallback would make some effect on
servers. i'd like to see what happens.

> No matter to the code, 512 is fine by me unless we get complaints of
noncompliant resolvers...

[...]

> Why are we checking for timeouts each err case?  Can we return the error and
> move these checks and coercion to DNSError to tryOneName?

that's fine. well, i'm still not sure what's the timeout for. it's a minimum
seconds btw retries, right?

[axaxs, 2014-06-13 05:22:11]

On 2014/06/13 04:35:40, mikio wrote:
> > Not sure I follow udp fragmentation attack at a dns client level.  If you
have
> > any literature on it would be interested to read - it seems such attacks are
> > normally targeted at a recursive resolver.
> 
> but indirectly, resolvers that prefer tcp fallback would make some effect on
> servers. i'd like to see what happens.
> 
> > No matter to the code, 512 is fine by me unless we get complaints of
> noncompliant resolvers...
> 
> [...]
> 
> > Why are we checking for timeouts each err case?  Can we return the error and
> > move these checks and coercion to DNSError to tryOneName?
> 
> that's fine. well, i'm still not sure what's the timeout for. it's a minimum
> seconds btw retries, right?


I have no idea why we are collecting it really.  I was going to suggest
scrapping it, then realized it is used for the exported Timeout function that we
can't scrap.  And Temporary().  Maybe we can get rid of the IsTimeout value and
have the timeout function done there?  Not sure really, up to you.  But
constantly checking every error for it doesn't seem to be doing anything useful.

Thanks,
Alex

[axaxs, 2014-06-13 05:24:23]

On 2014/06/13 05:22:11, axaxs wrote:
> On 2014/06/13 04:35:40, mikio wrote:
> > > Not sure I follow udp fragmentation attack at a dns client level.  If you
> have
> > > any literature on it would be interested to read - it seems such attacks
are
> > > normally targeted at a recursive resolver.
> > 
> > but indirectly, resolvers that prefer tcp fallback would make some effect on
> > servers. i'd like to see what happens.
> > 
> > > No matter to the code, 512 is fine by me unless we get complaints of
> > noncompliant resolvers...
> > 
> > [...]
> > 
> > > Why are we checking for timeouts each err case?  Can we return the error
and
> > > move these checks and coercion to DNSError to tryOneName?
> > 
> > that's fine. well, i'm still not sure what's the timeout for. it's a minimum
> > seconds btw retries, right?
> 
> 
> I have no idea why we are collecting it really.  I was going to suggest
> scrapping it, then realized it is used for the exported Timeout function that
we
> can't scrap.  And Temporary().  Maybe we can get rid of the IsTimeout value
and
> have the timeout function done there?  Not sure really, up to you.  But
> constantly checking every error for it doesn't seem to be doing anything
useful.
> 
> Thanks,
> Alex

And replying to myself at this hour, also realize IsTimeout is exported
obviously...so can't scrap that either...

Thanks,
Alex

[mikio, 2014-06-13 05:48:37]

> I have no idea why we are collecting it really.  I was going to suggest
> scrapping it, then realized it is used for the exported Timeout function that
we
> can't scrap.  And Temporary().  Maybe we can get rid of the IsTimeout value
and
> have the timeout function done there?  Not sure really, up to you.  But
> constantly checking every error for it doesn't seem to be doing anything
useful.

we cannot scrap it but have to try to reduce its cost. well, to be clear, what
i'm confusing is; does the timeout value of dnsconfig mean "minimum" seconds
between retries? i mean, should we wait a few seconds (timeout) for sending next
query?

[axaxs, 2014-06-13 15:33:55]

https://codereview.appspot.com/101220044/diff/80001/src/pkg/net/dnsclient_uni...
File src/pkg/net/dnsclient_unix.go (left):

https://codereview.appspot.com/101220044/diff/80001/src/pkg/net/dnsclient_uni...
src/pkg/net/dnsclient_unix.go:83: continue
When this occurs, it's going to retry the entire query over TCP.  This should
reset the timeout value.  A deadline is a hard time in the future.  If it is set
to 5 seconds, then deadline as passed from tryOneName is 5 seconds in future
from when it was calculated in other function (which itself is a little scary
since it ideally should be started RIGHT before the dial...).  Anyhow, if
someone has slow internet that takes 3s to do a round trip, every query will
fail because deadline is not reset.  So the udp query takes 3 seconds, then it
is truncated so it retries over tcp.  This fails after 2 seconds of tcp trying
as we are now past the originally calculated deadline.
So, please pass either the timeout value or else the cfg to exchange function
and calculate deadline at top of function.  Then, here in the truncate section,
recalculate the deadline time and update the 'deadline' variable and set
d.Deadline to the new deadline time.

https://codereview.appspot.com/101220044/diff/80001/src/pkg/net/dnsclient_uni...
File src/pkg/net/dnsclient_unix.go (right):

https://codereview.appspot.com/101220044/diff/80001/src/pkg/net/dnsclient_uni...
src/pkg/net/dnsclient_unix.go:141: func exchange(server, name string, qtype
uint16, deadline time.Time) (*dnsMsg, error) {
Do not pass deadline here as an argument.  Pass the timeout or else cfg as we
did initially.  See truncate check comments.

[axaxs, 2014-06-20 00:49:31]

Hi Mikio,

Any update?  This change is shaping up nicely, would like to see it land
soon and we can start on performance.

Thanks
Alex


On Fri, Jun 13, 2014 at 11:33 AM, <alex@lx.lc> wrote:

>
> https://codereview.appspot.com/101220044/diff/80001/src/
> pkg/net/dnsclient_unix.go
> File src/pkg/net/dnsclient_unix.go (left):
>
> https://codereview.appspot.com/101220044/diff/80001/src/
> pkg/net/dnsclient_unix.go#oldcode83
> src/pkg/net/dnsclient_unix.go:83: continue
> When this occurs, it's going to retry the entire query over TCP.  This
> should reset the timeout value.  A deadline is a hard time in the
> future.  If it is set to 5 seconds, then deadline as passed from
> tryOneName is 5 seconds in future from when it was calculated in other
> function (which itself is a little scary since it ideally should be
> started RIGHT before the dial...).  Anyhow, if someone has slow internet
> that takes 3s to do a round trip, every query will fail because deadline
> is not reset.  So the udp query takes 3 seconds, then it is truncated so
> it retries over tcp.  This fails after 2 seconds of tcp trying as we are
> now past the originally calculated deadline.
> So, please pass either the timeout value or else the cfg to exchange
> function and calculate deadline at top of function.  Then, here in the
> truncate section, recalculate the deadline time and update the
> 'deadline' variable and set d.Deadline to the new deadline time.
>
> https://codereview.appspot.com/101220044/diff/80001/src/
> pkg/net/dnsclient_unix.go
> File src/pkg/net/dnsclient_unix.go (right):
>
> https://codereview.appspot.com/101220044/diff/80001/src/
> pkg/net/dnsclient_unix.go#newcode141
> src/pkg/net/dnsclient_unix.go:141: func exchange(server, name string,
> qtype uint16, deadline time.Time) (*dnsMsg, error) {
> Do not pass deadline here as an argument.  Pass the timeout or else cfg
> as we did initially.  See truncate check comments.
>
> https://codereview.appspot.com/101220044/
>

[mikio, 2014-06-26 02:37:41]

sorry for being late, ptal.

https://codereview.appspot.com/101220044/diff/80001/src/pkg/net/dnsclient_uni...
File src/pkg/net/dnsclient_unix.go (left):

https://codereview.appspot.com/101220044/diff/80001/src/pkg/net/dnsclient_uni...
src/pkg/net/dnsclient_unix.go:83: continue
i'm still not convinced of the timeout in /etc/resolv.conf. 

On some BSD, timeout: Specifies the total amount of time allowed for a name
resolution.  This time interval is divided by the number of nameservers and the
number of retries allowed for each nameserver.

And other BSDs, timeout: sets the initial amount of time the resolver will wait
for a response from a remote name server before retrying the query via a
different name server.  The resolver may wait longer during subsequent retries
of the current query since an exponential back-off is applied to the timeout
value.  Measured in seconds, the default is RES_TIMEOUT, the allowed maximum is
RES_MAXRETRANS (see <resolv.h>).

On Linux, timeout: sets the amount of time the resolver will wait for a response
from a remote name server before retrying the query via a different name server.
Measured in econds,  the default is RES_TIMEOUT (currently 5, see <resolv.h>).
The value for this option is silently capped to 30.

for now i take your suggestion and i'd like to revisit this in your cl review.

https://codereview.appspot.com/101220044/diff/80001/src/pkg/net/dnsclient_uni...
File src/pkg/net/dnsclient_unix.go (right):

https://codereview.appspot.com/101220044/diff/80001/src/pkg/net/dnsclient_uni...
src/pkg/net/dnsclient_unix.go:141: func exchange(server, name string, qtype
uint16, deadline time.Time) (*dnsMsg, error) {
On 2014/06/13 15:33:55, axaxs wrote:
> Do not pass deadline here as an argument.  Pass the timeout or else cfg as we
> did initially.  See truncate check comments.

Done.

[axaxs, 2014-06-26 04:51:11]

Thanks.  LGTM.  Will revise and resubmit other CL once this lands.


On Wed, Jun 25, 2014 at 10:37 PM, <mikioh.mikioh@gmail.com> wrote:

> sorry for being late, ptal.
>
>
>
> https://codereview.appspot.com/101220044/diff/80001/src/
> pkg/net/dnsclient_unix.go
> File src/pkg/net/dnsclient_unix.go (left):
>
> https://codereview.appspot.com/101220044/diff/80001/src/
> pkg/net/dnsclient_unix.go#oldcode83
> src/pkg/net/dnsclient_unix.go:83: continue
> i'm still not convinced of the timeout in /etc/resolv.conf.
>
> On some BSD, timeout: Specifies the total amount of time allowed for a
> name resolution.  This time interval is divided by the number of
> nameservers and the number of retries allowed for each nameserver.
>
> And other BSDs, timeout: sets the initial amount of time the resolver
> will wait for a response from a remote name server before retrying the
> query via a different name server.  The resolver may wait longer during
> subsequent retries of the current query since an exponential back-off is
> applied to the timeout value.  Measured in seconds, the default is
> RES_TIMEOUT, the allowed maximum is RES_MAXRETRANS (see <resolv.h>).
>
> On Linux, timeout: sets the amount of time the resolver will wait for a
> response from a remote name server before retrying the query via a
> different name server. Measured in econds,  the default is RES_TIMEOUT
> (currently 5, see <resolv.h>). The value for this option is silently
> capped to 30.
>
> for now i take your suggestion and i'd like to revisit this in your cl
> review.
>
>
> https://codereview.appspot.com/101220044/diff/80001/src/
> pkg/net/dnsclient_unix.go
> File src/pkg/net/dnsclient_unix.go (right):
>
> https://codereview.appspot.com/101220044/diff/80001/src/
> pkg/net/dnsclient_unix.go#newcode141
> src/pkg/net/dnsclient_unix.go:141: func exchange(server, name string,
> qtype uint16, deadline time.Time) (*dnsMsg, error) {
> On 2014/06/13 15:33:55, axaxs wrote:
>
>> Do not pass deadline here as an argument.  Pass the timeout or else
>>
> cfg as we
>
>> did initially.  See truncate check comments.
>>
>
> Done.
>
> https://codereview.appspot.com/101220044/
>

[axaxs, 2014-07-10 04:59:18]

Just following up since this is blocking other changes.  Is it waiting for
more eyes?

Thanks,
Alex

On Thursday, June 26, 2014, Alex Skinner <alex@lx.lc> wrote:

> Thanks.  LGTM.  Will revise and resubmit other CL once this lands.
>
>
> On Wed, Jun 25, 2014 at 10:37 PM, <mikioh.mikioh@gmail.com
> <javascript:_e(%7B%7D,'cvml','mikioh.mikioh@gmail.com');>> wrote:
>
>> sorry for being late, ptal.
>>
>>
>>
>> https://codereview.appspot.com/101220044/diff/80001/src/
>> pkg/net/dnsclient_unix.go
>> File src/pkg/net/dnsclient_unix.go (left):
>>
>> https://codereview.appspot.com/101220044/diff/80001/src/
>> pkg/net/dnsclient_unix.go#oldcode83
>> src/pkg/net/dnsclient_unix.go:83: continue
>> i'm still not convinced of the timeout in /etc/resolv.conf.
>>
>> On some BSD, timeout: Specifies the total amount of time allowed for a
>> name resolution.  This time interval is divided by the number of
>> nameservers and the number of retries allowed for each nameserver.
>>
>> And other BSDs, timeout: sets the initial amount of time the resolver
>> will wait for a response from a remote name server before retrying the
>> query via a different name server.  The resolver may wait longer during
>> subsequent retries of the current query since an exponential back-off is
>> applied to the timeout value.  Measured in seconds, the default is
>> RES_TIMEOUT, the allowed maximum is RES_MAXRETRANS (see <resolv.h>).
>>
>> On Linux, timeout: sets the amount of time the resolver will wait for a
>> response from a remote name server before retrying the query via a
>> different name server. Measured in econds,  the default is RES_TIMEOUT
>> (currently 5, see <resolv.h>). The value for this option is silently
>> capped to 30.
>>
>> for now i take your suggestion and i'd like to revisit this in your cl
>> review.
>>
>>
>> https://codereview.appspot.com/101220044/diff/80001/src/
>> pkg/net/dnsclient_unix.go
>> File src/pkg/net/dnsclient_unix.go (right):
>>
>> https://codereview.appspot.com/101220044/diff/80001/src/
>> pkg/net/dnsclient_unix.go#newcode141
>> src/pkg/net/dnsclient_unix.go:141: func exchange(server, name string,
>> qtype uint16, deadline time.Time) (*dnsMsg, error) {
>> On 2014/06/13 15:33:55, axaxs wrote:
>>
>>> Do not pass deadline here as an argument.  Pass the timeout or else
>>>
>> cfg as we
>>
>>> did initially.  See truncate check comments.
>>>
>>
>> Done.
>>
>> https://codereview.appspot.com/101220044/
>>
>
>

[axaxs, 2014-07-10 12:13:54]

Just following up since this is blocking other changes.  Is it waiting for
more eyes?

Thanks,
Alex

On Thursday, June 26, 2014, Alex Skinner <alex@lx.lc> wrote:

> Thanks.  LGTM.  Will revise and resubmit other CL once this lands.
>
>
> On Wed, Jun 25, 2014 at 10:37 PM, <mikioh.mikioh@gmail.com
> <javascript:_e(%7B%7D,'cvml','mikioh.mikioh@gmail.com');>> wrote:
>
>> sorry for being late, ptal.
>>
>>
>>
>> https://codereview.appspot.com/101220044/diff/80001/src/
>> pkg/net/dnsclient_unix.go
>> File src/pkg/net/dnsclient_unix.go (left):
>>
>> https://codereview.appspot.com/101220044/diff/80001/src/
>> pkg/net/dnsclient_unix.go#oldcode83
>> src/pkg/net/dnsclient_unix.go:83: continue
>> i'm still not convinced of the timeout in /etc/resolv.conf.
>>
>> On some BSD, timeout: Specifies the total amount of time allowed for a
>> name resolution.  This time interval is divided by the number of
>> nameservers and the number of retries allowed for each nameserver.
>>
>> And other BSDs, timeout: sets the initial amount of time the resolver
>> will wait for a response from a remote name server before retrying the
>> query via a different name server.  The resolver may wait longer during
>> subsequent retries of the current query since an exponential back-off is
>> applied to the timeout value.  Measured in seconds, the default is
>> RES_TIMEOUT, the allowed maximum is RES_MAXRETRANS (see <resolv.h>).
>>
>> On Linux, timeout: sets the amount of time the resolver will wait for a
>> response from a remote name server before retrying the query via a
>> different name server. Measured in econds,  the default is RES_TIMEOUT
>> (currently 5, see <resolv.h>). The value for this option is silently
>> capped to 30.
>>
>> for now i take your suggestion and i'd like to revisit this in your cl
>> review.
>>
>>
>> https://codereview.appspot.com/101220044/diff/80001/src/
>> pkg/net/dnsclient_unix.go
>> File src/pkg/net/dnsclient_unix.go (right):
>>
>> https://codereview.appspot.com/101220044/diff/80001/src/
>> pkg/net/dnsclient_unix.go#newcode141
>> src/pkg/net/dnsclient_unix.go:141: func exchange(server, name string,
>> qtype uint16, deadline time.Time) (*dnsMsg, error) {
>> On 2014/06/13 15:33:55, axaxs wrote:
>>
>>> Do not pass deadline here as an argument.  Pass the timeout or else
>>>
>> cfg as we
>>
>>> did initially.  See truncate check comments.
>>>
>>
>> Done.
>>
>> https://codereview.appspot.com/101220044/
>>
>
>

[mikio, 2014-07-16 00:47:00]

On 2014/07/10 12:13:54, axaxs wrote:

> Just following up since this is blocking other changes.

oops, sorry for being late. the big football games consumed my spare time. 

> Is it waiting for more eyes?

yup, we need a sign off from established people.

[gobot, 2014-07-16 00:47:53]

R=iant@golang.org (assigned by mikioh.mikioh@gmail.com)

[mikio, 2014-07-28 04:50:33]

iant?

[minux, 2014-07-28 04:52:49]

On Mon, Jul 28, 2014 at 12:50 AM, <mikioh.mikioh@gmail.com> wrote:

> iant?
>
FYI, iant is on vacation.

[mikio, 2014-07-28 04:57:26]

On Mon, Jul 28, 2014 at 1:52 PM, minux <minux@golang.org> wrote:

> FYI, iant is on vacation.

thanks. minux, do you have a spare time?

[minux, 2014-07-28 05:44:25]

R=golang-dev

Sorry, I'm not familiar with the code. Given that rsc is also on vacation,
let's ask for reviewers on golang-dev.

[kevlar, 2014-07-28 20:44:05]

Drive-by comments.

https://codereview.appspot.com/101220044/diff/120001/src/pkg/net/dnsclient_un...
File src/pkg/net/dnsclient_unix.go (right):

https://codereview.appspot.com/101220044/diff/120001/src/pkg/net/dnsclient_un...
src/pkg/net/dnsclient_unix.go:110: if n < len(b) {
From io.Writer:

"Write must return a non-nil error if it returns n < len(p)."

https://codereview.appspot.com/101220044/diff/120001/src/pkg/net/dnsclient_un...
src/pkg/net/dnsclient_unix.go:134: default:
I think I would

case "udp", "udp4", "udp6":
  return c.(*UDPConn), nil
}
panic("unreachable")

just to be incrementally more explicit (and also potentially guard against
future accidentally missed additions of networks or something).

Could just be me though, I don't know if the stdlib has common idioms for this
already in place.

https://codereview.appspot.com/101220044/diff/120001/src/pkg/net/dnsclient_un...
src/pkg/net/dnsclient_unix.go:139: // exchange sends a query on the connection
and hope for a response.
and hopes for

https://codereview.appspot.com/101220044/diff/120001/src/pkg/net/dnsclient_un...
src/pkg/net/dnsclient_unix.go:140: // Up to cfg.attempts attempts.
cfg is not an argument anymore.

https://codereview.appspot.com/101220044/diff/120001/src/pkg/net/dnsclient_un...
src/pkg/net/dnsclient_unix.go:143: out := dnsMsg{}
out := dnsMsg{
  recursion_desired: true,
  question:          []dnsQuestion{
    {name, qtype, dnsClassINET},
  },
}

https://codereview.appspot.com/101220044/diff/120001/src/pkg/net/dnsclient_un...
src/pkg/net/dnsclient_unix.go:178: return "", nil, &DNSError{Err:
errDNSNoServers.Error(), Name: name}
I'm personally not a fan of these canned error messages.  There seems to be very
little point in defining them consistently if clients can't make use of them.  I
tend to much prefer using `fmt.Errorf` to add in context to error messages,
which (while not canned) at least contain enough extra information to be useful
in debugging.  In particular it seems odd to define an `error` to just call
.Error() to get its string.

I don't know what promises we've made about the text of error messages being
consistent, but I don't think we have, though we should try to keep common
strings like "no DNS servers" if we think people might've hard-coded them.

https://codereview.appspot.com/101220044/diff/120001/src/pkg/net/dnsclient_un...
src/pkg/net/dnsclient_unix.go:183: timeout := time.Duration(cfg.timeout) *
time.Second
can cfg.timeout be changed to be a time.Duration itself?

https://codereview.appspot.com/101220044/diff/120001/src/pkg/net/dnsclient_un...
src/pkg/net/dnsclient_unix.go:187: lastErr = &DNSError{Err:
errDNSNoAnswer.Error(), Name: name, Server: server, IsTimeout: true}
write this out on lines.

https://codereview.appspot.com/101220044/diff/120001/src/pkg/net/dnsclient_un...
src/pkg/net/dnsclient_unix.go:200: if err != nil {
I think you can always set lastErr = err here.

https://codereview.appspot.com/101220044/diff/120001/src/pkg/net/dnsclient_un...
File src/pkg/net/dnsclient_unix_test.go (right):

https://codereview.appspot.com/101220044/diff/120001/src/pkg/net/dnsclient_un...
src/pkg/net/dnsclient_unix_test.go:19: var dnsTransportTests = []struct {
While you're at it, you might add some NXDOMAIN tests too.  My favorite is
invalid., which RFC6761 says should always be NXDOMAIN.

Possible downside...
If you're on an ISP that is doing some DNS hijack, and consequently ignoring the
RFCs, that may cause this test to fail...
Part of me thinks that might be a good thing, though.

[mikio, 2014-08-04 06:12:13]

ptal

https://codereview.appspot.com/101220044/diff/120001/src/pkg/net/dnsclient_un...
File src/pkg/net/dnsclient_unix.go (right):

https://codereview.appspot.com/101220044/diff/120001/src/pkg/net/dnsclient_un...
src/pkg/net/dnsclient_unix.go:110: if n < len(b) {
i don't understand what you are suggesting.

https://codereview.appspot.com/101220044/diff/120001/src/pkg/net/dnsclient_un...
src/pkg/net/dnsclient_unix.go:134: default:
On 2014/07/28 20:44:04, kevlar wrote:
> I think I would
> 
> case "udp", "udp4", "udp6":
>   return c.(*UDPConn), nil
> }
> panic("unreachable")
> 
> just to be incrementally more explicit (and also potentially guard against
> future accidentally missed additions of networks or something).
> 
> Could just be me though, I don't know if the stdlib has common idioms for this
> already in place.

Done.

https://codereview.appspot.com/101220044/diff/120001/src/pkg/net/dnsclient_un...
src/pkg/net/dnsclient_unix.go:139: // exchange sends a query on the connection
and hope for a response.
On 2014/07/28 20:44:04, kevlar wrote:
> and hopes for

Done.

https://codereview.appspot.com/101220044/diff/120001/src/pkg/net/dnsclient_un...
src/pkg/net/dnsclient_unix.go:140: // Up to cfg.attempts attempts.
On 2014/07/28 20:44:05, kevlar wrote:
> cfg is not an argument anymore.

Done.

https://codereview.appspot.com/101220044/diff/120001/src/pkg/net/dnsclient_un...
src/pkg/net/dnsclient_unix.go:143: out := dnsMsg{}
On 2014/07/28 20:44:05, kevlar wrote:
> out := dnsMsg{
>   recursion_desired: true,
>   question:          []dnsQuestion{
>     {name, qtype, dnsClassINET},
>   },
> }

Done.

https://codereview.appspot.com/101220044/diff/120001/src/pkg/net/dnsclient_un...
src/pkg/net/dnsclient_unix.go:178: return "", nil, &DNSError{Err:
errDNSNoServers.Error(), Name: name}
fine, dropped. anyway we'll revisit all error messages when we smash issue 4856.

https://codereview.appspot.com/101220044/diff/120001/src/pkg/net/dnsclient_un...
src/pkg/net/dnsclient_unix.go:183: timeout := time.Duration(cfg.timeout) *
time.Second
On 2014/07/28 20:44:04, kevlar wrote:
> can cfg.timeout be changed to be a time.Duration itself?

it would be in another cl; alex is working on 75180043

https://codereview.appspot.com/101220044/diff/120001/src/pkg/net/dnsclient_un...
src/pkg/net/dnsclient_unix.go:187: lastErr = &DNSError{Err:
errDNSNoAnswer.Error(), Name: name, Server: server, IsTimeout: true}
On 2014/07/28 20:44:05, kevlar wrote:
> write this out on lines.

Done.

https://codereview.appspot.com/101220044/diff/120001/src/pkg/net/dnsclient_un...
src/pkg/net/dnsclient_unix.go:200: if err != nil {
On 2014/07/28 20:44:04, kevlar wrote:
> I think you can always set lastErr = err here.

Done.

https://codereview.appspot.com/101220044/diff/120001/src/pkg/net/dnsclient_un...
File src/pkg/net/dnsclient_unix_test.go (right):

https://codereview.appspot.com/101220044/diff/120001/src/pkg/net/dnsclient_un...
src/pkg/net/dnsclient_unix_test.go:19: var dnsTransportTests = []struct {
On 2014/07/28 20:44:05, kevlar wrote:
> While you're at it, you might add some NXDOMAIN tests too.  My favorite is
> invalid., which RFC6761 says should always be NXDOMAIN.
> 
> Possible downside...
> If you're on an ISP that is doing some DNS hijack, and consequently ignoring
the
> RFCs, that may cause this test to fail...
> Part of me thinks that might be a good thing, though.

Done.

[kevlar, 2014-08-04 18:08:04]

https://codereview.appspot.com/101220044/diff/120001/src/pkg/net/dnsclient_un...
File src/pkg/net/dnsclient_unix.go (right):

https://codereview.appspot.com/101220044/diff/120001/src/pkg/net/dnsclient_un...
src/pkg/net/dnsclient_unix.go:110: if n < len(b) {
> i don't understand what you are suggesting.

This check is redundant.  If n < len(b), Write would've returned an error.  This
should either be checked first or removed.  Since nothing is checking the
errDNSShortWrite (it's not exported), I'd opt for just removing it.

https://codereview.appspot.com/101220044/diff/160001/src/pkg/net/dnsclient_un...
File src/pkg/net/dnsclient_unix.go (right):

https://codereview.appspot.com/101220044/diff/160001/src/pkg/net/dnsclient_un...
src/pkg/net/dnsclient_unix.go:41: func (c *UDPConn) readDNSResponse() (*dnsMsg,
error) {
Is attaching unexported methods to UDPConn/TCPConn/Dialer a pattern that is used
elsewhere in net?  Normally I would say that it'd be better to make your own
type that embeds them, but I do see the appeal.

https://codereview.appspot.com/101220044/diff/160001/src/pkg/net/dnsclient_un...
src/pkg/net/dnsclient_unix.go:192: lastErr = &DNSError{Err: nerr.Error(), Name:
name, Server: server, IsTimeout: true}
(optional, nitpick) I'd write all of your &DNSErrors out one-field-per-line

https://codereview.appspot.com/101220044/diff/160001/src/pkg/net/dnsclient_un...
File src/pkg/net/dnsclient_unix_test.go (right):

https://codereview.appspot.com/101220044/diff/160001/src/pkg/net/dnsclient_un...
src/pkg/net/dnsclient_unix_test.go:37: {"8.8.8.8:53", "com.", dnsTypeALL, 2,
[]int{dnsRcodeSuccess, dnsRcodeServerFailure}},
If the only purpose of the rcodes slice is to allow RcodeServerFailure and you
want to allow that for all tests, you might as well just add that to the test
and make rcode singular.

[mikio, 2014-08-05 12:28:31]

https://codereview.appspot.com/101220044/diff/120001/src/pkg/net/dnsclient_un...
File src/pkg/net/dnsclient_unix.go (right):

https://codereview.appspot.com/101220044/diff/120001/src/pkg/net/dnsclient_un...
src/pkg/net/dnsclient_unix.go:110: if n < len(b) {
ah, got it, thanks.

https://codereview.appspot.com/101220044/diff/160001/src/pkg/net/dnsclient_un...
File src/pkg/net/dnsclient_unix.go (right):

https://codereview.appspot.com/101220044/diff/160001/src/pkg/net/dnsclient_un...
src/pkg/net/dnsclient_unix.go:41: func (c *UDPConn) readDNSResponse() (*dnsMsg,
error) {
no, this is a special case. we'll make new dns endpoint type and move builtin
dns resolver stuff into some internal package for avoiding the issue of circular
dependency when we need  to implement t-dns and/or edns1.

https://codereview.appspot.com/101220044/diff/160001/src/pkg/net/dnsclient_un...
src/pkg/net/dnsclient_unix.go:192: lastErr = &DNSError{Err: nerr.Error(), Name:
name, Server: server, IsTimeout: true}
On 2014/08/04 18:08:04, kevlar wrote:
> (optional, nitpick) I'd write all of your &DNSErrors out one-field-per-line

Done.

https://codereview.appspot.com/101220044/diff/160001/src/pkg/net/dnsclient_un...
File src/pkg/net/dnsclient_unix_test.go (right):

https://codereview.appspot.com/101220044/diff/160001/src/pkg/net/dnsclient_un...
src/pkg/net/dnsclient_unix_test.go:37: {"8.8.8.8:53", "com.", dnsTypeALL, 2,
[]int{dnsRcodeSuccess, dnsRcodeServerFailure}},
On 2014/08/04 18:08:04, kevlar wrote:
> If the only purpose of the rcodes slice is to allow RcodeServerFailure and you
> want to allow that for all tests, you might as well just add that to the test
> and make rcode singular.

Done.

[kevlar, 2014-08-05 19:45:46]

LGTM

from my perspective... not sure how much weight you want to give to me though
:).

[mikio, 2014-08-06 00:57:33]

On Wed, Aug 6, 2014 at 4:45 AM,  <kevlar@google.com> wrote:

> from my perspective... not sure how much weight you want to give to me
> though :).

thanks. ah... https://codereview.appspot.com/105110044/ is also
waiting for your review. ;)

[mikio, 2014-08-06 00:58:58]

*** Submitted as https://code.google.com/p/go/source/detail?r=ed83901ff17e ***

net: separate DNS transport from DNS query-response interaction

Before fixing issue 6579 this CL separates DNS transport from
DNS message interaction to make it easier to add builtin DNS
resolver control logic.

Update issue 6579

LGTM=alex, kevlar
R=golang-codereviews, alex, gobot, iant, minux, kevlar
CC=golang-codereviews
https://codereview.appspot.com/101220044

[gobot, 2014-08-06 01:04:57]

This CL appears to have broken the netbsd-386-minux builder.
See http://build.golang.org/log/c9df177ad34350f0240b81a1b0fb80bafbe7e095