syscall: GetsockoptInt.

syscall: GetsockoptInt.

[albert.strasheim, 2011-03-23 07:26:45]

Hello all

Please review this change.

GetsockoptInt is needed to implement CL 4306042.

Regards

Albert

[albert.strasheim, 2011-03-23 07:34:00]

Whoops, not quite right yet. Fixing...

On 2011/03/23 07:26:45, albert.strasheim wrote:
> Hello all
> 
> Please review this change.
> 
> GetsockoptInt is needed to implement CL 4306042.
> 
> Regards
> 
> Albert

[albert.strasheim, 2011-03-23 08:07:02]

PTAL. Linux only for now.

[rsc, 2011-03-23 14:30:39]

http://codereview.appspot.com/4271060/diff/5001/src/pkg/syscall/syscall_linux.go
File src/pkg/syscall/syscall_linux.go (right):

http://codereview.appspot.com/4271060/diff/5001/src/pkg/syscall/syscall_linux...
src/pkg/syscall/syscall_linux.go:420: vallen := 4
should size this properly.

len := int32(4)

http://codereview.appspot.com/4271060/diff/5001/src/pkg/syscall/syscall_linux...
File src/pkg/syscall/syscall_linux_arm.go (right):

http://codereview.appspot.com/4271060/diff/5001/src/pkg/syscall/syscall_linux...
src/pkg/syscall/syscall_linux_arm.go:60: //sys	getsockopt(s int, level int, name
int, val uintptr, vallen *int) (errno int)
Regenerate arm zsyscall file?

[albert.strasheim, 2011-03-23 15:19:03]

PTAL

On 2011/03/23 14:30:39, rsc wrote:
>
http://codereview.appspot.com/4271060/diff/5001/src/pkg/syscall/syscall_linux.go
> File src/pkg/syscall/syscall_linux.go (right):
>
http://codereview.appspot.com/4271060/diff/5001/src/pkg/syscall/syscall_linux...
> src/pkg/syscall/syscall_linux.go:420: vallen := 4
> should size this properly.
> len := int32(4)

setsockopt has vallen as an int in its signature so I tried to do the same thing
with getsockopt.

I've changed the code to:

vallen := int(4)

but I'm not sure if that addresses your concern.

>
http://codereview.appspot.com/4271060/diff/5001/src/pkg/syscall/syscall_linux...
> File src/pkg/syscall/syscall_linux_arm.go (right):
>
http://codereview.appspot.com/4271060/diff/5001/src/pkg/syscall/syscall_linux...
> src/pkg/syscall/syscall_linux_arm.go:60: //sys	getsockopt(s int, level int,
name
> int, val uintptr, vallen *int) (errno int)
> Regenerate arm zsyscall file?

Fixed.

[rsc, 2011-03-23 15:26:37]

http://codereview.appspot.com/4271060/diff/8001/src/pkg/syscall/syscall_linux...
File src/pkg/syscall/syscall_linux_386.go (right):

