[golang-dev] Re: code review 6674043: net, os/user: update docs and sync API for Plan 9 (issue 6674043)

[golang-dev] Re: code review 6674043: net, os/user: update docs and sync API for Plan 9 (issue 6674043)

	Ality
	Nov 8, 2012 at 3:37 am

Gentle ping.

This shouldn't require knowledge of Plan 9 to review.

Thanks,
Anthony

https://codereview.appspot.com/6674043/

Search Discussions

15 responses
Oldest
Nested

Dave

at Nov 8, 2012 at 3:54 am

⇧

A few small comments, i'm probably not understanding something.

https://codereview.appspot.com/6674043/diff/8001/src/pkg/net/tcpsock_plan9.go
File src/pkg/net/tcpsock_plan9.go (right):

https://codereview.appspot.com/6674043/diff/8001/src/pkg/net/tcpsock_plan9.go#newcode23
src/pkg/net/tcpsock_plan9.go:23: }
Is this needed? io.Copy will use this method in preference of one that
won't return EPLAN9.

https://codereview.appspot.com/6674043/diff/8001/src/pkg/net/tcpsock_plan9.go#newcode56
src/pkg/net/tcpsock_plan9.go:56: }
Similarly, are these methods needed to satisfy an interface, if not, I'm
not sure adding them helps.

https://codereview.appspot.com/6674043/diff/8001/src/pkg/net/udpsock_plan9.go
File src/pkg/net/udpsock_plan9.go (right):

https://codereview.appspot.com/6674043/diff/8001/src/pkg/net/udpsock_plan9.go#newcode186
src/pkg/net/udpsock_plan9.go:186: if err != nil {
if _, err := l.ctl.WriteString("headers"); err != nil { ... }

https://codereview.appspot.com/6674043/

reply | permalink

Ality

at Nov 8, 2012 at 4:34 am

⇧

https://codereview.appspot.com/6674043/diff/8001/src/pkg/net/tcpsock_plan9.go
File src/pkg/net/tcpsock_plan9.go (right):

https://codereview.appspot.com/6674043/diff/8001/src/pkg/net/tcpsock_plan9.go#newcode23
src/pkg/net/tcpsock_plan9.go:23: }

On 2012/11/08 03:54:48, dfc wrote:
Is this needed? io.Copy will use this method in preference of one that won't
return EPLAN9.

Good catch. I added it just to appease cmd/api.
Should I remove it or wrap io.Copy like the
genericReadFrom in sock_posix.go?

https://codereview.appspot.com/6674043/diff/8001/src/pkg/net/tcpsock_plan9.go#newcode56
src/pkg/net/tcpsock_plan9.go:56: }

On 2012/11/08 03:54:48, dfc wrote:
Similarly, are these methods needed to satisfy an interface, if not, I'm not
sure adding them helps.

Again, these were added to appease cmd/api.
Are they required by the Go 1 api?

https://codereview.appspot.com/6674043/

reply | permalink

Dave

at Nov 8, 2012 at 4:36 am

⇧

I don't think that plan9 is covered by the api tool, so you may have
leverage to make changes here. Best to consult with the keeps of the
API.

https://codereview.appspot.com/6674043/

reply | permalink

Mikio Hara Counter proposal: https://codereview.appspot.com/6814101/

Mikio Hara at Nov 8, 2012 at 10:24 am ⇧

On Thu, Nov 8, 2012 at 12:37 PM, wrote:

This shouldn't require knowledge of Plan 9 to review.
Counter proposal: https://codereview.appspot.com/6814101/

reply | permalink
Anthony Martin I think this is probably the way to go. I'll seperate out the os/user changes into another CL and abandon this one. Thanks, Anthony

Anthony Martin at Nov 8, 2012 at 12:39 pm ⇧

Mikio Hara once said:
On Thu, Nov 8, 2012 at 12:37 PM, wrote:

This shouldn't require knowledge of Plan 9 to review.
Counter proposal: https://codereview.appspot.com/6814101/
I think this is probably the way to go.
I'll seperate out the os/user changes
into another CL and abandon this one.

