Merge pull request #3374 from kolyshkin/cli-nits

Assorted CLI nitpicks

Author: AkihiroSuda

checkpoint.go

@@ -60,16 +60,24 @@ checkpointed.`,
 			return err
 		}
 		if status == libcontainer.Created || status == libcontainer.Stopped {
-			fatal(fmt.Errorf("Container cannot be checkpointed in %s state", status.String()))
+			return fmt.Errorf("Container cannot be checkpointed in %s state", status.String())
 		}
-		options := criuOptions(context)
+		options, err := criuOptions(context)
+		if err != nil {
+			return err
+		}
+
 		if !(options.LeaveRunning || options.PreDump) {
 			// destroy container unless we tell CRIU to keep it
 			defer destroy(container)
 		}
 		// these are the mandatory criu options for a container
-		setPageServer(context, options)
-		setManageCgroupsMode(context, options)
+		if err := setPageServer(context, options); err != nil {
+			return err
+		}
+		if err := setManageCgroupsMode(context, options); err != nil {
+			return err
+		}
 		if err := setEmptyNsMask(context, options); err != nil {
 			return err
 		}
@@ -109,27 +117,28 @@ func prepareImagePaths(context *cli.Context) (string, string, error) {
 	return imagePath, parentPath, nil
 }
 
-func setPageServer(context *cli.Context, options *libcontainer.CriuOpts) {
+func setPageServer(context *cli.Context, options *libcontainer.CriuOpts) error {
 	// xxx following criu opts are optional
 	// The dump image can be sent to a criu page server
 	if psOpt := context.String("page-server"); psOpt != "" {
 		address, port, err := net.SplitHostPort(psOpt)
 
 		if err != nil || address == "" || port == "" {
-			fatal(errors.New("Use --page-server ADDRESS:PORT to specify page server"))
+			return errors.New("Use --page-server ADDRESS:PORT to specify page server")
 		}
 		portInt, err := strconv.Atoi(port)
 		if err != nil {
-			fatal(errors.New("Invalid port number"))
+			return errors.New("Invalid port number")
 		}
 		options.PageServer = libcontainer.CriuPageServerInfo{
 			Address: address,
 			Port:    int32(portInt),
 		}
 	}
+	return nil
 }
 
-func setManageCgroupsMode(context *cli.Context, options *libcontainer.CriuOpts) {
+func setManageCgroupsMode(context *cli.Context, options *libcontainer.CriuOpts) error {
 	if cgOpt := context.String("manage-cgroups-mode"); cgOpt != "" {
 		switch cgOpt {
 		case "soft":
@@ -139,9 +148,10 @@ func setManageCgroupsMode(context *cli.Context, options *libcontainer.CriuOpts)
 		case "strict":
 			options.ManageCgroupsMode = criu.CriuCgMode_STRICT
 		default:
-			fatal(errors.New("Invalid manage cgroups mode"))
+			return errors.New("Invalid manage cgroups mode")
 		}
 	}
+	return nil
 }
 
 var namespaceMapping = map[specs.LinuxNamespaceType]int{

list.go

@@ -5,7 +5,6 @@ import (
 	"errors"
 	"fmt"
 	"os"
-	"path/filepath"
 	"syscall"
 	"text/tabwriter"
 	"time"
@@ -111,18 +110,20 @@ To list containers created using a non-default value for "--root":
 }
 
 func getContainers(context *cli.Context) ([]containerState, error) {
-	factory, err := loadFactory(context)
-	if err != nil {
-		return nil, err
-	}
 	root := context.GlobalString("root")
-	absRoot, err := filepath.Abs(root)
+	list, err := os.ReadDir(root)
 	if err != nil {
+		if errors.Is(err, os.ErrNotExist) && context.IsSet("root") {
+			// Ignore non-existing default root directory
+			// (no containers created yet).
+			return nil, nil
+		}
+		// Report other errors, including non-existent custom --root.
 		return nil, err
 	}
-	list, err := os.ReadDir(absRoot)
+	factory, err := libcontainer.New(root)
 	if err != nil {
-		fatal(err)
+		return nil, err
 	}
 
 	var s []containerState
@@ -136,7 +137,7 @@ func getContainers(context *cli.Context) ([]containerState, error) {
 				// Possible race with runc delete.
 				continue
 			}
-			fatal(err)
+			return nil, err
 		}
 		// This cast is safe on Linux.
 		uid := st.Sys().(*syscall.Stat_t).Uid

main.go

@@ -71,13 +71,12 @@ func main() {
 	}
 	app.Version = strings.Join(v, "\n")
 
-	xdgRuntimeDir := ""
 	root := "/run/runc"
-	if shouldHonorXDGRuntimeDir() {
-		if runtimeDir := os.Getenv("XDG_RUNTIME_DIR"); runtimeDir != "" {
-			root = runtimeDir + "/runc"
-			xdgRuntimeDir = root
-		}
+	xdgDirUsed := false
+	xdgRuntimeDir := os.Getenv("XDG_RUNTIME_DIR")
+	if xdgRuntimeDir != "" && shouldHonorXDGRuntimeDir() {
+		root = xdgRuntimeDir + "/runc"
+		xdgDirUsed = true
 	}
 
 	app.Flags = []cli.Flag{
@@ -135,7 +134,7 @@ func main() {
 		featuresCommand,
 	}
 	app.Before = func(context *cli.Context) error {
-		if !context.IsSet("root") && xdgRuntimeDir != "" {
+		if !context.IsSet("root") && xdgDirUsed {
 			// According to the XDG specification, we need to set anything in
 			// XDG_RUNTIME_DIR to have a sticky bit if we don't want it to get
 			// auto-pruned.

restore.go

@@ -109,7 +109,10 @@ using the runc checkpoint command.`,
 			logrus.Warn("runc checkpoint is untested with rootless containers")
 		}
 
