code review 4066044: net: Add IPv4 Multicast support to UDPConn

net: Add IPv4 Multicast support to UDPConn

notes:
Darwin is very particular about joining a multicast group if the
listneing socket is not created in "udp4" mode, the other supported
OS's are more flexible.

A simple example sets up a socket to listen on the mdns/bonjour
group 224.0.0.251:5353

// ensure the sock is udp4, and the IP is a 4 byte IPv4
socket, err := net.ListenUDP("udp4", &net.UDPAddr {
        IP: net.IPv4zero,
        // currently darwin will not allow you to bind to
        // a port if it is already bound to another process
        Port: 5353,
})
if err != nil {
        log.Exitf("listen %s", err)
}
defer socket.Close()
err = socket.JoinGroup(net.IPv4(224, 0, 0, 251))
if err != nil {
        log.Exitf("join group %s", err)
}

[dave_cheney.net, 2011-01-27 12:21:16]

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

I'd like you to review this change.

[rsc, 2011-01-27 13:16:41]

> Support for multicast differs between darwin, linux and freebsd.

If that's the case then will the uses of package syscall
here work on all 3 systems?  If not, then the support routines
should be in fd_$GOOS.go instead of udpsock.go, so
that different code can be used on each.

Russ

[dave_cheney.net, 2011-01-27 21:40:20]

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

Please take another look.

[dave_cheney.net, 2011-01-27 21:46:13]

Hi Russ,

That might have been the wrong turn of phrase. What I mean to say is
darwin is more picky that linux and freebsd.

socket, err := net.ListenUDP("udp", &net.UDPAddr {
        IP: net.IPv4zero,
        Port: 5356,
})
socket.JoinGroup(net.IPv4(224, 0, 0, 251))

Will work fine under linux and freebsd but throw

2011/01/28 08:15:48 join group setsockopt: invalid argument

under darwin. Setting ListenUDP to "udp4" resolves this issue. I am
thinking that this is due to a difference in the network stack, and
some preference for IPv6 over IPv4. Looking at netstat, when opening
the socket in "udp" mode I get

lucky(~) % netstat -an | grep 5356
udp46      0      0  *.5356                 *.*

on linux/386/ubuntu10.10 I get

dave@odessa:~/devel/multicast$ netstat -an | grep 5356
udp6       0      0 :::5356                 :::*

yet it will respond to IPv4 multicast datagrams just fine (tested on
port 5353 to listen to mdns traffic)

Opening a "udp4" socket explicitly gives the following on darwin and
linux respectively

lucky(~/devel/multicast) % netstat -an | grep 5356
udp4       0      0  *.5356                 *.*

dave@odessa:~/devel/multicast$ netstat -an | grep 5356
udp        0      0 0.0.0.0:5356            0.0.0.0:*

I will reword the comment blocks to emphasise that the socket must be
udp4 and change the wording on the commit to add an explanation.

Cheers

Dave



On Fri, Jan 28, 2011 at 12:16 AM, Russ Cox <rsc@golang.org> wrote:
>> Support for multicast differs between darwin, linux and freebsd.
>
> If that's the case then will the uses of package syscall
> here work on all 3 systems?  If not, then the support routines
> should be in fd_$GOOS.go instead of udpsock.go, so
> that different code can be used on each.
>
> Russ
>

[rsc, 2011-02-01 16:41:53]

http://codereview.appspot.com/4066044/diff/5001/src/pkg/net/udpsock.go
File src/pkg/net/udpsock.go (right):

http://codereview.appspot.com/4066044/diff/5001/src/pkg/net/udpsock.go#newcod...
src/pkg/net/udpsock.go:285: // JoinGroup establishes a membership of an IPv4
Multicast group on UDPConn
// JoinGroup joins the IPv4 multicast group named by addr.
// The UDPConn must use the "udp4" network.

