From a031cae447e5f77a88f9d7a8c2c1d1dcb1fda11c Mon Sep 17 00:00:00 2001 From: Ainar Garipov Date: Fri, 14 May 2021 19:41:45 +0300 Subject: [PATCH] Pull request: home: imp err handling, marshalling Merge in DNS/adguard-home from imp-code to master Squashed commit of the following: commit 9433fb9b0154a1cfaf804edbfa8531efbbcbf68a Author: Ainar Garipov Date: Fri May 14 19:24:32 2021 +0300 home: imp err handling, marshalling --- HACKING.md | 6 +++++- internal/aghnet/net.go | 8 +++---- internal/dhcpd/dhcpd.go | 10 ++++----- internal/home/filter.go | 46 ++++++++++++++-------------------------- internal/home/service.go | 18 ++++++++++------ internal/home/upgrade.go | 31 +++++++++++++-------------- 6 files changed, 56 insertions(+), 63 deletions(-) diff --git a/HACKING.md b/HACKING.md index 561f07e7..e1b3bebb 100644 --- a/HACKING.md +++ b/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. --> * -[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 diff --git a/internal/aghnet/net.go b/internal/aghnet/net.go index d23a17f7..643b61f8 100644 --- a/internal/aghnet/net.go +++ b/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), }) } diff --git a/internal/dhcpd/dhcpd.go b/internal/dhcpd/dhcpd.go index 8fcb4310..f652a42e 100644 --- a/internal/dhcpd/dhcpd.go +++ b/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,11 +59,11 @@ 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), }) } @@ -71,8 +71,8 @@ func (l *Lease) MarshalJSON() ([]byte, error) { 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), } diff --git a/internal/home/filter.go b/internal/home/filter.go index ccad20cb..3bdbea31 100644 --- a/internal/home/filter.go +++ b/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{{ diff --git a/internal/home/service.go b/internal/home/service.go index 69375cef..63820159 100644 --- a/internal/home/service.go +++ b/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) } } diff --git a/internal/home/upgrade.go b/internal/home/upgrade.go index dbfd3902..278f3774 100644 --- a/internal/home/upgrade.go +++ b/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 {