all: imp tests, logging, etc

HACKING.md

@@ -62,9 +62,12 @@ The rules are mostly sorted in the alphabetical order.
  *  Don't use underscores in file and package names, unless they're build tags
     or for tests.  This is to prevent accidental build errors with weird tags.
 
- *  Don't write code with more than four (**4**) levels of indentation.  Just
-    like [Linus said], plus an additional level for an occasional error check or
-    struct initialization.
+ *  Don't write non-test code with more than four (**4**) levels of indentation.
+    Just like [Linus said], plus an additional level for an occasional error
+    check or struct initialization.
+
+    The exception proving the rule is the table-driven test code, where an
+    additional level of indentation is allowed.
 
  *  Eschew external dependencies, including transitive, unless
     absolutely necessary.

internal/dnsforward/dns_test.go

@@ -113,33 +113,33 @@ func TestProcessClientID(t *testing.T) {
 
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
+			srv := &Server{
+				conf: ServerConfig{
+					TLSConfig: TLSConfig{ServerName: tc.hostSrvName},
+				},
+			}
+
 			var conn net.Conn
 			if tc.proto == proxy.ProtoTLS {
-				conn = testTLSConn{serverName: tc.cliSrvName}
+				conn = testTLSConn{
+					serverName: tc.cliSrvName,
+				}
 			}
 
 			var qs quic.Session
 			if tc.proto == proxy.ProtoQUIC {
-				qs = testQUICSession{serverName: tc.cliSrvName}
-			}
-
-			pctx := &proxy.DNSContext{
-				Proto:       tc.proto,
-				Conn:        conn,
-				QUICSession: qs,
-			}
-
-			tlsConf := TLSConfig{ServerName: tc.hostSrvName}
-			srvConf := ServerConfig{
-				TLSConfig: tlsConf,
-			}
-			srv := &Server{
-				conf: srvConf,
+				qs = testQUICSession{
+					serverName: tc.cliSrvName,
+				}
 			}
 
 			dctx := &dnsContext{
-				proxyCtx: pctx,
-				srv:      srv,
+				srv: srv,
+				proxyCtx: &proxy.DNSContext{
+					Proto:       tc.proto,
+					Conn:        conn,
+					QUICSession: qs,
+				},
 			}
 
 			res := processClientID(dctx)
@@ -209,15 +209,16 @@ func TestProcessClientID_https(t *testing.T) {
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
 			r := &http.Request{
-				URL: &url.URL{Path: tc.path},
-			}
-			pctx := &proxy.DNSContext{
-				Proto:       proxy.ProtoHTTPS,
-				HTTPRequest: r,
+				URL: &url.URL{
+					Path: tc.path,
+				},
 			}
 
 			dctx := &dnsContext{
-				proxyCtx: pctx,
+				proxyCtx: &proxy.DNSContext{
+					Proto:       proxy.ProtoHTTPS,
+					HTTPRequest: r,
+				},
 			}
 
 			res := processClientID(dctx)

internal/home/clients.go

@@ -559,14 +559,14 @@ func (clients *clientsContainer) SetWhoisInfo(ip string, info [][]string) {
 
 	_, ok := clients.findLocked(ip)
 	if ok {
-		log.Debug("clients: client for %s is already created, ignore WHOIS info", ip)
+		log.Debug("clients: client for %s is already created, ignore whois info", ip)
 		return
 	}
 
 	ch, ok := clients.ipHost[ip]
 	if ok {
 		ch.WhoisInfo = info
-		log.Debug("clients: set WHOIS info for auto-client %s: %v", ch.Host, ch.WhoisInfo)
+		log.Debug("clients: set whois info for auto-client %s: %v", ch.Host, ch.WhoisInfo)
 		return
 	}
 
@@ -576,7 +576,7 @@ func (clients *clientsContainer) SetWhoisInfo(ip string, info [][]string) {
 	}
 	ch.WhoisInfo = info
 	clients.ipHost[ip] = ch
-	log.Debug("clients: set WHOIS info for auto-client with IP %s: %v", ip, ch.WhoisInfo)
+	log.Debug("clients: set whois info for auto-client with IP %s: %q", ip, info)
 }
 
 // AddHost adds a new IP-hostname pairing.  The priorities of the sources is

internal/home/clients_test.go

@@ -23,27 +23,30 @@ func TestClients(t *testing.T) {
 			Name: "client1",
 		}
 
-		b, err := clients.Add(c)
-		assert.True(t, b)
+		ok, err := clients.Add(c)
+		assert.True(t, ok)
 		assert.Nil(t, err)
 
 		c = &Client{
 			IDs:  []string{"2.2.2.2"},
 			Name: "client2",
 		}
 
-		b, err = clients.Add(c)
-		assert.True(t, b)
+		ok, err = clients.Add(c)
+		assert.True(t, ok)
 		assert.Nil(t, err)
 
-		c, b = clients.Find("1.1.1.1")
-		assert.True(t, b && c.Name == "client1")
+		c, ok = clients.Find("1.1.1.1")
+		assert.True(t, ok)
+		assert.Equal(t, "client1", c.Name)
 
-		c, b = clients.Find("1:2:3::4")
-		assert.True(t, b && c.Name == "client1")
+		c, ok = clients.Find("1:2:3::4")
+		assert.True(t, ok)
+		assert.Equal(t, "client1", c.Name)
 
-		c, b = clients.Find("2.2.2.2")
-		assert.True(t, b && c.Name == "client2")
+		c, ok = clients.Find("2.2.2.2")
+		assert.True(t, ok)
+		assert.Equal(t, "client2", c.Name)
 
 		assert.True(t, !clients.Exists("1.2.3.4", ClientSourceHostsFile))
 		assert.True(t, clients.Exists("1.1.1.1", ClientSourceHostsFile))
@@ -56,8 +59,8 @@ func TestClients(t *testing.T) {
 			Name: "client1",
 		}
 
-		b, err := clients.Add(c)
-		assert.False(t, b)
+		ok, err := clients.Add(c)
+		assert.False(t, ok)
 		assert.Nil(t, err)
 	})
 
