code review 2331044: syscall, net: Add Recvmsg and Sendmsg on Linux.

syscall, net: Add Recvmsg and Sendmsg on Linux.

Working on issue 1101.

[albert.strasheim, 2010-10-04 12:00:35]

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

I'd like you to review this change.

[albert.strasheim, 2010-10-07 21:42:00]

Ping? Thanks.

[albert.strasheim, 2010-10-08 07:52:48]

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

Please take another look.

[albert.strasheim, 2010-10-12 06:44:41]

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

Please take another look.

[albert.strasheim, 2010-10-12 07:06:27]

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

Please take another look.

[albert.strasheim, 2010-10-12 08:32:34]

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

Please take another look.

[albert.strasheim, 2010-10-12 08:34:26]

Hello all

I've started writing tests to check the implementation of this API, but I would
really appreciate some comments on the general design.

Thanks!

Regards

Albert

[rsc, 2010-10-12 13:23:15]

> I've started writing tests to check the implementation of this API, but
> I would really appreciate some comments on the general design.

I guess it looks fine.
(I'm disappointed by how ugly it all is,
but it's not your code: it's the API that
is forced on you by the awful socket interface.)

Please don't use the hungarian notation
for the extra data: ancil is better than bancil or pancil,
though I think oob (out of band) might be better
than all of them.

Can we not expose MSG_* in package net?

Finally, I don't believe that this is general
enough to warrant changing net.PacketConn.
For now at least I would just write the specific
methods on the concrete types and let callers
be clear about which protocol they're trying to
use.  I am skeptical that sending oob data
generalizes enough that you wouldn't have
to tailor it to the specific protocol.

Thanks for looking into this.  I know it's
a royal pain.

Russ

[albert.strasheim, 2010-10-12 16:26:46]

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

Please take another look.

[albert.strasheim, 2010-10-12 16:31:45]

Hello all

On 2010/10/12 16:26:46, albert.strasheim wrote:
> Hello rsc (cc: mailto:golang-dev@googlegroups.com),
> Please take another look.

Russ, I think I've incorporated all your feedback.

The latest patch focuses on Unix sockets.

Next step is to convert the CMSG_SPACE, CMSG_LEN and CMSG_DATA macros to Go
code, after which everything will be in place to do the high-level API and
tests.

I have written some tests here already, and things are working nicely.

I borrowed the trick of always sending at least 1 normal byte from here:

http://svn.python.org/projects/python/branches/py3k/Modules/_multiprocessing/...

recvmsg doesn't receive messages containing only ancillary data without this, as
AGL pointed out a while ago.

Regards

Albert

[rsc1, 2010-10-12 17:55:33]

Looking pretty good.

http://codereview.appspot.com/2331044/diff/28001/src/pkg/net/fd.go
File src/pkg/net/fd.go (right):

