Pull request 1930: fewer-globals

Squashed commit of the following:

commit ce882cfff4c1f7afdf0cba13b39e6ee568eb812f
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Wed Jul 19 15:56:52 2023 +0300

    all: imp code, lint

commit 96fc5c589e7474f4bba291b0a20a0834148bb9c1
Merge: 3e91eea6b b0185201c
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Tue Jul 18 21:22:32 2023 +0300

    Merge branch 'master' into fewer-globals

commit 3e91eea6b68bac51251784e3069b1c9d241da439
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Tue Jul 18 19:01:45 2023 +0300

    home: rm globals

internal/home/control.go

@@ -176,12 +176,16 @@ func handleStatus(w http.ResponseWriter, r *http.Request) {
 // ------------------------
 // registration of handlers
 // ------------------------
-func registerControlHandlers() {
+func registerControlHandlers(web *webAPI) {
+	Context.mux.HandleFunc(
+		"/control/version.json",
+		postInstall(optionalAuth(web.handleVersionJSON)),
+	)
+	httpRegister(http.MethodPost, "/control/update", web.handleUpdate)
+
 	httpRegister(http.MethodGet, "/control/status", handleStatus)
 	httpRegister(http.MethodPost, "/control/i18n/change_language", handleI18nChangeLanguage)
 	httpRegister(http.MethodGet, "/control/i18n/current_language", handleI18nCurrentLanguage)
-	Context.mux.HandleFunc("/control/version.json", postInstall(optionalAuth(handleVersionJSON)))
-	httpRegister(http.MethodPost, "/control/update", handleUpdate)
 	httpRegister(http.MethodGet, "/control/profile", handleGetProfile)
 	httpRegister(http.MethodPut, "/control/profile/update", handlePutProfile)
 

internal/home/controlinstall.go

@@ -448,7 +448,7 @@ func (web *webAPI) handleInstallConfigure(w http.ResponseWriter, r *http.Request
 	web.conf.BindHost = req.Web.IP
 	web.conf.BindPort = req.Web.Port
 
-	registerControlHandlers()
+	registerControlHandlers(web)
 
 	aghhttp.OK(w)
 	if f, ok := w.(http.Flusher); ok {

internal/home/controlupdate.go

@@ -29,9 +29,9 @@ type temporaryError interface {
 // handleVersionJSON is the handler for the POST /control/version.json HTTP API.
 //
 // TODO(a.garipov): Find out if this API used with a GET method by anyone.
-func handleVersionJSON(w http.ResponseWriter, r *http.Request) {
+func (web *webAPI) handleVersionJSON(w http.ResponseWriter, r *http.Request) {
 	resp := &versionResponse{}
-	if Context.disableUpdate {
+	if web.conf.disableUpdate {
 		resp.Disabled = true
 		_ = aghhttp.WriteJSONResponse(w, r, resp)
 
@@ -52,7 +52,7 @@ func handleVersionJSON(w http.ResponseWriter, r *http.Request) {
 		}
 	}
 
-	err = requestVersionInfo(resp, req.Recheck)
+	err = web.requestVersionInfo(resp, req.Recheck)
 	if err != nil {
 		// Don't wrap the error, because it's informative enough as is.
 		aghhttp.Error(r, w, http.StatusBadGateway, "%s", err)
@@ -73,9 +73,10 @@ func handleVersionJSON(w http.ResponseWriter, r *http.Request) {
 
 // requestVersionInfo sets the VersionInfo field of resp if it can reach the
 // update server.
-func requestVersionInfo(resp *versionResponse, recheck bool) (err error) {
+func (web *webAPI) requestVersionInfo(resp *versionResponse, recheck bool) (err error) {
+	updater := web.conf.updater
 	for i := 0; i != 3; i++ {
-		resp.VersionInfo, err = Context.updater.VersionInfo(recheck)
+		resp.VersionInfo, err = updater.VersionInfo(recheck)
 		if err != nil {
 			var terr temporaryError
 			if errors.As(err, &terr) && terr.Temporary() {
@@ -95,7 +96,7 @@ func requestVersionInfo(resp *versionResponse, recheck bool) (err error) {
 	}
 
 	if err != nil {
-		vcu := Context.updater.VersionCheckURL()
+		vcu := updater.VersionCheckURL()
 
 		return fmt.Errorf("getting version info from %s: %w", vcu, err)
 	}
@@ -104,8 +105,9 @@ func requestVersionInfo(resp *versionResponse, recheck bool) (err error) {
 }
 
 // handleUpdate performs an update to the latest available version procedure.
-func handleUpdate(w http.ResponseWriter, r *http.Request) {
+func (web *webAPI) handleUpdate(w http.ResponseWriter, r *http.Request) {
-	if Context.updater.NewVersion() == "" {
+	updater := web.conf.updater
+	if updater.NewVersion() == "" {
 		aghhttp.Error(r, w, http.StatusBadRequest, "/update request isn't allowed now")
 
 		return
@@ -122,7 +124,7 @@ func handleUpdate(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	err = Context.updater.Update(false)
+	err = updater.Update(false)
 	if err != nil {
 		aghhttp.Error(r, w, http.StatusInternalServerError, "%s", err)
 
@@ -137,7 +139,7 @@ func 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(), execPath)
+	go finishUpdate(context.Background(), execPath, web.conf.runningAsService)
 }
 
 // versionResponse is the response for /control/version.json endpoint.
@@ -178,7 +180,7 @@ func tlsConfUsesPrivilegedPorts(c *tlsConfigSettings) (ok bool) {
 }
 
 // finishUpdate completes an update procedure.
-func finishUpdate(ctx context.Context, execPath string) {
+func finishUpdate(ctx context.Context, execPath string, runningAsService bool) {
 	var err error
 
 	log.Info("stopping all tasks")
@@ -187,7 +189,7 @@ func finishUpdate(ctx context.Context, execPath string) {
 	cleanupAlways()
 
 	if runtime.GOOS == "windows" {
-		if Context.runningAsService {
+		if runningAsService {
 			// NOTE: We can't restart the service via "kardianos/service"
 			// package, because it kills the process first we can't start a new
 			// instance, because Windows doesn't allow it.

internal/home/home.go

@@ -64,33 +64,24 @@ type homeContext struct {
 	// configuration files, for example /etc/hosts.
 	etcHosts *aghnet.HostsContainer
 
-	updater *updater.Updater
-
 	// mux is our custom http.ServeMux.
 	mux *http.ServeMux
 
 	// Runtime properties
 	// --
 
-	configFilename   string // Config filename (can be overridden via the command line arguments)
+	configFilename string // Config filename (can be overridden via the command line arguments)
-	workDir          string // Location of our directory, used to protect against CWD being somewhere else
+	workDir        string // Location of our directory, used to protect against CWD being somewhere else
-	pidFileName      string // PID file name.  Empty if no PID file was created.
+	pidFileName    string // PID file name.  Empty if no PID file was created.
-	controlLock      sync.Mutex
+	controlLock    sync.Mutex
-	tlsRoots         *x509.CertPool // list of root CAs for TLSv1.2
+	tlsRoots       *x509.CertPool // list of root CAs for TLSv1.2
-	appSignalChannel chan os.Signal // Channel for receiving OS signals by the console app
 
 	// tlsCipherIDs are the ID of the cipher suites that AdGuard Home must use.
 	tlsCipherIDs []uint16
 
-	// disableUpdate, if true, tells AdGuard Home to not check for updates.
-	disableUpdate bool
-
 	// firstRun, if true, tells AdGuard Home to only start the web interface
 	// service, and only serve the first-run APIs.
 	firstRun bool
-
-	// runningAsService flag is set to true when options are passed from the service runner
-	runningAsService bool
 }
 
 // getDataDir returns path to the directory where we store databases and filters
@@ -113,11 +104,11 @@ func Main(clientBuildFS fs.FS) {
 	// package flag.
 	opts := loadCmdLineOpts()
 
-	Context.appSignalChannel = make(chan os.Signal)
+	signals := make(chan os.Signal, 1)
-	signal.Notify(Context.appSignalChannel, syscall.SIGINT, syscall.SIGTERM, syscall.SIGHUP, syscall.SIGQUIT)
+	signal.Notify(signals, syscall.SIGINT, syscall.SIGTERM, syscall.SIGHUP, syscall.SIGQUIT)
 	go func() {
 		for {
-			sig := <-Context.appSignalChannel
+			sig := <-signals
 			log.Info("Received signal %q", sig)
 			switch sig {
 			case syscall.SIGHUP:
@@ -132,7 +123,7 @@ func Main(clientBuildFS fs.FS) {
 	}()
 
 	if opts.serviceControlAction != "" {
-		handleServiceControlAction(opts, clientBuildFS)
+		handleServiceControlAction(opts, clientBuildFS, signals)
 
 		return
 	}
@@ -144,61 +135,48 @@ func Main(clientBuildFS fs.FS) {
 // setupContext initializes [Context] fields.  It also reads and upgrades
 // config file if necessary.
 func setupContext(opts options) (err error) {
-	setupContextFlags(opts)
+	Context.firstRun = detectFirstRun()
 
 	Context.tlsRoots = aghtls.SystemRootCAs()
 	Context.mux = http.NewServeMux()
 
-	if !Context.firstRun {
+	if Context.firstRun {
-		// Do the upgrade if necessary.
+		log.Info("This is the first time AdGuard Home is launched")
-		err = upgradeConfig()
+		checkPermissions()
+
+		return nil
+	}
+
+	// Do the upgrade if necessary.
+	err = upgradeConfig()
+	if err != nil {
+		// Don't wrap the error, because it's informative enough as is.
+		return err
+	}
+
+	if err = parseConfig(); err != nil {
+		log.Error("parsing configuration file: %s", err)
+
+		os.Exit(1)
+	}
+
+	if opts.checkConfig {
+		log.Info("configuration file is ok")
+
+		os.Exit(0)
+	}
+
+	if !opts.noEtcHosts && config.Clients.Sources.HostsFile {
+		err = setupHostsContainer()
 		if err != nil {
 			// Don't wrap the error, because it's informative enough as is.
 			return err
 		}
-
-		if err = parseConfig(); err != nil {
-			log.Error("parsing configuration file: %s", err)
-
-			os.Exit(1)
-		}
-
-		if opts.checkConfig {
-			log.Info("configuration file is ok")
-
-			os.Exit(0)
-		}
-
-		if !opts.noEtcHosts && config.Clients.Sources.HostsFile {
-			err = setupHostsContainer()
-			if err != nil {
-				// Don't wrap the error, because it's informative enough as is.
-				return err
-			}
-		}
 	}
 
 	return nil
 }
 
-// setupContextFlags sets global flags and prints their status to the log.
-func setupContextFlags(opts options) {
-	Context.firstRun = detectFirstRun()
-	if Context.firstRun {
-		log.Info("This is the first time AdGuard Home is launched")
-		checkPermissions()
-	}
-
-	Context.runningAsService = opts.runningAsService
-	// Don't print the runningAsService flag, since that has already been done
-	// in [run].
-
-	Context.disableUpdate = opts.disableUpdate || version.Channel() == version.ChannelDevelopment
-	if Context.disableUpdate {
-		log.Info("AdGuard Home updates are disabled")
-	}
-}
-
 // logIfUnsupported logs a formatted warning if the error is one of the
 // unsupported errors and returns nil.  If err is nil, logIfUnsupported returns
 // nil.  Otherwise, it returns err.
@@ -303,7 +281,7 @@ func initContextClients() (err error) {
 		return err
 	}
 
-	//lint:ignore SA1019  Migration is not over.
+	//lint:ignore SA1019 Migration is not over.
 	config.DHCP.WorkDir = Context.workDir
 	config.DHCP.DataDir = Context.getDataDir()
 	config.DHCP.HTTPRegister = httpRegister
@@ -318,18 +296,6 @@ func initContextClients() (err error) {
 		return fmt.Errorf("initing dhcp: %w", err)
 	}
 
-	Context.updater = updater.NewUpdater(&updater.Config{
-		Client:   config.DNS.DnsfilterConf.HTTPClient,
-		Version:  version.Version(),
-		Channel:  version.Channel(),
-		GOARCH:   runtime.GOARCH,
-		GOOS:     runtime.GOOS,
-		GOARM:    version.GOARM(),
-		GOMIPS:   version.GOMIPS(),
-		WorkDir:  Context.workDir,
-		ConfName: config.getConfigFilename(),
-	})
-
 	var arpdb aghnet.ARPDB
 	if config.Clients.Sources.ARP {
 		arpdb = aghnet.NewARPDB()
@@ -493,7 +459,7 @@ func checkPorts() (err error) {
 	return nil
 }
 
-func initWeb(opts options, clientBuildFS fs.FS) (web *webAPI, err error) {
+func initWeb(opts options, clientBuildFS fs.FS, upd *updater.Updater) (web *webAPI, err error) {
 	var clientFS fs.FS
 	if opts.localFrontend {
 		log.Info("warning: using local frontend files")
@@ -506,8 +472,16 @@ func initWeb(opts options, clientBuildFS fs.FS) (web *webAPI, err error) {
 		}
 	}
 
-	webConf := webConfig{
+	disableUpdate := opts.disableUpdate || version.Channel() == version.ChannelDevelopment
-		firstRun: Context.firstRun,
+	if disableUpdate {
+		log.Info("AdGuard Home updates are disabled")
+	}
+
+	webConf := &webConfig{
+		updater: upd,
+
+		clientFS: clientFS,
+
 		BindHost: config.HTTPConfig.Address.Addr(),
 		BindPort: int(config.HTTPConfig.Address.Port()),
 
@@ -515,12 +489,13 @@ func initWeb(opts options, clientBuildFS fs.FS) (web *webAPI, err error) {
 		ReadHeaderTimeout: readHdrTimeout,
 		WriteTimeout:      writeTimeout,
 
-		clientFS: clientFS,
+		firstRun:         Context.firstRun,
-
+		disableUpdate:    disableUpdate,
-		serveHTTP3: config.DNS.ServeHTTP3,
+		runningAsService: opts.runningAsService,
+		serveHTTP3:       config.DNS.ServeHTTP3,
 	}
 
-	web = newWebAPI(&webConf)
+	web = newWebAPI(webConf)
 	if web == nil {
 		return nil, fmt.Errorf("initializing web: %w", err)
 	}
@@ -571,9 +546,21 @@ func run(opts options, clientBuildFS fs.FS) {
 	err = setupOpts(opts)
 	fatalOnError(err)
 
+	upd := updater.NewUpdater(&updater.Config{
+		Client:   config.DNS.DnsfilterConf.HTTPClient,
+		Version:  version.Version(),
+		Channel:  version.Channel(),
+		GOARCH:   runtime.GOARCH,
+		GOOS:     runtime.GOOS,
+		GOARM:    version.GOARM(),
+		GOMIPS:   version.GOMIPS(),
+		WorkDir:  Context.workDir,
+		ConfName: config.getConfigFilename(),
+	})
+
 	// TODO(e.burkov): This could be made earlier, probably as the option's
 	// effect.
-	cmdlineUpdate(opts)
+	cmdlineUpdate(opts, upd)
 
 	if !Context.firstRun {
 		// Save the updated config.
@@ -602,7 +589,7 @@ func run(opts options, clientBuildFS fs.FS) {
 		onConfigModified()
 	}
 
-	Context.web, err = initWeb(opts, clientBuildFS)
+	Context.web, err = initWeb(opts, clientBuildFS, upd)
 	fatalOnError(err)
 
 	if !Context.firstRun {
@@ -984,7 +971,7 @@ type jsonError struct {
 }
 
 // cmdlineUpdate updates current application and exits.
-func cmdlineUpdate(opts options) {
+func cmdlineUpdate(opts options, upd *updater.Updater) {
 	if !opts.performUpdate {
 		return
 	}
@@ -999,10 +986,9 @@ func cmdlineUpdate(opts options) {
 
 	log.Info("cmdline update: performing update")
 
-	updater := Context.updater
+	info, err := upd.VersionInfo(true)
-	info, err := updater.VersionInfo(true)
 	if err != nil {
-		vcu := updater.VersionCheckURL()
+		vcu := upd.VersionCheckURL()
 		log.Error("getting version info from %s: %s", vcu, err)
 
 		os.Exit(1)
@@ -1014,7 +1000,7 @@ func cmdlineUpdate(opts options) {
 		os.Exit(0)
 	}
 
-	err = updater.Update(Context.firstRun)
+	err = upd.Update(Context.firstRun)
 	fatalOnError(err)
 
 	err = restartService()

internal/home/service.go

@@ -33,9 +33,13 @@ const (
 // daemon.
 type program struct {
 	clientBuildFS fs.FS
+	signals       chan os.Signal
 	opts          options
 }
 
+// type check
+var _ service.Interface = (*program)(nil)
+
 // Start implements service.Interface interface for *program.
 func (p *program) Start(_ service.Service) (err error) {
 	// Start should not block.  Do the actual work async.
@@ -48,14 +52,14 @@ func (p *program) Start(_ service.Service) (err error) {
 }
 
 // Stop implements service.Interface interface for *program.
-func (p *program) Stop(_ service.Service) error {
+func (p *program) Stop(_ service.Service) (err error) {
-	// Stop should not block.  Return with a few seconds.
+	select {
-	if Context.appSignalChannel == nil {
+	case p.signals <- syscall.SIGINT:
-		os.Exit(0)
+		// Go on.
+	default:
+		// Stop should not block.
 	}
 
-	Context.appSignalChannel <- syscall.SIGINT
-
 	return nil
 }
 
@@ -194,7 +198,7 @@ func restartService() (err error) {
 //   - run:  This is a special command that is not supposed to be used directly
 //     it is specified when we register a service, and it indicates to the app
 //     that it is being run as a service/daemon.
-func handleServiceControlAction(opts options, clientBuildFS fs.FS) {
+func handleServiceControlAction(opts options, clientBuildFS fs.FS, signals chan os.Signal) {
 	// Call chooseSystem explicitly to introduce OpenBSD support for service
 	// package.  It's a noop for other GOOS values.
 	chooseSystem()
@@ -226,7 +230,11 @@ func handleServiceControlAction(opts options, clientBuildFS fs.FS) {
 	}
 	configureService(svcConfig)
 
-	s, err := service.New(&program{clientBuildFS: clientBuildFS, opts: runOpts}, svcConfig)
+	s, err := service.New(&program{
+		clientBuildFS: clientBuildFS,
+		signals:       signals,
+		opts:          runOpts,
+	}, svcConfig)
 	if err != nil {
 		log.Fatalf("service: initializing service: %s", err)
 	}

internal/home/web.go

@@ -11,6 +11,7 @@ import (
 
 	"github.com/AdguardTeam/AdGuardHome/internal/aghhttp"
 	"github.com/AdguardTeam/AdGuardHome/internal/aghnet"
+	"github.com/AdguardTeam/AdGuardHome/internal/updater"
 	"github.com/AdguardTeam/golibs/errors"
 	"github.com/AdguardTeam/golibs/log"
 	"github.com/AdguardTeam/golibs/netutil"
@@ -32,6 +33,8 @@ const (
 )
 
 type webConfig struct {
+	updater *updater.Updater
+
 	clientFS fs.FS
 
 	BindHost netip.Addr
@@ -51,6 +54,13 @@ type webConfig struct {
 
 	firstRun bool
 
+	// disableUpdate, if true, tells AdGuard Home to not check for updates.
+	disableUpdate bool
+
+	// runningAsService flag is set to true when options are passed from the
+	// service runner.
+	runningAsService bool
+
 	serveHTTP3 bool
 }
 
@@ -101,7 +111,7 @@ func newWebAPI(conf *webConfig) (w *webAPI) {
 		Context.mux.Handle("/install.html", preInstallHandler(clientFS))
 		w.registerInstallHandlers()
 	} else {
-		registerControlHandlers()
+		registerControlHandlers(w)
 	}
 
 	w.httpsServer.cond = sync.NewCond(&w.httpsServer.condLock)

scripts/make/go-lint.sh

@@ -189,6 +189,16 @@ run_linter gocognit --over 10\
 	./scripts/\
 	;
 
+# TODO(a.garipov): move these to the group above.
+run_linter gocognit --over 20 ./internal/aghnet/ ./internal/querylog/
+run_linter gocognit --over 19 ./internal/dnsforward/ ./internal/home/
+run_linter gocognit --over 18 ./internal/aghtls/
+run_linter gocognit --over 17 ./internal/filtering ./internal/filtering/rewrite/
+run_linter gocognit --over 15 ./internal/aghos/ ./internal/dhcpd/
+run_linter gocognit --over 14 ./internal/stats/
+run_linter gocognit --over 12 ./internal/updater/
+run_linter gocognit --over 11 ./internal/aghtest/
+
 run_linter ineffassign ./...
 
 run_linter unparam ./...