From 6847f7c8bb3a827cfe4b1798f64cd4e14d6c7173 Mon Sep 17 00:00:00 2001 From: Ainar Garipov Date: Fri, 21 May 2021 18:30:57 +0300 Subject: [PATCH] Pull request: all: avoid fallthrough Merge in DNS/adguard-home from rm-fallthrough to master Squashed commit of the following: commit 14308b6cd3580d2c51b6da9f40b8a37766046708 Author: Ainar Garipov Date: Fri May 21 18:16:42 2021 +0300 dhcpd: imp code commit 2009219df7d35713d06848010ce57b387e407c0e Author: Ainar Garipov Date: Fri May 21 17:54:38 2021 +0300 all: avoid fallthrough --- HACKING.md | 4 +++- internal/dhcpd/v46.go | 46 +++++++++++++++++++----------------- internal/dhcpd/v6.go | 44 +++++++++++++++++++--------------- internal/dnsforward/stats.go | 8 +++---- scripts/make/go-lint.sh | 2 ++ 5 files changed, 57 insertions(+), 47 deletions(-) diff --git a/HACKING.md b/HACKING.md index 0d9c0279..ec9528e9 100644 --- a/HACKING.md +++ b/HACKING.md @@ -69,7 +69,9 @@ on GitHub and most other Markdown renderers. --> first statement. If all you want there is a log message, use `agherr.LogPanic`. - * Avoid `errors.New`, use `aghnet.Error` instead. + * Avoid `fallthrough`. It makes it harder to rearrange `case`s, to reason + about the code, and also to switch the code to a handler approach, if that + becomes necessary later. * Avoid `goto`. diff --git a/internal/dhcpd/v46.go b/internal/dhcpd/v46.go index c8301875..6b39fe5d 100644 --- a/internal/dhcpd/v46.go +++ b/internal/dhcpd/v46.go @@ -84,40 +84,42 @@ func ifaceDNSIPAddrs( backoff time.Duration, ) (addrs []net.IP, err error) { var n int -waitForIP: for n = 1; n <= maxAttempts; n++ { addrs, err = ifaceIPAddrs(iface, ipv) if err != nil { return nil, fmt.Errorf("getting ip addrs: %w", err) } - switch len(addrs) { - case 0: - log.Debug("dhcpv%d: attempt %d: no ip addresses", ipv, n) - - time.Sleep(backoff) - case 1: - // Some Android devices use 8.8.8.8 if there is not - // a secondary DNS server. Fix that by setting the - // secondary DNS address to the same address. - // - // See https://github.com/AdguardTeam/AdGuardHome/issues/1708. - log.Debug("dhcpv%d: setting secondary dns ip to itself", ipv) - addrs = append(addrs, addrs[0]) - - fallthrough - default: - break waitForIP + if len(addrs) > 0 { + break } + + log.Debug("dhcpv%d: attempt %d: no ip addresses", ipv, n) + + time.Sleep(backoff) } - if len(addrs) == 0 { + switch len(addrs) { + case 0: // Don't return errors in case the users want to try and enable // the DHCP server later. - log.Error("dhcpv%d: no ip address for interface after %d attempts and %s", ipv, n, time.Duration(n)*backoff) - } else { - log.Debug("dhcpv%d: got addresses %s after %d attempts", ipv, addrs, n) + t := time.Duration(n) * backoff + log.Error("dhcpv%d: no ip for iface after %d attempts and %s", ipv, n, t) + + return nil, nil + case 1: + // Some Android devices use 8.8.8.8 if there is not a secondary + // DNS server. Fix that by setting the secondary DNS address to + // the same address. + // + // See https://github.com/AdguardTeam/AdGuardHome/issues/1708. + log.Debug("dhcpv%d: setting secondary dns ip to itself", ipv) + addrs = append(addrs, addrs[0]) + default: + // Go on. } + log.Debug("dhcpv%d: got addresses %s after %d attempts", ipv, addrs, n) + return addrs, nil } diff --git a/internal/dhcpd/v6.go b/internal/dhcpd/v6.go index 04698beb..994a7e11 100644 --- a/internal/dhcpd/v6.go +++ b/internal/dhcpd/v6.go @@ -66,7 +66,8 @@ func (s *v6Server) ResetLeases(ll []*Lease) { if l.Expiry.Unix() != leaseExpireStatic && !ip6InRange(s.conf.ipStart, l.IP) { - log.Debug("DHCPv6: skipping a lease with IP %v: not within current IP range", l.IP) + log.Debug("dhcpv6: skipping a lease with IP %v: not within current IP range", l.IP) + continue } @@ -122,7 +123,7 @@ func (s *v6Server) FindMACbyIP(ip net.IP) net.HardwareAddr { // Remove (swap) lease by index func (s *v6Server) leaseRemoveSwapByIndex(i int) { s.ipAddrs[s.leases[i].IP[15]] = 0 - log.Debug("DHCPv6: removed lease %s", s.leases[i].HWAddr) + log.Debug("dhcpv6: removed lease %s", s.leases[i].HWAddr) n := len(s.leases) if i != n-1 { @@ -220,7 +221,7 @@ func (s *v6Server) RemoveStaticLease(l Lease) (err error) { func (s *v6Server) addLease(l *Lease) { s.leases = append(s.leases, l) s.ipAddrs[l.IP[15]] = 1 - log.Debug("DHCPv6: added lease %s <-> %s", l.IP, l.HWAddr) + log.Debug("dhcpv6: added lease %s <-> %s", l.IP, l.HWAddr) } // Remove a lease with the same properties @@ -321,6 +322,7 @@ func (s *v6Server) checkCID(msg *dhcpv6.Message) error { if msg.Options.ClientID() == nil { return fmt.Errorf("dhcpv6: no ClientID option in request") } + return nil } @@ -336,15 +338,14 @@ func (s *v6Server) checkSID(msg *dhcpv6.Message) error { if sid != nil { return fmt.Errorf("dhcpv6: drop packet: ServerID option in message %s", msg.Type().String()) } - case dhcpv6.MessageTypeRequest, dhcpv6.MessageTypeRenew, dhcpv6.MessageTypeRelease, dhcpv6.MessageTypeDecline: - if sid == nil { return fmt.Errorf("dhcpv6: drop packet: no ServerID option in message %s", msg.Type().String()) } + if !sid.Equal(s.sid) { return fmt.Errorf("dhcpv6: drop packet: mismatched ServerID option in message %s: %s", msg.Type().String(), sid.String()) @@ -417,13 +418,14 @@ func (s *v6Server) process(msg *dhcpv6.Message, req, resp dhcpv6.DHCPv6) bool { mac, err := dhcpv6.ExtractMAC(req) if err != nil { - log.Debug("DHCPv6: dhcpv6.ExtractMAC: %s", err) + log.Debug("dhcpv6: dhcpv6.ExtractMAC: %s", err) + return false } lease := s.findLease(mac) if lease == nil { - log.Debug("DHCPv6: no lease for: %s", mac) + log.Debug("dhcpv6: no lease for: %s", mac) switch msg.Type() { @@ -440,7 +442,8 @@ func (s *v6Server) process(msg *dhcpv6.Message, req, resp dhcpv6.DHCPv6) bool { err = s.checkIA(msg, lease) if err != nil { - log.Debug("DHCPv6: %s", err) + log.Debug("dhcpv6: %s", err) + return false } @@ -497,11 +500,12 @@ func (s *v6Server) process(msg *dhcpv6.Message, req, resp dhcpv6.DHCPv6) bool { func (s *v6Server) packetHandler(conn net.PacketConn, peer net.Addr, req dhcpv6.DHCPv6) { msg, err := req.GetInnerMessage() if err != nil { - log.Error("DHCPv6: %s", err) + log.Error("dhcpv6: %s", err) + return } - log.Debug("DHCPv6: received: %s", req.Summary()) + log.Debug("dhcpv6: received: %s", req.Summary()) err = s.checkCID(msg) if err != nil { @@ -521,10 +525,11 @@ func (s *v6Server) packetHandler(conn net.PacketConn, peer net.Addr, req dhcpv6. case dhcpv6.MessageTypeSolicit: if msg.GetOneOption(dhcpv6.OptionRapidCommit) == nil { resp, err = dhcpv6.NewAdvertiseFromSolicit(msg) + break } - fallthrough + resp, err = dhcpv6.NewReplyFromMessage(msg) case dhcpv6.MessageTypeRequest, dhcpv6.MessageTypeConfirm, dhcpv6.MessageTypeRenew, @@ -532,14 +537,14 @@ func (s *v6Server) packetHandler(conn net.PacketConn, peer net.Addr, req dhcpv6. dhcpv6.MessageTypeRelease, dhcpv6.MessageTypeInformationRequest: resp, err = dhcpv6.NewReplyFromMessage(msg) - default: - log.Error("DHCPv6: message type %d not supported", msg.Type()) + log.Error("dhcpv6: message type %d not supported", msg.Type()) + return } - if err != nil { - log.Error("DHCPv6: %s", err) + log.Error("dhcpv6: %s", err) + return } @@ -547,11 +552,12 @@ func (s *v6Server) packetHandler(conn net.PacketConn, peer net.Addr, req dhcpv6. _ = s.process(msg, req, resp) - log.Debug("DHCPv6: sending: %s", resp.Summary()) + log.Debug("dhcpv6: sending: %s", resp.Summary()) _, err = conn.WriteTo(resp.ToBytes(), peer) if err != nil { - log.Error("DHCPv6: conn.Write to %s failed: %s", peer, err) + log.Error("dhcpv6: conn.Write to %s failed: %s", peer, err) + return } } @@ -660,10 +666,10 @@ func (s *v6Server) Stop() { return } - log.Debug("DHCPv6: stopping") + log.Debug("dhcpv6: stopping") err = s.srv.Close() if err != nil { - log.Error("DHCPv6: srv.Close: %s", err) + log.Error("dhcpv6: srv.Close: %s", err) } // now server.Serve() will return diff --git a/internal/dnsforward/stats.go b/internal/dnsforward/stats.go index c493de0e..f03f6f64 100644 --- a/internal/dnsforward/stats.go +++ b/internal/dnsforward/stats.go @@ -92,11 +92,9 @@ func (s *Server) updateStats(ctx *dnsContext, elapsed time.Duration, res filteri e.Result = stats.RParental case filtering.FilteredSafeSearch: e.Result = stats.RSafeSearch - case filtering.FilteredBlockList: - fallthrough - case filtering.FilteredInvalid: - fallthrough - case filtering.FilteredBlockedService: + case filtering.FilteredBlockList, + filtering.FilteredInvalid, + filtering.FilteredBlockedService: e.Result = stats.RFiltered } diff --git a/scripts/make/go-lint.sh b/scripts/make/go-lint.sh index f30980b7..c1f14572 100644 --- a/scripts/make/go-lint.sh +++ b/scripts/make/go-lint.sh @@ -106,6 +106,8 @@ underscores() { } } +# TODO(a.garipov): Add an analyser to look for `fallthrough`, `goto`, and `new`? + # Helpers