From 3ce2a7d026fb7c09dd535705e807275860abd21b Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 5 Oct 2022 16:35:26 +0200 Subject: [PATCH 1/4] pkg/pidfile: pkg/pidfile: use strconv instead of fmt.Sprintf(), and unconvert Signed-off-by: Sebastiaan van Stijn --- pkg/pidfile/pidfile.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/pidfile/pidfile.go b/pkg/pidfile/pidfile.go index a4dac5d025..ffbb4db9c5 100644 --- a/pkg/pidfile/pidfile.go +++ b/pkg/pidfile/pidfile.go @@ -36,10 +36,10 @@ func New(path string) (*PIDFile, error) { return nil, err } // Note MkdirAll returns nil if a directory already exists - if err := system.MkdirAll(filepath.Dir(path), os.FileMode(0755)); err != nil { + if err := system.MkdirAll(filepath.Dir(path), 0o755); err != nil { return nil, err } - if err := os.WriteFile(path, []byte(fmt.Sprintf("%d", os.Getpid())), 0644); err != nil { + if err := os.WriteFile(path, []byte(strconv.Itoa(os.Getpid())), 0o644); err != nil { return nil, err } From 4917bcc0390b1fe224465467f6526cbd3b7cea76 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sat, 8 Oct 2022 19:13:13 +0200 Subject: [PATCH 2/4] pkg/pidfile: don't ignore all errors when reading file It's ok to ignore if the file doesn't exist, or if the file doesn't have a PID in it, but we should produce an error if the file exists, but we're unable to read it for other reasons. Signed-off-by: Sebastiaan van Stijn --- pkg/pidfile/pidfile.go | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/pkg/pidfile/pidfile.go b/pkg/pidfile/pidfile.go index ffbb4db9c5..19a23f2f0f 100644 --- a/pkg/pidfile/pidfile.go +++ b/pkg/pidfile/pidfile.go @@ -19,12 +19,17 @@ type PIDFile struct { } func checkPIDFileAlreadyExists(path string) error { - if pidByte, err := os.ReadFile(path); err == nil { - pidString := strings.TrimSpace(string(pidByte)) - if pid, err := strconv.Atoi(pidString); err == nil { - if processExists(pid) { - return fmt.Errorf("pid file found, ensure docker is not running or delete %s", path) - } + pidByte, err := os.ReadFile(path) + if err != nil { + if os.IsNotExist(err) { + return nil + } + return err + } + pidString := strings.TrimSpace(string(pidByte)) + if pid, err := strconv.Atoi(pidString); err == nil { + if processExists(pid) { + return fmt.Errorf("pid file found, ensure docker is not running or delete %s", path) } } return nil From dd8983f96cf4d45395c81101c7385c393d72db18 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sat, 8 Oct 2022 19:17:53 +0200 Subject: [PATCH 3/4] pkg/pidfile: reduce cyclomatic complexity, and small optimisation Use bytes.TrimSpace instead of using the strings package, which is more performant, and allows us to skip the intermediate variable. Also combined some "if" statements to reduce cyclomatic complexity. Signed-off-by: Sebastiaan van Stijn --- pkg/pidfile/pidfile.go | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/pkg/pidfile/pidfile.go b/pkg/pidfile/pidfile.go index 19a23f2f0f..04c68e20a0 100644 --- a/pkg/pidfile/pidfile.go +++ b/pkg/pidfile/pidfile.go @@ -4,11 +4,11 @@ package pidfile // import "github.com/docker/docker/pkg/pidfile" import ( + "bytes" "fmt" "os" "path/filepath" "strconv" - "strings" "github.com/docker/docker/pkg/system" ) @@ -26,11 +26,9 @@ func checkPIDFileAlreadyExists(path string) error { } return err } - pidString := strings.TrimSpace(string(pidByte)) - if pid, err := strconv.Atoi(pidString); err == nil { - if processExists(pid) { - return fmt.Errorf("pid file found, ensure docker is not running or delete %s", path) - } + pid, err := strconv.Atoi(string(bytes.TrimSpace(pidByte))) + if err == nil && processExists(pid) { + return fmt.Errorf("pid file found, ensure docker is not running or delete %s", path) } return nil } From 43d6eb7173e0402f395df39f0c4de8615c325787 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sat, 8 Oct 2022 19:28:51 +0200 Subject: [PATCH 4/4] pkg/pidfile: remove PIDFile type, rename New() to Write() This type felt really redundant; `pidfile.New()` takes the path of the file to create as an argument, so this is already known. The only thing the PIDFile type provided was a `Remove()` method, which was just calling `os.Remove()` on the path of the file. Signed-off-by: Sebastiaan van Stijn --- cmd/dockerd/daemon.go | 5 ++--- pkg/pidfile/pidfile.go | 27 +++++++-------------------- pkg/pidfile/pidfile_test.go | 26 ++++---------------------- 3 files changed, 13 insertions(+), 45 deletions(-) diff --git a/cmd/dockerd/daemon.go b/cmd/dockerd/daemon.go index b1d19bc407..3695af0d9e 100644 --- a/cmd/dockerd/daemon.go +++ b/cmd/dockerd/daemon.go @@ -139,13 +139,12 @@ func (cli *DaemonCli) start(opts *daemonOptions) (err error) { potentiallyUnderRuntimeDir := []string{cli.Config.ExecRoot} if cli.Pidfile != "" { - pf, err := pidfile.New(cli.Pidfile) - if err != nil { + if err := pidfile.Write(cli.Pidfile); err != nil { return errors.Wrap(err, "failed to start daemon") } potentiallyUnderRuntimeDir = append(potentiallyUnderRuntimeDir, cli.Pidfile) defer func() { - if err := pf.Remove(); err != nil { + if err := os.Remove(cli.Pidfile); err != nil { logrus.Error(err) } }() diff --git a/pkg/pidfile/pidfile.go b/pkg/pidfile/pidfile.go index 04c68e20a0..14773b653f 100644 --- a/pkg/pidfile/pidfile.go +++ b/pkg/pidfile/pidfile.go @@ -13,11 +13,6 @@ import ( "github.com/docker/docker/pkg/system" ) -// PIDFile is a file used to store the process ID of a running process. -type PIDFile struct { - path string -} - func checkPIDFileAlreadyExists(path string) error { pidByte, err := os.ReadFile(path) if err != nil { @@ -33,23 +28,15 @@ func checkPIDFileAlreadyExists(path string) error { return nil } -// New creates a PIDfile using the specified path. -func New(path string) (*PIDFile, error) { +// Write writes a "PID file" at the specified path. It returns an error if the +// file exists and contains a valid PID of a running process, or when failing +// to write the file. +func Write(path string) error { if err := checkPIDFileAlreadyExists(path); err != nil { - return nil, err + return err } - // Note MkdirAll returns nil if a directory already exists if err := system.MkdirAll(filepath.Dir(path), 0o755); err != nil { - return nil, err + return err } - if err := os.WriteFile(path, []byte(strconv.Itoa(os.Getpid())), 0o644); err != nil { - return nil, err - } - - return &PIDFile{path: path}, nil -} - -// Remove removes the PIDFile. -func (file PIDFile) Remove() error { - return os.Remove(file.path) + return os.WriteFile(path, []byte(strconv.Itoa(os.Getpid())), 0o644) } diff --git a/pkg/pidfile/pidfile_test.go b/pkg/pidfile/pidfile_test.go index 59860350a7..df3169acdb 100644 --- a/pkg/pidfile/pidfile_test.go +++ b/pkg/pidfile/pidfile_test.go @@ -1,37 +1,19 @@ package pidfile // import "github.com/docker/docker/pkg/pidfile" import ( - "os" "path/filepath" "testing" ) -func TestNewAndRemove(t *testing.T) { - dir, err := os.MkdirTemp(os.TempDir(), "test-pidfile") - if err != nil { - t.Fatal("Could not create test directory") - } - - path := filepath.Join(dir, "testfile") - file, err := New(path) +func TestWrite(t *testing.T) { + path := filepath.Join(t.TempDir(), "testfile") + err := Write(path) if err != nil { t.Fatal("Could not create test file", err) } - _, err = New(path) + err = Write(path) if err == nil { t.Fatal("Test file creation not blocked") } - - if err := file.Remove(); err != nil { - t.Fatal("Could not delete created test file") - } -} - -func TestRemoveInvalidPath(t *testing.T) { - file := PIDFile{path: filepath.Join("foo", "bar")} - - if err := file.Remove(); err == nil { - t.Fatal("Non-existing file doesn't give an error on delete") - } }