From 4776b4326a79338e9300fe4e47c67c78e71e236f Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Mon, 6 Jun 2016 20:26:35 +1000 Subject: [PATCH] libcontainer: refactor syncT handling To make the code cleaner, and more clear, refactor the syncT handling used when creating the `runc init` process. In addition, document the state changes so that people actually understand what is going on. Rather than only using syncT for the standard initProcess, use it for both initProcess and setnsProcess. This removes some special cases, as well as allowing for the use of syncT with setnsProcess. Also remove a bunch of the boilerplate around syncT handling. This patch is part of the console rewrite patchset. Signed-off-by: Aleksa Sarai --- libcontainer/factory_linux.go | 9 +-- libcontainer/generic_error.go | 14 ----- libcontainer/init_linux.go | 28 ++++------ libcontainer/process_linux.go | 56 ++++++++----------- libcontainer/sync.go | 102 ++++++++++++++++++++++++++++++++++ 5 files changed, 139 insertions(+), 70 deletions(-) create mode 100644 libcontainer/sync.go diff --git a/libcontainer/factory_linux.go b/libcontainer/factory_linux.go index 6e2bf3ad..fbaaf639 100644 --- a/libcontainer/factory_linux.go +++ b/libcontainer/factory_linux.go @@ -253,12 +253,9 @@ func (l *LinuxFactory) StartInitialization() (err error) { // send it back to the parent process in the form of an initError. // If container's init successed, syscall.Exec will not return, hence // this defer function will never be called. - if _, ok := i.(*linuxStandardInit); ok { - // Synchronisation only necessary for standard init. - if werr := utils.WriteJSON(pipe, syncT{procError}); werr != nil { - fmt.Fprintln(os.Stderr, err) - return - } + if werr := utils.WriteJSON(pipe, syncT{procError}); werr != nil { + fmt.Fprintln(os.Stderr, err) + return } if werr := utils.WriteJSON(pipe, newSystemError(err)); werr != nil { fmt.Fprintln(os.Stderr, err) diff --git a/libcontainer/generic_error.go b/libcontainer/generic_error.go index de37715c..6e7de2fe 100644 --- a/libcontainer/generic_error.go +++ b/libcontainer/generic_error.go @@ -9,20 +9,6 @@ import ( "github.com/opencontainers/runc/libcontainer/stacktrace" ) -type syncType uint8 - -const ( - procReady syncType = iota - procError - procRun - procHooks - procResume -) - -type syncT struct { - Type syncType `json:"type"` -} - var errorTemplate = template.Must(template.New("error").Parse(`Timestamp: {{.Timestamp}} Code: {{.ECode}} {{if .Message }} diff --git a/libcontainer/init_linux.go b/libcontainer/init_linux.go index aeccadd9..dd3673ab 100644 --- a/libcontainer/init_linux.go +++ b/libcontainer/init_linux.go @@ -155,19 +155,15 @@ func finalizeNamespace(config *initConfig) error { // indicate that it is cleared to Exec. func syncParentReady(pipe io.ReadWriter) error { // Tell parent. - if err := utils.WriteJSON(pipe, syncT{procReady}); err != nil { + if err := writeSync(pipe, procReady); err != nil { return err } + // Wait for parent to give the all-clear. - var procSync syncT - if err := json.NewDecoder(pipe).Decode(&procSync); err != nil { - if err == io.EOF { - return fmt.Errorf("parent closed synchronisation channel") - } - if procSync.Type != procRun { - return fmt.Errorf("invalid synchronisation flag from parent") - } + if err := readSync(pipe, procRun); err != nil { + return err } + return nil } @@ -176,19 +172,15 @@ func syncParentReady(pipe io.ReadWriter) error { // indicate that it is cleared to resume. func syncParentHooks(pipe io.ReadWriter) error { // Tell parent. - if err := utils.WriteJSON(pipe, syncT{procHooks}); err != nil { + if err := writeSync(pipe, procHooks); err != nil { return err } + // Wait for parent to give the all-clear. - var procSync syncT - if err := json.NewDecoder(pipe).Decode(&procSync); err != nil { - if err == io.EOF { - return fmt.Errorf("parent closed synchronisation channel") - } - if procSync.Type != procResume { - return fmt.Errorf("invalid synchronisation flag from parent") - } + if err := readSync(pipe, procResume); err != nil { + return err } + return nil } diff --git a/libcontainer/process_linux.go b/libcontainer/process_linux.go index 4b54e4b2..eef7e794 100644 --- a/libcontainer/process_linux.go +++ b/libcontainer/process_linux.go @@ -100,15 +100,24 @@ func (p *setnsProcess) start() (err error) { return newSystemErrorWithCause(err, "writing config to pipe") } + ierr := parseSync(p.parentPipe, func(sync *syncT) error { + // Currently this will noop. + switch sync.Type { + case procReady: + // This shouldn't happen. + panic("unexpected procReady in setns") + case procHooks: + // This shouldn't happen. + panic("unexpected procHooks in setns") + default: + return newSystemError(fmt.Errorf("invalid JSON payload from child")) + } + return nil + }) + if err := syscall.Shutdown(int(p.parentPipe.Fd()), syscall.SHUT_WR); err != nil { return newSystemErrorWithCause(err, "calling shutdown on init pipe") } - // wait for the child process to fully complete and receive an error message - // if one was encoutered - var ierr *genericError - if err := json.NewDecoder(p.parentPipe).Decode(&ierr); err != nil && err != io.EOF { - return newSystemErrorWithCause(err, "decoding init error from pipe") - } // Must be done after Shutdown so the child will exit and we can wait for it. if ierr != nil { p.wait() @@ -270,22 +279,12 @@ func (p *initProcess) start() error { return newSystemErrorWithCause(err, "sending config to init process") } var ( - procSync syncT sentRun bool sentResume bool - ierr *genericError ) - dec := json.NewDecoder(p.parentPipe) -loop: - for { - if err := dec.Decode(&procSync); err != nil { - if err == io.EOF { - break loop - } - return newSystemErrorWithCause(err, "decoding sync type from init pipe") - } - switch procSync.Type { + ierr := parseSync(p.parentPipe, func(sync *syncT) error { + switch sync.Type { case procReady: if err := p.manager.Set(p.config.Config); err != nil { return newSystemErrorWithCause(err, "setting cgroup config for ready process") @@ -316,7 +315,7 @@ loop: } } // Sync with child. - if err := utils.WriteJSON(p.parentPipe, syncT{procRun}); err != nil { + if err := writeSync(p.parentPipe, procRun); err != nil { return newSystemErrorWithCause(err, "writing syncT run type") } sentRun = true @@ -336,25 +335,17 @@ loop: } } // Sync with child. - if err := utils.WriteJSON(p.parentPipe, syncT{procResume}); err != nil { + if err := writeSync(p.parentPipe, procResume); err != nil { return newSystemErrorWithCause(err, "writing syncT resume type") } sentResume = true - case procError: - // wait for the child process to fully complete and receive an error message - // if one was encoutered - if err := dec.Decode(&ierr); err != nil && err != io.EOF { - return newSystemErrorWithCause(err, "decoding proc error from init") - } - if ierr != nil { - break loop - } - // Programmer error. - panic("No error following JSON procError payload.") default: return newSystemError(fmt.Errorf("invalid JSON payload from child")) } - } + + return nil + }) + if !sentRun { return newSystemErrorWithCause(ierr, "container init") } @@ -364,6 +355,7 @@ loop: if err := syscall.Shutdown(int(p.parentPipe.Fd()), syscall.SHUT_WR); err != nil { return newSystemErrorWithCause(err, "shutting down init pipe") } + // Must be done after Shutdown so the child will exit and we can wait for it. if ierr != nil { p.wait() diff --git a/libcontainer/sync.go b/libcontainer/sync.go new file mode 100644 index 00000000..aef1238d --- /dev/null +++ b/libcontainer/sync.go @@ -0,0 +1,102 @@ +package libcontainer + +import ( + "encoding/json" + "fmt" + "io" + + "github.com/opencontainers/runc/libcontainer/utils" +) + +type syncType uint8 + +// Constants that are used for synchronisation between the parent and child +// during container setup. They come in pairs (with procError being a generic +// response which is followed by a &genericError). +// +// [ child ] <-> [ parent ] +// +// procHooks --> [run hooks] +// <-- procResume +// +// procReady --> [final setup] +// <-- procRun +const ( + procError syncType = iota + procReady + procRun + procHooks + procResume +) + +type syncT struct { + Type syncType `json:"type"` +} + +// writeSync is used to write to a synchronisation pipe. An error is returned +// if there was a problem writing the payload. +func writeSync(pipe io.Writer, sync syncType) error { + if err := utils.WriteJSON(pipe, syncT{sync}); err != nil { + return err + } + return nil +} + +// readSync is used to read from a synchronisation pipe. An error is returned +// if we got a genericError, the pipe was closed, or we got an unexpected flag. +func readSync(pipe io.Reader, expected syncType) error { + var procSync syncT + if err := json.NewDecoder(pipe).Decode(&procSync); err != nil { + if err == io.EOF { + return fmt.Errorf("parent closed synchronisation channel") + } + + if procSync.Type == procError { + var ierr genericError + + if err := json.NewDecoder(pipe).Decode(&ierr); err != nil { + return fmt.Errorf("failed reading error from parent: %v", err) + } + + return &ierr + } + + if procSync.Type != expected { + return fmt.Errorf("invalid synchronisation flag from parent") + } + } + return nil +} + +// parseSync runs the given callback function on each syncT received from the +// child. It will return once io.EOF is returned from the given pipe. +func parseSync(pipe io.Reader, fn func(*syncT) error) error { + dec := json.NewDecoder(pipe) + for { + var sync syncT + if err := dec.Decode(&sync); err != nil { + if err == io.EOF { + break + } + return err + } + + // We handle this case outside fn for cleanliness reasons. + var ierr *genericError + if sync.Type == procError { + if err := dec.Decode(&ierr); err != nil && err != io.EOF { + return newSystemErrorWithCause(err, "decoding proc error from init") + } + if ierr != nil { + return ierr + } + // Programmer error. + panic("No error following JSON procError payload.") + } + + if err := fn(&sync); err != nil { + return err + } + } + return nil +}