all: imp code

internal/agh/agh.go

@@ -37,6 +37,11 @@ type exitErr struct {
 	code osutil.ExitCode
 }
 
+// newExitErr returns a properly initialized exitErr with the provided code.
+func newExitErr(code osutil.ExitCode) (err exitErr) {
+	return exitErr{code: code}
+}
+
 // type check
 var _ executil.ExitCodeError = exitErr{}
 
@@ -62,7 +67,7 @@ type ExternalCommand struct {
 	Out string
 
 	// Code is returned as the exit code if non-zero.
-	Code int
+	Code osutil.ExitCode
 }
 
 // keyCommand builds a key for a command lookup.
@@ -87,9 +92,9 @@ func parseCommand(s string) (path string, args []string) {
 // NewMultipleCommandConstructor is a helper function that returns a mock
 // [executil.CommandConstructor] for tests that supports multiple commands.
 //
-// TODO(s.chzhen):  Use this.
-//
-// TODO(s.chzhen):  Move to aghtest once the import cycle is resolved.
+// TODO(s.chzhen):  Move to aghtest once the import cycle is resolved, since it
+// will be called from the aghnet package, which imports the whois package,
+// which in turn imports aghnet.
 func NewMultipleCommandConstructor(cmds ...ExternalCommand) (cs executil.CommandConstructor) {
 	table := make(map[string]ExternalCommand, len(cmds))
 	for _, ec := range cmds {
@@ -97,44 +102,44 @@ func NewMultipleCommandConstructor(cmds ...ExternalCommand) (cs executil.Command
 		table[keyCommand(p, a)] = ec
 	}
 
-	return &fakeexec.CommandConstructor{
-		OnNew: func(
-			_ context.Context,
-			conf *executil.CommandConfig,
-		) (c executil.Command, err error) {
-			ec := table[keyCommand(conf.Path, conf.Args)]
+	onNew := func(
+		_ context.Context,
+		conf *executil.CommandConfig,
+	) (c executil.Command, err error) {
+		ec := table[keyCommand(conf.Path, conf.Args)]
 
-			cmd := fakeexec.NewCommand()
-			cmd.OnStart = func(_ context.Context) (err error) {
-				if ec.Out != "" {
-					_, _ = conf.Stdout.Write([]byte(ec.Out))
-				}
-
-				return nil
+		cmd := fakeexec.NewCommand()
+		cmd.OnStart = func(_ context.Context) (err error) {
+			if ec.Out != "" {
+				_, _ = conf.Stdout.Write([]byte(ec.Out))
 			}
 
-			cmd.OnWait = func(_ context.Context) (err error) {
-				if ec.Err != nil {
-					return ec.Err
-				}
+			return nil
+		}
 
-				if ec.Code != 0 {
-					return exitErr{code: ec.Code}
-				}
-
-				return nil
+		cmd.OnWait = func(_ context.Context) (err error) {
+			if ec.Err != nil {
+				return ec.Err
 			}
 
-			return cmd, nil
-		},
+			if ec.Code != 0 {
+				return newExitErr(ec.Code)
+			}
+
+			return nil
+		}
+
+		return cmd, nil
 	}
+
+	return &fakeexec.CommandConstructor{OnNew: onNew}
 }
 
 // NewCommandConstructor is a helper function that returns a mock
 // [executil.CommandConstructor] for tests.
 func NewCommandConstructor(
 	_ string,
-	code int,
+	code osutil.ExitCode,
 	stdout string,
 	cmdErr error,
 ) (cs executil.CommandConstructor) {
@@ -158,7 +163,7 @@ func NewCommandConstructor(
 				}
 
 				if code != 0 {
-					return exitErr{code: code}
+					return newExitErr(code)
 				}
 
 				return nil

internal/aghos/os.go

@@ -87,9 +87,9 @@ func RunCommand(
 		return code, stderrBuf.Bytes(), nil
 	}
 
-	return osutil.ExitCodeFailure,
-		nil,
-		fmt.Errorf("command %q failed: %w: %s", command, err, stdoutBuf.Bytes())
+	code = osutil.ExitCodeFailure
+
+	return code, nil, fmt.Errorf("command %q failed: %w: %s", command, err, &stdoutBuf)
 }
 
 // psArgs holds the default ps arguments to avoid per-call slice allocations.
@@ -116,6 +116,12 @@ func PIDByCommand(
 	stdoutBuf := bytes.Buffer{}
 
 	// TODO(s.chzhen):  Catch stderr.
+	//
+	// TODO(s.chzhen):  Consider streaming the output if needed.  Using
+	// [io.Pipe] here is unnecessary; it complicates lifecycle management
+	// because you must read concurrently and explicitly Close the PipeWriter to
+	// signal EOF.  Since this command’s output is small, a bytes.Buffer via
+	// executil.Run is simplest and safe.
 	runErr := executil.Run(
 		ctx,
 		executil.SystemCommandConstructor{},

internal/home/controlinstall.go

@@ -130,6 +130,7 @@ func (req *checkConfReq) validateDNS(
 	ctx context.Context,
 	l *slog.Logger,
 	tcpPorts aghalg.UniqChecker[tcpPort],
+	cmdCons executil.CommandConstructor,
 ) (canAutofix bool, err error) {
 	defer func() { err = errors.Annotate(err, "validating ports: %w") }()
 
@@ -161,7 +162,7 @@ func (req *checkConfReq) validateDNS(
 	// Try to fix automatically.
 	canAutofix = checkDNSStubListener(ctx, l)
 	if canAutofix && req.DNS.Autofix {
-		if derr := disableDNSStubListener(ctx, l); derr != nil {
+		if derr := disableDNSStubListener(ctx, l, cmdCons); derr != nil {
 			l.ErrorContext(ctx, "disabling DNSStubListener", slogutil.KeyError, err)
 		}
 
@@ -189,7 +190,8 @@ func (web *webAPI) handleInstallCheckConfig(w http.ResponseWriter, r *http.Reque
 		resp.Web.Status = err.Error()
 	}
 
-	if resp.DNS.CanAutofix, err = req.validateDNS(r.Context(), web.logger, tcpPorts); err != nil {
+	resp.DNS.CanAutofix, err = req.validateDNS(r.Context(), web.logger, tcpPorts, web.cmdCons)
+	if err != nil {
 		resp.DNS.Status = err.Error()
 	} else if !req.DNS.IP.IsUnspecified() {
 		resp.StaticIP = handleStaticIP(req.DNS.IP, req.SetStaticIP)
@@ -283,7 +285,11 @@ const resolvConfPath = "/etc/resolv.conf"
 
 // disableDNSStubListener deactivates DNSStubListerner and returns an error, if
 // any.
-func disableDNSStubListener(ctx context.Context, l *slog.Logger) (err error) {
+func disableDNSStubListener(
+	ctx context.Context,
+	l *slog.Logger,
+	cmdCons executil.CommandConstructor,
+) (err error) {
 	dir := filepath.Dir(resolvedConfPath)
 	err = os.MkdirAll(dir, 0o755)
 	if err != nil {
@@ -302,9 +308,7 @@ func disableDNSStubListener(ctx context.Context, l *slog.Logger) (err error) {
 		return fmt.Errorf("os.Symlink: %s: %w", resolvConfPath, err)
 	}
 
-	const (
-		systemctlCmd = "systemctl"
-	)
+	const systemctlCmd = "systemctl"
 
 	systemctlArgs := []string{"reload-or-restart", "systemd-resolved"}
 
@@ -312,7 +316,7 @@ func disableDNSStubListener(ctx context.Context, l *slog.Logger) (err error) {
 
 	err = executil.RunWithPeek(
 		ctx,
-		executil.SystemCommandConstructor{},
+		cmdCons,
 		agh.DefaultOutputLimit,
 		systemctlCmd,
 		systemctlArgs...,

internal/home/controlupdate.go

@@ -148,7 +148,13 @@ func (web *webAPI) handleUpdate(w http.ResponseWriter, r *http.Request) {
 	// The background context is used because the underlying functions wrap it
 	// with timeout and shut down the server, which handles current request.  It
 	// also should be done in a separate goroutine for the same reason.
-	go finishUpdate(context.Background(), web.logger, execPath, web.conf.runningAsService)
+	go finishUpdate(
+		context.Background(),
+		web.logger,
+		web.cmdCons,
+		execPath,
+		web.conf.runningAsService,
+	)
 }
 
 // versionResponse is the response for /control/version.json endpoint.
@@ -187,7 +193,13 @@ func tlsConfUsesPrivilegedPorts(c *tlsConfigSettings) (ok bool) {
 
 // finishUpdate completes an update procedure.  It is intended to be used as a
 // goroutine.
-func finishUpdate(ctx context.Context, l *slog.Logger, execPath string, runningAsService bool) {
+func finishUpdate(
+	ctx context.Context,
+	l *slog.Logger,
+	cmdCons executil.CommandConstructor,
+	execPath string,
+	runningAsService bool,
+) {
 	defer slogutil.RecoverAndExit(ctx, l, osutil.ExitCodeFailure)
 
 	l.InfoContext(ctx, "stopping all tasks")
@@ -195,8 +207,6 @@ func finishUpdate(ctx context.Context, l *slog.Logger, execPath string, runningA
 	cleanup(ctx)
 	cleanupAlways()
 
-	cons := executil.SystemCommandConstructor{}
-
 	var err error
 	if runtime.GOOS == "windows" {
 		if runningAsService {
@@ -206,7 +216,7 @@ func finishUpdate(ctx context.Context, l *slog.Logger, execPath string, runningA
 			//
 			// TODO(a.garipov): Recheck the claim above.
 			var cmd executil.Command
-			cmd, err = cons.New(ctx, &executil.CommandConfig{
+			cmd, err = cmdCons.New(ctx, &executil.CommandConfig{
 				Path: "cmd",
 				Args: []string{"/c", "net stop AdGuardHome & net start AdGuardHome"},
 			})
@@ -225,7 +235,7 @@ func finishUpdate(ctx context.Context, l *slog.Logger, execPath string, runningA
 		l.InfoContext(ctx, "restarting", "exec_path", execPath, "args", os.Args[1:])
 
 		var cmd executil.Command
-		cmd, err = cons.New(ctx, &executil.CommandConfig{
+		cmd, err = cmdCons.New(ctx, &executil.CommandConfig{
 			Path:   execPath,
 			Args:   os.Args[1:],
 			Stdin:  os.Stdin,

internal/home/home.go

@@ -589,12 +589,13 @@ func initWeb(
 	disableUpdate := !isUpdateEnabled(ctx, baseLogger, &opts, isCustomUpdURL)
 
 	webConf := &webConfig{
-		updater:      upd,
-		logger:       logger,
-		baseLogger:   baseLogger,
-		confModifier: confModifier,
-		tlsManager:   tlsMgr,
-		auth:         auth,
+		CommandConstructor: executil.SystemCommandConstructor{},
+		updater:            upd,
+		logger:             logger,
+		baseLogger:         baseLogger,
+		confModifier:       confModifier,
+		tlsManager:         tlsMgr,
+		auth:               auth,
 
 		clientFS: clientFS,
 

internal/home/service_linux.go

@@ -60,6 +60,7 @@ func (sys *sysvSystem) New(i service.Interface, c *service.Config) (s service.Se
 	}
 
 	return &sysvService{
+		cmdCons: executil.SystemCommandConstructor{},
 		Service: s,
 		name:    c.Name,
 	}, nil
@@ -68,6 +69,9 @@ func (sys *sysvSystem) New(i service.Interface, c *service.Config) (s service.Se
 // sysvService is a wrapper for a SysV [service.Service] that supplements the
 // installation and uninstallation.
 type sysvService struct {
+	// cmdCons is used to run external commands.  It must not be nil.
+	cmdCons executil.CommandConstructor
+
 	// Service must have an unexported type *service.sysv.
 	service.Service
 
@@ -87,9 +91,8 @@ func (svc *sysvService) Install() (err error) {
 		return err
 	}
 
-	cmdCons := executil.SystemCommandConstructor{}
 	// TODO(s.chzhen):  Pass context.
-	_, _, err = aghos.RunCommand(context.TODO(), cmdCons, "update-rc.d", svc.name, "defaults")
+	_, _, err = aghos.RunCommand(context.TODO(), svc.cmdCons, "update-rc.d", svc.name, "defaults")
 
 	// Don't wrap an error since it's informative enough as is.
 	return err
@@ -104,9 +107,8 @@ func (svc *sysvService) Uninstall() (err error) {
 		return err
 	}
 
-	cmdCons := executil.SystemCommandConstructor{}
 	// TODO(s.chzhen):  Pass context.
-	_, _, err = aghos.RunCommand(context.TODO(), cmdCons, "update-rc.d", svc.name, "remove")
+	_, _, err = aghos.RunCommand(context.TODO(), svc.cmdCons, "update-rc.d", svc.name, "remove")
 
 	// Don't wrap an error since it's informative enough as is.
 	return err
@@ -133,6 +135,7 @@ func (sys *systemdSystem) New(i service.Interface, c *service.Config) (s service
 	}
 
 	return &systemdService{
+		cmdCons:  executil.SystemCommandConstructor{},
 		Service:  s,
 		unitName: fmt.Sprintf("%s.service", c.Name),
 	}, nil
@@ -144,6 +147,9 @@ var _ service.Service = (*systemdService)(nil)
 // systemdService is a wrapper for a systemd [service.Service] that enriches the
 // service status information.
 type systemdService struct {
+	// cmdCons is used to run external commands.  It must not be nil.
+	cmdCons executil.CommandConstructor
+
 	// Service is expected to have an unexported type *service.systemd.
 	service.Service
 
@@ -163,12 +169,15 @@ func (s *systemdService) Status() (status service.Status, err error) {
 		systemctlStdout bytes.Buffer
 	)
 
-	// TODO(s.chzhen):  Pass context.
-	ctx := context.TODO()
-
+	// TODO(s.chzhen):  Consider streaming the output if needed.  Using
+	// [io.Pipe] here is unnecessary; it complicates lifecycle management
+	// because you must read concurrently and explicitly Close the PipeWriter to
+	// signal EOF.  Since this command’s output is small, a bytes.Buffer via
+	// executil.Run is simplest and safe.
 	err = executil.Run(
-		ctx,
-		executil.SystemCommandConstructor{},
+		// TODO(s.chzhen):  Pass context.
+		context.TODO(),
+		s.cmdCons,
 		&executil.CommandConfig{
 			Path:   systemctlCmd,
 			Args:   systemctlArgs,

internal/home/service_openbsd.go

@@ -59,16 +59,18 @@ func (openbsdSystem) Interactive() (ok bool) {
 // New implements service.System interface for openbsdSystem.
 func (openbsdSystem) New(i service.Interface, c *service.Config) (s service.Service, err error) {
 	return &openbsdRunComService{
-		i:   i,
-		cfg: c,
+		cmdCons: executil.SystemCommandConstructor{},
+		i:       i,
+		cfg:     c,
 	}, nil
 }
 
 // openbsdRunComService is the RunCom-based service.Service to be used on the
 // OpenBSD.
 type openbsdRunComService struct {
-	i   service.Interface
-	cfg *service.Config
+	cmdCons executil.CommandConstructor
+	i       service.Interface
+	cfg     *service.Config
 }
 
 // Platform implements service.Service interface for *openbsdRunComService.
@@ -214,9 +216,8 @@ func (s *openbsdRunComService) configureSysStartup(enable bool) (err error) {
 
 	// TODO(s.chzhen):  Pass context.
 	ctx := context.TODO()
-	cmdCons := executil.SystemCommandConstructor{}
 	var code int
-	code, _, err = aghos.RunCommand(ctx, cmdCons, "rcctl", cmd, s.cfg.Name)
+	code, _, err = aghos.RunCommand(ctx, s.cmdCons, "rcctl", cmd, s.cfg.Name)
 	if err != nil {
 		return err
 	} else if code != 0 {
@@ -319,13 +320,12 @@ func (s *openbsdRunComService) runCom(cmd string) (out string, err error) {
 
 	// TODO(s.chzhen):  Pass context.
 	ctx := context.TODO()
-	cmdCons := executil.SystemCommandConstructor{}
 
 	// TODO(e.burkov):  It's possible that os.ErrNotExist is caused by
 	// something different than the service script's non-existence.  Keep it
 	// in mind, when replace the aghos.RunCommand.
 	var outData []byte
-	_, outData, err = aghos.RunCommand(ctx, cmdCons, scriptPath, cmd)
+	_, outData, err = aghos.RunCommand(ctx, s.cmdCons, scriptPath, cmd)
 	if errors.Is(err, os.ErrNotExist) {
 		return "", service.ErrNotInstalled
 	}

internal/home/web.go

@@ -20,6 +20,7 @@ import (
 	"github.com/AdguardTeam/golibs/netutil/httputil"
 	"github.com/AdguardTeam/golibs/netutil/urlutil"
 	"github.com/AdguardTeam/golibs/osutil"
+	"github.com/AdguardTeam/golibs/osutil/executil"
 	"github.com/NYTimes/gziphandler"
 	"github.com/quic-go/quic-go/http3"
 	"golang.org/x/net/http2"
@@ -39,6 +40,9 @@ const (
 )
 
 type webConfig struct {
+	// CommandConstructor is used to run external commands.  It must not be nil.
+	CommandConstructor executil.CommandConstructor
+
 	updater *updater.Updater
 
 	// logger is a slog logger used in webAPI. It must not be nil.
@@ -111,6 +115,9 @@ type webAPI struct {
 	// confModifier is used to update the global configuration.
 	confModifier agh.ConfigModifier
 
+	// cmdCons is used to run external commands.
+	cmdCons executil.CommandConstructor
+
 	// TODO(a.garipov): Refactor all these servers.
 	httpServer *http.Server
 
@@ -143,6 +150,7 @@ func newWebAPI(ctx context.Context, conf *webConfig) (w *webAPI) {
 	w = &webAPI{
 		conf:         conf,
 		confModifier: conf.confModifier,
+		cmdCons:      conf.CommandConstructor,
 		logger:       conf.logger,
 		baseLogger:   conf.baseLogger,
 		tlsManager:   conf.tlsManager,

scripts/translations/main.go

@@ -487,6 +487,11 @@ func changedLocales(
 	gitArgs := []string{"diff", "--numstat", localesDir}
 	l.DebugContext(ctx, "executing", "cmd", gitCmd, "args", gitArgs)
 
+	// TODO(s.chzhen):  Consider streaming the output if needed.  Using
+	// [io.Pipe] here is unnecessary; it complicates lifecycle management
+	// because you must read concurrently and explicitly Close the PipeWriter to
+	// signal EOF.  Since this command’s output is small, a bytes.Buffer via
+	// executil.Run is simplest and safe.
 	var out bytes.Buffer
 	err = executil.Run(ctx, cmdCons, &executil.CommandConfig{
 		Path:   gitCmd,