http://codereview.appspot.com/2331044/diff/28001/src/pkg/net/fd.go#newcode404
src/pkg/net/fd.go:404: func (fd *netFD) Recvmsg(p []byte, oob []byte) (n, oobn,
flags int, sa syscall.Sockaddr, err os.Error) {
I wonder if these should be ReadMsg and WriteMsg.
Recvfrom and Sendto are ReadFrom and WriteTo.
I think that makes sense.

http://codereview.appspot.com/2331044/diff/28001/src/pkg/net/unixsock.go
File src/pkg/net/unixsock.go (right):

http://codereview.appspot.com/2331044/diff/28001/src/pkg/net/unixsock.go#newc...
src/pkg/net/unixsock.go:15: MSG_TRUNC       = syscall.MSG_TRUNC
I really don't want to expose these constants.
They seem like a per-OS detail more than a general net API.
What are they for?  Can we make people import syscall 
if they need them?

http://codereview.appspot.com/2331044/diff/28001/src/pkg/net/unixsock.go#newc...
src/pkg/net/unixsock.go:288: func (c *UnixConn) RecvmsgUnix(b, oob []byte) (n,
oobn, flags int, addr *UnixAddr, err os.Error) {
so then this would be ReadMsgUnix and WriteMsgUnix.

http://codereview.appspot.com/2331044/diff/28001/src/pkg/syscall/syscall_linu...
File src/pkg/syscall/syscall_linux.go (right):

http://codereview.appspot.com/2331044/diff/28001/src/pkg/syscall/syscall_linu...
src/pkg/syscall/syscall_linux.go:499: if p != nil && len(p) > 0 {
if len(p) > 0 {

is enough.  if p == nil then len(p) == 0.

http://codereview.appspot.com/2331044/diff/28001/src/pkg/syscall/syscall_linu...
src/pkg/syscall/syscall_linux.go:503: var dummy byte
might as well put this where it gets used (inside the if len(p) == 0 )

http://codereview.appspot.com/2331044/diff/28001/src/pkg/syscall/syscall_linu...
src/pkg/syscall/syscall_linux.go:504: if oob != nil && len(oob) > 0 {
if len(oob) > 0 {

http://codereview.appspot.com/2331044/diff/28001/src/pkg/syscall/syscall_linu...
src/pkg/syscall/syscall_linux.go:506: if p == nil || len(p) == 0 {
if len(p) == 0 {

[rsc, 2010-10-12 18:23:12]

> I borrowed the trick of always sending at least 1 normal byte from here:
>
>
http://svn.python.org/projects/python/branches/py3k/Modules/_multiprocessing/...

Yes, that's what everyone resorts to.  Sigh.

Russ

[albert.strasheim, 2010-10-13 06:28:56]

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

Please take another look.

[albert.strasheim, 2010-10-13 15:30:31]

Hello

I've been doing more tests.

Interesting things happen when one side closes the socket or gets killed while
the other side is in recvmsg.

From the Linux manual page:

"These calls return the number of bytes received, or -1 if an error occurred. 
The return value will be 0 when the peer has performed an orderly shutdown."

The Solaris man page says the same thing.

This is probably related to the need for the 1-byte read buffer.

A check for this situation probably needs to go into fd.go/ReadMsg.

I'm thinking something like:

if n == 0 { oserr = os.EOF }

but thoughts would be appreciated.

Regards

Albert

[albert.strasheim, 2010-10-14 05:39:17]

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

Please take another look.

[albert.strasheim, 2010-10-20 09:11:19]

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

Please take another look.

[albert.strasheim, 2010-10-27 16:11:16]

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

Please take another look.

[albert.strasheim, 2010-10-27 16:13:29]

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

Please take another look.

[rsc1, 2010-11-01 21:40:20]

http://codereview.appspot.com/2331044/diff/53001/src/pkg/net/unixsock_test.go
File src/pkg/net/unixsock_test.go (right):

http://codereview.appspot.com/2331044/diff/53001/src/pkg/net/unixsock_test.go...
src/pkg/net/unixsock_test.go:15: addr := "@unixsock_test"
This will only work on Linux.
if runtime.GOOS != "linux" { return }

http://codereview.appspot.com/2331044/diff/53001/src/pkg/syscall/mkerrors.sh
File src/pkg/syscall/mkerrors.sh (right):

http://codereview.appspot.com/2331044/diff/53001/src/pkg/syscall/mkerrors.sh#...
src/pkg/syscall/mkerrors.sh:175: $GCC $ccflags -o _errors _errors.c && $GORUN
./_errors
unedit

http://codereview.appspot.com/2331044/diff/53001/src/pkg/syscall/syscall_linu...
File src/pkg/syscall/syscall_linux.go (right):

http://codereview.appspot.com/2331044/diff/53001/src/pkg/syscall/syscall_linu...
src/pkg/syscall/syscall_linux.go:450: func Recvmsg(fd int, p, oob []byte, from
Sockaddr, flags int) (n, oobn int, recvflags int, errno int) {
These are fine for Linux but in order to not break the build we need
implementations for the others too.  Please add to syscall_bsd.go:



func Recvmsg(fd int, p, oob []byte, from Sockaddr, flags int) (n, oobn int,
recvflags int errno int) {
	return 0, 0, 0, EAFNOSUPPORT
}

func Sendmsg(fd int, p, oob []byte, to Sockaddr, flags int) (errno int) {
	return EAFNOSUPPORT
}

[albert.strasheim, 2010-11-02 08:13:15]

Hello

On 2010/11/01 21:40:20, rsc1 wrote:
> http://codereview.appspot.com/2331044/diff/53001/src/pkg/net/unixsock_test.go
> File src/pkg/net/unixsock_test.go (right):
> 
>
http://codereview.appspot.com/2331044/diff/53001/src/pkg/net/unixsock_test.go...
> src/pkg/net/unixsock_test.go:15: addr := "@unixsock_test"
> This will only work on Linux.
> if runtime.GOOS != "linux" { return }

Unfortunately, it's not as simple as this.

On darwin, the test fails to compile with:

unixsock_test.go:33: undefined: syscall.SO_PASSCRED
unixsock_test.go:52: undefined: syscall.SCM_CREDENTIALS
unixsock_test.go:53: cmsg.SetLen undefined (type syscall.Cmsghdr has no field or
method SetLen)
unixsock_test.go:54: undefined: syscall.Ucred

It seems SCM_CREDENTIALS on Linux is called SCM_CREDS on BSD.

Also, it seems SO_PASSCRED on Linux is called LOCAL_PEERCRED on BSD.

How do we handle this situation?

We could leave out the test for now and write the same test on top of the
high-level API to come that could test both the Linux and BSD implementations.

Or gotest needs to be enhanced to understand about only compiling certain tests
for certain values of GOOS...

Regards

Albert

[rsc, 2010-11-04 17:27:05]

I would add

const (
    SCM_CREDENTIALS = SCM_CREDS
    SO_PASSCRED = LOCAL_PASSCRED
)

to syscall_bsd.go.

Russ

[albert.strasheim, 2010-11-05 12:25:07]

Hello Russ

On 2010/11/04 17:27:05, rsc wrote:
> I would add
> const (
>     SCM_CREDENTIALS = SCM_CREDS
>     SO_PASSCRED = LOCAL_PASSCRED
> )
> to syscall_bsd.go.

struct Ucred is also completely different.

I think the best option would be to leave out unixsock_test.go from this
changeset, since it really is only a Linux test.

Once we have this Linux changeset merged, I'll do the same work on Mac, and then
write a higher-level test for both.

Regards

Albert

[albert.strasheim, 2010-11-05 12:42:30]

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

Please take another look.

[rsc1, 2010-11-05 19:03:33]

I'm trying to figure out where all the new non-SIO constants came from
in the zerrors files.  What Linux distribution did you generate the
file using?  We're trying to use Ubuntu 10.04 for them right now.

[albert.strasheim, 2010-11-07 10:14:36]

Hello Russ

On 2010/11/05 19:03:33, rsc1 wrote:
> I'm trying to figure out where all the new non-SIO constants came from
> in the zerrors files.  What Linux distribution did you generate the
> file using?  We're trying to use Ubuntu 10.04 for them right now.

I'm on Fedora 14, which has a new kernel and glibc.

Finding Ubuntu is a bit tricky here. Maybe the person doing the commit could
just run mkall.sh? If that isn't possible, I'll look into installing Ubuntu on a
VM.

Regards

Albert

[albert.strasheim, 2010-11-08 07:51:32]

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

Please take another look.

[albert.strasheim, 2010-11-08 07:52:57]

Hello Russ

On 2010/11/08 07:51:32, albert.strasheim wrote:
> Hello rsc (cc: mailto:golang-dev@googlegroups.com),
> 
> Please take another look.

Synced in the fd dup stuff and did mkall.sh on Ubuntu 10.4.

Regards

Albert

[rsc, 2010-12-07 18:36:27]

LGTM

Testing on OS X and then will submit.

[rsc, 2010-12-07 18:40:18]

*** Submitted as 793cb11665ac ***

syscall, net: Add Recvmsg and Sendmsg on Linux.

Working on issue 1101.

R=rsc
CC=golang-dev
http://codereview.appspot.com/2331044

Committer: Russ Cox <rsc@golang.org>