http://codereview.appspot.com/4066044/diff/5001/src/pkg/net/udpsock.go#newcod...
src/pkg/net/udpsock.go:295: return os.EINVAL
This should be a more detailed error, like:

return &OpError{"joingroup", "udp", &IPAddr{ip}, errInvalidMulticast}

where there is a global

var errInvalidMulticast = os.ErrorString("invalid IPv4 multicast address")

Same below in LeaveGroup.

http://codereview.appspot.com/4066044/diff/5001/src/pkg/net/udpsock.go#newcod...
src/pkg/net/udpsock.go:297: return os.NewSyscallError("setsockopt",
syscall.SetsockoptString(c.fd.sysfd, syscall.IPPROTO_IP,
syscall.IP_ADD_MEMBERSHIP, makeMReqString(ip)))
If there's an error it should turn into an OpError so that it can
record context about the call, same as in the invalid address case above.

Same below in LeaveGroup.

http://codereview.appspot.com/4066044/diff/5001/src/pkg/net/udpsock.go#newcod...
src/pkg/net/udpsock.go:300: // LeaveGroup will cancel an existing IPv4 Multicast
group membership
// LeaveGroup exits the IPv4 multicast group named by addr.

http://codereview.appspot.com/4066044/diff/5001/src/pkg/net/udpsock.go#newcod...
src/pkg/net/udpsock.go:315: // construct a string to pass to
syscall.SetsocketoptString
If there is a defined struct, we should use the struct.
This will require changing package syscall, probably.
Look at the way that SetsockoptLinger gets defined and used.

[dave_cheney.net, 2011-02-01 22:16:30]

>
http://codereview.appspot.com/4066044/diff/5001/src/pkg/net/udpsock.go#newcod...
> src/pkg/net/udpsock.go:285: // JoinGroup establishes a membership of an IPv4
> Multicast group on UDPConn
> // JoinGroup joins the IPv4 multicast group named by addr.
> // The UDPConn must use the "udp4" network.

Done

>
http://codereview.appspot.com/4066044/diff/5001/src/pkg/net/udpsock.go#newcod...
> src/pkg/net/udpsock.go:295: return os.EINVAL
> This should be a more detailed error, like:
> 
> return &OpError{"joingroup", "udp", &IPAddr{ip}, errInvalidMulticast}
> 
> where there is a global
> 
> var errInvalidMulticast = os.ErrorString("invalid IPv4 multicast address")
> 
> Same below in LeaveGroup.

Thanks, I was not aware of that pattern (I was trying to avoid importing "fmt"),
I will improve the error returned.

>
http://codereview.appspot.com/4066044/diff/5001/src/pkg/net/udpsock.go#newcod...
> src/pkg/net/udpsock.go:297: return os.NewSyscallError("setsockopt",
> syscall.SetsockoptString(c.fd.sysfd, syscall.IPPROTO_IP,
> syscall.IP_ADD_MEMBERSHIP, makeMReqString(ip)))
> If there's an error it should turn into an OpError so that it can
> record context about the call, same as in the invalid address case above.

I copied that pattern from UDPConn.BindToDevice, I will improve it if possible
(see below)

> Same below in LeaveGroup.
> 
>
http://codereview.appspot.com/4066044/diff/5001/src/pkg/net/udpsock.go#newcod...
> src/pkg/net/udpsock.go:300: // LeaveGroup will cancel an existing IPv4
Multicast
> group membership
> // LeaveGroup exits the IPv4 multicast group named by addr.

Done

>
http://codereview.appspot.com/4066044/diff/5001/src/pkg/net/udpsock.go#newcod...
> src/pkg/net/udpsock.go:315: // construct a string to pass to
> syscall.SetsocketoptString
> If there is a defined struct, we should use the struct.
> This will require changing package syscall, probably.
> Look at the way that SetsockoptLinger gets defined and used.

Yes, I have code for this and was intending to submit it as a follow up CL. I'll
include this in the next delta.

Thanks for your comments Russ.

