code review 4667066: syscall: Parse and encode SCM_RIGHTS and SCM_CREDENTIALS.

syscall: Parse and encode SCM_RIGHTS and SCM_CREDENTIALS.

[albert.strasheim, 2011-07-07 10:11:46]

Hello rsc@golang.org, agl@golang.org, mikioh.mikioh@gmail.com (cc:
golang-dev@googlegroups.com),

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

[albert.strasheim, 2011-07-07 10:14:12]

Hello all

On 2011/07/07 10:11:46, albert.strasheim wrote:
> Hello mailto:rsc@golang.org, mailto:agl@golang.org,
mailto:mikioh.mikioh@gmail.com (cc:
> mailto:golang-dev@googlegroups.com),
> 
> I'd like you to review this change to
> https://go.googlecode.com/hg/

I exported CmsgLen and CmsgSpace so that code that receives these messages can
use them to calculate a receive buffer size.

I wrote a test too, but there is a circular dependency trying to use gotest in
the syscall package. It might be useful if gotest could allow Test functions
without the *testing.T parameter.

Regards

Albert

[mikio, 2011-07-07 12:00:57]

Hi,

On Thu, Jul 7, 2011 at 7:11 PM,  <fullung@gmail.com> wrote:

> Description:
> syscall: Parse and encode SCM_RIGHTS and SCM_CREDENTIALS.
>
> Please review this at http://codereview.appspot.com/4667066/
>
> Affected files:
>  M src/pkg/syscall/sockcmsg_unix.go

This CL will break the build on both BSD variants, FreeBSD and
Darwin. So could you please split your CL into three files: for Unix
variants, for BSD variants and for Linux.

And please modify types_(darwin|freebsd).c, pick the files generated
by mkall.sh re-run up and add them to this CL.

[albert.strasheim, 2011-07-07 13:18:22]

PTAL.

On 2011/07/07 12:00:57, mikioh wrote:
> Hi,
> 
> On Thu, Jul 7, 2011 at 7:11 PM,  <mailto:fullung@gmail.com> wrote:
> 
> > Description:
> > syscall: Parse and encode SCM_RIGHTS and SCM_CREDENTIALS.
> >
> > Please review this at http://codereview.appspot.com/4667066/
> >
> > Affected files:
> > &nbsp;M src/pkg/syscall/sockcmsg_unix.go
> 
> This CL will break the build on both BSD variants, FreeBSD and
> Darwin. So could you please split your CL into three files: for Unix
> variants, for BSD variants and for Linux.

I did a quick bit of googling and greping, and it seems the SCM_RIGHTS stuff is
available on all these platforms.

> And please modify types_(darwin|freebsd).c, pick the files generated
> by mkall.sh re-run up and add them to this CL.

SCM_CREDENTIALS is Linux-only, so I've moved that to sockcmsg_linux.go.

FreeBSD and Darwin also have ucred stuff, but it seems to differ quite a bit
from what Linux has (the constant is called SCM_CREDS, for starters). I don't
have access to these platforms, so I'd prefer to leave the ucred stuff on these
platforms for another CL.

Regards

Albert

[agl1, 2011-07-07 13:48:13]

http://codereview.appspot.com/4667066/diff/10003/src/pkg/syscall/sockcmsg_lin...
File src/pkg/syscall/sockcmsg_linux.go (right):