@@ -67,8 +70,8 @@ func TestClients(t *testing.T) {
 			Name: "client3",
 		}
 
-		b, err := clients.Add(c)
-		assert.False(t, b)
+		ok, err := clients.Add(c)
+		assert.False(t, ok)
 		assert.NotNil(t, err)
 	})
 
@@ -121,44 +124,46 @@ func TestClients(t *testing.T) {
 		err = clients.Update("client1", c)
 		assert.Nil(t, err)
 
-		c, b := clients.Find("1.1.1.2")
-		assert.True(t, b)
-		assert.True(t, c.Name == "client1-renamed")
-		assert.True(t, c.IDs[0] == "1.1.1.2")
+		c, ok := clients.Find("1.1.1.2")
+		assert.True(t, ok)
+		assert.Equal(t, "client1-renamed", c.Name)
 		assert.True(t, c.UseOwnSettings)
 		assert.Nil(t, clients.list["client1"])
+		if assert.Len(t, c.IDs, 1) {
+			assert.Equal(t, "1.1.1.2", c.IDs[0])
+		}
 	})
 
 	t.Run("del_success", func(t *testing.T) {
-		b := clients.Del("client1-renamed")
-		assert.True(t, b)
+		ok := clients.Del("client1-renamed")
+		assert.True(t, ok)
 		assert.False(t, clients.Exists("1.1.1.2", ClientSourceHostsFile))
 	})
 
 	t.Run("del_fail", func(t *testing.T) {
-		b := clients.Del("client3")
-		assert.False(t, b)
+		ok := clients.Del("client3")
+		assert.False(t, ok)
 	})
 
 	t.Run("addhost_success", func(t *testing.T) {
-		b, err := clients.AddHost("1.1.1.1", "host", ClientSourceARP)
-		assert.True(t, b)
+		ok, err := clients.AddHost("1.1.1.1", "host", ClientSourceARP)
+		assert.True(t, ok)
 		assert.Nil(t, err)
 
-		b, err = clients.AddHost("1.1.1.1", "host2", ClientSourceARP)
-		assert.True(t, b)
+		ok, err = clients.AddHost("1.1.1.1", "host2", ClientSourceARP)
+		assert.True(t, ok)
 		assert.Nil(t, err)
 
-		b, err = clients.AddHost("1.1.1.1", "host3", ClientSourceHostsFile)
-		assert.True(t, b)
+		ok, err = clients.AddHost("1.1.1.1", "host3", ClientSourceHostsFile)
+		assert.True(t, ok)
 		assert.Nil(t, err)
 
 		assert.True(t, clients.Exists("1.1.1.1", ClientSourceHostsFile))
 	})
 
 	t.Run("addhost_fail", func(t *testing.T) {
-		b, err := clients.AddHost("1.1.1.1", "host1", ClientSourceRDNS)
-		assert.False(t, b)
+		ok, err := clients.AddHost("1.1.1.1", "host1", ClientSourceRDNS)
+		assert.False(t, ok)
 		assert.Nil(t, err)
 	})
 }