[rsc, 2011-02-01 22:18:15]

>> src/pkg/net/udpsock.go:297: return os.NewSyscallError("setsockopt",
>> syscall.SetsockoptString(c.fd.sysfd, syscall.IPPROTO_IP,
>> syscall.IP_ADD_MEMBERSHIP, makeMReqString(ip)))
>> If there's an error it should turn into an OpError so that it can
>> record context about the call, same as in the invalid address case
>
> above.
>
> I copied that pattern from UDPConn.BindToDevice, I will improve it if
> possible (see below)

Thanks.  Feel free to fix BindToDevice if you're so inclined.  :-)

>> src/pkg/net/udpsock.go:315: // construct a string to pass to
>> syscall.SetsocketoptString
>> If there is a defined struct, we should use the struct.
>> This will require changing package syscall, probably.
>> Look at the way that SetsockoptLinger gets defined and used.
>
> Yes, I have code for this and was intending to submit it as a follow up
> CL. I'll include this in the next delta.

Thanks.
Russ

[dave_cheney.net, 2011-02-01 22:48:53]

> Yes, I have code for this and was intending to submit it as a follow up CL.
I'll
> include this in the next delta.

On reflection I think i'll submit the underlying changes to the syscall pkg as a
separate CL as the resulting diff is quite large due to regenerating all the
ztypes_*.go files.

[rsc, 2011-02-01 23:15:38]

> On reflection I think i'll submit the underlying changes to the syscall
> pkg as a separate CL as the resulting diff is quite large due to
> regenerating all the ztypes_*.go files.

okay.  if it looks like there are unrelated diffs
we should generate them on a different system.
the linux ones are ubuntu lucid and the darwin
ones are os x 10.6

[dave_cheney.net, 2011-02-01 23:49:11]

Will do, the major difference is godef has changed template for padded structs
which makes the diff look ugly. 

Dave


Sent from my iPhone

On 02/02/2011, at 10:15, Russ Cox <rsc@golang.org> wrote:

>> On reflection I think i'll submit the underlying changes to the syscall
>> pkg as a separate CL as the resulting diff is quite large due to
>> regenerating all the ztypes_*.go files.
> 
> okay.  if it looks like there are unrelated diffs
> we should generate them on a different system.
> the linux ones are ubuntu lucid and the darwin
> ones are os x 10.6

[dave_cheney.net, 2011-02-03 22:29:17]

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

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

[dave_cheney.net, 2011-02-03 22:30:18]

PTAL

http://codereview.appspot.com/4066044/diff/5001/src/pkg/net/udpsock.go
File src/pkg/net/udpsock.go (right):

http://codereview.appspot.com/4066044/diff/5001/src/pkg/net/udpsock.go#newcod...
src/pkg/net/udpsock.go:285: // JoinGroup establishes a membership of an IPv4
Multicast group on UDPConn
On 2011/02/01 16:41:54, rsc wrote:
> // JoinGroup joins the IPv4 multicast group named by addr.
> // The UDPConn must use the "udp4" network.

Done.

http://codereview.appspot.com/4066044/diff/5001/src/pkg/net/udpsock.go#newcod...
src/pkg/net/udpsock.go:295: return os.EINVAL
On 2011/02/01 16:41:54, rsc wrote:
> This should be a more detailed error, like:
> 
> return &OpError{"joingroup", "udp", &IPAddr{ip}, errInvalidMulticast}
> 
> where there is a global
> 
> var errInvalidMulticast = os.ErrorString("invalid IPv4 multicast address")
> 
> Same below in LeaveGroup.

Done.

http://codereview.appspot.com/4066044/diff/5001/src/pkg/net/udpsock.go#newcod...
src/pkg/net/udpsock.go:297: return os.NewSyscallError("setsockopt",
syscall.SetsockoptString(c.fd.sysfd, syscall.IPPROTO_IP,
syscall.IP_ADD_MEMBERSHIP, makeMReqString(ip)))
On 2011/02/01 16:41:54, rsc wrote:
> If there's an error it should turn into an OpError so that it can
> record context about the call, same as in the invalid address case above.
> 
> Same below in LeaveGroup.

