From 550b1798a169d07139f8f079bc5c83271a181f7f Mon Sep 17 00:00:00 2001 From: Eugene Burkov Date: Wed, 18 Aug 2021 13:20:56 +0300 Subject: [PATCH] Pull request: 3457 fix service reload Merge in DNS/adguard-home from 3457-darwin-svc-reload to master Closes #3457. Squashed commit of the following: commit e3d6fbccf8373194360b6480e2d702ccd0ec7107 Author: Eugene Burkov Date: Wed Aug 18 00:52:39 2021 +0300 aghos: imp docs commit 220d37ebc1e0c2e9ba37a34650bff1480bd2fcf6 Author: Eugene Burkov Date: Tue Aug 17 15:45:52 2021 +0300 all: fix ps --- CHANGELOG.md | 2 + internal/aghnet/systemresolvers_windows.go | 10 +-- internal/aghos/os.go | 98 ++++++++++++++++++++-- internal/aghos/os_bsd.go | 4 - internal/aghos/os_freebsd.go | 4 - internal/aghos/os_linux.go | 4 - internal/aghos/os_test.go | 98 ++++++++++++++++++++++ internal/aghos/os_windows.go | 6 -- internal/home/service.go | 47 ++++++----- 9 files changed, 220 insertions(+), 53 deletions(-) create mode 100644 internal/aghos/os_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 08b14548..8c1138c8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -65,6 +65,7 @@ and this project adheres to ### Fixed +- `reload` service action on macOS and FreeBSD ([#3457]). - Inaccurate using of service actions in the installation script ([#3450]). - Client ID checking ([#3437]). - Discovering other DHCP servers on `darwin` and `freebsd` ([#3417]). @@ -128,6 +129,7 @@ and this project adheres to [#3435]: https://github.com/AdguardTeam/AdGuardHome/issues/3435 [#3437]: https://github.com/AdguardTeam/AdGuardHome/issues/3437 [#3450]: https://github.com/AdguardTeam/AdGuardHome/issues/3450 +[#3457]: https://github.com/AdguardTeam/AdGuardHome/issues/3457 diff --git a/internal/aghnet/systemresolvers_windows.go b/internal/aghnet/systemresolvers_windows.go index 584b3636..6341ef4f 100644 --- a/internal/aghnet/systemresolvers_windows.go +++ b/internal/aghnet/systemresolvers_windows.go @@ -13,8 +13,6 @@ import ( "sync" "time" - "github.com/AdguardTeam/AdGuardHome/internal/aghio" - "github.com/AdguardTeam/AdGuardHome/internal/aghos" "github.com/AdguardTeam/golibs/errors" "github.com/AdguardTeam/golibs/log" ) @@ -115,12 +113,6 @@ func (sr *systemResolvers) getAddrs() (addrs []string, err error) { return nil, fmt.Errorf("getting the command's stdout pipe: %w", err) } - var stdoutLimited io.Reader - stdoutLimited, err = aghio.LimitReader(stdout, aghos.MaxCmdOutputSize) - if err != nil { - return nil, fmt.Errorf("limiting stdout reader: %w", err) - } - go writeExit(stdin) err = cmd.Start() @@ -128,7 +120,7 @@ func (sr *systemResolvers) getAddrs() (addrs []string, err error) { return nil, fmt.Errorf("start command executing: %w", err) } - s := bufio.NewScanner(stdoutLimited) + s := bufio.NewScanner(stdout) addrs = scanAddrs(s) err = cmd.Wait() diff --git a/internal/aghos/os.go b/internal/aghos/os.go index 2ecd87b2..7eb0b3cf 100644 --- a/internal/aghos/os.go +++ b/internal/aghos/os.go @@ -4,10 +4,16 @@ package aghos import ( + "bufio" "fmt" + "io" "os/exec" + "path" "runtime" - "syscall" + "strconv" + "strings" + + "github.com/AdguardTeam/golibs/log" ) // UnsupportedError is returned by functions and methods when a particular @@ -43,11 +49,6 @@ func HaveAdminRights() (bool, error) { return haveAdminRights() } -// SendProcessSignal sends signal to a process. -func SendProcessSignal(pid int, sig syscall.Signal) error { - return sendProcessSignal(pid, sig) -} - // MaxCmdOutputSize is the maximum length of performed shell command output. const MaxCmdOutputSize = 2 * 1024 @@ -65,6 +66,91 @@ func RunCommand(command string, arguments ...string) (int, string, error) { return cmd.ProcessState.ExitCode(), string(out), nil } +// PIDByCommand searches for process named command and returns its PID ignoring +// the PIDs from except. If no processes found, the error returned. +func PIDByCommand(command string, except ...int) (pid int, err error) { + // Don't use -C flag here since it's a feature of linux's ps + // implementation. Use POSIX-compatible flags instead. + // + // See https://github.com/AdguardTeam/AdGuardHome/issues/3457. + cmd := exec.Command("ps", "-A", "-o", "pid=", "-o", "comm=") + + var stdout io.ReadCloser + if stdout, err = cmd.StdoutPipe(); err != nil { + return 0, fmt.Errorf("getting the command's stdout pipe: %w", err) + } + + if err = cmd.Start(); err != nil { + return 0, fmt.Errorf("start command executing: %w", err) + } + + var instNum int + pid, instNum, err = parsePSOutput(stdout, command, except...) + if err != nil { + return 0, err + } + + if err = cmd.Wait(); err != nil { + return 0, fmt.Errorf("executing the command: %w", err) + } + + switch instNum { + case 0: + // TODO(e.burkov): Use constant error. + return 0, fmt.Errorf("no %s instances found", command) + case 1: + // Go on. + default: + log.Info("warning: %d %s instances found", instNum, command) + } + + if code := cmd.ProcessState.ExitCode(); code != 0 { + return 0, fmt.Errorf("ps finished with code %d", code) + } + + return pid, nil +} + +// parsePSOutput scans the output of ps searching the largest PID of the process +// associated with cmdName ignoring PIDs from ignore. Valid r's line shoud be +// like: +// +// 123 ./example-cmd +// 1230 some/base/path/example-cmd +// 3210 example-cmd +// +func parsePSOutput(r io.Reader, cmdName string, ignore ...int) (largest, instNum int, err error) { + s := bufio.NewScanner(r) +ScanLoop: + for s.Scan() { + fields := strings.Fields(s.Text()) + if len(fields) != 2 || path.Base(fields[1]) != cmdName { + continue + } + + cur, aerr := strconv.Atoi(fields[0]) + if aerr != nil || cur < 0 { + continue + } + + for _, pid := range ignore { + if cur == pid { + continue ScanLoop + } + } + + instNum++ + if cur > largest { + largest = cur + } + } + if err = s.Err(); err != nil { + return 0, 0, fmt.Errorf("scanning stdout: %w", err) + } + + return largest, instNum, nil +} + // IsOpenWrt returns true if host OS is OpenWrt. func IsOpenWrt() (ok bool) { return isOpenWrt() diff --git a/internal/aghos/os_bsd.go b/internal/aghos/os_bsd.go index d8557b43..48b76609 100644 --- a/internal/aghos/os_bsd.go +++ b/internal/aghos/os_bsd.go @@ -20,10 +20,6 @@ func haveAdminRights() (bool, error) { return os.Getuid() == 0, nil } -func sendProcessSignal(pid int, sig syscall.Signal) error { - return syscall.Kill(pid, sig) -} - func isOpenWrt() (ok bool) { return false } diff --git a/internal/aghos/os_freebsd.go b/internal/aghos/os_freebsd.go index b3ec7e77..33cd9d3f 100644 --- a/internal/aghos/os_freebsd.go +++ b/internal/aghos/os_freebsd.go @@ -20,10 +20,6 @@ func haveAdminRights() (bool, error) { return os.Getuid() == 0, nil } -func sendProcessSignal(pid int, sig syscall.Signal) error { - return syscall.Kill(pid, sig) -} - func isOpenWrt() (ok bool) { return false } diff --git a/internal/aghos/os_linux.go b/internal/aghos/os_linux.go index 5b1e5e87..349fc18a 100644 --- a/internal/aghos/os_linux.go +++ b/internal/aghos/os_linux.go @@ -25,10 +25,6 @@ func haveAdminRights() (bool, error) { return os.Getuid() == 0, nil } -func sendProcessSignal(pid int, sig syscall.Signal) error { - return syscall.Kill(pid, sig) -} - func isOpenWrt() (ok bool) { var err error ok, err = FileWalker(func(r io.Reader) (_ []string, cont bool, err error) { diff --git a/internal/aghos/os_test.go b/internal/aghos/os_test.go new file mode 100644 index 00000000..0ca567d8 --- /dev/null +++ b/internal/aghos/os_test.go @@ -0,0 +1,98 @@ +package aghos + +import ( + "bytes" + "testing" + + "github.com/AdguardTeam/AdGuardHome/internal/aghio" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestLargestLabeled(t *testing.T) { + const ( + comm = `command-name` + nl = "\n" + ) + + testCases := []struct { + name string + data []byte + wantPID int + wantInstNum int + }{{ + name: "success", + data: []byte(nl + + ` 123 not-a-` + comm + nl + + ` 321 ` + comm + nl, + ), + wantPID: 321, + wantInstNum: 1, + }, { + name: "several", + data: []byte(nl + + `1 ` + comm + nl + + `5 /` + comm + nl + + `2 /some/path/` + comm + nl + + `4 ./` + comm + nl + + `3 ` + comm + nl + + `10 .` + comm + nl, + ), + wantPID: 5, + wantInstNum: 5, + }, { + name: "no_any", + data: []byte(nl + + `1 ` + `not-a-` + comm + nl + + `2 ` + `not-a-` + comm + nl + + `3 ` + `not-a-` + comm + nl, + ), + wantPID: 0, + wantInstNum: 0, + }, { + name: "weird_input", + data: []byte(nl + + `abc ` + comm + nl + + `-1 ` + comm + nl, + ), + wantPID: 0, + wantInstNum: 0, + }} + + for _, tc := range testCases { + r := bytes.NewReader(tc.data) + + t.Run(tc.name, func(t *testing.T) { + pid, instNum, err := parsePSOutput(r, comm) + require.NoError(t, err) + + assert.Equal(t, tc.wantPID, pid) + assert.Equal(t, tc.wantInstNum, instNum) + }) + } + + t.Run("scanner_fail", func(t *testing.T) { + lr, err := aghio.LimitReader(bytes.NewReader([]byte{1, 2, 3}), 0) + require.NoError(t, err) + + target := &aghio.LimitReachedError{} + _, _, err = parsePSOutput(lr, "") + require.ErrorAs(t, err, &target) + + assert.EqualValues(t, 0, target.Limit) + }) + + t.Run("ignore", func(t *testing.T) { + r := bytes.NewReader([]byte(nl + + `1 ` + comm + nl + + `2 ` + comm + nl + + `3` + comm + nl, + )) + + pid, instances, err := parsePSOutput(r, comm, 1, 3) + require.NoError(t, err) + + assert.Equal(t, 2, pid) + assert.Equal(t, 1, instances) + }) +} diff --git a/internal/aghos/os_windows.go b/internal/aghos/os_windows.go index 8972e7b5..bff5a3f0 100644 --- a/internal/aghos/os_windows.go +++ b/internal/aghos/os_windows.go @@ -4,8 +4,6 @@ package aghos import ( - "syscall" - "golang.org/x/sys/windows" ) @@ -34,10 +32,6 @@ func haveAdminRights() (bool, error) { return true, nil } -func sendProcessSignal(pid int, sig syscall.Signal) error { - return Unsupported("kill") -} - func isOpenWrt() (ok bool) { return false } diff --git a/internal/home/service.go b/internal/home/service.go index c1b88af9..3b2216d0 100644 --- a/internal/home/service.go +++ b/internal/home/service.go @@ -97,41 +97,48 @@ func sendSigReload() { } pidfile := fmt.Sprintf("/var/run/%s.pid", serviceName) + var pid int data, err := os.ReadFile(pidfile) if errors.Is(err, os.ErrNotExist) { - var code int - var psdata string - code, psdata, err = aghos.RunCommand("ps", "-C", serviceName, "-o", "pid=") - if err != nil || code != 0 { - log.Error("finding AdGuardHome process: code: %d, error: %s", code, err) + if pid, err = aghos.PIDByCommand(serviceName, os.Getpid()); err != nil { + log.Error("finding AdGuardHome process: %s", err) return } - - data = []byte(psdata) } else if err != nil { log.Error("reading pid file %s: %s", pidfile, err) return + + } else { + parts := strings.SplitN(string(data), "\n", 2) + if len(parts) == 0 { + log.Error("can't read pid file %s: bad value", pidfile) + + return + } + + if pid, err = strconv.Atoi(strings.TrimSpace(parts[0])); err != nil { + log.Error("can't read pid file %s: %s", pidfile, err) + + return + } } - parts := strings.SplitN(string(data), "\n", 2) - if len(parts) == 0 { - log.Error("Can't read PID file %s: bad value", pidfile) + var proc *os.Process + if proc, err = os.FindProcess(pid); err != nil { + log.Error("can't send signal to pid %d: %s", pid, err) + return } - pid, err := strconv.Atoi(strings.TrimSpace(parts[0])) - if err != nil { - log.Error("Can't read PID file %s: %s", pidfile, err) + if err = proc.Signal(syscall.SIGHUP); err != nil { + log.Error("Can't send signal to pid %d: %s", pid, err) + return } - err = aghos.SendProcessSignal(pid, syscall.SIGHUP) - if err != nil { - log.Error("Can't send signal to PID %d: %s", pid, err) - return - } - log.Debug("Sent signal to PID %d", pid) + + log.Debug("sent signal to PID %d", pid) } // handleServiceControlAction one of the possible control actions: @@ -541,6 +548,6 @@ name="{{.Name}}" {{.Name}}_user="root" pidfile="/var/run/${name}.pid" command="/usr/sbin/daemon" -command_args="-P ${pidfile} -f -r {{.WorkingDirectory}}/{{.Name}}" +command_args="-p ${pidfile} -f -r {{.WorkingDirectory}}/{{.Name}}" run_rc_command "$1" `