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

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:
6 years, 12 months ago by bradfitz
Modified:
6 years, 12 months 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 ...
6 years, 12 months 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/
6 years, 12 months 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 ...
6 years, 12 months ago (2014-01-22 01:59:16 UTC) #3
bradfitz
(No public API change was necessary)
6 years, 12 months 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 ...
6 years, 12 months ago (2014-01-22 02:10:15 UTC) #5
rsc
LGTM after s/zeroPtr/nil/
6 years, 12 months 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 ...
6 years, 12 months 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 ...
6 years, 12 months 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 ...
6 years, 12 months ago (2014-01-22 02:54:51 UTC) #9
ebfe
Hi, this CL doesn't touch freebsd/arm and netbsd/arm. Is this intentional?
6 years, 12 months 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 ...
6 years, 12 months 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 ...
6 years, 12 months 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, ...
6 years, 12 months 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 ...
6 years, 12 months 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 ...
6 years, 12 months ago (2014-01-23 18:37:21 UTC) #15
rsc
6 years, 12 months 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