code review 122200043: net: fix CNAME resolving on Windows

net: fix CNAME resolving on Windows

Fixes issue 8492

[egon, 2014-08-08 09:08:45]

Hello golang-codereviews@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go

[egon, 2014-08-09 14:06:07]

On 2014/08/08 10:46:55, brainman wrote:

PTAL

[brainman, 2014-08-11 02:50:24]

Thank you for taking care of this.

Alex

https://codereview.appspot.com/122200043/diff/160001/src/pkg/net/lookup_windo...
File src/pkg/net/lookup_windows.go (right):

https://codereview.appspot.com/122200043/diff/160001/src/pkg/net/lookup_windo...
src/pkg/net/lookup_windows.go:11: "unicode"
You introduce dependency on unicode here. I suspect it might be very expensive.

https://codereview.appspot.com/122200043/diff/160001/src/pkg/net/lookup_windo...
src/pkg/net/lookup_windows.go:331: dnsSectionAnswer     = 1
This belongs to syscall (it is defined by Microsoft). So make it
syscall.DnsSectionAnswer. Put it into ztype_windows.go next to other DNS related
consts.

https://codereview.appspot.com/122200043/diff/160001/src/pkg/net/lookup_windo...
src/pkg/net/lookup_windows.go:332: dnsSectionAdditional = 3
Same as above.

https://codereview.appspot.com/122200043/diff/160001/src/pkg/net/lookup_windo...
src/pkg/net/lookup_windows.go:336: Cname:
This is to subtle for me. I would split it into 2 steps: find all CNAME records
first and put names into array, then do another loop matching both p.Type and
p.Name equal to either name in collected array.