Thanks,
Anthony

reply | permalink

Mikio Hara

at Nov 10, 2012 at 5:33 am

⇧

Hi,

On Thu, Nov 8, 2012 at 9:39 PM, Anthony Martin wrote:

I think this is probably the way to go.

I revised the counter proposal, splitting into two CLs
and changed consolidation approach to using embeds.

https://codereview.appspot.com/6821100/
https://codereview.appspot.com/6820124/

I'll seperate out the os/user changes
into another CL and abandon this one.

I think the remaining part would be covered by this CL.

reply | permalink

Bradfitz

at Nov 8, 2012 at 6:17 pm

⇧

https://codereview.appspot.com/6674043/diff/8001/src/pkg/net/ipsock_plan9.go
File src/pkg/net/ipsock_plan9.go (right):

https://codereview.appspot.com/6674043/diff/8001/src/pkg/net/ipsock_plan9.go#newcode311
src/pkg/net/ipsock_plan9.go:311: // Accept implements the Accept method
in the Listener interface; it waits
I'd remove everything from the semicolon on. Redundant.

https://codereview.appspot.com/6674043/diff/8001/src/pkg/net/ipsock_plan9.go#newcode328
src/pkg/net/ipsock_plan9.go:328: // Addr returns the listener's network
address, a *TCPAddr.
I'd write this as:

// Addr returns the listener's address.
// The address will be of type *TCPAddr.

https://codereview.appspot.com/6674043/diff/8001/src/pkg/net/tcpsock_plan9.go
File src/pkg/net/tcpsock_plan9.go (right):

https://codereview.appspot.com/6674043/diff/8001/src/pkg/net/tcpsock_plan9.go#newcode56
src/pkg/net/tcpsock_plan9.go:56: }

On 2012/11/08 04:34:43, ality wrote:

On 2012/11/08 03:54:48, dfc wrote:
Similarly, are these methods needed to satisfy an interface, if not,

I'm not

sure adding them helps.

Again, these were added to appease cmd/api.
Are they required by the Go 1 api?

Yes, they are. go1.txt says:

pkg net, method (*TCPConn) SetLinger(int) error

... which means people can depend on that being available (compiling at
least) on all operating systems and architectures.

https://codereview.appspot.com/6674043/diff/8001/src/pkg/net/tcpsock_plan9.go#newcode97
src/pkg/net/tcpsock_plan9.go:97: // Close stops listening on the TCP
address. Already Accepted connections
s/Accepted/accepted/

https://codereview.appspot.com/6674043/diff/8001/src/pkg/net/tcpsock_plan9.go#newcode130
src/pkg/net/tcpsock_plan9.go:130: // AcceptTCP accepts the next incoming
call and returns the new connection
// AcceptTCP accepts the next incoming call.

https://codereview.appspot.com/6674043/

reply | permalink

Rsc

at Nov 12, 2012 at 9:46 pm

⇧

I am not sure I understand the relationship between this CL and Mikio's.
Has this CL been updated to remove the things that Mikio's CLs take care
of?

https://codereview.appspot.com/6674043/

reply | permalink

Mikioh Mikioh Hi Anthony, of? I've submitted a CL that related to ipsock_plan9.go, so pls update, merge in this CL. https://codereview.appspot.com/6674043/

Mikioh Mikioh at Nov 13, 2012 at 8:30 am ⇧

Hi Anthony,
On 2012/11/12 21:46:17, rsc wrote:
I am not sure I understand the relationship between this CL and
Mikio's. Has
this CL been updated to remove the things that Mikio's CLs take care
of?

I've submitted a CL that related to ipsock_plan9.go, so pls update,
merge in this CL.

https://codereview.appspot.com/6674043/

reply | permalink

Ality

at Nov 22, 2012 at 7:09 pm

⇧

Hello golang-dev@googlegroups.com, dave@cheney.net,
mikioh.mikioh@gmail.com, bradfitz@golang.org, rsc@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

http://codereview.appspot.com/6674043/

reply | permalink

Mikioh Mikioh

at Nov 23, 2012 at 5:10 am

⇧

