lint: enable more linters fix many issues (#213)

Enable many golangci-linters that seem somewhat useful and don't show
ridiculous numbers of issues out of the gate.

- update golangci-lint to latest 1.43.0
- consistent import ordering enforced by gci
- nilnil and nilerr linters actually found various bugs where a nil
  error was returned after detecting an error.
- fix issues in bwtester and ssh so that exclude-rules can be removed:
  - bwtester: requires removing the dot-import of the bwtestlib. For
    this, renamed it to shorter `bwtest` and renamed various items in it
    to remove the redundant Bwtest from the name.
  - ssh: minimally handle various errors, or explicitly ignore them
    Reply the error state for `WantReply` requests.
- enable lint for examples (and fix all issues)
- run go mod tidy; no linter for this, unfortunately. A `--diff` option
  for `go mod tidy` does not currently exist, but is planned (for go-1.19).

Author: matzf

.golangci.yml

@@ -14,31 +14,47 @@ linters:
     #- typecheck
     #- unused
     #- varcheck
-    - gofmt
-    - stylecheck # replacement for golint
+    - asciicheck
+    - bidichk
+    - contextcheck
     - dupl
+    - durationcheck
+    - errname
+    - errorlint
+    - exportloopref
+    - gci
+    - gofmt
+    - ifshort
+    - importas
+    - makezero
     - misspell
-
+    - nilerr
+    - nilnil
+    - nolintlint
+    - prealloc
+    - stylecheck
+    - thelper
+    - wastedassign
+    - tenv
 
 issues:
   # XXX exclude some linters where there are too many issues to fix now
   exclude-rules:
-    - path: bwtester/
-      linters:
-        - stylecheck
     - path: webapp/
       linters:
         - stylecheck
         - staticcheck
         - errcheck
-    - path: ssh/
-      linters:
-        - errcheck
-        
-
   max-same-issues: 0
 
+linters-settings:
+  gci:
+    local-prefixes: github.com/netsec-ethz/scion-apps
+  exhaustive:
+    default-signifies-exhaustive: true
+
 run:
+  build-tags: integration
   # Bat is mostly copied third-party code that we don't care to fix
   # Modifications are only (?) in bat/bat.go
   skip-dirs:

Makefile

@@ -12,6 +12,11 @@ DESTDIR = $(shell set -a; eval $$( go env ); gopath=$${GOPATH%:*}; echo $${GOBIN
 # HINT: build with TAGS=norains to build without rains support
 TAGS =
 
+# EXAMPLES lists each ./_examples/ subdirectory containing .go files. This is
+# used to invoke various go commands, as the _examples directory is otherwise
+# ignored (due to the underscore prefix).
+EXAMPLES = $(shell find ./_examples/ -name *.go -printf '%h\n' | sort -u)
+
 all: build lint
 
 build: scion-bat \
@@ -28,27 +33,27 @@ build: scion-bat \
 	example-shttp-client example-shttp-server example-shttp-fileserver example-shttp-proxy
 
 clean:
-	go clean ./...
+	go clean ./... ${EXAMPLES}
 	rm -f bin/*
 
 test:
-	go test -tags=$(TAGS) ./...
+	go test -tags=$(TAGS) ./... ${EXAMPLES}
 
 setup_lint:
 	@# Install golangci-lint (as dumb as this looks, this is the recommended way to install)
-	curl -sfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh| sh -s -- -b ${DESTDIR} v1.37.1
+	curl -sfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh| sh -s -- -b ${DESTDIR} v1.43.0
 
 lint:
 	@type golangci-lint > /dev/null || ( echo "golangci-lint not found. Install it manually or by running 'make setup_lint'."; exit 1 )
-	golangci-lint run --build-tags=$(TAGS) --timeout=2m0s
+	golangci-lint run --timeout=2m0s ./... ${EXAMPLES}
 
 install: all
   # Note: install everything but the examples
 	mkdir -p $(DESTDIR)
 	cp -t $(DESTDIR) $(BIN)/scion-*
 
 integration: build
-	go test -tags=integration,$(TAGS) --count=1 ./... ./_examples/helloworld/
+	go test -tags=integration,$(TAGS) --count=1 ./... ${EXAMPLES}
 
 .PHONY: scion-bat
 scion-bat:

_examples/helloquic/helloquic.go

@@ -41,7 +41,7 @@ func main() {
 	flag.Parse()
 
 	if (listen.Get().Port() > 0) == (len(*remoteAddr) > 0) {
-		check(fmt.Errorf("Either specify -port for server or -remote for client"))
+		check(fmt.Errorf("either specify -port for server or -remote for client"))
 	}
 
 	if listen.Get().Port() > 0 {
@@ -93,6 +93,9 @@ func workSession(session quic.Session) error {
 		}
 		fmt.Printf("%s\n", data)
 		_, err = stream.Write([]byte("gotcha: "))
+		if err != nil {
+			return err
+		}
 		_, err = stream.Write(data)
 		if err != nil {
 			return err
@@ -131,10 +134,12 @@ func runClient(address string, count int) error {
 		}
 		stream.Close()
 		reply, err := ioutil.ReadAll(stream)
+		if err != nil {
+			return err
+		}
 		fmt.Printf("%s\n", reply)
 	}
-	session.CloseWithError(quic.ApplicationErrorCode(0), "")
-	return nil
+	return session.CloseWithError(quic.ApplicationErrorCode(0), "")
 }
 
 // Check just ensures the error is nil, or complains and quits

_examples/helloworld/helloworld.go

@@ -37,7 +37,7 @@ func main() {
 	flag.Parse()
 
 	if (listen.Get().Port() > 0) == (len(*remoteAddr) > 0) {
-		check(fmt.Errorf("Either specify -listen for server or -remote for client"))
+		check(fmt.Errorf("either specify -listen for server or -remote for client"))
 	}
 
 	if listen.Get().Port() > 0 {
@@ -67,6 +67,9 @@ func runServer(listen netaddr.IPPort) error {
 		fmt.Printf("Received %s: %s\n", from, data)
 		msg := fmt.Sprintf("take it back! %s", time.Now().Format("15:04:05.0"))
 		n, err = conn.WriteTo([]byte(msg), from)
+		if err != nil {
+			return err
+		}
 		fmt.Printf("Wrote %d bytes.\n", n)
 	}
 }
@@ -90,7 +93,9 @@ func runClient(address string, count int) error {
 		fmt.Printf("Wrote %d bytes.\n", nBytes)
 
 		buffer := make([]byte, 16*1024)
-		conn.SetReadDeadline(time.Now().Add(1 * time.Second))
+		if err = conn.SetReadDeadline(time.Now().Add(1 * time.Second)); err != nil {
+			return err
+		}
 		n, err := conn.Read(buffer)
 		if errors.Is(err, os.ErrDeadlineExceeded) {
 			continue

_examples/helloworld/helloworld_integration_test.go

@@ -12,6 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+//go:build integration
 // +build integration
 
 package main

_examples/shttp/fileserver/main.go

@@ -23,6 +23,7 @@ import (
 	"os"
 
 	"github.com/gorilla/handlers"
+
 	"github.com/netsec-ethz/scion-apps/pkg/shttp"
 )
 

_examples/shttp/proxy/main.go

@@ -10,6 +10,7 @@ import (
 	"os"
 
 	"github.com/gorilla/handlers"
+
 	"github.com/netsec-ethz/scion-apps/pkg/shttp"
 )
 

_examples/shttp/server/main.go

@@ -24,6 +24,7 @@ import (
 	"time"
 
 	"github.com/gorilla/handlers"
+
 	"github.com/netsec-ethz/scion-apps/pkg/shttp"
 )
 
@@ -38,7 +39,7 @@ func main() {
 	m.HandleFunc("/hello", func(w http.ResponseWriter, r *http.Request) {
 		// Status 200 OK will be set implicitly
 		w.Header().Set("Content-Type", "text/plain")
-		w.Write([]byte(`Oh, hello!`))
+		_, _ = w.Write([]byte(`Oh, hello!`))
 	})
 
 	// handler that responds with an image file
@@ -75,7 +76,10 @@ func main() {
 	// POST handler that responds by parsing form values and returns them as string
 	m.HandleFunc("/form", func(w http.ResponseWriter, r *http.Request) {
 		if r.Method == http.MethodPost {
-			r.ParseForm()
+			if err := r.ParseForm(); err != nil {
+				http.Error(w, "invalid form data", http.StatusBadRequest)
+				return
+			}
 			w.Header().Set("Content-Type", "text/plain")
 			fmt.Fprint(w, "received following data:\n")
 			for s := range r.PostForm {

bwtester/bwtestlib/bwtestlib.go bwtester/bwtest/bwtest.go

@@ -12,7 +12,9 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package bwtestlib
+// package bwtest contains the definitions shared between bwtestserver and
+// bwtestclient.
+package bwtest
 
 import (
 	"bytes"
@@ -41,21 +43,17 @@ const (
 	MaxPacketSize int64 = 66000
 	// Make sure the port number is a port the server application can connect to
 	MinPort uint16 = 1024
-
-	MaxTries               = 5 // Number of times to try to reach server
-	Timeout  time.Duration = time.Millisecond * 500
-	MaxRTT   time.Duration = time.Millisecond * 1000
 )
 
-type BwtestParameters struct {
+type Parameters struct {
 	BwtestDuration time.Duration
 	PacketSize     int64
 	NumPackets     int64
 	PrgKey         []byte
 	Port           uint16
 }
 
-type BwtestResult struct {
+type Result struct {
 	NumPacketsReceived int64
 	CorrectlyReceived  int64
 	IPAvar             int64
@@ -107,45 +105,45 @@ func (f *prgFiller) Fill(iv int, data []byte) {
 	}
 }
 
-// Encode BwtestResult into a sufficiently large byte buffer that is passed in, return the number of bytes written
-func EncodeBwtestResult(res BwtestResult, buf []byte) (int, error) {
+// Encode Result into a sufficiently large byte buffer that is passed in, return the number of bytes written
+func EncodeResult(res Result, buf []byte) (int, error) {
 	var bb bytes.Buffer
 	enc := gob.NewEncoder(&bb)
 	err := enc.Encode(res)
 	copy(buf, bb.Bytes())
 	return bb.Len(), err
 }
 
-// Decode BwtestResult from byte buffer that is passed in, returns BwtestResult structure and number of bytes consumed
-func DecodeBwtestResult(buf []byte) (BwtestResult, int, error) {
+// Decode Result from byte buffer that is passed in, returns Result structure and number of bytes consumed
+func DecodeResult(buf []byte) (Result, int, error) {
 	bb := bytes.NewBuffer(buf)
 	is := bb.Len()
 	dec := gob.NewDecoder(bb)
-	var v BwtestResult
+	var v Result
 	err := dec.Decode(&v)
 	return v, is - bb.Len(), err
 }
 
-// Encode BwtestParameters into a sufficiently large byte buffer that is passed in, return the number of bytes written
-func EncodeBwtestParameters(bwtp BwtestParameters, buf []byte) (int, error) {
+// Encode Parameters into a sufficiently large byte buffer that is passed in, return the number of bytes written
+func EncodeParameters(bwtp Parameters, buf []byte) (int, error) {
 	var bb bytes.Buffer
 	enc := gob.NewEncoder(&bb)
 	err := enc.Encode(bwtp)
 	copy(buf, bb.Bytes())
 	return bb.Len(), err
 }
 
-// Decode BwtestParameters from byte buffer that is passed in, returns BwtestParameters structure and number of bytes consumed
-func DecodeBwtestParameters(buf []byte) (BwtestParameters, int, error) {
+// Decode Parameters from byte buffer that is passed in, returns BwtestParameters structure and number of bytes consumed
+func DecodeParameters(buf []byte) (Parameters, int, error) {
 	bb := bytes.NewBuffer(buf)
 	is := bb.Len()
 	dec := gob.NewDecoder(bb)
-	var v BwtestParameters
+	var v Parameters
 	err := dec.Decode(&v)
 	return v, is - bb.Len(), err
 }
 
-func HandleDCConnSend(bwp BwtestParameters, udpConnection io.Writer) error {
+func HandleDCConnSend(bwp Parameters, udpConnection io.Writer) error {
 	sb := make([]byte, bwp.PacketSize)
 	t0 := time.Now()
 	interPktInterval := bwp.BwtestDuration
@@ -167,7 +165,7 @@ func HandleDCConnSend(bwp BwtestParameters, udpConnection io.Writer) error {
 	return nil
 }
 
-func HandleDCConnReceive(bwp BwtestParameters, udpConnection io.Reader) BwtestResult {
+func HandleDCConnReceive(bwp Parameters, udpConnection io.Reader) Result {
 	var numPacketsReceived int64
 	var correctlyReceived int64
 	interPacketArrivalTime := make(map[int]int64, bwp.NumPackets)
@@ -203,7 +201,7 @@ func HandleDCConnReceive(bwp BwtestParameters, udpConnection io.Reader) BwtestRe
 		}
 	}
 
-	res := BwtestResult{
+	res := Result{
 		NumPacketsReceived: numPacketsReceived,
 		CorrectlyReceived:  correctlyReceived,
 		PrgKey:             bwp.PrgKey,
@@ -214,8 +212,8 @@ func HandleDCConnReceive(bwp BwtestParameters, udpConnection io.Reader) BwtestRe
 
 func aggrInterArrivalTime(bwr map[int]int64) (IPAvar, IPAmin, IPAavg, IPAmax int64) {
 	// reverse map, mapping timestamps to sequence numbers
-	revMap := make(map[int64]int)
-	var keys []int64 // keys are the timestamps of the received packets
+	revMap := make(map[int64]int, len(bwr))
+	keys := make([]int64, 0, len(bwr)) // keys are the timestamps of the received packets
 	// fill the reverse map and the keep track of the timestamps
 	for k, v := range bwr {
 		revMap[v] = k