Done.

http://codereview.appspot.com/4066044/diff/5001/src/pkg/net/udpsock.go#newcod...
src/pkg/net/udpsock.go:300: // LeaveGroup will cancel an existing IPv4 Multicast
group membership
On 2011/02/01 16:41:54, rsc wrote:
> // LeaveGroup exits the IPv4 multicast group named by addr.
> 

Done.

http://codereview.appspot.com/4066044/diff/5001/src/pkg/net/udpsock.go#newcod...
src/pkg/net/udpsock.go:315: // construct a string to pass to
syscall.SetsocketoptString
On 2011/02/01 16:41:54, rsc wrote:
> If there is a defined struct, we should use the struct.
> This will require changing package syscall, probably.
> Look at the way that SetsockoptLinger gets defined and used.

Done. r 7f8ffd20a1

[dave_cheney.net, 2011-02-10 23:32:56]

PTAL

[rsc, 2011-02-11 19:38:40]

looks good

http://codereview.appspot.com/4066044/diff/18002/src/pkg/net/multicast_test.go
File src/pkg/net/multicast_test.go (right):

http://codereview.appspot.com/4066044/diff/18002/src/pkg/net/multicast_test.g...
src/pkg/net/multicast_test.go:17: socket, err := ListenUDP("udp4", addr)
s/socket/conn/

not all the world is sockets

http://codereview.appspot.com/4066044/diff/18002/src/pkg/net/multicast_test.g...
src/pkg/net/multicast_test.go:19: t.Fatalf("Unable to bind to %s: %s", addr,
err)
t.Fatal(err)

ListenUDP will say what it was doing in the error

http://codereview.appspot.com/4066044/diff/18002/src/pkg/net/multicast_test.g...
src/pkg/net/multicast_test.go:27: t.Fatalf("Unable to join group %s: %s", addr,
err)
t.Fatal(err)

http://codereview.appspot.com/4066044/diff/18002/src/pkg/net/multicast_test.g...
src/pkg/net/multicast_test.go:33: t.Fatalf("Unable to leave group %s: %s", addr,
err)
t.Fatal(err)

http://codereview.appspot.com/4066044/diff/18002/src/pkg/net/multicast_test.g...
src/pkg/net/multicast_test.go:46: t.Fatalf("Unable to bind to %s: %s", addr,
err)
t.Fatal(err)

http://codereview.appspot.com/4066044/diff/18002/src/pkg/net/multicast_test.g...
src/pkg/net/multicast_test.go:53: t.Fatalf("Should fail")
t.Fatal("JoinGroup succeeded, should fail")

http://codereview.appspot.com/4066044/diff/18002/src/pkg/net/udpsock.go
File src/pkg/net/udpsock.go (right):

http://codereview.appspot.com/4066044/diff/18002/src/pkg/net/udpsock.go#newco...
src/pkg/net/udpsock.go:283: // Multicast specific methods.
delete

http://codereview.appspot.com/4066044/diff/18002/src/pkg/net/udpsock.go#newco...
src/pkg/net/udpsock.go:299: Interface: [4]byte{0, 0, 0, 0},
can delete since zero value is implied

http://codereview.appspot.com/4066044/diff/18002/src/pkg/net/udpsock.go#newco...
src/pkg/net/udpsock.go:319: Interface: [4]byte{0, 0, 0, 0},
can delete

[dave_cheney.net, 2011-02-12 01:28:40]

Thanks Russ, please take another look

http://codereview.appspot.com/4066044/diff/18002/src/pkg/net/multicast_test.go
File src/pkg/net/multicast_test.go (right):

