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 <asarai@suse.de>

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 {
+		if werr := utils.WriteJSON(pipe, syncT{procError}); werr != nil {
-			//  Synchronisation only necessary for standard init.
+			fmt.Fprintln(os.Stderr, err)
-			if werr := utils.WriteJSON(pipe, syncT{procError}); werr != nil {
+			return
-				fmt.Fprintln(os.Stderr, err)
-				return
-			}
 		}
 		if werr := utils.WriteJSON(pipe, newSystemError(err)); werr != nil {
 			fmt.Fprintln(os.Stderr, err)

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 }}

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 := readSync(pipe, procRun); err != nil {
-	if err := json.NewDecoder(pipe).Decode(&procSync); err != nil {
+		return err
-		if err == io.EOF {
-			return fmt.Errorf("parent closed synchronisation channel")
-		}
-		if procSync.Type != procRun {
-			return fmt.Errorf("invalid synchronisation flag from parent")
-		}
 	}
+
 	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 := readSync(pipe, procResume); err != nil {
-	if err := json.NewDecoder(pipe).Decode(&procSync); err != nil {
+		return err
-		if err == io.EOF {
-			return fmt.Errorf("parent closed synchronisation channel")
-		}
-		if procSync.Type != procResume {
-			return fmt.Errorf("invalid synchronisation flag from parent")
-		}
 	}
+
 	return nil
 }
 

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)
+	ierr := parseSync(p.parentPipe, func(sync *syncT) error {
-loop:
+		switch sync.Type {
-	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 {
 		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()

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
+}