-		options := criuOptions(context)
+		options, err := criuOptions(context)
+		if err != nil {
+			return err
+		}
 		if err := setEmptyNsMask(context, options); err != nil {
 			return err
 		}
@@ -124,10 +127,10 @@ using the runc checkpoint command.`,
 	},
 }
 
-func criuOptions(context *cli.Context) *libcontainer.CriuOpts {
+func criuOptions(context *cli.Context) (*libcontainer.CriuOpts, error) {
 	imagePath, parentPath, err := prepareImagePaths(context)
 	if err != nil {
-		fatal(err)
+		return nil, err
 	}
 
 	return &libcontainer.CriuOpts{
@@ -145,5 +148,5 @@ func criuOptions(context *cli.Context) *libcontainer.CriuOpts {
 		StatusFd:                context.Int("status-fd"),
 		LsmProfile:              context.String("lsm-profile"),
 		LsmMountContext:         context.String("lsm-mount-context"),
-	}
+	}, nil
 }

rootless_linux.go

@@ -52,9 +52,6 @@ func shouldUseRootlessCgroupManager(context *cli.Context) (bool, error) {
 }
 
 func shouldHonorXDGRuntimeDir() bool {
-	if os.Getenv("XDG_RUNTIME_DIR") == "" {
-		return false
-	}
 	if os.Geteuid() != 0 {
 		return true
 	}

utils.go

@@ -1,6 +1,7 @@
 package main
 
 import (
+	"errors"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -96,17 +97,25 @@ func revisePidFile(context *cli.Context) error {
 	return context.Set("pid-file", pidFile)
 }
 
-// reviseRootDir convert the root to absolute path
+// reviseRootDir ensures that the --root option argument,
+// if specified, is converted to an absolute and cleaned path,
+// and that this path is sane.
 func reviseRootDir(context *cli.Context) error {
-	root := context.GlobalString("root")
-	if root == "" {
+	if !context.IsSet("root") {
 		return nil
 	}
-
-	root, err := filepath.Abs(root)
+	root, err := filepath.Abs(context.GlobalString("root"))
 	if err != nil {
 		return err
 	}
+	if root == "/" {
+		// This can happen if --root argument is
+		//  - "" (i.e. empty);
+		//  - "." (and the CWD is /);
+		//  - "../../.." (enough to get to /);
+		//  - "/" (the actual /).
+		return errors.New("Option --root argument should not be set to /")
+	}
 
 	return context.GlobalSet("root", root)
 }

utils_linux.go

@@ -23,25 +23,15 @@ import (
 
 var errEmptyID = errors.New("container id cannot be empty")
 
-// loadFactory returns the configured factory instance for execing containers.
-func loadFactory(context *cli.Context) (libcontainer.Factory, error) {
-	root := context.GlobalString("root")
-	abs, err := filepath.Abs(root)
-	if err != nil {
-		return nil, err
-	}
-
-	return libcontainer.New(abs)
-}
-
 // getContainer returns the specified container instance by loading it from state
 // with the default factory.
 func getContainer(context *cli.Context) (libcontainer.Container, error) {
 	id := context.Args().First()
 	if id == "" {
 		return nil, errEmptyID
 	}
-	factory, err := loadFactory(context)
+	root := context.GlobalString("root")
+	factory, err := libcontainer.New(root)
 	if err != nil {
 		return nil, err
 	}
@@ -194,7 +184,8 @@ func createContainer(context *cli.Context, id string, spec *specs.Spec) (libcont
 		return nil, err
 	}
 
-	factory, err := loadFactory(context)
+	root := context.GlobalString("root")
+	factory, err := libcontainer.New(root)
 	if err != nil {
 		return nil, err
 	}