http://codereview.appspot.com/4271060/diff/8001/src/pkg/syscall/syscall_linux...
src/pkg/syscall/syscall_linux_386.go:133: func getsockopt(s int, level int, name
int, val uintptr, vallen *int) (errno int) {
Aha.  This should be *int32 or whatever the size is
that getsockopt needs to be passed.

The function prototype says socklen_t which seems
to be int32 on both darwin and linux.

[albert.strasheim, 2011-03-23 15:35:42]

PTAL.

On 2011/03/23 15:26:37, rsc wrote:
>
http://codereview.appspot.com/4271060/diff/8001/src/pkg/syscall/syscall_linux...
> File src/pkg/syscall/syscall_linux_386.go (right):
>
http://codereview.appspot.com/4271060/diff/8001/src/pkg/syscall/syscall_linux...
> src/pkg/syscall/syscall_linux_386.go:133: func getsockopt(s int, level int,
name
> int, val uintptr, vallen *int) (errno int) {
> Aha.  This should be *int32 or whatever the size is
> that getsockopt needs to be passed.
> The function prototype says socklen_t which seems
> to be int32 on both darwin and linux.

It seems most of the other socket functions use _Socklen, so I've changed both
getsockopt and setsockopt to use that.

[rsc, 2011-03-23 17:35:06]

very close

http://codereview.appspot.com/4271060/diff/11001/src/pkg/syscall/syscall_linu...
File src/pkg/syscall/syscall_linux_386.go (right):

http://codereview.appspot.com/4271060/diff/11001/src/pkg/syscall/syscall_linu...
src/pkg/syscall/syscall_linux_386.go:138: func setsockopt(s int, level int, name
int, val uintptr, vallen _Socklen) (errno int) {
You only need _Socklen when you are dealing iwth a pointer.
The rest of the time it is just overkill.  Please revert this part of the
change.

[albert.strasheim, 2011-03-23 18:29:14]

PTAL.

On 2011/03/23 17:35:06, rsc wrote:
> very close
> 
>
http://codereview.appspot.com/4271060/diff/11001/src/pkg/syscall/syscall_linu...
> File src/pkg/syscall/syscall_linux_386.go (right):
> 
>
http://codereview.appspot.com/4271060/diff/11001/src/pkg/syscall/syscall_linu...
> src/pkg/syscall/syscall_linux_386.go:138: func setsockopt(s int, level int,
name
> int, val uintptr, vallen _Socklen) (errno int) {
> You only need _Socklen when you are dealing iwth a pointer.
> The rest of the time it is just overkill.  Please revert this part of the
> change.

[rsc, 2011-03-23 18:33:33]

LGTM

Thanks.  Need to get Darwin and FreeBSD in order
to submit the other CL.  Okay to do them blind and
test via dashboard if you don't have the machines
available.  Should be all auto-generated.

[rsc, 2011-03-23 18:33:50]

*** Submitted as 1ca16d1788ba ***

syscall: GetsockoptInt.

R=rsc, iant
CC=golang-dev
http://codereview.appspot.com/4271060

Committer: Russ Cox <rsc@golang.org>

[albert.strasheim, 2011-03-23 18:40:45]

Hello

On Wed, Mar 23, 2011 at 8:33 PM, Russ Cox <rsc@golang.org> wrote:
> LGTM
>
> Thanks.  Need to get Darwin and FreeBSD in order
> to submit the other CL.  Okay to do them blind and
> test via dashboard if you don't have the machines
> available.  Should be all auto-generated.

Thanks. I can get access to a Darwin machine tomorrow to tackle that.

Could you provide some guidance on how you see the next step, i.e.,
how to deal with Shutdown? I'd like to see if I can finish that off
tomorrow.

Regards

Albert

[rsc, 2011-03-25 18:43:00]

> Could you provide some guidance on how you see the next step, i.e.,
> how to deal with Shutdown? I'd like to see if I can finish that off
> tomorrow.

I have no idea how to deal with Shutdown, unfortunately.

Russ

[albert.strasheim, 2011-03-26 08:00:09]

On Fri, Mar 25, 2011 at 8:42 PM, Russ Cox <rsc@golang.org> wrote:
>> Could you provide some guidance on how you see the next step, i.e.,
>> how to deal with Shutdown? I'd like to see if I can finish that off
>> tomorrow.
> I have no idea how to deal with Shutdown, unfortunately.

It seems that if you know that no goroutines are reading or writing,
which will typically be the case with a server that just wants to rid
itself of a socket already passed to a client process, you can do:

conn.Fd().Close()
conn.Close()

However, if the server has multiple goroutines doing this kind of
thing, I think you could end up calling Shutdown on a "valid invalid"
file descriptor, because conn.Fd().Close() isn't going to set fd.sysfd
to -1.

So maybe we need conn.CloseFd() so that this can be achieved?

Regards

Albert

[rsc, 2011-03-26 21:51:22]

> It seems that if you know that no goroutines are reading or writing,
> which will typically be the case with a server that just wants to rid
> itself of a socket already passed to a client process, you can do:
>
> conn.Fd().Close()
> conn.Close()
>
> However, if the server has multiple goroutines doing this kind of
> thing, I think you could end up calling Shutdown on a "valid invalid"
> file descriptor, because conn.Fd().Close() isn't going to set fd.sysfd
> to -1.
>
> So maybe we need conn.CloseFd() so that this can be achieved?

I hope not.  I'd prefer to remove the use of shutdown entirely,
but I am not sure whether we can.  We're not actually trying to
shut down the network connection by using it.  What we want to
avoid is:

    * goroutine 1: find fd = 3, decide to use it.
    * goroutine 2: find fd = 3, close(3)
    * goroutine 3: create new network connection, gets fd 3
    * goroutine 1: use fd 3, expecting old network connection,
          but actually get new one.

The current code calls shutdown during Close so that the
i/o gets stopped without confusing goroutine 1 in the example
above.  With a little more coordination we might be able to
go back to using close directly.

Russ