Pull request: home: imp err handling, marshalling

Merge in DNS/adguard-home from imp-code to master

Squashed commit of the following:

commit 9433fb9
Author: Ainar Garipov <[email protected]>
Date:   Fri May 14 19:24:32 2021 +0300

    home: imp err handling, marshalling

Author: ainar-g

HACKING.md

@@ -95,6 +95,9 @@ on GitHub and most other Markdown renderers. -->
  *  Constructors should validate their arguments and return meaningful errors.
     As a corollary, avoid lazy initialization.
 
+ *  Define `MarshalFoo` methods on non-pointer receivers, as pointer receivers
+    [can have surprising results][staticcheck-911].
+
  *  Don't mix horizontal and vertical placement of arguments in function and
     method calls.  That is, either this:
 
@@ -286,9 +289,10 @@ on GitHub and most other Markdown renderers. -->
 
  *  <https://go-proverbs.github.io/>
 
-[constant errors]:          https://dave.cheney.net/2016/04/07/constant-errors
 [Linus said]:               https://www.kernel.org/doc/html/v4.17/process/coding-style.html#indentation
 [Text, Including Comments]: #text-including-comments
+[constant errors]:          https://dave.cheney.net/2016/04/07/constant-errors
+[staticcheck-911]:          https://github.com/dominikh/go-tools/issues/911
 
 
 

internal/aghnet/net.go

@@ -84,17 +84,17 @@ type NetInterface struct {
 	Subnets []*net.IPNet `json:"-"`
 }
 
