code review 6497044: net: delete unused IP-level socket option helpers

net: delete unused IP-level socket option helpers

[mikio, 2012-08-28 15:50:25]

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

I'd like you to review this change to
https://code.google.com/p/go

[bradfitz, 2012-08-28 15:59:54]

That's a lot of deleted lines.

Normally I'd quickly approve deleting a helper function or two that is no
longer needed, but this is a scary amount of deletes.

I must have missed something.


On Tue, Aug 28, 2012 at 8:50 AM, <mikioh.mikioh@gmail.com> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> net: delete unused IP-level socket option helpers
>
> Please review this at
http://codereview.appspot.com/**6497044/<http://codereview.appspot.com/6497044/>
>
> Affected files:
>   M src/pkg/net/multicast_test.go
>   M src/pkg/net/sockoptip.go
>   M src/pkg/net/sockoptip_bsd.go
>   R src/pkg/net/sockoptip_darwin.**go
>   R src/pkg/net/sockoptip_freebsd.**go
>   M src/pkg/net/sockoptip_linux.go
>   R src/pkg/net/sockoptip_netbsd.**go
>   R src/pkg/net/sockoptip_openbsd.**go
>   M src/pkg/net/sockoptip_windows.**go
>   M src/pkg/net/udpsock_posix.go
>   M src/pkg/net/unicast_test.go
>
>
>

[dave_cheney.net, 2012-08-28 23:35:36]

That's a lot of deleted lines.
> 
> Normally I'd quickly approve deleting a helper function or two that is no
> longer needed, but this is a scary amount of deletes.
> 
> I must have missed something.

I think most of the logic is being relocated here to the go.net subrepo,
http://codereview.appspot.com/6482044/.

[mikio, 2012-08-29 03:36:38]

On 2012/08/28 23:35:36, dfc wrote:

> I think most of the logic is being relocated here to the http://go.net
subrepo,

yup, thanks for the mention.
test passed on darwin, linux and freebsd.

[gobot, 2012-09-01 14:40:45]

R=rsc (assigned by rsc)

[dave_cheney.net, 2012-09-03 11:00:54]

On 2012/09/01 14:40:45, gobot wrote:
> R=rsc (assigned by rsc)

I'm concerned that the version of these methods that is moving into go.net/ipv4,
CL 6482044, are missing the calls to fd.incref()/decref(). Is this no longer
needed ?

[mikio, 2012-09-03 11:51:28]

On Mon, Sep 3, 2012 at 8:00 PM,  <dave@cheney.net> wrote:

> I'm concerned that the version of these methods that is moving into
> go.net/ipv4, CL 6482044, are missing the calls to fd.incref()/decref().
> Is this no longer needed ?

My guess is that both fd.incref/decref are just for closing multiple socket
read/writers, network I/O goroutines, on the same socket gracefully.
So we probably might drop fd.incref/decref from socket option handlers
in net package.

[dave_cheney.net, 2012-09-05 10:31:46]

> My guess is that both fd.incref/decref are just for closing multiple socket
> read/writers, network I/O goroutines, on the same socket gracefully.
> So we probably might drop fd.incref/decref from socket option handlers
> in net package.

From my reading this evening I believe incref/decref are primarily to prevent a
fd being closed while there is a pending (ie, in the pollserver) Read/Write
operation waiting on the underlying socket to become ready. In that context it
is probably ok to skip these calls for setsocketopt type operations if the
caller is prepared to deal with errors if the socket is closed concurrently.

[dave_cheney.net, 2012-09-05 10:40:30]

LGTM once http://codereview.appspot.com/6482044/ goes in.

[rsc, 2012-09-10 16:04:58]

LGTM

I'm surprised we had all this dead code lying around but...

[mikio, 2012-09-10 23:56:35]

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

net: delete unused IP-level socket option helpers

R=rsc, dave
CC=bradfitz, gobot, golang-dev
http://codereview.appspot.com/6497044