Pull request 2149: 6711 watch hosts

Updates #6711.

Squashed commit of the following:

commit 3ddfe809f76325c2d4cda0715a7bcc15e76a2388
Merge: 185957cd0 d338451fa
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Tue Feb 13 13:01:30 2024 +0300

    Merge branch 'master' into 6711-watch-hosts

commit 185957cd01516e5955e388108615e6f131d6ad71
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Fri Feb 9 18:11:41 2024 +0300

    aghos: imp docs

commit 3afbbcbb7ab6cc60c7c40ef8acd5b3ddf52cb3d1
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Fri Feb 9 15:40:02 2024 +0300

    all: upd golibs, imp fswatcher

CHANGELOG.md

@@ -57,12 +57,17 @@ NOTE: Add new changes BELOW THIS COMMENT.
 
 - Go 1.21 support.  Future versions will require at least Go 1.22 to build.
 
+### Fixed
+
+- Incorrect tracking of the system hosts file's changes ([#6711]).
+
 ### Removed
 
 - Go 1.20 support, as it has reached end of life.
 
 [#5992]: https://github.com/AdguardTeam/AdGuardHome/issues/5992
 [#6679]: https://github.com/AdguardTeam/AdGuardHome/issues/6679
+[#6711]: https://github.com/AdguardTeam/AdGuardHome/issues/6711
 
 [go-toolchain]: https://go.dev/blog/toolchain
 

go.mod

@@ -4,7 +4,7 @@ go 1.21.7
 
 require (
 	github.com/AdguardTeam/dnsproxy v0.65.0
-	github.com/AdguardTeam/golibs v0.20.0
+	github.com/AdguardTeam/golibs v0.20.1
 	github.com/AdguardTeam/urlfilter v0.17.3
 	github.com/NYTimes/gziphandler v1.1.1
 	github.com/ameshkov/dnscrypt/v2 v2.2.7

go.sum

@@ -1,7 +1,7 @@
 github.com/AdguardTeam/dnsproxy v0.65.0 h1:mqJjVSkqoqPwThY3tTvnLHQ/AYBYrfWmK2ER91fu4FE=
 github.com/AdguardTeam/dnsproxy v0.65.0/go.mod h1:AGYMLPk2zX+I3NIUYS12KUI296mkCyfoMF/luy2uqdk=
-github.com/AdguardTeam/golibs v0.20.0 h1:A9FIdYq7wUKhFYy3z+YZ/Aw5oFUYgW+xgaVAJ0pnnPY=
-github.com/AdguardTeam/golibs v0.20.0/go.mod h1:3WunclLLfrVAq7fYQRhd6f168FHOEMssnipVXCxDL/w=
+github.com/AdguardTeam/golibs v0.20.1 h1:ol8qLjWGZhU9paMMwN+OLWVTUigGsXa29iVTyd62VKY=
+github.com/AdguardTeam/golibs v0.20.1/go.mod h1:bgcMgRviCKyU6mkrX+RtT/OsKPFzyppelfRsksMG3KU=
 github.com/AdguardTeam/urlfilter v0.17.3 h1:fg/ObbnO0Cv6aw0tW6N/ETDMhhNvmcUUOZ7HlmKC3rw=
 github.com/AdguardTeam/urlfilter v0.17.3/go.mod h1:Jru7jFfeH2CoDf150uDs+rRYcZBzHHBz05r9REyDKyE=
 github.com/NYTimes/gziphandler v1.1.1 h1:ZUDjpQae29j0ryrS0u/B8HZfJBtBQHjqw2rQ2cqUQ3I=

internal/aghnet/hostscontainer_test.go

@@ -67,6 +67,7 @@ func TestNewHostsContainer(t *testing.T) {
 			}
 
 			hc, err := aghnet.NewHostsContainer(testFS, &aghtest.FSWatcher{
+				OnStart:  func() (_ error) { panic("not implemented") },
 				OnEvents: onEvents,
 				OnAdd:    onAdd,
 				OnClose:  func() (err error) { return nil },
@@ -93,6 +94,7 @@ func TestNewHostsContainer(t *testing.T) {
 	t.Run("nil_fs", func(t *testing.T) {
 		require.Panics(t, func() {
 			_, _ = aghnet.NewHostsContainer(nil, &aghtest.FSWatcher{
+				OnStart: func() (_ error) { panic("not implemented") },
 				// Those shouldn't panic.
 				OnEvents: func() (e <-chan struct{}) { return nil },
 				OnAdd:    func(name string) (err error) { return nil },
@@ -111,6 +113,7 @@ func TestNewHostsContainer(t *testing.T) {
 		const errOnAdd errors.Error = "error"
 
 		errWatcher := &aghtest.FSWatcher{
+			OnStart:  func() (_ error) { panic("not implemented") },
 			OnEvents: func() (e <-chan struct{}) { panic("not implemented") },
 			OnAdd:    func(name string) (err error) { return errOnAdd },
 			OnClose:  func() (err error) { return nil },
@@ -155,6 +158,7 @@ func TestHostsContainer_refresh(t *testing.T) {
 	t.Cleanup(func() { close(eventsCh) })
 
 	w := &aghtest.FSWatcher{
+		OnStart:  func() (_ error) { panic("not implemented") },
 		OnEvents: func() (e <-chan event) { return eventsCh },
 		OnAdd: func(name string) (err error) {
 			assert.Equal(t, "dir", name)

internal/aghnet/net.go

@@ -17,6 +17,7 @@ import (
 	"github.com/AdguardTeam/dnsproxy/upstream"
 	"github.com/AdguardTeam/golibs/errors"
 	"github.com/AdguardTeam/golibs/log"
+	"github.com/AdguardTeam/golibs/osutil"
 )
 
 // DialContextFunc is the semantic alias for dialing functions, such as
@@ -32,7 +33,7 @@ var (
 	netInterfaceAddrs = net.InterfaceAddrs
 
 	// rootDirFS is the filesystem pointing to the root directory.
-	rootDirFS = aghos.RootDirFS()
+	rootDirFS = osutil.RootDirFS()
 )
 
 // ErrNoStaticIPInfo is returned by IfaceHasStaticIP when no information about

internal/aghos/fswatcher.go

@@ -8,6 +8,8 @@ import (
 
 	"github.com/AdguardTeam/golibs/errors"
 	"github.com/AdguardTeam/golibs/log"
+	"github.com/AdguardTeam/golibs/osutil"
+	"github.com/AdguardTeam/golibs/stringutil"
 	"github.com/fsnotify/fsnotify"
 )
 
@@ -18,31 +20,38 @@ type event = struct{}
 // FSWatcher tracks all the fyle system events and notifies about those.
 //
 // TODO(e.burkov, a.garipov): Move into another package like aghfs.
+//
+// TODO(e.burkov):  Add tests.
 type FSWatcher interface {
+	// Start starts watching the added files.
+	Start() (err error)
+
+	// Close stops watching the files and closes an update channel.
 	io.Closer
 
-	// Events should return a read-only channel which notifies about events.
+	// Events returns the channel to notify about the file system events.
 	Events() (e <-chan event)
 
-	// Add should check if the file named name is accessible and starts tracking
-	// it.
+	// Add starts tracking the file.  It returns an error if the file can't be
+	// tracked.  It must not be called after Start.
 	Add(name string) (err error)
 }
 
 // osWatcher tracks the file system provided by the OS.
 type osWatcher struct {
-	// w is the actual notifier that is handled by osWatcher.
-	w *fsnotify.Watcher
+	// watcher is the actual notifier that is handled by osWatcher.
+	watcher *fsnotify.Watcher
 
 	// events is the channel to notify.
 	events chan event
+
+	// files is the set of tracked files.
+	files *stringutil.Set
 }
 
-const (
-	// osWatcherPref is a prefix for logging and wrapping errors in osWathcer's
-	// methods.
-	osWatcherPref = "os watcher"
-)
+// osWatcherPref is a prefix for logging and wrapping errors in osWathcer's
+// methods.
+const osWatcherPref = "os watcher"
 
 // NewOSWritesWatcher creates FSWatcher that tracks the real file system of the
 // OS and notifies only about writing events.
@@ -55,25 +64,27 @@ func NewOSWritesWatcher() (w FSWatcher, err error) {
 		return nil, fmt.Errorf("creating watcher: %w", err)
 	}
 
-	fsw := &osWatcher{
-		w:      watcher,
+	return &osWatcher{
+		watcher: watcher,
 		events:  make(chan event, 1),
-	}
-
-	go fsw.handleErrors()
-	go fsw.handleEvents()
-
-	return fsw, nil
+		files:   stringutil.NewSet(),
+	}, nil
 }
 
-// handleErrors handles accompanying errors.  It is intended to be used as a
-// goroutine.
-func (w *osWatcher) handleErrors() {
-	defer log.OnPanic(fmt.Sprintf("%s: handling errors", osWatcherPref))
+// type check
+var _ FSWatcher = (*osWatcher)(nil)
 
-	for err := range w.w.Errors {
-		log.Error("%s: %s", osWatcherPref, err)
-	}
+// Start implements the FSWatcher interface for *osWatcher.
+func (w *osWatcher) Start() (err error) {
+	go w.handleErrors()
+	go w.handleEvents()
+
+	return nil
+}
+
+// Close implements the FSWatcher interface for *osWatcher.
+func (w *osWatcher) Close() (err error) {
+	return w.watcher.Close()
 }
 
 // Events implements the FSWatcher interface for *osWatcher.
@@ -81,22 +92,30 @@ func (w *osWatcher) Events() (e <-chan event) {
 	return w.events
 }
 
-// Add implements the FSWatcher interface for *osWatcher.
+// Add implements the [FSWatcher] interface for *osWatcher.
 //
 // TODO(e.burkov):  Make it accept non-existing files to detect it's creating.
 func (w *osWatcher) Add(name string) (err error) {
 	defer func() { err = errors.Annotate(err, "%s: %w", osWatcherPref) }()
 
-	if _, err = fs.Stat(RootDirFS(), name); err != nil {
+	fi, err := fs.Stat(osutil.RootDirFS(), name)
+	if err != nil {
 		return fmt.Errorf("checking file %q: %w", name, err)
 	}
 
-	return w.w.Add(filepath.Join("/", name))
-}
+	name = filepath.Join("/", name)
+	w.files.Add(name)
 
-// Close implements the FSWatcher interface for *osWatcher.
-func (w *osWatcher) Close() (err error) {
-	return w.w.Close()
+	// Watch the directory and filter the events by the file name, since the
+	// common recomendation to the fsnotify package is to watch the directory
+	// instead of the file itself.
+	//
+	// See https://pkg.go.dev/github.com/fsnotify/fsnotify@v1.7.0#readme-watching-a-file-doesn-t-work-well.
+	if !fi.IsDir() {
+		name = filepath.Dir(name)
+	}
+
+	return w.watcher.Add(name)
 }
 
 // handleEvents notifies about the received file system's event if needed.  It
@@ -106,9 +125,9 @@ func (w *osWatcher) handleEvents() {
 
 	defer close(w.events)
 
-	ch := w.w.Events
+	ch := w.watcher.Events
 	for e := range ch {
-		if e.Op&fsnotify.Write == 0 {
+		if e.Op&fsnotify.Write == 0 || !w.files.Has(e.Name) {
 			continue
 		}
 
@@ -131,3 +150,13 @@ func (w *osWatcher) handleEvents() {
 		}
 	}
 }
+
+// handleErrors handles accompanying errors.  It used to be called in a separate
+// goroutine.
+func (w *osWatcher) handleErrors() {
+	defer log.OnPanic(fmt.Sprintf("%s: handling errors", osWatcherPref))
+
+	for err := range w.watcher.Errors {
+		log.Error("%s: %s", osWatcherPref, err)
+	}
+}

internal/aghos/os.go

@@ -7,7 +7,6 @@ import (
 	"bufio"
 	"fmt"
 	"io"
-	"io/fs"
 	"os"
 	"os/exec"
 	"path"
@@ -18,7 +17,6 @@ import (
 
 	"github.com/AdguardTeam/golibs/errors"
 	"github.com/AdguardTeam/golibs/log"
-	"github.com/AdguardTeam/golibs/mathutil"
 )
 
 // UnsupportedError is returned by functions and methods when a particular
@@ -63,7 +61,7 @@ func RunCommand(command string, arguments ...string) (code int, output []byte, e
 	cmd := exec.Command(command, arguments...)
 	out, err := cmd.Output()
 
-	out = out[:mathutil.Min(len(out), MaxCmdOutputSize)]
+	out = out[:min(len(out), MaxCmdOutputSize)]
 
 	if err != nil {
 		if eerr := new(exec.ExitError); errors.As(err, &eerr) {
@@ -142,7 +140,7 @@ func parsePSOutput(r io.Reader, cmdName string, ignore []int) (largest, instNum
 		}
 
 		instNum++
-		largest = mathutil.Max(largest, cur)
+		largest = max(largest, cur)
 	}
 	if err = s.Err(); err != nil {
 		return 0, 0, fmt.Errorf("scanning stdout: %w", err)
@@ -156,13 +154,6 @@ func IsOpenWrt() (ok bool) {
 	return isOpenWrt()
 }
 
-// RootDirFS returns the [fs.FS] rooted at the operating system's root.  On
-// Windows it returns the fs.FS rooted at the volume of the system directory
-// (usually, C:).
-func RootDirFS() (fsys fs.FS) {
-	return rootDirFS()
-}
-
 // NotifyReconfigureSignal notifies c on receiving reconfigure signals.
 func NotifyReconfigureSignal(c chan<- os.Signal) {
 	notifyReconfigureSignal(c)

internal/aghos/os_linux.go

@@ -7,6 +7,7 @@ import (
 	"os"
 	"syscall"
 
+	"github.com/AdguardTeam/golibs/osutil"
 	"github.com/AdguardTeam/golibs/stringutil"
 )
 
@@ -40,7 +41,7 @@ func isOpenWrt() (ok bool) {
 		}
 
 		return nil, !stringutil.ContainsFold(string(data), osNameData), nil
-	}).Walk(RootDirFS(), etcReleasePattern)
+	}).Walk(osutil.RootDirFS(), etcReleasePattern)
 
 	return err == nil && ok
 }

internal/aghos/os_unix.go

@@ -3,17 +3,12 @@
 package aghos
 
 import (
-	"io/fs"
 	"os"
 	"os/signal"
 
 	"golang.org/x/sys/unix"
 )
 
-func rootDirFS() (fsys fs.FS) {
-	return os.DirFS("/")
-}
-
 func notifyReconfigureSignal(c chan<- os.Signal) {
 	signal.Notify(c, unix.SIGHUP)
 }

internal/aghos/os_windows.go

@@ -3,29 +3,13 @@
 package aghos
 
 import (
-	"io/fs"
 	"os"
 	"os/signal"
-	"path/filepath"
 	"syscall"
 
-	"github.com/AdguardTeam/golibs/log"
 	"golang.org/x/sys/windows"
 )
 
-func rootDirFS() (fsys fs.FS) {
-	// TODO(a.garipov): Use a better way if golang/go#44279 is ever resolved.
-	sysDir, err := windows.GetSystemDirectory()
-	if err != nil {
-		log.Error("aghos: getting root filesystem: %s; using C:", err)
-
-		// Assume that C: is the safe default.
-		return os.DirFS("C:")
-	}
-
-	return os.DirFS(filepath.VolumeName(sysDir))
-}
-
 func setRlimit(val uint64) (err error) {
 	return Unsupported("setrlimit")
 }

internal/aghtest/interface.go

@@ -26,14 +26,25 @@ import (
 
 // FSWatcher is a fake [aghos.FSWatcher] implementation for tests.
 type FSWatcher struct {
+	OnStart  func() (err error)
+	OnClose  func() (err error)
 	OnEvents func() (e <-chan struct{})
 	OnAdd    func(name string) (err error)
-	OnClose  func() (err error)
 }
 
 // type check
 var _ aghos.FSWatcher = (*FSWatcher)(nil)
 
+// Start implements the [aghos.FSWatcher] interface for *FSWatcher.
+func (w *FSWatcher) Start() (err error) {
+	return w.OnStart()
+}
+
+// Close implements the [aghos.FSWatcher] interface for *FSWatcher.
+func (w *FSWatcher) Close() (err error) {
+	return w.OnClose()
+}
+
 // Events implements the [aghos.FSWatcher] interface for *FSWatcher.
 func (w *FSWatcher) Events() (e <-chan struct{}) {
 	return w.OnEvents()
@@ -44,11 +55,6 @@ func (w *FSWatcher) Add(name string) (err error) {
 	return w.OnAdd(name)
 }
 
-// Close implements the [aghos.FSWatcher] interface for *FSWatcher.
-func (w *FSWatcher) Close() (err error) {
-	return w.OnClose()
-}
-
 // Package agh
 
 // ServiceWithConfig is a fake [agh.ServiceWithConfig] implementation for tests.

internal/arpdb/arpdb.go

@@ -14,6 +14,7 @@ import (
 	"github.com/AdguardTeam/golibs/errors"
 	"github.com/AdguardTeam/golibs/log"
 	"github.com/AdguardTeam/golibs/netutil"
+	"github.com/AdguardTeam/golibs/osutil"
 )
 
 // Variables and functions to substitute in tests.
@@ -22,7 +23,7 @@ var (
 	aghosRunCommand = aghos.RunCommand
 
 	// rootDirFS is the filesystem pointing to the root directory.
-	rootDirFS = aghos.RootDirFS()
+	rootDirFS = osutil.RootDirFS()
 )
 
 // Interface stores and refreshes the network neighborhood reported by ARP

internal/dnsforward/dnsforward_test.go

@@ -1330,6 +1330,7 @@ func TestPTRResponseFromHosts(t *testing.T) {
 
 	var eventsCalledCounter uint32
 	hc, err := aghnet.NewHostsContainer(testFS, &aghtest.FSWatcher{
+		OnStart: func() (_ error) { panic("not implemented") },
 		OnEvents: func() (e <-chan struct{}) {
 			assert.Equal(t, uint32(1), atomic.AddUint32(&eventsCalledCounter, 1))
 

internal/dnsforward/http_test.go

@@ -418,6 +418,7 @@ func TestServer_HandleTestUpstreamDNS(t *testing.T) {
 			},
 		},
 		&aghtest.FSWatcher{
+			OnStart:  func() (_ error) { panic("not implemented") },
 			OnEvents: func() (e <-chan struct{}) { return nil },
 			OnAdd:    func(_ string) (err error) { return nil },
 			OnClose:  func() (err error) { return nil },

internal/filtering/hosts_test.go

@@ -40,6 +40,7 @@ func TestDNSFilter_CheckHost_hostsContainer(t *testing.T) {
 		},
 	}
 	watcher := &aghtest.FSWatcher{
+		OnStart:  func() (_ error) { panic("not implemented") },
 		OnEvents: func() (e <-chan struct{}) { return nil },
 		OnAdd:    func(name string) (err error) { return nil },
 		OnClose:  func() (err error) { return nil },

internal/filtering/rewrites.go

@@ -8,7 +8,6 @@ import (
 
 	"github.com/AdguardTeam/golibs/errors"
 	"github.com/AdguardTeam/golibs/log"
-	"github.com/AdguardTeam/golibs/mathutil"
 	"github.com/miekg/dns"
 )
 
@@ -181,7 +180,7 @@ func findRewrites(
 		if isWildcard(r.Domain) {
 			// Don't use rewrites[:0], because we need to return at least one
 			// item here.
-			rewrites = rewrites[:mathutil.Max(1, i)]
+			rewrites = rewrites[:max(1, i)]
 
 			break
 		}

internal/home/control.go

@@ -15,7 +15,6 @@ import (
 	"github.com/AdguardTeam/AdGuardHome/internal/version"
 	"github.com/AdguardTeam/golibs/httphdr"
 	"github.com/AdguardTeam/golibs/log"
-	"github.com/AdguardTeam/golibs/mathutil"
 	"github.com/AdguardTeam/golibs/netutil"
 	"github.com/NYTimes/gziphandler"
 )
@@ -145,10 +144,7 @@ func handleStatus(w http.ResponseWriter, r *http.Request) {
 			// Make sure that we don't send negative numbers to the frontend,
 			// since enough time might have passed to make the difference less
 			// than zero.
-			protectionDisabledDuration = mathutil.Max(
-				0,
-				time.Until(*protectionDisabledUntil).Milliseconds(),
-			)
+			protectionDisabledDuration = max(0, time.Until(*protectionDisabledUntil).Milliseconds())
 		}
 
 		resp = statusResponse{

internal/home/home.go

@@ -250,7 +250,7 @@ func setupHostsContainer() (err error) {
 		return errors.Join(fmt.Errorf("initializing hosts container: %w", err), closeErr)
 	}
 
-	return nil
+	return hostsWatcher.Start()
 }
 
 // setupOpts sets up command-line options.