-// MarshalJSON implements the json.Marshaler interface for *NetInterface.
-func (iface *NetInterface) MarshalJSON() ([]byte, error) {
+// MarshalJSON implements the json.Marshaler interface for NetInterface.
+func (iface NetInterface) MarshalJSON() ([]byte, error) {
 	type netInterface NetInterface
 	return json.Marshal(&struct {
 		HardwareAddr string `json:"hardware_address"`
 		Flags        string `json:"flags"`
-		*netInterface
+		netInterface
 	}{
 		HardwareAddr: iface.HardwareAddr.String(),
 		Flags:        iface.Flags.String(),
-		netInterface: (*netInterface)(iface),
+		netInterface: netInterface(iface),
 	})
 }
 

internal/dhcpd/dhcpd.go

@@ -43,8 +43,8 @@ func (l *Lease) IsStatic() (ok bool) {
 	return l != nil && l.Expiry.Unix() == leaseExpireStatic
 }
 
-// MarshalJSON implements the json.Marshaler interface for *Lease.
-func (l *Lease) MarshalJSON() ([]byte, error) {
+// MarshalJSON implements the json.Marshaler interface for Lease.
+func (l Lease) MarshalJSON() ([]byte, error) {
 	var expiryStr string
 	if !l.IsStatic() {
 		// The front-end is waiting for RFC 3999 format of the time
@@ -59,20 +59,20 @@ func (l *Lease) MarshalJSON() ([]byte, error) {
 	return json.Marshal(&struct {
 		HWAddr string `json:"mac"`
 		Expiry string `json:"expires,omitempty"`
-		*lease
+		lease
 	}{
 		HWAddr: l.HWAddr.String(),
 		Expiry: expiryStr,
-		lease:  (*lease)(l),
+		lease:  lease(l),
 	})
 }
 
 // UnmarshalJSON implements the json.Unmarshaler interface for *Lease.
 func (l *Lease) UnmarshalJSON(data []byte) (err error) {
 	type lease Lease
 	aux := struct {
-		HWAddr string `json:"mac"`
 		*lease
+		HWAddr string `json:"mac"`
 	}{
 		lease: (*lease)(l),
 	}

internal/home/filter.go

@@ -2,6 +2,7 @@ package home
 
 import (
 	"bufio"
+	"errors"
 	"fmt"
 	"hash/crc32"
 	"io"
@@ -622,29 +623,32 @@ func (f *Filtering) updateIntl(filter *filter) (updated bool, err error) {
 }
 
 // loads filter contents from the file in dataDir
-func (f *Filtering) load(filter *filter) error {
+func (f *Filtering) load(filter *filter) (err error) {
 	filterFilePath := filter.Path()
-	log.Tracef("Loading filter %d contents to: %s", filter.ID, filterFilePath)
 
-	if _, err := os.Stat(filterFilePath); os.IsNotExist(err) {
-		// do nothing, file doesn't exist
-		return err
-	}
+	log.Tracef("filtering: loading filter %d contents to: %s", filter.ID, filterFilePath)
 
 	file, err := os.Open(filterFilePath)
-	if err != nil {
-		return err
+	if errors.Is(err, os.ErrNotExist) {
+		// Do nothing, file doesn't exist.
+		return nil
+	} else if err != nil {
+		return fmt.Errorf("opening filter file: %w", err)
 	}
 	defer file.Close()
-	st, _ := file.Stat()
 
-	log.Tracef("File %s, id %d, length %d",
-		filterFilePath, filter.ID, st.Size())
+	st, err := file.Stat()
+	if err != nil {
+		return fmt.Errorf("getting filter file stat: %w", err)
+	}
+
+	log.Tracef("filtering: File %s, id %d, length %d", filterFilePath, filter.ID, st.Size())
+
 	rulesCount, checksum, _ := f.parseFilterContents(file)
 
 	filter.RulesCount = rulesCount
 	filter.checksum = checksum
-	filter.LastUpdated = filter.LastTimeUpdated()
+	filter.LastUpdated = st.ModTime()
 
 	return nil
 }
@@ -660,24 +664,6 @@ func (filter *filter) Path() string {
 	return filepath.Join(Context.getDataDir(), filterDir, strconv.FormatInt(filter.ID, 10)+".txt")
 }
 
-// LastTimeUpdated returns the time when the filter was last time updated
-func (filter *filter) LastTimeUpdated() time.Time {
-	filterFilePath := filter.Path()
-	s, err := os.Stat(filterFilePath)
-	if os.IsNotExist(err) {
-		// if the filter file does not exist, return 0001-01-01
-		return time.Time{}
-	}
-
-	if err != nil {
-		// if the filter file does not exist, return 0001-01-01
-		return time.Time{}
-	}
-
-	// filter file modified time
-	return s.ModTime()
-}
-
 func enableFilters(async bool) {
 	var whiteFilters []dnsfilter.Filter
 	filters := []dnsfilter.Filter{{

internal/home/service.go

@@ -1,6 +1,7 @@
 package home
 
 import (
+	"errors"
 	"fmt"
 	"io/ioutil"
 	"os"
@@ -88,24 +89,27 @@ func svcAction(s service.Service, action string) (err error) {
 // If pid-file doesn't exist, find our PID using 'ps' command
 func sendSigReload() {
 	if runtime.GOOS == "windows" {
-		log.Error("Not implemented on Windows")
+		log.Error("not implemented on windows")
+
 		return
 	}
 
 	pidfile := fmt.Sprintf("/var/run/%s.pid", serviceName)
 	data, err := ioutil.ReadFile(pidfile)
-	if os.IsNotExist(err) {
+	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("Can't find AdGuardHome process: %s  code:%d", err, code)
+			log.Error("finding AdGuardHome process: code: %d, error: %s", code, err)
+
 			return
 		}
-		data = []byte(psdata)
 
+		data = []byte(psdata)
 	} else if err != nil {
-		log.Error("Can't read PID file %s: %s", pidfile, err)
+		log.Error("reading pid file %s: %s", pidfile, err)
+
 		return
 	}
 
@@ -258,12 +262,12 @@ func handleServiceUninstallCommand(s service.Service) {
 	if runtime.GOOS == "darwin" {
 		// Remove log files on cleanup and log errors.
 		err = os.Remove(launchdStdoutPath)
-		if err != nil && !os.IsNotExist(err) {
+		if err != nil && !errors.Is(err, os.ErrNotExist) {
 			log.Printf("removing stdout file: %s", err)
 		}
 
 		err = os.Remove(launchdStderrPath)
-		if err != nil && !os.IsNotExist(err) {
+		if err != nil && !errors.Is(err, os.ErrNotExist) {
 			log.Printf("removing stderr file: %s", err)
 		}
 	}

internal/home/upgrade.go

@@ -1,6 +1,7 @@
 package home
 
 import (
+	"errors"
 	"fmt"
 	"net/url"
 	"os"
@@ -115,17 +116,16 @@ func upgradeConfigSchema(oldVersion int, diskConf yobj) (err error) {
 
 // The first schema upgrade:
 // No more "dnsfilter.txt", filters are now kept in data/filters/
-func upgradeSchema0to1(diskConf yobj) error {
+func upgradeSchema0to1(diskConf yobj) (err error) {
 	log.Printf("%s(): called", funcName())
 
 	dnsFilterPath := filepath.Join(Context.workDir, "dnsfilter.txt")
-	if _, err := os.Stat(dnsFilterPath); !os.IsNotExist(err) {
-		log.Printf("Deleting %s as we don't need it anymore", dnsFilterPath)
-		err = os.Remove(dnsFilterPath)
-		if err != nil {
-			log.Printf("Cannot remove %s due to %s", dnsFilterPath, err)
-			// not fatal, move on
-		}
+	log.Printf("deleting %s as we don't need it anymore", dnsFilterPath)
+	err = os.Remove(dnsFilterPath)
+	if err != nil && !errors.Is(err, os.ErrNotExist) {
+		log.Info("warning: %s", err)
+
+		// Go on.
 	}
 
 	diskConf["schema_version"] = 1
@@ -136,17 +136,16 @@ func upgradeSchema0to1(diskConf yobj) error {
 // Second schema upgrade:
 // coredns is now dns in config
 // delete 'Corefile', since we don't use that anymore
-func upgradeSchema1to2(diskConf yobj) error {
+func upgradeSchema1to2(diskConf yobj) (err error) {
 	log.Printf("%s(): called", funcName())
 
 	coreFilePath := filepath.Join(Context.workDir, "Corefile")
-	if _, err := os.Stat(coreFilePath); !os.IsNotExist(err) {
-		log.Printf("Deleting %s as we don't need it anymore", coreFilePath)
-		err = os.Remove(coreFilePath)
-		if err != nil {
-			log.Printf("Cannot remove %s due to %s", coreFilePath, err)
-			// not fatal, move on
-		}
+	log.Printf("deleting %s as we don't need it anymore", coreFilePath)
+	err = os.Remove(coreFilePath)
+	if err != nil && !errors.Is(err, os.ErrNotExist) {
+		log.Info("warning: %s", err)
+
+		// Go on.
 	}
 
 	if _, ok := diskConf["dns"]; !ok {