From 1545ea69b7688872f2233d4ff333fd1b02367f47 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Wed, 4 Aug 2021 20:08:40 -0700 Subject: [PATCH 1/3] delete, start: remove newline from errors Error messages should not usually contain newlines. Testing shows that the error runc delete prints is the same before and after this commit: [kir@kir-rhat runc-tst]$ sudo ../runc/runc delete xx3 ERRO[0000] cannot delete container xx3 that is not stopped: running [kir@kir-rhat runc-tst]$ Signed-off-by: Kir Kolyshkin --- delete.go | 2 +- start.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/delete.go b/delete.go index 6e6b463ef..746b0df54 100644 --- a/delete.go +++ b/delete.go @@ -79,7 +79,7 @@ status of "ubuntu01" as "stopped" the following will delete resources held for if force { return killContainer(container) } - return fmt.Errorf("cannot delete container %s that is not stopped: %s\n", id, s) + return fmt.Errorf("cannot delete container %s that is not stopped: %s", id, s) } return nil diff --git a/start.go b/start.go index 3a1769a43..338737c0a 100644 --- a/start.go +++ b/start.go @@ -48,7 +48,7 @@ your host.`, case libcontainer.Running: return errors.New("cannot start an already running container") default: - return fmt.Errorf("cannot start a container in the %s state\n", status) + return fmt.Errorf("cannot start a container in the %s state", status) } }, } From 9ba2f65d6be5482b37203e11b6aa7cfc9a4def5f Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 14 Sep 2021 10:46:48 -0700 Subject: [PATCH 2/3] startContainer: minor refactor All three callers* of startContainer call revisePidFile and createSpec before calling it, so it makes sense to move those calls to inside of the startContainer, and drop the spec argument. * -- in fact restore does not call revisePidFile, but it should. Signed-off-by: Kir Kolyshkin --- create.go | 9 +-------- restore.go | 6 +----- run.go | 9 +-------- utils_linux.go | 10 +++++++++- 4 files changed, 12 insertions(+), 22 deletions(-) diff --git a/create.go b/create.go index 5f3ac6095..a27e7c45f 100644 --- a/create.go +++ b/create.go @@ -55,14 +55,7 @@ command(s) that get executed on start, edit the args parameter of the spec. See if err := checkArgs(context, 1, exactArgs); err != nil { return err } - if err := revisePidFile(context); err != nil { - return err - } - spec, err := setupSpec(context) - if err != nil { - return err - } - status, err := startContainer(context, spec, CT_ACT_CREATE, nil) + status, err := startContainer(context, CT_ACT_CREATE, nil) if err != nil { return err } diff --git a/restore.go b/restore.go index 4e2841ba2..5f6c72d0d 100644 --- a/restore.go +++ b/restore.go @@ -104,15 +104,11 @@ using the runc checkpoint command.`, logrus.Warn("runc checkpoint is untested with rootless containers") } - spec, err := setupSpec(context) - if err != nil { - return err - } options := criuOptions(context) if err := setEmptyNsMask(context, options); err != nil { return err } - status, err := startContainer(context, spec, CT_ACT_RESTORE, options) + status, err := startContainer(context, CT_ACT_RESTORE, options) if err != nil { return err } diff --git a/run.go b/run.go index c3b184716..4c46f4f6c 100644 --- a/run.go +++ b/run.go @@ -68,14 +68,7 @@ command(s) that get executed on start, edit the args parameter of the spec. See if err := checkArgs(context, 1, exactArgs); err != nil { return err } - if err := revisePidFile(context); err != nil { - return err - } - spec, err := setupSpec(context) - if err != nil { - return err - } - status, err := startContainer(context, spec, CT_ACT_RUN, nil) + status, err := startContainer(context, CT_ACT_RUN, nil) if err == nil { // exit with the container's exit status so any external supervisor is // notified of the exit with the correct exit status. diff --git a/utils_linux.go b/utils_linux.go index 5404f2eb9..11957f781 100644 --- a/utils_linux.go +++ b/utils_linux.go @@ -395,7 +395,15 @@ const ( CT_ACT_RESTORE ) -func startContainer(context *cli.Context, spec *specs.Spec, action CtAct, criuOpts *libcontainer.CriuOpts) (int, error) { +func startContainer(context *cli.Context, action CtAct, criuOpts *libcontainer.CriuOpts) (int, error) { + if err := revisePidFile(context); err != nil { + return -1, err + } + spec, err := setupSpec(context) + if err != nil { + return -1, err + } + id := context.Args().First() if id == "" { return -1, errEmptyID From d974b22ac4877d15348f6747608be799bf5c343a Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Wed, 4 Aug 2021 20:13:39 -0700 Subject: [PATCH 3/3] create, run: amend final errors As the error may contain anything, it may not be clear to a user that the whole (create or run) operation failed. Amend the errors. Also, change the code flow in create to match that of run, so we don't have to add the fake "return nil" at the end. Signed-off-by: Kir Kolyshkin --- create.go | 12 ++++++------ run.go | 3 ++- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/create.go b/create.go index a27e7c45f..97854b846 100644 --- a/create.go +++ b/create.go @@ -1,6 +1,7 @@ package main import ( + "fmt" "os" "github.com/urfave/cli" @@ -56,12 +57,11 @@ command(s) that get executed on start, edit the args parameter of the spec. See return err } status, err := startContainer(context, CT_ACT_CREATE, nil) - if err != nil { - return err + if err == nil { + // exit with the container's exit status so any external supervisor + // is notified of the exit with the correct exit status. + os.Exit(status) } - // exit with the container's exit status so any external supervisor is - // notified of the exit with the correct exit status. - os.Exit(status) - return nil + return fmt.Errorf("runc create failed: %w", err) }, } diff --git a/run.go b/run.go index 4c46f4f6c..82781669d 100644 --- a/run.go +++ b/run.go @@ -1,6 +1,7 @@ package main import ( + "fmt" "os" "github.com/urfave/cli" @@ -74,6 +75,6 @@ command(s) that get executed on start, edit the args parameter of the spec. See // notified of the exit with the correct exit status. os.Exit(status) } - return err + return fmt.Errorf("runc run failed: %w", err) }, }