Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(513)

Issue 55410043: code review 55410043: syscall: use unsafe.Pointer instead of uintptr in places (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 1 month ago by bradfitz
Modified:
11 years, 1 month ago
Reviewers:
minux1, rsc, ebfe
CC:
rsc, golang-codereviews
Visibility:
Public.

Description

syscall: use unsafe.Pointer instead of uintptr in net syscalls In particular: setsockopt, getsockopt, bind, connect. There are probably more. All platforms cross-compile with make.bash, and all.bash still pases on linux/amd64. Update Issue 7169

Patch Set 1 #

Patch Set 2 : diff -r 27dcbbb2fd4e https://go.googlecode.com/hg/ #

Total comments: 2

Patch Set 3 : diff -r 27dcbbb2fd4e https://go.googlecode.com/hg/ #

Patch Set 4 : diff -r 5dd2679d2a0d https://go.googlecode.com/hg/ #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+140 lines, -140 lines) Patch
M src/pkg/syscall/lsf_linux.go View 1 1 chunk +2 lines, -2 lines 2 comments Download
M src/pkg/syscall/syscall_bsd.go View 1 2 3 6 chunks +24 lines, -24 lines 0 comments Download
M src/pkg/syscall/syscall_freebsd.go View 1 1 chunk +2 lines, -2 lines 0 comments Download
M src/pkg/syscall/syscall_linux.go View 1 2 3 9 chunks +23 lines, -23 lines 0 comments Download
M src/pkg/syscall/syscall_linux_386.go View 1 2 3 5 chunks +7 lines, -7 lines 0 comments Download
M src/pkg/syscall/syscall_linux_amd64.go View 1 1 chunk +5 lines, -5 lines 0 comments Download
M src/pkg/syscall/syscall_linux_arm.go View 1 2 3 1 chunk +5 lines, -5 lines 0 comments Download
M src/pkg/syscall/syscall_unix.go View 1 4 chunks +12 lines, -12 lines 0 comments Download
M src/pkg/syscall/zsyscall_darwin_386.go View 1 2 5 chunks +5 lines, -5 lines 0 comments Download
M src/pkg/syscall/zsyscall_darwin_amd64.go View 1 2 5 chunks +5 lines, -5 lines 0 comments Download
M src/pkg/syscall/zsyscall_dragonfly_386.go View 1 2 5 chunks +5 lines, -5 lines 0 comments Download
M src/pkg/syscall/zsyscall_dragonfly_amd64.go View 1 2 5 chunks +5 lines, -5 lines 0 comments Download
M src/pkg/syscall/zsyscall_freebsd_386.go View 1 2 5 chunks +5 lines, -5 lines 0 comments Download
M src/pkg/syscall/zsyscall_freebsd_amd64.go View 1 5 chunks +5 lines, -5 lines 0 comments Download
M src/pkg/syscall/zsyscall_linux_amd64.go View 1 5 chunks +5 lines, -5 lines 0 comments Download
M src/pkg/syscall/zsyscall_linux_arm.go View 1 2 3 5 chunks +5 lines, -5 lines 0 comments Download
M src/pkg/syscall/zsyscall_netbsd_386.go View 1 2 5 chunks +5 lines, -5 lines 0 comments Download
M src/pkg/syscall/zsyscall_netbsd_amd64.go View 1 2 5 chunks +5 lines, -5 lines 0 comments Download
M src/pkg/syscall/zsyscall_openbsd_386.go View 1 2 5 chunks +5 lines, -5 lines 0 comments Download
M src/pkg/syscall/zsyscall_openbsd_amd64.go View 1 2 5 chunks +5 lines, -5 lines 0 comments Download

Messages

Total messages: 16
rsc
This looks good, thanks for starting it. If it turns out we need changes to ...
11 years, 1 month ago (2014-01-22 01:56:34 UTC) #1
bradfitz
Hello rsc@golang.org (cc: golang-codereviews@googlegroups.com), I'd like you to review this change to https://go.googlecode.com/hg/
11 years, 1 month ago (2014-01-22 01:58:28 UTC) #2
bradfitz
PTAL All BSDs, Windows, Linux, and Darwin should work now. Will rebuild them all to ...
11 years, 1 month ago (2014-01-22 01:59:16 UTC) #3
bradfitz
(No public API change was necessary)
11 years, 1 month ago (2014-01-22 01:59:37 UTC) #4
rsc
Actually, I thought you were going to take the address of zeroPtr, but it looks ...
11 years, 1 month ago (2014-01-22 02:10:15 UTC) #5
rsc
LGTM after s/zeroPtr/nil/
11 years, 1 month ago (2014-01-22 02:11:18 UTC) #6
bradfitz
On Tue, Jan 21, 2014 at 6:10 PM, Russ Cox <rsc@golang.org> wrote: > Actually, I ...
11 years, 1 month ago (2014-01-22 02:12:58 UTC) #7
rsc
Dmitriy mentioned setsockopt on issue 7159 but every log in the issue is a crash ...
11 years, 1 month ago (2014-01-22 02:18:21 UTC) #8
bradfitz
*** Submitted as https://code.google.com/p/go/source/detail?r=87208c254908 *** syscall: use unsafe.Pointer instead of uintptr in net syscalls In ...
11 years, 1 month ago (2014-01-22 02:54:51 UTC) #9
ebfe
Hi, this CL doesn't touch freebsd/arm and netbsd/arm. Is this intentional?
11 years, 1 month ago (2014-01-22 07:36:16 UTC) #10
bradfitz
My build tool didn't include them because they weren't on the build dashboard and that's ...
11 years, 1 month ago (2014-01-22 08:09:35 UTC) #11
minux1
https://codereview.appspot.com/55410043/diff/60001/src/pkg/syscall/lsf_linux.go File src/pkg/syscall/lsf_linux.go (right): https://codereview.appspot.com/55410043/diff/60001/src/pkg/syscall/lsf_linux.go#newcode72 src/pkg/syscall/lsf_linux.go:72: return setsockopt(fd, SOL_SOCKET, SO_ATTACH_FILTER, unsafe.Pointer(&p), unsafe.Sizeof(p)) perhaps I'm missing ...
11 years, 1 month ago (2014-01-23 06:46:57 UTC) #12
bradfitz
https://codereview.appspot.com/55410043/diff/60001/src/pkg/syscall/lsf_linux.go File src/pkg/syscall/lsf_linux.go (right): https://codereview.appspot.com/55410043/diff/60001/src/pkg/syscall/lsf_linux.go#newcode72 src/pkg/syscall/lsf_linux.go:72: return setsockopt(fd, SOL_SOCKET, SO_ATTACH_FILTER, unsafe.Pointer(&p), unsafe.Sizeof(p)) On 2014/01/23 06:46:58, ...
11 years, 1 month ago (2014-01-23 14:51:41 UTC) #13
minux1
On Thu, Jan 23, 2014 at 9:51 AM, <bradfitz@golang.org> wrote: > This code had to ...
11 years, 1 month ago (2014-01-23 18:16:47 UTC) #14
bradfitz
If one finds this confusing, one should not be using unsafe. Without using unsafe, there ...
11 years, 1 month ago (2014-01-23 18:37:21 UTC) #15
rsc
11 years, 1 month ago (2014-01-23 20:04:27 UTC) #16
On Thu, Jan 23, 2014 at 1:16 PM, minux <minux.ma@gmail.com> wrote:

>
> On Thu, Jan 23, 2014 at 9:51 AM, <bradfitz@golang.org> wrote:
>
>> This code had to change because setsockopt itself had to change.
>>
> Yes, I was asking the more generic case here.
>
>>
>> But also: p being in scope doesn't mean it won't be collected.  There's
>> a difference between in scope and "live".
>>
> My thinking is that they probably should be the same or it will be very
> confusing.
>

They're just not. Forget about unsafe. If I have

foo, _ := ioutil.ReadFile("foo")
use(foo)
bar, _ := ioutil.ReadFile("bar")
use(bar)
baz, _ := ioutil.ReadFile("baz")
use(baz)

there is no reason that foo, bar, and baz should not all use the same
storage to hold the file contents. Introducing artificial restrictions like
tying them to scope just creates leaks (and worse it assigns semantic
meaning to { }, which triggers the nonsense people write for RAII in C++).

Russ
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b