[project] Switch most syscalls to golang.org/x/sys by tophj-ibm · Pull Request #33399 · moby/moby

tophj-ibm · 2017-05-25T14:29:18Z

The go syscall package has been frozen since go1.4. Any new features, and most updates for syscall are implemented in the golang.org/x/sys package instead. This is a WIP to change over all syscall references to /x/sys. This is called "most" syscall because of three exclusions that haven't been implemented yet are syscall.Errno, Signal, and SysProcAttr For more info see #31925

The original plan was to convert just pkg (which I've already done here) but the unit tests won't pass until I change most of everything so I'm going to do it all at the same time here.

While most of the code is more or less find and replace, some sections will have to be split. Where syscall would work on both windows + unix, /x/sys does not, so windows and unix logic will have to split.

Some other differences I've found that need to be addressed here:
1.) s390x forces signed char in /x/sys/unix, so change the buildtags for s390x in /pkg/platform/utsname per golang/sys@33267e0

img src=http://gopherholemuseum.ca

fntlnz · 2017-05-25T21:31:12Z

Is this intentionally duplicated ?

Yeah, this will cover the freebsd / solaris case. It's not ideal because I only really need 2 files instead of 3, but these already existed and seemed like a good place for them anyway.

I've still got a lot to do, but thanks for the review!

fntlnz · 2017-05-25T21:51:00Z

fntlnz · 2017-05-25T21:52:11Z

wouldn't be better to just return avoiding the named return? most people are not comfortable with it and it decreases readability

fntlnz · 2017-05-25T21:53:54Z

the unmount windows? Wrong replacement?

fntlnz · 2017-05-25T21:54:02Z

Wrong replacement?