http://codereview.appspot.com/4066044/diff/18002/src/pkg/net/multicast_test.g...
src/pkg/net/multicast_test.go:17: socket, err := ListenUDP("udp4", addr)
On 2011/02/11 19:38:40, rsc wrote:
> s/socket/conn/
> 
> not all the world is sockets

Done.

http://codereview.appspot.com/4066044/diff/18002/src/pkg/net/multicast_test.g...
src/pkg/net/multicast_test.go:19: t.Fatalf("Unable to bind to %s: %s", addr,
err)
On 2011/02/11 19:38:40, rsc wrote:
> t.Fatal(err)
> 
> ListenUDP will say what it was doing in the error

Done.

http://codereview.appspot.com/4066044/diff/18002/src/pkg/net/multicast_test.g...
src/pkg/net/multicast_test.go:27: t.Fatalf("Unable to join group %s: %s", addr,
err)
On 2011/02/11 19:38:40, rsc wrote:
> t.Fatal(err)

Done.

http://codereview.appspot.com/4066044/diff/18002/src/pkg/net/multicast_test.g...
src/pkg/net/multicast_test.go:33: t.Fatalf("Unable to leave group %s: %s", addr,
err)
On 2011/02/11 19:38:40, rsc wrote:
> t.Fatal(err)

Done.

http://codereview.appspot.com/4066044/diff/18002/src/pkg/net/multicast_test.g...
src/pkg/net/multicast_test.go:46: t.Fatalf("Unable to bind to %s: %s", addr,
err)
On 2011/02/11 19:38:40, rsc wrote:
> t.Fatal(err)

Done.

http://codereview.appspot.com/4066044/diff/18002/src/pkg/net/multicast_test.g...
src/pkg/net/multicast_test.go:53: t.Fatalf("Should fail")
On 2011/02/11 19:38:40, rsc wrote:
> t.Fatal("JoinGroup succeeded, should fail")

Done.

http://codereview.appspot.com/4066044/diff/18002/src/pkg/net/udpsock.go
File src/pkg/net/udpsock.go (right):

http://codereview.appspot.com/4066044/diff/18002/src/pkg/net/udpsock.go#newco...
src/pkg/net/udpsock.go:283: // Multicast specific methods.
On 2011/02/11 19:38:40, rsc wrote:
> delete

Done.

http://codereview.appspot.com/4066044/diff/18002/src/pkg/net/udpsock.go#newco...
src/pkg/net/udpsock.go:299: Interface: [4]byte{0, 0, 0, 0},
On 2011/02/11 19:38:40, rsc wrote:
> can delete since zero value is implied

Done.

http://codereview.appspot.com/4066044/diff/18002/src/pkg/net/udpsock.go#newco...
src/pkg/net/udpsock.go:319: Interface: [4]byte{0, 0, 0, 0},
On 2011/02/11 19:38:40, rsc wrote:
> can delete

Done.

[dave_cheney.net, 2011-02-12 01:29:22]

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

Please take another look.

[rsc, 2011-02-16 20:06:41]

LGTM

[rsc, 2011-02-16 20:07:15]

*** Submitted as 4892f94182a5 ***

net: add IPv4 multicast to UDPConn

notes:
Darwin is very particular about joining a multicast group if the
listneing socket is not created in "udp4" mode, the other supported
OS's are more flexible.

A simple example sets up a socket to listen on the mdns/bonjour
group 224.0.0.251:5353

// ensure the sock is udp4, and the IP is a 4 byte IPv4
socket, err := net.ListenUDP("udp4", &net.UDPAddr {
        IP: net.IPv4zero,
        // currently darwin will not allow you to bind to
        // a port if it is already bound to another process
        Port: 5353,
})
if err != nil {
        log.Exitf("listen %s", err)
}
defer socket.Close()
err = socket.JoinGroup(net.IPv4(224, 0, 0, 251))
if err != nil {
        log.Exitf("join group %s", err)
}

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

Committer: Russ Cox <rsc@golang.org>