Pull request: 3457 fix service reload

Merge in DNS/adguard-home from 3457-darwin-svc-reload to master

Closes AdguardTeam#3457.

Squashed commit of the following:

commit e3d6fbc
Author: Eugene Burkov <e.burkov@adguard.com>
Date:   Wed Aug 18 00:52:39 2021 +0300

    aghos: imp docs

commit 220d37e
Author: Eugene Burkov <e.burkov@adguard.com>
Date:   Tue Aug 17 15:45:52 2021 +0300

    all: fix ps

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
 
 
 

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,20 +113,14 @@ 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()
 	if err != nil {
 		return nil, fmt.Errorf("start command executing: %w", err)
 	}
 
-	s := bufio.NewScanner(stdoutLimited)
+	s := bufio.NewScanner(stdout)
 	addrs = scanAddrs(s)
 
 	err = cmd.Wait()

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()

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
 }

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
 }

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) {

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)
+	})
+}

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
 }

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
-	}
 
-	parts := strings.SplitN(string(data), "\n", 2)
-	if len(parts) == 0 {
-		log.Error("Can't read PID file %s: bad value", pidfile)
-		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
+		}
 	}
 
-	pid, err := strconv.Atoi(strings.TrimSpace(parts[0]))
-	if err != nil {
-		log.Error("Can't read PID file %s: %s", pidfile, err)
+	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
 	}
-	err = aghos.SendProcessSignal(pid, syscall.SIGHUP)
-	if err != nil {
-		log.Error("Can't send signal to PID %d: %s", pid, err)
+
+	if err = proc.Signal(syscall.SIGHUP); 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"
 `