ahh sed fail :(

fntlnz · 2017-05-25T21:57:07Z

@tophj-ibm just did some early "macro" review, will try this asap.
Also this needs some gofmt love

14:37:54 These files are not properly gofmt'd:
14:37:54  - pkg/archive/archive_unix.go
14:37:54  - pkg/archive/archive_windows.go

tophj-ibm · 2017-06-02T18:13:22Z

Okay the tests appear to be going, and all the work should be done here, I'm going to move this out of WIP.

cc @thaJeztah

because this touches the pkg directory, @vieux @dnephin you might want to take a look.

thaJeztah · 2017-06-06T18:40:48Z

Thanks @tophj-ibm - I moved this to code review 👍

cpuguy83 · 2017-06-22T04:33:44Z

Looks ok, but needs a rebase.

GordonTheTurtle · 2017-06-22T16:31:03Z

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "move-all-of-pkg-to-syscall" [email protected]:tophj-ibm/moby.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842353877392
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

tophj-ibm · 2017-06-23T12:49:35Z

thanks for the review, rebased.

justincormack · 2017-06-26T17:27:42Z

The casting rules are different if you assign to a variable first, weirdly. An extra cast seems ok.

…

On 26 Jun 2017 15:55, "Christopher Jones" ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pkg/system/utimes_linux.go <#33399 (comment)>: > if err != nil { return err } - if _, _, err := syscall.Syscall6(syscall.SYS_UTIMENSAT, uintptr(atFdCwd), uintptr(unsafe.Pointer(_path)), uintptr(unsafe.Pointer(&ts[0])), uintptr(atSymLinkNoFollow), 0, 0); err != 0 && err != syscall.ENOSYS { + if _, _, err := unix.Syscall6(unix.SYS_UTIMENSAT, uintptr(unix.AT_FDCWD), uintptr(unsafe.Pointer(_path)), uintptr(unsafe.Pointer(&ts[0])), uintptr(unix.AT_SYMLINK_NOFOLLOW), 0, 0); err != 0 && err != unix.ENOSYS { @cpuguy83 <https://github.com/cpuguy83> go question: do you have any idea why both unix.AT_FDCWD and the old atFdCwd would both reflect as type int, but only the former throws an error here? I can get this to work if I cast unix.AT_FDCWD to an int beforehand, but I'm pretty sure that's just because it's bypassing the type checker. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#33399 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAdcPCTXz83AZ3yOypLyynFwZwUoX3T2ks5sH8ZjgaJpZM4Nmc11> .

tophj-ibm · 2017-07-06T17:04:59Z

rebased again and finally green 😎

thaJeztah

Some questions (but perhaps it's for a reason 😄)

Also needs a minor rebase again 😢

thaJeztah · 2017-07-10T05:41:54Z

nit: extra blank line here

thaJeztah · 2017-07-10T05:43:19Z

nit: extra blank line here (I see there's some other blank lines above as well)

thaJeztah · 2017-07-10T05:49:11Z

Any reason syscall.Stat_t is still being used in this file (instead of unix.Stat_t)?

Yes, it's because os.Stat().Sys() implements type syscall.Stat_t and not unix.Stat_t. The difference is unix.Stat_t doesn't have additional padding at the end. There are multiple asserts to make sure it's of type syscall.Stat_t, so those fail.

I could probably remove all of the type assertions looking for syscall.Stat_t, but I wasn't sure if that would mess up anything else and this seemed fine. I can look more into changing it if you think that's better. 🤷‍♂️

difference is unix.Stat_t doesn't have additional padding at the end

Oh, I see it now, they looked so similar 😊

I could probably remove all of the type assertions looking for syscall.Stat_t

It's fine to keep it as-is for this PR. Thanks for explaining!

thaJeztah · 2017-07-10T05:50:33Z

same question (syscall.Stat_t)

thaJeztah · 2017-07-10T05:51:02Z

Same question for this file

thaJeztah

~~LGTM~~ see below 😅

thaJeztah · 2017-07-11T03:59:44Z

oh, oops, looks like it needs a gofmt;

13:55:02 These files are not properly gofmt'd:
13:55:02  - daemon/graphdriver/aufs/aufs.go
13:55:02  - daemon/graphdriver/devmapper/deviceset.go
13:55:02

diff --git a/daemon/graphdriver/aufs/aufs.go b/daemon/graphdriver/aufs/aufs.go
index b04aede71..c68c98cf8 100644
--- a/daemon/graphdriver/aufs/aufs.go
+++ b/daemon/graphdriver/aufs/aufs.go
@@ -36,7 +36,6 @@ import (
        "time"

        "github.com/Sirupsen/logrus"
-       "github.com/vbatts/tar-split/tar/storage"
        "github.com/docker/docker/daemon/graphdriver"
        "github.com/docker/docker/pkg/archive"
        "github.com/docker/docker/pkg/chrootarchive"
@@ -47,6 +46,7 @@ import (
        "github.com/docker/docker/pkg/system"
        rsystem "github.com/opencontainers/runc/libcontainer/system"
        "github.com/opencontainers/selinux/go-selinux/label"
+       "github.com/vbatts/tar-split/tar/storage"
        "golang.org/x/sys/unix"
 )

diff --git a/daemon/graphdriver/devmapper/deviceset.go b/daemon/graphdriver/devmapper/deviceset.go
index ca9e67906..522277936 100644
--- a/daemon/graphdriver/devmapper/deviceset.go
+++ b/daemon/graphdriver/devmapper/deviceset.go
@@ -28,8 +28,8 @@ import (
        "github.com/docker/docker/pkg/mount"
        "github.com/docker/docker/pkg/parsers"
        units "github.com/docker/go-units"
-       "github.com/pkg/errors"
        "github.com/opencontainers/selinux/go-selinux/label"
+       "github.com/pkg/errors"
        "golang.org/x/sys/unix"
 )

missed the gofmt

Changes most references of syscall to golang.org/x/sys/ Ones aren't changes include, Errno, Signal and SysProcAttr as they haven't been implemented in /x/sys/. Signed-off-by: Christopher Jones <[email protected]> [s390x] switch utsname from unsigned to signed per golang/sys@33267e0 char in s390x in the /x/sys/unix package is now signed, so change the buildtags Signed-off-by: Christopher Jones <[email protected]>

tophj-ibm · 2017-07-11T17:15:30Z

@thaJeztah updated, and back to green

thaJeztah

Whoop! LGTM

estesp

LGTM

tiborvass · 2017-07-11T20:57:20Z

LGTM

tiborvass · 2017-07-11T20:57:41Z

Lol @estesp was faster <3

Switch some more usage of the Stat function and the Stat_t type from the syscall package to golang.org/x/sys. Those were missing in PR moby#33399. Signed-off-by: Tobias Klauser <[email protected]>

Automatic merge from submit-queue Switch from package syscall to golang.org/x/sys/unix **What this PR does / why we need it**: The syscall package is locked down and the comment in https://github.com/golang/go/blob/master/src/syscall/syscall.go#L21-L24 advises to switch code to use the corresponding package from golang.org/x/sys. This PR does so and replaces usage of package syscall with package golang.org/x/sys/unix where applicable. This will also allow to get updates and fixes without having to use a new go version. In order to get the latest functionality, golang.org/x/sys/ is re-vendored. This also allows to use Eventfd() from this package instead of calling the eventfd() C function. **Special notes for your reviewer**: This follows previous works in other Go projects, see e.g. moby/moby#33399, cilium/cilium#588 **Release note**: ```release-note NONE ```

thaJeztah · 2018-03-09T10:00:43Z

@bcmills don't know if there was a specific reason, other than that the "freebsd" code was contributed 3 .. 4 years ago, but never reached completion. If you're interested to help out on making those changes, please do! 👍

Automatic merge from submit-queue Switch from package syscall to golang.org/x/sys/unix **What this PR does / why we need it**: The syscall package is locked down and the comment in https://github.com/golang/go/blob/master/src/syscall/syscall.go#L21-L24 advises to switch code to use the corresponding package from golang.org/x/sys. This PR does so and replaces usage of package syscall with package golang.org/x/sys/unix where applicable. This will also allow to get updates and fixes without having to use a new go version. In order to get the latest functionality, golang.org/x/sys/ is re-vendored. This also allows to use Eventfd() from this package instead of calling the eventfd() C function. **Special notes for your reviewer**: This follows previous works in other Go projects, see e.g. moby/moby#33399, cilium/cilium#588 **Release note**: ```release-note NONE ```

Automatic merge from submit-queue Switch from package syscall to golang.org/x/sys/unix **What this PR does / why we need it**: The syscall package is locked down and the comment in https://github.com/golang/go/blob/master/src/syscall/syscall.go#L21-L24 advises to switch code to use the corresponding package from golang.org/x/sys. This PR does so and replaces usage of package syscall with package golang.org/x/sys/unix where applicable. This will also allow to get updates and fixes without having to use a new go version. In order to get the latest functionality, golang.org/x/sys/ is re-vendored. This also allows to use Eventfd() from this package instead of calling the eventfd() C function. **Special notes for your reviewer**: This follows previous works in other Go projects, see e.g. moby/moby#33399, cilium/cilium#588 **Release note**: ```release-note NONE ``` Kubernetes-commit: 5d24a2c19923d6da46110b827619f4b21cf689ac

GordonTheTurtle added the status/0-triage label May 25, 2017

fntlnz reviewed May 25, 2017

View reviewed changes

tophj-ibm force-pushed the move-all-of-pkg-to-syscall branch 10 times, most recently from b5091f1 to 630d687 Compare June 2, 2017 18:05

tophj-ibm changed the title ~~[WIP] Switch all syscalls to golang.org/x/sys~~ [project] Switch most syscalls to golang.org/x/sys Jun 2, 2017

tophj-ibm force-pushed the move-all-of-pkg-to-syscall branch 2 times, most recently from 07d2aec to 92980a8 Compare June 2, 2017 21:26

thaJeztah added rebuild/experimental status/2-code-review and removed status/0-triage labels Jun 6, 2017

GordonTheTurtle removed the rebuild/experimental label Jun 6, 2017

GordonTheTurtle assigned tiborvass Jun 9, 2017

GordonTheTurtle added the dco/no Automatically set by a bot when one of the commits lacks proper signature label Jun 22, 2017

tophj-ibm force-pushed the move-all-of-pkg-to-syscall branch from 7580892 to 165558f Compare June 22, 2017 17:05

GordonTheTurtle removed the dco/no Automatically set by a bot when one of the commits lacks proper signature label Jun 22, 2017

tophj-ibm force-pushed the move-all-of-pkg-to-syscall branch from 165558f to 355b367 Compare June 23, 2017 12:29

tophj-ibm force-pushed the move-all-of-pkg-to-syscall branch from 519954d to 5bf9143 Compare June 26, 2017 18:34

tophj-ibm force-pushed the move-all-of-pkg-to-syscall branch from 5bf9143 to 3035b58 Compare July 6, 2017 13:11

thaJeztah requested changes Jul 10, 2017

View reviewed changes

tophj-ibm force-pushed the move-all-of-pkg-to-syscall branch from 3035b58 to a32ca69 Compare July 10, 2017 13:42

thaJeztah previously approved these changes Jul 11, 2017

View reviewed changes

tophj-ibm force-pushed the move-all-of-pkg-to-syscall branch from a32ca69 to 069fdc8 Compare July 11, 2017 12:00

thaJeztah approved these changes Jul 11, 2017

View reviewed changes

estesp approved these changes Jul 11, 2017

View reviewed changes

estesp merged commit 54251b5 into moby:master Jul 11, 2017

tophj-ibm deleted the move-all-of-pkg-to-syscall branch July 12, 2017 11:57

tklauser mentioned this pull request Jul 20, 2017

Switch from package syscall to golang.org/x/sys/unix kubernetes/kubernetes#49300

Merged

tklauser mentioned this pull request Jul 27, 2017

Switch Stat syscalls to x/sys/unix #34279

Merged

andrewhsu mentioned this pull request Aug 15, 2017

WIP [17.07] update vendor of components/engine in cli docker-archive/docker-ce#195

Closed

This was referenced Mar 9, 2020

pkg/system: replace more uses of "syscall" #40645

Merged

sys: windows: use golang.org/x/sys/windows containerd/containerd#4093

Merged

Conversation

tophj-ibm commented May 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fntlnz commented May 25, 2017

Uh oh!

tophj-ibm commented Jun 2, 2017

Uh oh!

thaJeztah commented Jun 6, 2017

Uh oh!

cpuguy83 commented Jun 22, 2017

Uh oh!

GordonTheTurtle commented Jun 22, 2017

Uh oh!

tophj-ibm commented Jun 23, 2017

Uh oh!

justincormack commented Jun 26, 2017 via email

Uh oh!

tophj-ibm commented Jul 6, 2017

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Jul 11, 2017

Uh oh!

tophj-ibm commented Jul 11, 2017

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

estesp left a comment

Choose a reason for hiding this comment

Uh oh!

tiborvass commented Jul 11, 2017

Uh oh!

tiborvass commented Jul 11, 2017

Uh oh!

thaJeztah commented Mar 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

tophj-ibm commented May 25, 2017 •

edited

Loading

thaJeztah left a comment •

edited

Loading