code review 3851041: net/dnsclient.go: Add LookupReverse(addr string) []str...

net/dnsclient.go: Add LookupReverse(addr string) []string, os.Error

	  I have added a function to perform reverse lookups by retrieving the appropriate PTR records.
	  There is also an accompanying function/test to compute the in-addr.arpa. and ip6.arpa.

    I also added a reverse static lookup for /etc/hosts.

[Kyle Lemons, 2010-12-30 11:36:37]

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

I'd like you to review this change.

[Kyle Lemons, 2010-12-30 20:42:40]

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

Please take another look.

[adg, 2011-01-04 05:29:20]

http://codereview.appspot.com/3851041/diff/20001/src/pkg/net/dnsclient.go
File src/pkg/net/dnsclient.go (right):

http://codereview.appspot.com/3851041/diff/20001/src/pkg/net/dnsclient.go#new...
src/pkg/net/dnsclient.go:369: type PTR struct {
This isn't used anywhere as far as I can see.

http://codereview.appspot.com/3851041/diff/20001/src/pkg/net/dnsclient.go#new...
src/pkg/net/dnsclient.go:376: if strings.Count(addr, ".") == 3 {
First handle the error case, and return.
Then handle this case, and return.
Finally, handle the ipv6 case.

http://codereview.appspot.com/3851041/diff/20001/src/pkg/net/dnsclient.go#new...
src/pkg/net/dnsclient.go:378: arpa = octets[3] + "." + octets[2] + "." +
octets[1] + "." + octets[0] + ".in-addr.arpa."
This doesn't validate the input at all. Is that desirable?

http://codereview.appspot.com/3851041/diff/20001/src/pkg/net/dnsclient.go#new...
src/pkg/net/dnsclient.go:379: } else if strings.Count(addr, ":") >= 2 {
This section on decoding the ipv6 address is much more complex than it need be,
largely because it tries to manipulate text instead of the actual semantic data.

First parse the IP address using the ParseIP function, then transform it to the
desired output format with something like:

        s := fmt.Sprintf("%x", ip)
        var buf bytes.Buffer
        for i := len(s)-1; i >= 0; i-- {
                if buf.Len() > 0 {
                        buf.WriteByte('.')
                }
                buf.WriteByte(s[i])
        }
        return buf.String() + ".ip6.arpa."

http://codereview.appspot.com/3851041/diff/20001/src/pkg/net/hosts.go
File src/pkg/net/hosts.go (right):

http://codereview.appspot.com/3851041/diff/20001/src/pkg/net/hosts.go#newcode50
src/pkg/net/hosts.go:50: is[f[0]] = append(is[f[0]], h)
Forward mapping is not the same as reverse mapping. This cache would prevent
doing an RDNS lookup of an IP that is in the hosts file.

[Kyle E. Lemons, 2011-01-05 00:48:54]

PTAL.

Comments and questions inline.

On 2011/01/04 05:29:20, adg wrote:
> http://codereview.appspot.com/3851041/diff/20001/src/pkg/net/dnsclient.go
> File src/pkg/net/dnsclient.go (right):
> 
>
http://codereview.appspot.com/3851041/diff/20001/src/pkg/net/dnsclient.go#new...
> src/pkg/net/dnsclient.go:369: type PTR struct {
> This isn't used anywhere as far as I can see.
> 
>
http://codereview.appspot.com/3851041/diff/20001/src/pkg/net/dnsclient.go#new...

Removed.

> src/pkg/net/dnsclient.go:376: if strings.Count(addr, ".") == 3 {
> First handle the error case, and return.
> Then handle this case, and return.
> Finally, handle the ipv6 case.
> 
>
http://codereview.appspot.com/3851041/diff/20001/src/pkg/net/dnsclient.go#new...

Rewritten.

> src/pkg/net/dnsclient.go:378: arpa = octets[3] + "." + octets[2] + "." +
> octets[1] + "." + octets[0] + ".in-addr.arpa."
> This doesn't validate the input at all. Is that desirable?
> 
>
http://codereview.appspot.com/3851041/diff/20001/src/pkg/net/dnsclient.go#new...
> src/pkg/net/dnsclient.go:379: } else if strings.Count(addr, ":") >= 2 {
> This section on decoding the ipv6 address is much more complex than it need
be,
> largely because it tries to manipulate text instead of the actual semantic
data.
> 
> First parse the IP address using the ParseIP function, then transform it to
the
> desired output format with something like:
> 
>         s := fmt.Sprintf("%x", ip)
>         var buf bytes.Buffer
>         for i := len(s)-1; i >= 0; i-- {
>                 if buf.Len() > 0 {
>                         buf.WriteByte('.')
>                 }
>                 buf.WriteByte(s[i])
>         }
>         return buf.String() + ".ip6.arpa."
> 
> http://codereview.appspot.com/3851041/diff/20001/src/pkg/net/hosts.go
> File src/pkg/net/hosts.go (right):
> 
>
http://codereview.appspot.com/3851041/diff/20001/src/pkg/net/hosts.go#newcode50

Done.

> src/pkg/net/hosts.go:50: is[f[0]] = append(is[f[0]], h)
> Forward mapping is not the same as reverse mapping. This cache would prevent
> doing an RDNS lookup of an IP that is in the hosts file.

Is that not desireable?  The primary reason I added this was because local IPs
in the hosts file won't resolve and (for whatever reason) ::1 doesn't resolve to
localhost when you look up the PTR record for 1.*.ip6.arpa. even though
1.0.0.127.in-addr.arpa. looks up to localhost.  It is in my hosts file however,
so with this added I get the proper reverse lookup for ::1.  My machine also has
fe80::1 mapped to localhost, which is in the hosts file, which probably wouldn't
rDNS properly either.  Should I do the RDNS and only look in the hosts file if
the RDNS doesn't come back?

~Kyle

[adg, 2011-01-05 02:39:39]

http://codereview.appspot.com/3851041/diff/26001/src/pkg/net/dnsclient.go
File src/pkg/net/dnsclient.go (right):

http://codereview.appspot.com/3851041/diff/26001/src/pkg/net/dnsclient.go#new...
src/pkg/net/dnsclient.go:369: // Get the in-addr.arpa. or ip6.arpa. for the
address
// reverseaddr returns the..

See the other top-level comments on functions/types.

http://codereview.appspot.com/3851041/diff/26001/src/pkg/net/dnsclient.go#new...
src/pkg/net/dnsclient.go:371: // Parse the IP
These comments are obvious. You can remove all the comments, except as noted.

http://codereview.appspot.com/3851041/diff/26001/src/pkg/net/dnsclient.go#new...
src/pkg/net/dnsclient.go:381: // Otherwise, assume it's an IPv6 IP
// must be ipv6

http://codereview.appspot.com/3851041/diff/26001/src/pkg/net/dnsclient.go#new...
src/pkg/net/dnsclient.go:395: // This function looks up the PTR records for the
given address and returns
See the comment for LookupHost to get an idea as to how to re-write this
comment.

http://codereview.appspot.com/3851041/diff/26001/src/pkg/net/dnsclient.go#new...
src/pkg/net/dnsclient.go:400: if entries != nil && len(entries) > 0 {
if len(entries) > 0 {

http://codereview.appspot.com/3851041/diff/26001/src/pkg/net/dnsclient.go#new...
src/pkg/net/dnsclient.go:413: for i := 0; i < len(records); i++ {
for i := range records {

http://codereview.appspot.com/3851041/diff/26001/src/pkg/net/net_test.go
File src/pkg/net/net_test.go (right):

http://codereview.appspot.com/3851041/diff/26001/src/pkg/net/net_test.go#newc...
src/pkg/net/net_test.go:86: var revAddrTests = []RevAddrTest{
You can (and should) put the struct definition here in-line:

var revAddrTests = []struct{
         Addr      string
         Reverse   string
         ErrPrefix string
}{

http://codereview.appspot.com/3851041/diff/26001/src/pkg/net/net_test.go#newc...
src/pkg/net/net_test.go:104: t.Errorf("#%d: nil error, expected '%s'", i,
tt.ErrPrefix)
error messages should be of the form
"expected x, got y"
in this case
"expected %q, got <nil>"

same for the other errors

[Kyle E. Lemons, 2011-01-05 03:40:13]

PTAL.

I still left the local hosts file lookup before the PTR lookup.  Let me know if
you think it should be the other way around or separated into two functions or
the hosts lookup left out completely.

On 2011/01/05 02:39:39, adg wrote:
> http://codereview.appspot.com/3851041/diff/26001/src/pkg/net/dnsclient.go
> File src/pkg/net/dnsclient.go (right):
> 
>
http://codereview.appspot.com/3851041/diff/26001/src/pkg/net/dnsclient.go#new...
> src/pkg/net/dnsclient.go:369: // Get the in-addr.arpa. or ip6.arpa. for the
> address
> // reverseaddr returns the..
> 
> See the other top-level comments on functions/types.
> 
>
http://codereview.appspot.com/3851041/diff/26001/src/pkg/net/dnsclient.go#new...
> src/pkg/net/dnsclient.go:371: // Parse the IP
> These comments are obvious. You can remove all the comments, except as noted.
> 
>
http://codereview.appspot.com/3851041/diff/26001/src/pkg/net/dnsclient.go#new...
> src/pkg/net/dnsclient.go:381: // Otherwise, assume it's an IPv6 IP
> // must be ipv6
> 
>
http://codereview.appspot.com/3851041/diff/26001/src/pkg/net/dnsclient.go#new...
> src/pkg/net/dnsclient.go:395: // This function looks up the PTR records for
the
> given address and returns
> See the comment for LookupHost to get an idea as to how to re-write this
> comment.
> 
>
http://codereview.appspot.com/3851041/diff/26001/src/pkg/net/dnsclient.go#new...
> src/pkg/net/dnsclient.go:400: if entries != nil && len(entries) > 0 {
> if len(entries) > 0 {
> 
>
http://codereview.appspot.com/3851041/diff/26001/src/pkg/net/dnsclient.go#new...
> src/pkg/net/dnsclient.go:413: for i := 0; i < len(records); i++ {
> for i := range records {
> 
> http://codereview.appspot.com/3851041/diff/26001/src/pkg/net/net_test.go
> File src/pkg/net/net_test.go (right):
> 
>
http://codereview.appspot.com/3851041/diff/26001/src/pkg/net/net_test.go#newc...
> src/pkg/net/net_test.go:86: var revAddrTests = []RevAddrTest{
> You can (and should) put the struct definition here in-line:
> 
> var revAddrTests = []struct{
>          Addr      string
>          Reverse   string
>          ErrPrefix string
> }{
> 
>
http://codereview.appspot.com/3851041/diff/26001/src/pkg/net/net_test.go#newc...
> src/pkg/net/net_test.go:104: t.Errorf("#%d: nil error, expected '%s'", i,
> tt.ErrPrefix)
> error messages should be of the form
> "expected x, got y"
> in this case
> "expected %q, got <nil>"
> 
> same for the other errors

[adg, 2011-01-09 23:20:58]

Looking pretty good. Adding Russ to reviewers.

http://codereview.appspot.com/3851041/diff/31001/src/pkg/net/dnsclient.go
File src/pkg/net/dnsclient.go (right):

http://codereview.appspot.com/3851041/diff/31001/src/pkg/net/dnsclient.go#new...
src/pkg/net/dnsclient.go:395: // It returns the an array of qualified hostnames
found in the PTR records
s/It/and/
and put a period at the line.

(This is public documentation.)

http://codereview.appspot.com/3851041/diff/31001/src/pkg/net/hosts.go
File src/pkg/net/hosts.go (right):

http://codereview.appspot.com/3851041/diff/31001/src/pkg/net/hosts.go#newcode76
src/pkg/net/hosts.go:76: func rlookupStaticHost(host string) []string {
s/host/addr/

and throughout the rest of the function

[rsc, 2011-01-11 16:51:29]

http://codereview.appspot.com/3851041/diff/31001/src/pkg/net/dnsclient.go
File src/pkg/net/dnsclient.go (right):

http://codereview.appspot.com/3851041/diff/31001/src/pkg/net/dnsclient.go#new...
src/pkg/net/dnsclient.go:19: "fmt"
I'm not happy about importing fmt and bytes
into such a low-level package, but package net
is up for a rewrite anyway, so these can stay.

Please sort the import list.

http://codereview.appspot.com/3851041/diff/31001/src/pkg/net/dnsclient.go#new...
src/pkg/net/dnsclient.go:375: return "", os.NewError("Improperly formatted IPv4
or IPv6 address: " + addr)
errors begin with lowercase.
also should be like the other errors in this file.

return "", &DNSError{Error: "unrecognized address", Name: addr}

http://codereview.appspot.com/3851041/diff/31001/src/pkg/net/dnsclient.go#new...
src/pkg/net/dnsclient.go:394: // LookupReverse looks for addr using the local
hosts file and rDNS
// LookupAddr performs a reverse lookup for the given address,
// returning a list of names mapping to that address.

http://codereview.appspot.com/3851041/diff/31001/src/pkg/net/dnsclient.go#new...
src/pkg/net/dnsclient.go:396: func LookupReverse(addr string) (entries []string,
err os.Error) {
s/entries/name/

http://codereview.appspot.com/3851041/diff/31001/src/pkg/net/dnsclient.go#new...
src/pkg/net/dnsclient.go:396: func LookupReverse(addr string) (entries []string,
err os.Error) {
s/LookupReverse/LookupAddr/

http://codereview.appspot.com/3851041/diff/31001/src/pkg/net/hosts.go
File src/pkg/net/hosts.go (right):

http://codereview.appspot.com/3851041/diff/31001/src/pkg/net/hosts.go#newcode22
src/pkg/net/hosts.go:22: hostips map[string][]string
byName

http://codereview.appspot.com/3851041/diff/31001/src/pkg/net/hosts.go#newcode23
src/pkg/net/hosts.go:23: iphosts map[string][]string
byAddr

[Kyle Lemons, 2011-01-11 18:41:48]

PTAL.

Thanks for the reviews, guys.

On 2011/01/11 16:51:29, rsc wrote:
> http://codereview.appspot.com/3851041/diff/31001/src/pkg/net/dnsclient.go
> File src/pkg/net/dnsclient.go (right):
> 
>
http://codereview.appspot.com/3851041/diff/31001/src/pkg/net/dnsclient.go#new...
> src/pkg/net/dnsclient.go:19: "fmt"
> I'm not happy about importing fmt and bytes
> into such a low-level package, but package net
> is up for a rewrite anyway, so these can stay.
> 
> Please sort the import list.
> 
>
http://codereview.appspot.com/3851041/diff/31001/src/pkg/net/dnsclient.go#new...
> src/pkg/net/dnsclient.go:375: return "", os.NewError("Improperly formatted
IPv4
> or IPv6 address: " + addr)
> errors begin with lowercase.
> also should be like the other errors in this file.
> 
> return "", &DNSError{Error: "unrecognized address", Name: addr}
> 
>
http://codereview.appspot.com/3851041/diff/31001/src/pkg/net/dnsclient.go#new...
> src/pkg/net/dnsclient.go:394: // LookupReverse looks for addr using the local
> hosts file and rDNS
> // LookupAddr performs a reverse lookup for the given address,
> // returning a list of names mapping to that address.
> 
>
http://codereview.appspot.com/3851041/diff/31001/src/pkg/net/dnsclient.go#new...
> src/pkg/net/dnsclient.go:396: func LookupReverse(addr string) (entries
[]string,
> err os.Error) {
> s/entries/name/
> 
>
http://codereview.appspot.com/3851041/diff/31001/src/pkg/net/dnsclient.go#new...
> src/pkg/net/dnsclient.go:396: func LookupReverse(addr string) (entries
[]string,
> err os.Error) {
> s/LookupReverse/LookupAddr/
> 
> http://codereview.appspot.com/3851041/diff/31001/src/pkg/net/hosts.go
> File src/pkg/net/hosts.go (right):
> 
>
http://codereview.appspot.com/3851041/diff/31001/src/pkg/net/hosts.go#newcode22
> src/pkg/net/hosts.go:22: hostips map[string][]string
> byName
> 
>
http://codereview.appspot.com/3851041/diff/31001/src/pkg/net/hosts.go#newcode23
> src/pkg/net/hosts.go:23: iphosts map[string][]string
> byAddr

[rsc, 2011-01-19 19:52:07]

LGTM

[rsc, 2011-01-19 20:11:05]

*** Submitted as 944ddec476c5 ***

net: add LookupAddr

R=adg, rsc
CC=golang-dev
http://codereview.appspot.com/3851041

Committer: Russ Cox <rsc@golang.org>