From 803f79d144d9b7cfb9c7a659dedec7de27cac0e4 Mon Sep 17 00:00:00 2001 From: Stanislav Chzhen Date: Thu, 28 Aug 2025 14:22:48 +0300 Subject: [PATCH] all: imp code --- internal/agh/agh.go | 65 +++++++++++++++++--------------- internal/aghos/os.go | 12 ++++-- internal/home/controlinstall.go | 18 +++++---- internal/home/controlupdate.go | 22 ++++++++--- internal/home/home.go | 13 ++++--- internal/home/service_linux.go | 27 ++++++++----- internal/home/service_openbsd.go | 16 ++++---- internal/home/web.go | 8 ++++ scripts/translations/main.go | 5 +++ 9 files changed, 117 insertions(+), 69 deletions(-) diff --git a/internal/agh/agh.go b/internal/agh/agh.go index c1cec8f3..221b82e6 100644 --- a/internal/agh/agh.go +++ b/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 diff --git a/internal/aghos/os.go b/internal/aghos/os.go index e756923d..abd3cee1 100644 --- a/internal/aghos/os.go +++ b/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{}, diff --git a/internal/home/controlinstall.go b/internal/home/controlinstall.go index b5b640cb..f214158a 100644 --- a/internal/home/controlinstall.go +++ b/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..., diff --git a/internal/home/controlupdate.go b/internal/home/controlupdate.go index 238d637b..e791633d 100644 --- a/internal/home/controlupdate.go +++ b/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, diff --git a/internal/home/home.go b/internal/home/home.go index 78a8ae68..995e1683 100644 --- a/internal/home/home.go +++ b/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, diff --git a/internal/home/service_linux.go b/internal/home/service_linux.go index d3534a42..3be6e8ea 100644 --- a/internal/home/service_linux.go +++ b/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, diff --git a/internal/home/service_openbsd.go b/internal/home/service_openbsd.go index db8f1d65..6d5fecc7 100644 --- a/internal/home/service_openbsd.go +++ b/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 } diff --git a/internal/home/web.go b/internal/home/web.go index 40f59230..276449b3 100644 --- a/internal/home/web.go +++ b/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, diff --git a/scripts/translations/main.go b/scripts/translations/main.go index c2e48b5a..14569109 100644 --- a/scripts/translations/main.go +++ b/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,