From 66a85861e18c332bc8e8f3c5f0d434d611f54328 Mon Sep 17 00:00:00 2001 From: Dominic Della Valle Date: Thu, 22 Jul 2021 12:06:14 -0400 Subject: [PATCH 1/3] unix: refactor command exec / run --- service_unix.go | 115 +++++++++++++++++++++++++----------------------- 1 file changed, 60 insertions(+), 55 deletions(-) diff --git a/service_unix.go b/service_unix.go index 69596224..5130d225 100644 --- a/service_unix.go +++ b/service_unix.go @@ -7,12 +7,13 @@ package service import ( - "bytes" + "errors" "fmt" "io" "io/ioutil" "log/syslog" "os/exec" + "strings" "syscall" ) @@ -64,76 +65,80 @@ func runWithOutput(command string, arguments ...string) (int, string, error) { return runCommand(command, true, arguments...) } -func runCommand(command string, readStdout bool, arguments ...string) (int, string, error) { - cmd := exec.Command(command, arguments...) - - var output string - var stdout io.ReadCloser - var err error - +func runCommand(command string, readStdout bool, arguments ...string) (exitStatus int, stdout string, err error) { + var ( + cmd = exec.Command(command, arguments...) + cmdErr = fmt.Errorf("exec `%s` failed", strings.Join(cmd.Args, " ")) + stdoutPipe, stderrPipe io.ReadCloser + ) + if stderrPipe, err = cmd.StderrPipe(); err != nil { + err = fmt.Errorf("%s to connect stderr pipe: %w", cmdErr, err) + return + } if readStdout { - // Connect pipe to read Stdout - stdout, err = cmd.StdoutPipe() - - if err != nil { - // Failed to connect pipe - return 0, "", fmt.Errorf("%q failed to connect stdout pipe: %v", command, err) + if stdoutPipe, err = cmd.StdoutPipe(); err != nil { + err = fmt.Errorf("%s to connect stdout pipe: %w", cmdErr, err) + return } } - // Connect pipe to read Stderr - stderr, err := cmd.StderrPipe() - - if err != nil { - // Failed to connect pipe - return 0, "", fmt.Errorf("%q failed to connect stderr pipe: %v", command, err) + // Execute the command. + if err = cmd.Start(); err != nil { + err = fmt.Errorf("%s: %w", cmdErr, err) + return } - // Do not use cmd.Run() - if err := cmd.Start(); err != nil { - // Problem while copying stdin, stdout, or stderr - return 0, "", fmt.Errorf("%q failed: %v", command, err) - } + // Process command outputs. + var ( + pipeErr = fmt.Errorf("%s while attempting to read", cmdErr) + stdoutErr = fmt.Errorf("%s from stdout", pipeErr) + stderrErr = fmt.Errorf("%s from stderr", pipeErr) - // Zero exit status - // Darwin: launchctl can fail with a zero exit status, - // so check for emtpy stderr - if command == "launchctl" { - slurp, _ := ioutil.ReadAll(stderr) - if len(slurp) > 0 && !bytes.HasSuffix(slurp, []byte("Operation now in progress\n")) { - return 0, "", fmt.Errorf("%q failed with stderr: %s", command, slurp) - } + errBuffer, readErr = ioutil.ReadAll(stderrPipe) + stderr = strings.TrimSuffix(string(errBuffer), "\n") + + haveStdErr = len(stderr) != 0 + ) + + // Always read stderr. + if readErr != nil { + err = fmt.Errorf("%s: %w", stderrErr, readErr) + return } + // Maybe read stdout. if readStdout { - out, err := ioutil.ReadAll(stdout) - if err != nil { - return 0, "", fmt.Errorf("%q failed while attempting to read stdout: %v", command, err) - } else if len(out) > 0 { - output = string(out) + outBuffer, readErr := ioutil.ReadAll(stdoutPipe) + if readErr != nil { + err = fmt.Errorf("%s: %w", stdoutErr, readErr) + return } + stdout = string(outBuffer) } - if err := cmd.Wait(); err != nil { - exitStatus, ok := isExitError(err) - if ok { - // Command didn't exit with a zero exit status. - return exitStatus, output, err + // Wait for command to finish. + if runErr := cmd.Wait(); runErr != nil { + var execErr *exec.ExitError + if errors.As(runErr, &execErr) { + if status, ok := execErr.Sys().(syscall.WaitStatus); ok { + exitStatus = status.ExitStatus() + } } - - // An error occurred and there is no exit status. - return 0, output, fmt.Errorf("%q failed: %v", command, err) + err = fmt.Errorf("%w: %s", cmdErr, runErr) + if haveStdErr { + err = fmt.Errorf("%w with stderr: %s", err, stderr) + } + return } - return 0, output, nil -} - -func isExitError(err error) (int, bool) { - if exiterr, ok := err.(*exec.ExitError); ok { - if status, ok := exiterr.Sys().(syscall.WaitStatus); ok { - return status.ExitStatus(), true - } + // Darwin: launchctl can fail with a zero exit status, + // so stderr must be inspected. + systemIsDarwin := command == "launchctl" + if systemIsDarwin && + haveStdErr && + !strings.HasSuffix(stderr, "Operation now in progress") { + err = fmt.Errorf("%w with stderr: %s", cmdErr, stderr) } - return 0, false + return } From 24fdf4f1cf9e6b8b6989c13ab76ebe6b92f41139 Mon Sep 17 00:00:00 2001 From: Dominic Della Valle Date: Thu, 22 Jul 2021 12:26:51 -0400 Subject: [PATCH 2/3] systemd: minor - get unit file name once --- service_systemd_linux.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/service_systemd_linux.go b/service_systemd_linux.go index 3dfcf8ae..6145d0d5 100644 --- a/service_systemd_linux.go +++ b/service_systemd_linux.go @@ -234,7 +234,8 @@ func (s *systemd) Run() (err error) { } func (s *systemd) Status() (Status, error) { - exitCode, out, err := runWithOutput("systemctl", "is-active", s.unitName()) + unitFile := s.unitName() + exitCode, out, err := runWithOutput("systemctl", "is-active", unitFile) if exitCode == 0 && err != nil { return StatusUnknown, err } @@ -244,7 +245,7 @@ func (s *systemd) Status() (Status, error) { return StatusRunning, nil case strings.HasPrefix(out, "inactive"): // inactive can also mean its not installed, check unit files - exitCode, out, err := runWithOutput("systemctl", "list-unit-files", "-t", "service", s.unitName()) + exitCode, out, err := runWithOutput("systemctl", "list-unit-files", "-t", "service", unitFile) if exitCode == 0 && err != nil { return StatusUnknown, err } From c51d18362b0fa5c79cf95867c9490f917fb3fb4f Mon Sep 17 00:00:00 2001 From: Dominic Della Valle Date: Thu, 22 Jul 2021 12:27:39 -0400 Subject: [PATCH 3/3] systemd: fix service start/stop control - service status was not being checked during control actions. This allowed for sequences like double start or double stop to proceed when this is likely an error. Also inconsistent with other platforms. --- service_systemd_linux.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/service_systemd_linux.go b/service_systemd_linux.go index 6145d0d5..970318ec 100644 --- a/service_systemd_linux.go +++ b/service_systemd_linux.go @@ -265,10 +265,24 @@ func (s *systemd) Status() (Status, error) { } func (s *systemd) Start() error { + status, err := s.Status() + if err != nil { + return err + } + if status == StatusRunning { + return fmt.Errorf("%s already running", s.Config.Name) + } return s.runAction("start") } func (s *systemd) Stop() error { + status, err := s.Status() + if err != nil { + return err + } + if status == StatusStopped { + return fmt.Errorf("%s already stopped", s.Config.Name) + } return s.runAction("stop") }