code review 26770045: net: always check error from syscall.Getsockname

net: always check error from syscall.Getsockname

This was spotted during the sunos port.

Credit for identifying the problem goes to Aram Hăvărneanu.

[dave_cheney.net, 2013-11-15 23:45:49]

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

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

[iant, 2013-11-16 00:28:19]

https://codereview.appspot.com/26770045/diff/40001/src/pkg/net/sock_posix.go
File src/pkg/net/sock_posix.go (right):

https://codereview.appspot.com/26770045/diff/40001/src/pkg/net/sock_posix.go#...
src/pkg/net/sock_posix.go:156: lsa, err := syscall.Getsockname(fd.sysfd)
There are two other places in this file where it ignores the error return of
Getsockname; is this one more likely to fail for some reason?  For that matter,
is there really any plausible reason for getsockname to fail?

[dave_cheney.net, 2013-11-16 00:34:11]

> On 16 Nov 2013, at 11:28, iant@golang.org wrote:
> 
> 
> https://codereview.appspot.com/26770045/diff/40001/src/pkg/net/sock_posix.go
> File src/pkg/net/sock_posix.go (right):
> 
>
https://codereview.appspot.com/26770045/diff/40001/src/pkg/net/sock_posix.go#...
> src/pkg/net/sock_posix.go:156: lsa, err := syscall.Getsockname(fd.sysfd)
> There are two other places in this file where it ignores the error
> return of Getsockname; is this one more likely to fail for some reason?

This was just the first one that blew up the test. I will extend the CL to cover
the other cases. 


> For that matter, is there really any plausible reason for getsockname to
> fail?

Well, syscalls have been very problematic on our port, so for us, yes they do
fail. 

That said, the error value has already computed, alone with four or five above,
so avoiding checking this one sounds like false economy. 

> 
> https://codereview.appspot.com/26770045/

[mikio, 2013-11-16 01:16:11]

On Sat, Nov 16, 2013 at 8:45 AM,  <dave@cheney.net> wrote:

> -       lsa, _ := syscall.Getsockname(fd.sysfd)
> +       lsa, err := syscall.Getsockname(fd.sysfd)
> +       if err != nil {
> +               return err
> +       }

I'm fine with this error checking because in Go 1.2 we have separated
socket catapult routines that are aware of connection-type. But for
getpeername and other routines, Im not keen on error checking unless
you can show us the behavior of get{sock,peer}name to both named and
unnamed socket with each [address-family, connection-type, protocol]
tuple on all supported platforms.

[dave_cheney.net, 2013-11-16 01:19:14]

Understood. 

> On 16 Nov 2013, at 12:16, Mikio Hara <mikioh.mikioh@gmail.com> wrote:
> 
>> On Sat, Nov 16, 2013 at 8:45 AM,  <dave@cheney.net> wrote:
>> 
>> -       lsa, _ := syscall.Getsockname(fd.sysfd)
>> +       lsa, err := syscall.Getsockname(fd.sysfd)
>> +       if err != nil {
>> +               return err
>> +       }
> 
> I'm fine with this error checking because in Go 1.2 we have separated
> socket catapult routines that are aware of connection-type. But for
> getpeername and other routines, Im not keen on error checking unless
> you can show us the behavior of get{sock,peer}name to both named and
> unnamed socket with each [address-family, connection-type, protocol]
> tuple on all supported platforms.

[rsc, 2013-11-27 16:52:47]

The ignored error here is intentional. Even if getsockname fails the fd
should be usable and should not be thrown away.

Russ

[rsc, 2013-12-18 20:29:34]

not lgtm

R=close

[dave_cheney.net, 2013-12-18 23:46:11]

*** Abandoned ***