https://codereview.appspot.com/122200043/diff/160001/src/pkg/net/lookup_windo...
src/pkg/net/lookup_windows.go:339: if p.Dw&dnsSectionMask != dnsSectionAnswer &&
p.Dw&dnsSectionMask != dnsSectionAdditional {
Why do you include records with dnsSectionAdditional set? I have removed these,
and your tests still PASS. Do you have a test that breaks when we do not examine
records with dnsSectionAdditional set?

https://codereview.appspot.com/122200043/diff/160001/src/pkg/net/lookup_windo...
src/pkg/net/lookup_windows.go:343: if !sameDNSName(pname, name) {
Should you be using Microsoft's DnsNameCompare instead?

https://codereview.appspot.com/122200043/diff/160001/src/pkg/net/lookup_windo...
src/pkg/net/lookup_windows.go:360: func sameDNSName(a, b string) bool {
I don't know, if your logic here is correct. Should you be using Microsoft's
DnsNameCompare instead?

https://codereview.appspot.com/122200043/diff/160001/src/pkg/net/lookup_windo...
File src/pkg/net/lookup_windows_test.go (right):

https://codereview.appspot.com/122200043/diff/160001/src/pkg/net/lookup_windo...
src/pkg/net/lookup_windows_test.go:32: if !ok {
It is fine to continue here, but, t.Logf error message returned from nslookupMX,
so at least user knows what happened - you would have to change nslookupMX to
return error. Same for TestLookupNS and TestLookupTXT.

https://codereview.appspot.com/122200043/diff/160001/src/pkg/net/lookup_windo...
src/pkg/net/lookup_windows_test.go:36: mx, err := LookupMX(server)
Move this to the start of this loop body. You want this tested regardless of
nslookupMX failure. Maybe even test that ms has at least one element, just like
TestGmailMX does. Same for TestLookupNS and TestLookupTXT.

https://codereview.appspot.com/122200043/diff/160001/src/pkg/net/lookup_windo...
src/pkg/net/lookup_windows_test.go:170: func nslookupCNAME(name string) (cname
string, ok bool) {
No one calls this function. Why? Perhaps delete it.

[egon, 2014-08-11 07:47:16]

On 2014/08/11 02:50:24, brainman wrote:
> Thank you for taking care of this.
> 
> Alex
> 
>
https://codereview.appspot.com/122200043/diff/160001/src/pkg/net/lookup_windo...
> File src/pkg/net/lookup_windows.go (right):
> 
>
https://codereview.appspot.com/122200043/diff/160001/src/pkg/net/lookup_windo...
> src/pkg/net/lookup_windows.go:11: "unicode"
> You introduce dependency on unicode here. I suspect it might be very
expensive.

I can remove this but then the code wouldn't support IDN-s. Of course the
alternative would be to use DnsNameCompare, but I didn't know where to add it,
because syscall is frozen.

> 
>
https://codereview.appspot.com/122200043/diff/160001/src/pkg/net/lookup_windo...
> src/pkg/net/lookup_windows.go:331: dnsSectionAnswer     = 1
> This belongs to syscall (it is defined by Microsoft). So make it
> syscall.DnsSectionAnswer. Put it into ztype_windows.go next to other DNS
related
> consts.

Isn't syscall frozen? I thought about this, but I was not sure whether to put it
in there, or should it go to the go.sys package? Or should it go to some
internal pkg.

> 
>
https://codereview.appspot.com/122200043/diff/160001/src/pkg/net/lookup_windo...
> src/pkg/net/lookup_windows.go:332: dnsSectionAdditional = 3
> Same as above.
> 
>
https://codereview.appspot.com/122200043/diff/160001/src/pkg/net/lookup_windo...
> src/pkg/net/lookup_windows.go:336: Cname:
> This is to subtle for me. I would split it into 2 steps: find all CNAME
records
> first and put names into array, then do another loop matching both p.Type and
> p.Name equal to either name in collected array.

If a DNS server has a CNAME record for a given domain it must not have any other
domains for it. But I guess making it two passes would make it clearer. But,
it's sufficient to resolve to the last CNAME and then use that to gather all the
types.

> 
>
https://codereview.appspot.com/122200043/diff/160001/src/pkg/net/lookup_windo...
> src/pkg/net/lookup_windows.go:339: if p.Dw&dnsSectionMask != dnsSectionAnswer
&&
> p.Dw&dnsSectionMask != dnsSectionAdditional {
> Why do you include records with dnsSectionAdditional set? I have removed
these,
> and your tests still PASS. Do you have a test that breaks when we do not
examine
> records with dnsSectionAdditional set?

Nope; but if CNAME-s are concerned then sometimes additional information is
returned in the additional section so that you wouldn't need to do additional
queries. Of course Windows API probably already handles that section and puts
relevant information to the Answer section, so it might not be necessary.

> 
>
https://codereview.appspot.com/122200043/diff/160001/src/pkg/net/lookup_windo...
> src/pkg/net/lookup_windows.go:343: if !sameDNSName(pname, name) {
> Should you be using Microsoft's DnsNameCompare instead?
> 
>
https://codereview.appspot.com/122200043/diff/160001/src/pkg/net/lookup_windo...
> src/pkg/net/lookup_windows.go:360: func sameDNSName(a, b string) bool {
> I don't know, if your logic here is correct. Should you be using Microsoft's
> DnsNameCompare instead?

I believe the algorithm is the same and the syscall currently doesn't contain
DnsNameCompare.
ReactOS code https://gist.github.com/egonelbre/a5a82c17cdec87c4dc7d somewhat
confirms that this is the same code.

> 
>
https://codereview.appspot.com/122200043/diff/160001/src/pkg/net/lookup_windo...
> File src/pkg/net/lookup_windows_test.go (right):
> 
>
https://codereview.appspot.com/122200043/diff/160001/src/pkg/net/lookup_windo...
> src/pkg/net/lookup_windows_test.go:32: if !ok {
> It is fine to continue here, but, t.Logf error message returned from
nslookupMX,
> so at least user knows what happened - you would have to change nslookupMX to
> return error. Same for TestLookupNS and TestLookupTXT.
> 

Maybe t.Skipf("nslookup failed to return results: %s", err) would be better than
t.Logf?

>
https://codereview.appspot.com/122200043/diff/160001/src/pkg/net/lookup_windo...
> src/pkg/net/lookup_windows_test.go:36: mx, err := LookupMX(server)
> Move this to the start of this loop body. You want this tested regardless of
> nslookupMX failure. Maybe even test that ms has at least one element, just
like
> TestGmailMX does. Same for TestLookupNS and TestLookupTXT.
> 

I currently didn't do any of the tests when "nslookup" fails. IMHO it's safe to
assume when nslookup is down, then talking to outside is down, so these tests
would produce noise. Of course I could check that when 'nslookup' fails then the
LookupMX should fail. Or should I always fail when LookupMX fails?

>
https://codereview.appspot.com/122200043/diff/160001/src/pkg/net/lookup_windo...
> src/pkg/net/lookup_windows_test.go:170: func nslookupCNAME(name string) (cname
> string, ok bool) {
> No one calls this function. Why? Perhaps delete it.

I'll add a test for LookupCNAME.

[brainman, 2014-08-11 10:56:51]

On 2014/08/11 07:47:16, egon wrote:
> >
> > > src/pkg/net/lookup_windows.go:11: "unicode"
> > You introduce dependency on unicode here. I suspect it might be very
> expensive.
> 
> I can remove this but then the code wouldn't support IDN-s. Of course the
> alternative would be to use DnsNameCompare, but I didn't know where to add it,
> because syscall is frozen.

Well, we need access to DnsNameCompare. I am not clear what is going to happens
to syscall package, but for now lets put it in syscall. We’ll move it when time
comes.

> > src/pkg/net/lookup_windows.go:331: dnsSectionAnswer     = 1
> > This belongs to syscall (it is defined by Microsoft). So make it
> > syscall.DnsSectionAnswer. Put it into ztype_windows.go next to other DNS
> related
> > consts.
> 
> Isn't syscall frozen? I thought about this, but I was not sure whether to put
it
> in there, or should it go to the go.sys package? Or should it go to some
> internal pkg.

Lets put it all into syscall for now.

> If a DNS server has a CNAME record for a given domain it must not have any
other
> domains for it. But I guess making it two passes would make it clearer. But,
> it's sufficient to resolve to the last CNAME and then use that to gather all
the
> types.

See, if you can make it simpler.

> > Why do you include records with dnsSectionAdditional set? I have removed
> these,
> > and your tests still PASS. Do you have a test that breaks when we do not
> examine
> > records with dnsSectionAdditional set?
> 
> Nope; but if CNAME-s are concerned then sometimes additional information is
> returned in the additional section so that you wouldn't need to do additional
> queries. Of course Windows API probably already handles that section and puts
> relevant information to the Answer section, so it might not be necessary.

I still don’t see how records with dnsSectionAdditional are used here.

> > I don't know, if your logic here is correct. Should you be using Microsoft's
> > DnsNameCompare instead?
> 
> I believe the algorithm is the same and the syscall currently doesn't contain
> DnsNameCompare.
> ReactOS code https://gist.github.com/egonelbre/a5a82c17cdec87c4dc7d somewhat
> confirms that this is the same code.

Perhaps, but why guess if Microsoft provided a function to do the job.

> > It is fine to continue here, but, t.Logf error message returned from
> nslookupMX,
> > so at least user knows what happened - you would have to change nslookupMX
to
> > return error. Same for TestLookupNS and TestLookupTXT.
> > 
> 
> Maybe t.Skipf("nslookup failed to return results: %s", err) would be better
than
> t.Logf?

No, lets not skip the test. Lets test as much as we can test. Regardless of
nslookup present or fail to run for whatever reason. But when nslookup fails, I
would like to see failure shown, so I don’t kid myself that everything is good.
And I would like to see the error message (via t.Logf) so I can decide if
something important is a miss.

> I currently didn't do any of the tests when "nslookup" fails. IMHO it's safe
to
> assume when nslookup is down, then talking to outside is down, so these tests
> would produce noise. Of course I could check that when 'nslookup' fails then
the
> LookupMX should fail. Or should I always fail when LookupMX fails?

Lets not make any assumptions about nslookup failures. And lets not link those
to the failures of LookupMX. If LookupMX fails, fail the test. If nslookup
succeeds, compare nsllokup results with LookupMX results, and fail test, if
results are different.

Alex

[egon, 2014-08-11 13:08:48]

On 2014/08/11 10:56:51, brainman wrote:
> On 2014/08/11 07:47:16, egon wrote:
> > >
> > > > src/pkg/net/lookup_windows.go:11: "unicode"
> > > You introduce dependency on unicode here. I suspect it might be very
> > expensive.
> > 
> > I can remove this but then the code wouldn't support IDN-s. Of course the
> > alternative would be to use DnsNameCompare, but I didn't know where to add
it,
> > because syscall is frozen.
> 
> Well, we need access to DnsNameCompare. I am not clear what is going to
happens
> to syscall package, but for now lets put it in syscall. We’ll move it when
time
> comes.
> 
> > > src/pkg/net/lookup_windows.go:331: dnsSectionAnswer     = 1
> > > This belongs to syscall (it is defined by Microsoft). So make it
> > > syscall.DnsSectionAnswer. Put it into ztype_windows.go next to other DNS
> > related
> > > consts.
> > 
> > Isn't syscall frozen? I thought about this, but I was not sure whether to
put
> it
> > in there, or should it go to the go.sys package? Or should it go to some
> > internal pkg.
> 
> Lets put it all into syscall for now.
> 
> > If a DNS server has a CNAME record for a given domain it must not have any
> other
> > domains for it. But I guess making it two passes would make it clearer. But,
> > it's sufficient to resolve to the last CNAME and then use that to gather all
> the
> > types.
> 
> See, if you can make it simpler.
> 
> > > Why do you include records with dnsSectionAdditional set? I have removed
> > these,
> > > and your tests still PASS. Do you have a test that breaks when we do not
> > examine
> > > records with dnsSectionAdditional set?
> > 
> > Nope; but if CNAME-s are concerned then sometimes additional information is
> > returned in the additional section so that you wouldn't need to do
additional
> > queries. Of course Windows API probably already handles that section and
puts
> > relevant information to the Answer section, so it might not be necessary.
> 
> I still don’t see how records with dnsSectionAdditional are used here.
> 
> > > I don't know, if your logic here is correct. Should you be using
Microsoft's
> > > DnsNameCompare instead?
> > 
> > I believe the algorithm is the same and the syscall currently doesn't
contain
> > DnsNameCompare.
> > ReactOS code https://gist.github.com/egonelbre/a5a82c17cdec87c4dc7d somewhat
> > confirms that this is the same code.
> 
> Perhaps, but why guess if Microsoft provided a function to do the job.
> 
> > > It is fine to continue here, but, t.Logf error message returned from
> > nslookupMX,
> > > so at least user knows what happened - you would have to change nslookupMX
> to
> > > return error. Same for TestLookupNS and TestLookupTXT.
> > > 
> > 
> > Maybe t.Skipf("nslookup failed to return results: %s", err) would be better
> than
> > t.Logf?
> 
> No, lets not skip the test. Lets test as much as we can test. Regardless of
> nslookup present or fail to run for whatever reason. But when nslookup fails,
I
> would like to see failure shown, so I don’t kid myself that everything is
good.
> And I would like to see the error message (via t.Logf) so I can decide if
> something important is a miss.
> 
> > I currently didn't do any of the tests when "nslookup" fails. IMHO it's safe
> to
> > assume when nslookup is down, then talking to outside is down, so these
tests
> > would produce noise. Of course I could check that when 'nslookup' fails then
> the
> > LookupMX should fail. Or should I always fail when LookupMX fails?
> 
> Lets not make any assumptions about nslookup failures. And lets not link those
> to the failures of LookupMX. If LookupMX fails, fail the test. If nslookup
> succeeds, compare nsllokup results with LookupMX results, and fail test, if
> results are different.
> 
> Alex

PTAL, not sure about whether I was supposed to use all caps constants in
ztypes_windows.go or not.

Currently LookupCNAME doesn't document what it does, when there is no CNAME
record, and it seems unix and windows have different behavior. Should
LookupCNAME return an error when there is no CNAME record, or an empty string
without error, or the fqdn for the initial name? Also, while inspecting it, I
noticed in dnsclient_unix.go at line 432, 

func goLookupCNAME(name string) (cname string, err error) {
	_, rr, err := lookup(name, dnsTypeCNAME)
	if err != nil {
		return
	}
	cname = rr[0].(*dnsRR_CNAME).Cname
	return
}

This looks like it may blow up when len(rr) == 0.

The result checking code got a bit more complicated. I wasn't sure which one
would be better http://play.golang.org/p/JlGaQa0ybd. I currently went with
Version1, it also gets the nslookup results which should make debugging easier.

[brainman, 2014-08-12 05:23:16]

> 
> Currently LookupCNAME doesn't document what it does, when there is no CNAME
> record, and it seems unix and windows have different behavior. Should
> LookupCNAME return an error when there is no CNAME record, or an empty string
> without error, or the fqdn for the initial name? 

I don't know the answer to that question. I would create a new issue
https://code.google.com/p/go/issues/entry for that, see what others will say and
make change accordingly. In a separate CL.

> ... Also, while inspecting it, I
> noticed in dnsclient_unix.go at line 432, 
> 
> func goLookupCNAME(name string) (cname string, err error) {
> 	_, rr, err := lookup(name, dnsTypeCNAME)
> 	if err != nil {
> 		return
> 	}
> 	cname = rr[0].(*dnsRR_CNAME).Cname
> 	return
> }
> 
> This looks like it may blow up when len(rr) == 0.

I agree, it should check for len(rr) == 0. In a separate CL, if you like.

> The result checking code got a bit more complicated. I wasn't sure which one
> would be better http://play.golang.org/p/JlGaQa0ybd. I currently went with
> Version1, it also gets the nslookup results which should make debugging
easier.

How about something like that:

func TestLookupMX(t *testing.T) {
	if testing.Short() || !*testExternal {
		t.Skip("skipping test to avoid external network")
	}
	for _, server := range nslookupTestServers {
		mx, err := LookupMX(server)
		if err != nil {
			t.Errorf("failed %s: %s", server, err)
			continue
		}
		if len(mx) == 0 {
			t.Errorf("no results")
			continue
		}
		expected, err := nslookupMX(server)
		if err != nil {
			t.Logf("skipping failed nslookup %s test: %s", server, err)
			continue
		}
		sort.Sort(byPrefAndHost(expected))
		sort.Sort(byPrefAndHost(mx))
		if !reflect.DeepEqual(expected, mx) {
			t.Errorf("different results %s:\texp:%v\tgot:%v", server, toJson(expected),
toJson(mx))
		}
	}
}

?

It also seems that r is creating new go.sys repo right now. So we will have to
wait before he is done. Please, continue to make all your changes in syscall fro
the moment. But we will, probably, have to move some syscall related code before
we submit this CL.

Alex

https://codereview.appspot.com/122200043/diff/180001/src/pkg/net/lookup_windo...
File src/pkg/net/lookup_windows.go (right):

https://codereview.appspot.com/122200043/diff/180001/src/pkg/net/lookup_windo...
src/pkg/net/lookup_windows.go:335: syscall.DnsNameCompare(cname, p.Name) &&
Move syscall.DnsNameCompare check to be last. It is most expensive to run.

https://codereview.appspot.com/122200043/diff/180001/src/pkg/net/lookup_windo...
src/pkg/net/lookup_windows.go:336: p.Type == dnstype {
This is one long expression. Please break it like:

if p.Dw&syscall.DNS_SECTION_MASK != syscall.DNS_SECTION_ANSWER {
        continue
}
if p.Type != dnstype {
        continue
}
if !syscall.DnsNameCompare(cname, p.Name) {
        continue
}
rec = append(rec, p)

or similar.

https://codereview.appspot.com/122200043/diff/180001/src/pkg/net/lookup_windo...
src/pkg/net/lookup_windows.go:345: for cnameloop := 0; cnameloop < 10;
cnameloop++ {
I am still confused here. Why do you need cnameloop loop? If "found" is set to
true, then the cnameloop loop stops (see break at the end). But if "found" is
not changed, then why continue with cnameloop loop? The "name" variable will not
change, therefore the inner loop will do the same thing as before - we are
iterating over same list comparing them with the same "name" variable.

https://codereview.appspot.com/122200043/diff/180001/src/pkg/syscall/ztypes_w...
File src/pkg/syscall/ztypes_windows.go (right):

https://codereview.appspot.com/122200043/diff/180001/src/pkg/syscall/ztypes_w...
src/pkg/syscall/ztypes_windows.go:694: DNS_SECTION_MASK       = 0x0003
I cannot find that const defined anywhere on the web. Lets not have it exported
here.

https://codereview.appspot.com/122200043/diff/180001/src/pkg/syscall/ztypes_w...
src/pkg/syscall/ztypes_windows.go:695: DNS_SECTION_QUESTION   = 0x0000
s/DNS_SECTION_QUESTION/DnsSectionQuestion/

lets keep official names
http://msdn.microsoft.com/en-us/library/windows/desktop/ms682090(v=vs.85).aspx

https://codereview.appspot.com/122200043/diff/180001/src/pkg/syscall/ztypes_w...
src/pkg/syscall/ztypes_windows.go:696: DNS_SECTION_ANSWER     = 0x0001
s/DNS_SECTION_ANSWER/DnsSectionAnswer/

https://codereview.appspot.com/122200043/diff/180001/src/pkg/syscall/ztypes_w...
src/pkg/syscall/ztypes_windows.go:697: DNS_SECTION_AUTHORITY  = 0x0002
s/DNS_SECTION_AUTHORITY/DnsSectionAuthority/

https://codereview.appspot.com/122200043/diff/180001/src/pkg/syscall/ztypes_w...
src/pkg/syscall/ztypes_windows.go:698: DNS_SECTION_ADDITIONAL = 0x0003
s/DNS_SECTION_ADDITIONAL/DnsSectionAddtional/

[egon, 2014-08-12 05:51:46]

On 2014/08/12 05:23:16, brainman wrote:
> > 
> > Currently LookupCNAME doesn't document what it does, when there is no CNAME
> > record, and it seems unix and windows have different behavior. Should
> > LookupCNAME return an error when there is no CNAME record, or an empty
string
> > without error, or the fqdn for the initial name? 
> 
> I don't know the answer to that question. I would create a new issue
> https://code.google.com/p/go/issues/entry for that, see what others will say
and
> make change accordingly. In a separate CL.
> 
> > ... Also, while inspecting it, I
> > noticed in dnsclient_unix.go at line 432, 
> > 
> > func goLookupCNAME(name string) (cname string, err error) {
> > 	_, rr, err := lookup(name, dnsTypeCNAME)
> > 	if err != nil {
> > 		return
> > 	}
> > 	cname = rr[0].(*dnsRR_CNAME).Cname
> > 	return
> > }
> > 
> > This looks like it may blow up when len(rr) == 0.
> 
> I agree, it should check for len(rr) == 0. In a separate CL, if you like.

It makes sense to do that in the new issue, because I don't know what return
when len(rr) == 0.

> 
> > The result checking code got a bit more complicated. I wasn't sure which one
> > would be better http://play.golang.org/p/JlGaQa0ybd. I currently went with
> > Version1, it also gets the nslookup results which should make debugging
> easier.
> 
> How about something like that:
> 
> func TestLookupMX(t *testing.T) {
> 	if testing.Short() || !*testExternal {
> 		t.Skip("skipping test to avoid external network")
> 	}
> 	for _, server := range nslookupTestServers {
> 		mx, err := LookupMX(server)
> 		if err != nil {
> 			t.Errorf("failed %s: %s", server, err)
> 			continue
> 		}
> 		if len(mx) == 0 {
> 			t.Errorf("no results")
> 			continue
> 		}
> 		expected, err := nslookupMX(server)
> 		if err != nil {
> 			t.Logf("skipping failed nslookup %s test: %s", server, err)
> 			continue
> 		}
> 		sort.Sort(byPrefAndHost(expected))
> 		sort.Sort(byPrefAndHost(mx))
> 		if !reflect.DeepEqual(expected, mx) {
> 			t.Errorf("different results %s:\texp:%v\tgot:%v", server, toJson(expected),
> toJson(mx))
> 		}
> 	}
> }
> 
> ?
> 
> It also seems that r is creating new go.sys repo right now. So we will have to
> wait before he is done. Please, continue to make all your changes in syscall
fro
> the moment. But we will, probably, have to move some syscall related code
before
> we submit this CL.
> 
> Alex
> 
>
https://codereview.appspot.com/122200043/diff/180001/src/pkg/net/lookup_windo...
> File src/pkg/net/lookup_windows.go (right):
> 
>
https://codereview.appspot.com/122200043/diff/180001/src/pkg/net/lookup_windo...
> src/pkg/net/lookup_windows.go:335: syscall.DnsNameCompare(cname, p.Name) &&
> Move syscall.DnsNameCompare check to be last. It is most expensive to run.
> 
>
https://codereview.appspot.com/122200043/diff/180001/src/pkg/net/lookup_windo...
> src/pkg/net/lookup_windows.go:336: p.Type == dnstype {
> This is one long expression. Please break it like:
> 
> if p.Dw&syscall.DNS_SECTION_MASK != syscall.DNS_SECTION_ANSWER {
>         continue
> }
> if p.Type != dnstype {
>         continue
> }
> if !syscall.DnsNameCompare(cname, p.Name) {
>         continue
> }
> rec = append(rec, p)
> 
> or similar.
> 
>
https://codereview.appspot.com/122200043/diff/180001/src/pkg/net/lookup_windo...
> src/pkg/net/lookup_windows.go:345: for cnameloop := 0; cnameloop < 10;
> cnameloop++ {
> I am still confused here. Why do you need cnameloop loop? If "found" is set to
> true, then the cnameloop loop stops (see break at the end). But if "found" is
> not changed, then why continue with cnameloop loop? The "name" variable will
not
> change, therefore the inner loop will do the same thing as before - we are
> iterating over same list comparing them with the same "name" variable.

There can be multiple CNAME-s, e.g.

x CNAME IN y
y CNAME IN z
z CNAME IN w
w A IN 1.2.3.4

Most of DNS servers seem to return them in the correct order, although I have
not found something that guarantees that, therefore it's possible that you get:

z CNAME IN w
e A IN 1.2.3.4
y CNAME IN z
x CNAME IN y

This means that when I start a query for x and I encounter "x CNAME IN y", I
need to restart the cname resolving with "y".
But restarting from the beginning most of the time is unnecessary, because most
of the time they should be in correct order:

x CNAME IN y
y CNAME IN z
z CNAME IN w

So the code is saying: "If we don't find a new cname redirection we can stop the
resolving."

Also there's the possibility of DNS server returning a loop, which is why there
is the cnameloop limit.

x CNAME IN y
y CNAME IN z
z CNAME IN x

I guess it's possible to make a "map[string]string" and resolve the whole thing
in a single pass.


Of course now to think of it, the previous approach might have been easier to
understand, and the performance difference shouldn't be that big. e.g.

func resolveCNAME(name *uint16, r *syscall.DNSRecord) *uint16 {
Cname:
	for cnameloop := 0; cnameloop < 10; cnameloop++ {
		for p := r; p != nil; p = p.Next {
			if p.Dw&syscall.DNS_SECTION_MASK != syscall.DNS_SECTION_ANSWER {
				continue
			}
			if p.Type != syscall.DNS_TYPE_CNAME {
				continue
			}
			if syscall.DnsNameCompare(name, p.Name) {
				name = (*syscall.DNSPTRData)(unsafe.Pointer(&r.Data[0])).Host
				continue Cname
			}
		}
		break
	}
	return name
}

[brainman, 2014-08-12 06:53:39]

On 2014/08/12 05:51:46, egon wrote:
> ...
> Of course now to think of it, the previous approach might have been easier to
> understand, and the performance difference shouldn't be that big. e.g.
> 
> func resolveCNAME(name *uint16, r *syscall.DNSRecord) *uint16 {
> Cname:
> 	for cnameloop := 0; cnameloop < 10; cnameloop++ {
> 		for p := r; p != nil; p = p.Next {
> 			if p.Dw&syscall.DNS_SECTION_MASK != syscall.DNS_SECTION_ANSWER {
> 				continue
> 			}
> 			if p.Type != syscall.DNS_TYPE_CNAME {
> 				continue
> 			}
> 			if syscall.DnsNameCompare(name, p.Name) {
> 				name = (*syscall.DNSPTRData)(unsafe.Pointer(&r.Data[0])).Host
> 				continue Cname
> 			}
> 		}
> 		break
> 	}
> 	return name
> }

That sounds good to me, but, please, make it little bit more regular:

func resolveCNAME(name *uint16, r *syscall.DNSRecord) *uint16 {
Cname:
	for cnameloop := 0; loop < 10; cnameloop++ {
		for p := r; p != nil; p = p.Next {
			if p.Dw&syscall.DNS_SECTION_MASK != syscall.DNS_SECTION_ANSWER {
				continue
			}
			if p.Type != syscall.DNS_TYPE_CNAME {
				continue
			}
			if !syscall.DnsNameCompare(name, p.Name) {
				continue
			}
			name = (*syscall.DNSPTRData)(unsafe.Pointer(&r.Data[0])).Host
			continue Cname
		}
		break
	}
	return name
}

And add a comment to explain why you stop at 10 - about DNS server loop.

And thank you for explaining how it works.

Alex

[egon, 2014-08-14 08:56:08]

On 2014/08/12 06:53:39, brainman wrote:
> On 2014/08/12 05:51:46, egon wrote:
> > ...
> > Of course now to think of it, the previous approach might have been easier
to
> > understand, and the performance difference shouldn't be that big. e.g.
> > 
> > func resolveCNAME(name *uint16, r *syscall.DNSRecord) *uint16 {
> > Cname:
> > 	for cnameloop := 0; cnameloop < 10; cnameloop++ {
> > 		for p := r; p != nil; p = p.Next {
> > 			if p.Dw&syscall.DNS_SECTION_MASK != syscall.DNS_SECTION_ANSWER {
> > 				continue
> > 			}
> > 			if p.Type != syscall.DNS_TYPE_CNAME {
> > 				continue
> > 			}
> > 			if syscall.DnsNameCompare(name, p.Name) {
> > 				name = (*syscall.DNSPTRData)(unsafe.Pointer(&r.Data[0])).Host
> > 				continue Cname
> > 			}
> > 		}
> > 		break
> > 	}
> > 	return name
> > }
> 
> That sounds good to me, but, please, make it little bit more regular:
> 
> func resolveCNAME(name *uint16, r *syscall.DNSRecord) *uint16 {
> Cname:
> 	for cnameloop := 0; loop < 10; cnameloop++ {
> 		for p := r; p != nil; p = p.Next {
> 			if p.Dw&syscall.DNS_SECTION_MASK != syscall.DNS_SECTION_ANSWER {
> 				continue
> 			}
> 			if p.Type != syscall.DNS_TYPE_CNAME {
> 				continue
> 			}
> 			if !syscall.DnsNameCompare(name, p.Name) {
> 				continue
> 			}
> 			name = (*syscall.DNSPTRData)(unsafe.Pointer(&r.Data[0])).Host
> 			continue Cname
> 		}
> 		break
> 	}
> 	return name
> }
> 
> And add a comment to explain why you stop at 10 - about DNS server loop.
> 
> And thank you for explaining how it works.
> 
> Alex

PTAL, I also made the behavior of LookupCNAME the same as in Linux, not sure
whether it's appropriate to do this in this CL. The LookupCNAME issue is
reported here https://code.google.com/p/go/issues/detail?id=8516

To properly test LookupCNAME it would need some DNS entries for doing the
multiple redirections... e.g. could something like

dnstest2.golang.com. IN CNAME dnstest1.golang.com
dnstest1.golang.com. IN CNAME www.golang.com

be added to golang.com DNS configuration? I guess these tests can be added in
the issue 8516 as well.

+ egon

[brainman, 2014-08-14 12:32:59]

> 
> PTAL, ...

I will look tomorrow.

> I also made the behavior of LookupCNAME the same as in Linux, not sure
> whether it's appropriate to do this in this CL.

Sounds good.

> The LookupCNAME issue is
> reported here https://code.google.com/p/go/issues/detail?id=8516

Thank you for doing that.

> ... e.g. could something like
> 
> http://dnstest2.golang.com. IN CNAME http://dnstest1.golang.com
> http://dnstest1.golang.com. IN CNAME http://www.golang.com
> 
> be added to http://golang.com DNS configuration? ...

I don't think anyone would be willing to go to that length. We'll just have to
accept that our code is good.

Alex

[brainman, 2014-08-15 05:24:38]

LGTM

I understand you are not clear about LookupCNAME behaviour, but I think this CL
is a big improvement as is. You can address outstanding issues later.

Thank you very much. Please fix comment below, and I will submit.

Alex

https://codereview.appspot.com/122200043/diff/240001/src/pkg/net/lookup_windo...
File src/pkg/net/lookup_windows.go (right):

https://codereview.appspot.com/122200043/diff/240001/src/pkg/net/lookup_windo...
src/pkg/net/lookup_windows.go:213: if errno, ok := e.(syscall.Errno); ok &&
errno == syscall.DNS_INFO_NO_RECORDS {
This needs comment. I have no idea why you're doing that.

[egon, 2014-08-15 06:23:42]

On 2014/08/15 05:24:38, brainman wrote:
> LGTM
> 
> I understand you are not clear about LookupCNAME behaviour, but I think this
CL
> is a big improvement as is. You can address outstanding issues later.
> 
> Thank you very much. Please fix comment below, and I will submit.
> 
> Alex
> 
>
https://codereview.appspot.com/122200043/diff/240001/src/pkg/net/lookup_windo...
> File src/pkg/net/lookup_windows.go (right):
> 
>
https://codereview.appspot.com/122200043/diff/240001/src/pkg/net/lookup_windo...
> src/pkg/net/lookup_windows.go:213: if errno, ok := e.(syscall.Errno); ok &&
> errno == syscall.DNS_INFO_NO_RECORDS {
> This needs comment. I have no idea why you're doing that.

Done.

[brainman, 2014-08-15 06:37:29]

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

net: fix CNAME resolving on Windows

Fixes issue 8492

LGTM=alex.brainman
R=golang-codereviews, alex.brainman
CC=golang-codereviews
https://codereview.appspot.com/122200043

Committer: Alex Brainman <alex.brainman@gmail.com>

[gobot, 2014-08-15 07:18:11]

This CL appears to have broken the darwin-386-cheney builder.
See http://build.golang.org/log/4276fd7f8c75d29c007fe782cc92aa4df42bef72