Please move os/user/lookup stuff to its own CL.
Looks very close, thank you.

https://codereview.appspot.com/6674043/diff/17003/src/pkg/net/iprawsock_plan9.go
File src/pkg/net/iprawsock_plan9.go (right):

https://codereview.appspot.com/6674043/diff/17003/src/pkg/net/iprawsock_plan9.go#newcode17
src/pkg/net/iprawsock_plan9.go:17: type IPConn struct{}
type IPConn struct {
conn
}

Then you can drop placeholders for Conn, from Read to Close.

https://codereview.appspot.com/6674043/diff/17003/src/pkg/net/net.go
File src/pkg/net/net.go (right):

https://codereview.appspot.com/6674043/diff/17003/src/pkg/net/net.go#newcode368
src/pkg/net/net.go:368: type writerOnly struct {
I'd prefer these stuff stay in tcpsock.go.

https://codereview.appspot.com/6674043/diff/17003/src/pkg/net/unixsock_plan9.go
File src/pkg/net/unixsock_plan9.go (right):

https://codereview.appspot.com/6674043/diff/17003/src/pkg/net/unixsock_plan9.go#newcode17
src/pkg/net/unixsock_plan9.go:17: type UnixConn struct{}
type UnixConn struct {
conn
}

Then you can drop placeholders for Conn, from Read to Close.

https://codereview.appspot.com/6674043/

reply | permalink

Ality

at Nov 26, 2012 at 6:11 pm

⇧

https://codereview.appspot.com/6674043/diff/17003/src/pkg/net/iprawsock_plan9.go
File src/pkg/net/iprawsock_plan9.go (right):

https://codereview.appspot.com/6674043/diff/17003/src/pkg/net/iprawsock_plan9.go#newcode17
src/pkg/net/iprawsock_plan9.go:17: type IPConn struct{}

On 2012/11/23 05:04:43, mikio wrote:
type IPConn struct {
conn
}
Then you can drop placeholders for Conn, from Read to Close.

Done.

https://codereview.appspot.com/6674043/diff/17003/src/pkg/net/unixsock_plan9.go
File src/pkg/net/unixsock_plan9.go (right):

https://codereview.appspot.com/6674043/diff/17003/src/pkg/net/unixsock_plan9.go#newcode17
src/pkg/net/unixsock_plan9.go:17: type UnixConn struct{}

On 2012/11/23 05:04:43, mikio wrote:
type UnixConn struct {
conn
}
Then you can drop placeholders for Conn, from Read to Close.

Done.

https://codereview.appspot.com/6674043/

reply | permalink

Mikioh Mikioh LGTM https://codereview.appspot.com/6674043/

Mikioh Mikioh at Nov 27, 2012 at 12:54 am ⇧

LGTM

https://codereview.appspot.com/6674043/

reply | permalink
Anthony Martin I'll submit this once the builders get back to normal. A bunch of them are failing in the net package for various reasons and I don't want to muddy the waters. Anthony

Anthony Martin at Nov 27, 2012 at 1:14 am ⇧

mikioh.mikioh@gmail.com once said:
LGTM
I'll submit this once the builders get back
to normal. A bunch of them are failing in
the net package for various reasons and I
don't want to muddy the waters.

Anthony

reply | permalink

Ality

at Nov 30, 2012 at 7:42 pm

⇧

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

net: update docs and sync API for Plan 9

R=golang-dev, dave, mikioh.mikioh, bradfitz, rsc
CC=golang-dev
https://codereview.appspot.com/6674043

https://codereview.appspot.com/6674043/

reply | permalink

Related Discussions

Discussion Navigation

view	thread \| post

Discussion Overview

group	golang-dev @
categories	go
posted	Nov 8, '12 at 3:37a
active	Nov 30, '12 at 7:42p
posts	16
users	5
website	golang.org

responses	expanded	Hotkey: s
font	variable	Hotkey: f
user style	avatars	Hotkey: a

[golang-dev] Re: code review 6674043: net, os/user: update docs and sync API for Plan 9 (issue 6674043)

Search Discussions

Related Discussions

5 users in discussion