http://codereview.appspot.com/4667066/diff/10003/src/pkg/syscall/sockcmsg_lin...
src/pkg/syscall/sockcmsg_linux.go:13: func UnixCredentials(ucred *Ucred) []byte
{
Exported functions should have documentation comments. (Ditto for many other
functions.)

http://codereview.appspot.com/4667066/diff/10003/src/pkg/syscall/sockcmsg_lin...
src/pkg/syscall/sockcmsg_linux.go:14: buf := make([]byte,
CmsgSpace(SizeofUcred))
This code (and ParseUnixCredentials) is marshaling to and from a binary
structure. Why not do it without unsafe? encoding/binary might lack a
NativeEndian, but you could implement those functions easily enough here.

http://codereview.appspot.com/4667066/diff/10003/src/pkg/syscall/sockcmsg_uni...
File src/pkg/syscall/sockcmsg_unix.go (right):

http://codereview.appspot.com/4667066/diff/10003/src/pkg/syscall/sockcmsg_uni...
src/pkg/syscall/sockcmsg_unix.go:84: var sl = struct {
This also seems like excessive use of unsafe to me.

[albert.strasheim, 2011-07-07 14:06:30]

On 2011/07/07 13:48:13, agl1 wrote:
>
http://codereview.appspot.com/4667066/diff/10003/src/pkg/syscall/sockcmsg_lin...
> src/pkg/syscall/sockcmsg_linux.go:14: buf := make([]byte,
> CmsgSpace(SizeofUcred))
> This code (and ParseUnixCredentials) is marshaling to and from a binary
> structure. Why not do it without unsafe? encoding/binary might lack a
> NativeEndian, but you could implement those functions easily enough here.

I'm not quite sure I understand.

Do you want me to copy the encoder/decoder from encoding/binary to the syscall
package for this purpose?

I see this issue was discussed here:

http://groups.google.com/group/golang-dev/browse_thread/thread/88af59679c872973

Not quite sure how to proceed though.

>
http://codereview.appspot.com/4667066/diff/10003/src/pkg/syscall/sockcmsg_uni...
> File src/pkg/syscall/sockcmsg_unix.go (right):
> 
>
http://codereview.appspot.com/4667066/diff/10003/src/pkg/syscall/sockcmsg_uni...
> src/pkg/syscall/sockcmsg_unix.go:84: var sl = struct {
> This also seems like excessive use of unsafe to me.

I really just wanted a memcpy, but copy needs byte slices, not *byte...

Thanks.

Regards

Albert

[agl1, 2011-07-07 14:32:08]

On Thu, Jul 7, 2011 at 10:06 AM,  <fullung@gmail.com> wrote:
>
http://groups.google.com/group/golang-dev/browse_thread/thread/88af59679c872973
>
> Not quite sure how to proceed though.

Yes, rsc does seem to suggest using structs and unsafe is preferred
and you should always side with rsc over me.

I still feel that the slice code is excessive (and the fds should
probably be converted to int32s.)


Cheers

AGL

[mikio, 2011-07-07 15:51:04]

http://codereview.appspot.com/4667066/diff/10003/src/pkg/syscall/Makefile
File src/pkg/syscall/Makefile (right):

http://codereview.appspot.com/4667066/diff/10003/src/pkg/syscall/Makefile#new...
src/pkg/syscall/Makefile:41: sockcmsg_linux.go\
pls move up (sorted by filename).

[albert.strasheim, 2011-07-07 16:08:01]

Hello

On 2011/07/07 15:51:04, mikioh wrote:
> http://codereview.appspot.com/4667066/diff/10003/src/pkg/syscall/Makefile
> File src/pkg/syscall/Makefile (right):
> 
>
http://codereview.appspot.com/4667066/diff/10003/src/pkg/syscall/Makefile#new...
> src/pkg/syscall/Makefile:41: sockcmsg_linux.go\
> pls move up (sorted by filename).

Fixed.

I've also added comments.

I'll wait to see what rsc has to say about my unsafe abuse.

Thanks for the reviews so far.

Cheers

Albert

[albert.strasheim, 2011-07-07 19:25:40]

PTAL.

On 2011/07/07 14:32:08, agl1 wrote:
> On Thu, Jul 7, 2011 at 10:06 AM,  <mailto:fullung@gmail.com> wrote:
> >
>
http://groups.google.com/group/golang-dev/browse_thread/thread/88af59679c872973
> >
> > Not quite sure how to proceed though.
> Yes, rsc does seem to suggest using structs and unsafe is preferred
> and you should always side with rsc over me.
> I still feel that the slice code is excessive (and the fds should
> probably be converted to int32s.)

I've addressed your point about the int32s in UnixRights and ParseUnixRights and
in the process a nice way to get rid of the slice stuff presented itself.

Regards

Albert

[rsc, 2011-07-13 22:51:50]

LGTM

[albert.strasheim, 2011-07-18 15:21:01]

ping?

On 2011/07/13 22:51:50, rsc wrote:
> LGTM

[rsc, 2011-07-18 15:22:04]

*** Submitted as http://code.google.com/p/go/source/detail?r=4217a71283f9 ***

syscall: Parse and encode SCM_RIGHTS and SCM_CREDENTIALS.

R=rsc, agl, mikioh.mikioh
CC=golang-dev
http://codereview.appspot.com/4667066

Committer: Russ Cox <rsc@golang.org>