|
|
Created:
13 years, 1 month ago by dfc Modified:
13 years, 1 month ago Reviewers:
CC:
rsc, mikio, rsc1, dho, golang-dev Visibility:
Public. |
Descriptionnet: fix multicast tests
Patch Set 1 #Patch Set 2 : diff -r f814f61fdbab https://go.googlecode.com/hg/ #
Total comments: 4
Patch Set 3 : diff -r afc3d9f30d78 https://go.googlecode.com/hg/ #Patch Set 4 : diff -r afc3d9f30d78 https://go.googlecode.com/hg/ #Patch Set 5 : diff -r 8e157f1abc87 https://go.googlecode.com/hg/ #Patch Set 6 : diff -r 8e157f1abc87 https://go.googlecode.com/hg/ #MessagesTotal messages: 17
http://codereview.appspot.com/4174056/diff/2001/src/pkg/net/multicast_test.go File src/pkg/net/multicast_test.go (right): http://codereview.appspot.com/4174056/diff/2001/src/pkg/net/multicast_test.go... src/pkg/net/multicast_test.go:28: mcast := IPv4(224, 7, 7, 7) 224.0.1/24 is internet control block, I mean, not link-local. I would recommend you to read both RFC5771 and RFC2365. http://codereview.appspot.com/4174056/diff/2001/src/pkg/net/multicast_test.go... src/pkg/net/multicast_test.go:36: if err != nil { In this test case, I think we might ignore EADDRNOTAVAIL error with t.Logf, instead of t.Fatal.
Sign in to reply to this message.
I don't know anything about multicast addresses. If you guys can come to an agreement about which address to use, I'll submit the change. Just let me know.
Sign in to reply to this message.
On Thu, Feb 17, 2011 at 9:43 AM, <rsc@google.com> wrote: > If you guys can come to an agreement about which > address to use, I'll submit the change. Just let me know. I think 224.0.0.254 is appropriate. If we need more IPv4 multicast addresses for testing, then try admin-scoped range, e.g., 239.255.0.0/16. -- RFC4727: Experimental Values in IPv4, IPv6, ICMPv4, ICMPv6, UDP, and TCP Headers -- 2.4.2. IPv4 Multicast The globally routable group 224.0.1.20 is set aside for experimentation. For certain experiments, the administratively scoped multicast groups defined in [RFC2365] may be useful. This document assigns a single link-local scoped group, 224.0.0.254, and a single scope-relative group, 254.
Sign in to reply to this message.
So is the correct change 224.0.0.251 -> 224.0.0.254 ? Was there a reason the original code said 224.0.0.251? Russ
Sign in to reply to this message.
On Thu, Feb 17, 2011 at 10:33 AM, Russ Cox <rsc@google.com> wrote: > So is the correct change > 224.0.0.251 -> 224.0.0.254 > ? I think so. But the reason why test fail is still unclear. I just dug /usr/src/sys/netinet/in_mcast.c and only clear thing to me is that seems like LeaveGroup requested address is not found in IPv4 multicast address membership list. AddGroup silently failed? Deferred sockclose flush membership before setsockopt w/ DROP_MEMBERSHIP request? I'm not sure how that happened. -- Mikio
Sign in to reply to this message.
I can't reproduce it on a VM, and the freebsd/386 build passes with the same code. I will update the CL to use 224.0.0.254 and see if Devon can test locally. On Fri, Feb 18, 2011 at 2:58 AM, Mikio Hara <mikioh.mikioh@gmail.com> wrote: > On Thu, Feb 17, 2011 at 10:33 AM, Russ Cox <rsc@google.com> wrote: > >> So is the correct change >> 224.0.0.251 -> 224.0.0.254 >> ? > > I think so. > > But the reason why test fail is still unclear. I just dug > /usr/src/sys/netinet/in_mcast.c > and only clear thing to me is that seems like LeaveGroup requested > address is not found > in IPv4 multicast address membership list. > > AddGroup silently failed? > Deferred sockclose flush membership before setsockopt w/ > DROP_MEMBERSHIP request? > I'm not sure how that happened. > > -- Mikio >
Sign in to reply to this message.
Mikioh, RSC, please take another look. I've asked Devon to try this patch directly to see if it fixes the issue. http://codereview.appspot.com/4174056/diff/2001/src/pkg/net/multicast_test.go File src/pkg/net/multicast_test.go (right): http://codereview.appspot.com/4174056/diff/2001/src/pkg/net/multicast_test.go... src/pkg/net/multicast_test.go:28: mcast := IPv4(224, 7, 7, 7) On 2011/02/17 13:16:39, mikioh wrote: > 224.0.1/24 is internet control block, I mean, not link-local. I would recommend > you to read both RFC5771 and RFC2365. Thank you, i've changed this to 224.0.0.254 as per your advice. http://codereview.appspot.com/4174056/diff/2001/src/pkg/net/multicast_test.go... src/pkg/net/multicast_test.go:36: if err != nil { On 2011/02/17 13:16:39, mikioh wrote: > In this test case, I think we might ignore EADDRNOTAVAIL error with t.Logf, > instead of t.Fatal. Or just remove this part of the test completely, as we're closing the connection on return from the method.
Sign in to reply to this message.
Hi guys, I'll be able to test this momentarily when I get home... --dho 2011/2/17 <dave@cheney.net>: > Mikioh, RSC, please take another look. > > I've asked Devon to try this patch directly to see if it fixes the > issue. > > > http://codereview.appspot.com/4174056/diff/2001/src/pkg/net/multicast_test.go > File src/pkg/net/multicast_test.go (right): > > http://codereview.appspot.com/4174056/diff/2001/src/pkg/net/multicast_test.go... > src/pkg/net/multicast_test.go:28: mcast := IPv4(224, 7, 7, 7) > On 2011/02/17 13:16:39, mikioh wrote: >> >> 224.0.1/24 is internet control block, I mean, not link-local. I would > > recommend >> >> you to read both RFC5771 and RFC2365. > > Thank you, i've changed this to 224.0.0.254 as per your advice. > > http://codereview.appspot.com/4174056/diff/2001/src/pkg/net/multicast_test.go... > src/pkg/net/multicast_test.go:36: if err != nil { > On 2011/02/17 13:16:39, mikioh wrote: >> >> In this test case, I think we might ignore EADDRNOTAVAIL error with > > t.Logf, >> >> instead of t.Fatal. > > Or just remove this part of the test completely, as we're closing the > connection on return from the method. > > http://codereview.appspot.com/4174056/ >
Sign in to reply to this message.
So, I applied the requested patch, but it seems to only fix Makefiles. I tried changing the address to 224.0.0.254, but I still get "can't assign requested address." I do notice that there is something in my netstat -ia output that says "all-systems.mcast" -- but I don't know what would do that. I don't use mcast knowingly and I'm probably about as far out in the dark with it as rsc is, even given my FreeBSD knowledge :\ I can do some ssh tunneling magic and let you check out the system if you'd like. --dho 2011/2/17 Devon H. O'Dell <devon.odell@gmail.com>: > Hi guys, > > I'll be able to test this momentarily when I get home... > > --dho > > 2011/2/17 <dave@cheney.net>: >> Mikioh, RSC, please take another look. >> >> I've asked Devon to try this patch directly to see if it fixes the >> issue. >> >> >> http://codereview.appspot.com/4174056/diff/2001/src/pkg/net/multicast_test.go >> File src/pkg/net/multicast_test.go (right): >> >> http://codereview.appspot.com/4174056/diff/2001/src/pkg/net/multicast_test.go... >> src/pkg/net/multicast_test.go:28: mcast := IPv4(224, 7, 7, 7) >> On 2011/02/17 13:16:39, mikioh wrote: >>> >>> 224.0.1/24 is internet control block, I mean, not link-local. I would >> >> recommend >>> >>> you to read both RFC5771 and RFC2365. >> >> Thank you, i've changed this to 224.0.0.254 as per your advice. >> >> http://codereview.appspot.com/4174056/diff/2001/src/pkg/net/multicast_test.go... >> src/pkg/net/multicast_test.go:36: if err != nil { >> On 2011/02/17 13:16:39, mikioh wrote: >>> >>> In this test case, I think we might ignore EADDRNOTAVAIL error with >> >> t.Logf, >>> >>> instead of t.Fatal. >> >> Or just remove this part of the test completely, as we're closing the >> connection on return from the method. >> >> http://codereview.appspot.com/4174056/ >> >
Sign in to reply to this message.
Sorry, that may have been my mistake, I may have given you the wrong cl number. Can you pls try again with hg clpatch 4174056 As I added a slight delay between the join and leave operations. Sent from my iPhone On 18/02/2011, at 11:44, "Devon H. O'Dell" <devon.odell@gmail.com> wrote: > So, I applied the requested patch, but it seems to only fix Makefiles. > I tried changing the address to 224.0.0.254, but I still get "can't > assign requested address." > > I do notice that there is something in my netstat -ia output that says > "all-systems.mcast" -- but I don't know what would do that. I don't > use mcast knowingly and I'm probably about as far out in the dark with > it as rsc is, even given my FreeBSD knowledge :\ > > I can do some ssh tunneling magic and let you check out the system if > you'd like. > > --dho > > 2011/2/17 Devon H. O'Dell <devon.odell@gmail.com>: >> Hi guys, >> >> I'll be able to test this momentarily when I get home... >> >> --dho >> >> 2011/2/17 <dave@cheney.net>: >>> Mikioh, RSC, please take another look. >>> >>> I've asked Devon to try this patch directly to see if it fixes the >>> issue. >>> >>> >>> http://codereview.appspot.com/4174056/diff/2001/src/pkg/net/multicast_test.go >>> File src/pkg/net/multicast_test.go (right): >>> >>> http://codereview.appspot.com/4174056/diff/2001/src/pkg/net/multicast_test.go... >>> src/pkg/net/multicast_test.go:28: mcast := IPv4(224, 7, 7, 7) >>> On 2011/02/17 13:16:39, mikioh wrote: >>>> >>>> 224.0.1/24 is internet control block, I mean, not link-local. I would >>> >>> recommend >>>> >>>> you to read both RFC5771 and RFC2365. >>> >>> Thank you, i've changed this to 224.0.0.254 as per your advice. >>> >>> http://codereview.appspot.com/4174056/diff/2001/src/pkg/net/multicast_test.go... >>> src/pkg/net/multicast_test.go:36: if err != nil { >>> On 2011/02/17 13:16:39, mikioh wrote: >>>> >>>> In this test case, I think we might ignore EADDRNOTAVAIL error with >>> >>> t.Logf, >>>> >>>> instead of t.Fatal. >>> >>> Or just remove this part of the test completely, as we're closing the >>> connection on return from the method. >>> >>> http://codereview.appspot.com/4174056/ >>> >>
Sign in to reply to this message.
According to dho the last patch did not fix the issue. I have changed the test to not run by default on freebsd as well as changing the multicast address to the suggested one by mikioh. Please take another look
Sign in to reply to this message.
On 2011/02/18 01:58:02, dfc wrote: > According to dho the last patch did not fix the issue. I have changed the test > to not run by default on freebsd as well as changing the multicast address to > the suggested one by mikioh. > > Please take another look Seems like it was an environmental issue here. The machine in question was running 8.0-STABLE from sometime in 2009. Updating my tree and rebuilding seems to have fixed the issue. I think it's unlikely that people will hit this unless it's a problem with 8.0, and we're not seeing any bug reports.
Sign in to reply to this message.
Please re-enable the test and delete the flag, since we fixed FreeBSD.
Sign in to reply to this message.
Please take another look. This final patchset updates the mcast address, but leaves the test enabled for all builds.
Sign in to reply to this message.
LGTM Thanks.
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
*** Submitted as http://code.google.com/p/go/source/detail?r=3ccbde90814f *** net: fix multicast tests R=rsc, mikioh, rsc1, dho CC=golang-dev http://codereview.appspot.com/4174056 Committer: Russ Cox <rsc@golang.org>
Sign in to reply to this message.
|