@@ -172,12 +177,22 @@ func TestClientsWhois(t *testing.T) {
 	whois := [][]string{{"orgname", "orgname-val"}, {"country", "country-val"}}
 	// set whois info on new client
 	clients.SetWhoisInfo("1.1.1.255", whois)
-	assert.True(t, clients.ipHost["1.1.1.255"].WhoisInfo[0][1] == "orgname-val")
+	if assert.NotNil(t, clients.ipHost["1.1.1.255"]) {
+		h := clients.ipHost["1.1.1.255"]
+		if assert.Len(t, h.WhoisInfo, 2) && assert.Len(t, h.WhoisInfo[0], 2) {
+			assert.Equal(t, "orgname-val", h.WhoisInfo[0][1])
+		}
+	}
 
 	// set whois info on existing auto-client
 	_, _ = clients.AddHost("1.1.1.1", "host", ClientSourceRDNS)
 	clients.SetWhoisInfo("1.1.1.1", whois)
-	assert.True(t, clients.ipHost["1.1.1.1"].WhoisInfo[0][1] == "orgname-val")
+	if assert.NotNil(t, clients.ipHost["1.1.1.1"]) {
+		h := clients.ipHost["1.1.1.1"]
+		if assert.Len(t, h.WhoisInfo, 2) && assert.Len(t, h.WhoisInfo[0], 2) {
+			assert.Equal(t, "orgname-val", h.WhoisInfo[0][1])
+		}
+	}
 
 	// Check that we cannot set whois info on a manually-added client
 	c = &Client{
@@ -186,7 +201,7 @@ func TestClientsWhois(t *testing.T) {
 	}
 	_, _ = clients.Add(c)
 	clients.SetWhoisInfo("1.1.1.2", whois)
-	assert.True(t, clients.ipHost["1.1.1.2"] == nil)
+	assert.Nil(t, clients.ipHost["1.1.1.2"])
 	_ = clients.Del("client1")
 }
 

internal/home/dns.go

@@ -222,30 +222,28 @@ func getDNSEncryption() (de dnsEncryption) {
 			if tlsConf.PortHTTPS != 443 {
 				addr = net.JoinHostPort(addr, strconv.Itoa(tlsConf.PortHTTPS))
 			}
-			u := &url.URL{
+
+			de.https = (&url.URL{
 				Scheme: "https",
 				Host:   addr,
 				Path:   "/dns-query",
-			}
-			de.https = u.String()
+			}).String()
 		}
 
 		if tlsConf.PortDNSOverTLS != 0 {
 			addr := net.JoinHostPort(hostname, strconv.Itoa(tlsConf.PortDNSOverTLS))
-			u := &url.URL{
+			de.tls = (&url.URL{
 				Scheme: "tls",
 				Host:   addr,
-			}
-			de.tls = u.String()
+			}).String()
 		}
 
 		if tlsConf.PortDNSOverQUIC != 0 {
 			addr := net.JoinHostPort(hostname, strconv.Itoa(int(tlsConf.PortDNSOverQUIC)))
-			u := &url.URL{
+			de.quic = (&url.URL{
 				Scheme: "quic",
 				Host:   addr,
-			}
-			de.quic = u.String()
+			}).String()
 		}
 	}
 

internal/querylog/querylogfile.go

@@ -89,7 +89,11 @@ func (l *queryLog) rotate() error {
 	to := l.logFile + ".1"
 
 	err := os.Rename(from, to)
-	if err != nil && !errors.Is(err, os.ErrNotExist) {
+	if err != nil {
+		if errors.Is(err, os.ErrNotExist) {
+			return nil
+		}
+
 		log.Error("querylog: failed to rename file: %s", err)
 
 		return err