Adds forkexec library that supports user namespaces in Go. #150
Conversation
|
ping @crosbymichael |
| "unsafe" | ||
| ) | ||
|
|
||
| func BeforeFork() |
There was a problem hiding this comment.
Would it be worth noting in a comment here that these come from forklib.c?
|
Yes, i would add some comments to the exported functions of the package. |
| var wstatus WaitStatus | ||
|
|
||
| sys := attr.Sys | ||
| /* |
There was a problem hiding this comment.
Is this supposed to be commented out?
There was a problem hiding this comment.
Fixed to match what is done in exec_unix.go
|
I don't know about the name. Maybe something like "github.com/libcontainer/forkexec" or just fork? Just some ideas |
|
@crosbymichael Yeah, wasn't 100% sure on the name so just picked one to get a PR going. I don't mind forkexec. |
|
Renamed to forkexec (till we finalize a name) and added some comments. |
mrunalp
changed the title
mrunalp
changed the title
| var err1 Errno | ||
| var wstatus WaitStatus | ||
|
|
||
| sys := attr.Sys |
There was a problem hiding this comment.
isn't this a potentially unsafe ref if attr is nil?
There was a problem hiding this comment.
I will fix it. Thanks for reviewing!
Sent from my iPhone
On Aug 11, 2014, at 3:25 PM, Brandon Philips notifications@github.com wrote:
In forkexec/forkexec.go:
+// These functions are defined in forkexec.c
+func BeforeFork()
+func AfterFork()
+
+var zeroProcAttr ProcAttr
+var zeroSysProcAttr SysProcAttr
+
+// This is similar to the standard go library implementation of forkExec with the addition
+// of capability to write UID/GID mappings for user namespaces.
+func ForkExecNew(argv0 string, argv []string, attr *ProcAttr, uidMappings, gidMappings []IdMap) (pid int, err error) {
- var p [2]int
- var n int
- var err1 Errno
- var wstatus WaitStatus
- sys := attr.Sys
isn't this a potentially unsafe ref if attr is nil?—
Reply to this email directly or view it on GitHub.
|
lgtm besides the possible bug and dropping the BSD code. |
Docker-DCO-1.1-Signed-off-by: Mrunal Patel <mrunalp@gmail.com> (github: mrunalp)
|
Addressed @philips comments. |
|
lgtm |
|
LGTM |
1 similar comment
|
LGTM |
|
I'm running the example but I'm getting a panic now: fatal error: split stack after fork
runtime stack:
runtime.throw(0x5654ee)
/root/go/src/pkg/runtime/panic.c:520 +0x69
runtime.newstack()
/root/go/src/pkg/runtime/stack.c:679 +0x48
runtime.morestack()
/root/go/src/pkg/runtime/asm_amd64.s:228 +0x61
goroutine 16 [running]:
github.com/docker/libcontainer/forkexec.AfterFork()
/root/development/gocode/src/github.com/docker/libcontainer/forkexec/forkexec.c:10 +0x13 fp=0x7f707e949b78 sp=0x7f707e949b70
github.com/docker/libcontainer/forkexec.forkAndExecInChild(0xc208000180, 0xc208000190, 0x2, 0x2, 0xc20803c018, 0x1, 0x1, 0x0, 0x0, 0x7f707e949f00, ...)
/root/development/gocode/src/github.com/docker/libcontainer/forkexec/forkexec.go:157 +0x187 fp=0x7f707e949c48 sp=0x7f707e949b78
github.com/docker/libcontainer/forkexec.ForkExecNew(0x4d4530, 0x7, 0x7f707e949e90, 0x1, 0x1, 0x7f707e949f00, 0x7f707e949e44, 0x1, 0x1, 0x7f707e949e38, ...)
/root/development/gocode/src/github.com/docker/libcontainer/forkexec/forkexec.go:77 +0x49f fp=0x7f707e949d98 sp=0x7f707e949c48
main.main()
/root/development/gocode/src/github.com/docker/libcontainer/forkexec/usernstest/main.go:34 +0x346 fp=0x7f707e949f50 sp=0x7f707e949d98
runtime.main()
/root/go/src/pkg/runtime/proc.c:247 +0x11a fp=0x7f707e949fa8 sp=0x7f707e949f50
runtime.goexit()
/root/go/src/pkg/runtime/proc.c:1445 fp=0x7f707e949fb0 sp=0x7f707e949fa8
created by _rt0_go
/root/go/src/pkg/runtime/asm_amd64.s:97 +0x120
goroutine 17 [runnable]:
runtime.MHeap_Scavenger()
/root/go/src/pkg/runtime/mheap.c:507
runtime.goexit()
/root/go/src/pkg/runtime/proc.c:1445
goroutine 18 [runnable]:
bgsweep()
/root/go/src/pkg/runtime/mgc0.c:1976
runtime.goexit()
/root/go/src/pkg/runtime/proc.c:1445
goroutine 19 [runnable]:
runfinq()
/root/go/src/pkg/runtime/mgc0.c:2606
runtime.goexit()
/root/go/src/pkg/runtime/proc.c:1445 |
|
Hmm. Is it with unpatched go? Sent from my iPhone
|
|
@mrunalp There is a Go runtime patch required? I didn't see this mentioned in the PR. |
|
No, no Go patch required |
|
No, this is a workaround so we don't need to patch go. Might have issues so would be good if folks test this. I tried on 3 vms / machines and worked for me. But with the panic that crobymichael saw, this may not work out :( Sent from my iPhone
|
|
@crosbymichael Tested some more. This works with fedora default go 1.2.2 but fails like it did for you when I used golang 1.3 compiled from source. I guess back to pushing the patch upstream. |
|
@vmarmol Could you help get some attention/discussion around -- https://groups.google.com/forum/#!searchin/golang-nuts/User$20Namespaces/golang-nuts/J60fxDVSaco/EU_qJGqTNUkJ ? |
|
@mrunalp I think we might get more discussion after we propose the flag. No discussion usually means no one hates the suggestion :) But they might come back when there's existing code. I'll ping some go guys locally. |
|
@rjnagal Ahh, well -- that's good to know :) I will start on the proposal. Let us know what the go folks think. |
|
golang-nuts is the user list. You'd get more discussion on golang-dev. In this case I'd even say file a bug so we track it: https://code.google.com/p/go/issues/list |
|
@bradfitz Thanks for chiming in! :) I did open an issue -- https://code.google.com/p/go/issues/detail?id=8447, but trying to figure out the next steps. I have posted to golang-dev just now -- https://groups.google.com/forum/#!topic/golang-dev/qZeWlFQ_hLw. Should I wait for a discussion or start the code review process as described here -- http://golang.org/doc/contribute.html ? |
|
I'll close this for now until we can figure out if this should go in the Go stdlib |
|
SGTM. |
This PR adds a library that could be used to clone into user namespaces and write uid/gid mappings.
(This is based on joint effort between me and crosbymichael).
Docker-DCO-1.1-Signed-off-by: Mrunal Patel mrunalp@gmail.com (github: mrunalp)