Repository: scionproto/scion
Pull request #4693 (2025-02-03 16:55:10)

refactor: remove deprecated type ErrMsg
Follow up for #4586

jiceatscion: 

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/scionproto/scion/4693)
<!-- Reviewable:end -->


Patch:
diff --git a/control/beacon/BUILD.bazel b/control/beacon/BUILD.bazel
index 2029d2b1b..4a8d3c0e1 100644
--- a/control/beacon/BUILD.bazel
+++ b/control/beacon/BUILD.bazel
@@ -15,7 +15,6 @@ go_library(
     deps = [
         "//pkg/addr:go_default_library",
         "//pkg/log:go_default_library",
-        "//pkg/private/common:go_default_library",
         "//pkg/private/ptr:go_default_library",
         "//pkg/private/serrors:go_default_library",
         "//pkg/proto/control_plane:go_default_library",
diff --git a/control/beacon/db.go b/control/beacon/db.go
index f9b810514..6d85fb4ac 100644
--- a/control/beacon/db.go
+++ b/control/beacon/db.go
@@ -22,19 +22,10 @@ import (
 	"google.golang.org/protobuf/proto"
 
 	"github.com/scionproto/scion/pkg/addr"
-	"github.com/scionproto/scion/pkg/private/common"
 	cppb "github.com/scionproto/scion/pkg/proto/control_plane"
 	seg "github.com/scionproto/scion/pkg/segment"
 )
 
-const (
-	// ErrReadingRows is the error message in case we fail to read more from
-	// the database.
-	ErrReadingRows common.ErrMsg = "Failed to read rows"
-	// ErrParse is the error message in case the parsing a db entry fails.
-	ErrParse common.ErrMsg = "Failed to parse entry"
-)
-
 // InsertStats provides statistics about an insertion.
 type InsertStats struct {
 	Inserted, Updated, Filtered int
diff --git a/dispatcher/dispatcher.go b/dispatcher/dispatcher.go
index 4dca9e81e..02e442d68 100644
--- a/dispatcher/dispatcher.go
+++ b/dispatcher/dispatcher.go
@@ -15,6 +15,7 @@
 package dispatcher
 
 import (
+	"errors"
 	"fmt"
 	"net"
 	"net/netip"
@@ -32,7 +33,7 @@ import (
 	"github.com/scionproto/scion/pkg/slayers/path/scion"
 )
 
-const ErrUnsupportedL4 common.ErrMsg = "unsupported SCION L4 protocol"
+var ErrUnsupportedL4 = errors.New("unsupported SCION L4 protocol")
 
 // Server is the main object allowing to forward SCION packets coming
 // from legacy BR to the final endhost application and to handle SCMP
diff --git a/pkg/private/common/BUILD.bazel b/pkg/private/common/BUILD.bazel
index 5f6307f77..165b423fb 100644
--- a/pkg/private/common/BUILD.bazel
+++ b/pkg/private/common/BUILD.bazel
@@ -2,20 +2,16 @@ load("//tools/lint:go.bzl", "go_library", "go_test")
 
 go_library(
     name = "go_default_library",
-    srcs = [
-        "defs.go",
-        "errors.go",
-    ],
+    srcs = ["defs.go"],
     importpath = "github.com/scionproto/scion/pkg/private/common",
     visibility = ["//visibility:public"],
 )
 
 go_test(
     name = "go_default_test",
-    srcs = [
-        "defs_test.go",
-        "errors_test.go",
+    srcs = ["defs_test.go"],
+    deps = [
+        ":go_default_library",
+        "@com_github_stretchr_testify//assert:go_default_library",
     ],
-    embed = [":go_default_library"],
-    deps = ["@com_github_stretchr_testify//assert:go_default_library"],
 )
diff --git a/pkg/private/common/errors.go b/pkg/private/common/errors.go
deleted file mode 100644
index f932d0f24..000000000
--- a/pkg/private/common/errors.go
+++ /dev/null
@@ -1,24 +0,0 @@
-// Copyright 2016 ETH Zurich
-// Copyright 2019 ETH Zurich, Anapaya Systems
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-//   http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package common
-
-// Deprecated: ErrMsg is not useful. It mimics errors.New() but fails to return a singleton. Just
-// use errors.New() if you need cheap sentinel errors.
-type ErrMsg string
-
-func (e ErrMsg) Error() string {
-	return string(e)
-}
diff --git a/private/keyconf/BUILD.bazel b/private/keyconf/BUILD.bazel
index 84c0f433d..a866af013 100644
--- a/private/keyconf/BUILD.bazel
+++ b/private/keyconf/BUILD.bazel
@@ -5,10 +5,7 @@ go_library(
     srcs = ["keyconf.go"],
     importpath = "github.com/scionproto/scion/private/keyconf",
     visibility = ["//visibility:public"],
-    deps = [
-        "//pkg/private/common:go_default_library",
-        "//pkg/private/serrors:go_default_library",
-    ],
+    deps = ["//pkg/private/serrors:go_default_library"],
 )
 
 go_test(
diff --git a/private/keyconf/keyconf.go b/private/keyconf/keyconf.go
index 32b91ae61..447040ae4 100644
--- a/private/keyconf/keyconf.go
+++ b/private/keyconf/keyconf.go
@@ -16,12 +16,12 @@ package keyconf
 
 import (
 	"encoding/base64"
+	"errors"
 	"fmt"
 	"os"
 	"path/filepath"
 	"strings"
 
-	"github.com/scionproto/scion/pkg/private/common"
 	"github.com/scionproto/scion/pkg/private/serrors"
 )
 
@@ -33,10 +33,10 @@ const (
 )
 
 // Errors
-const (
-	ErrOpen    common.ErrMsg = "Unable to load key"
-	ErrParse   common.ErrMsg = "Unable to parse key file"
-	ErrUnknown common.ErrMsg = "Unknown algorithm"
+var (
+	ErrOpen    = errors.New("unable to load key")
+	ErrParse   = errors.New("unable to parse key file")
+	ErrUnknown = errors.New("unknown algorithm")
 )
 
 // loadKey decodes a base64 encoded key stored in file and returns the raw bytes.
diff --git a/private/storage/beacon/sqlite/db.go b/private/storage/beacon/sqlite/db.go
index 32a3a9f3e..0d2b67efa 100644
--- a/private/storage/beacon/sqlite/db.go
+++ b/private/storage/beacon/sqlite/db.go
@@ -142,11 +142,11 @@ func (e *executor) CandidateBeacons(
 		var rawBeacon sql.RawBytes
 		var inIfID iface.ID
 		if err = rows.Scan(&rawBeacon, &inIfID); err != nil {
-			return nil, db.NewReadError(beacon.ErrReadingRows, err)
+			return nil, db.NewReadError("failed to read rows", err)
 		}
 		s, err := beacon.UnpackBeacon(rawBeacon)
 		if err != nil {
-			return nil, db.NewDataError(beacon.ErrParse, err)
+			return nil, db.NewDataError("failed to parse entry", err)
 		}
 		beacons = append(beacons, beacon.Beacon{Segment: s, InIfID: uint16(inIfID)})
 	}
diff --git a/private/storage/db/BUILD.bazel b/private/storage/db/BUILD.bazel
index 9ed6145d9..38c27701f 100644
--- a/private/storage/db/BUILD.bazel
+++ b/private/storage/db/BUILD.bazel
@@ -14,7 +14,6 @@ go_library(
     importpath = "github.com/scionproto/scion/private/storage/db",
     visibility = ["//visibility:public"],
     deps = [
-        "//pkg/private/common:go_default_library",
         "//pkg/private/prom:go_default_library",
         "//pkg/private/serrors:go_default_library",
         "@org_modernc_sqlite//:go_default_library",
@@ -25,4 +24,5 @@ go_test(
     name = "go_default_test",
     srcs = ["errors_test.go"],
     embed = [":go_default_library"],
+    deps = ["@com_github_stretchr_testify//require:go_default_library"],
 )
diff --git a/private/storage/db/errors.go b/private/storage/db/errors.go
index 2658f5acb..7ee40e5bf 100644
--- a/private/storage/db/errors.go
+++ b/private/storage/db/errors.go
@@ -15,7 +15,6 @@
 package db
 
 import (
-	"github.com/scionproto/scion/pkg/private/common"
 	"github.com/scionproto/scion/pkg/private/serrors"
 )
 
@@ -32,27 +31,27 @@ var (
 	ErrTx = serrors.New("db: transaction error")
 )
 
-func NewTxError(msg common.ErrMsg, err error, logCtx ...any) error {
+func NewTxError(msg string, err error, logCtx ...any) error {
 	return serrors.JoinNoStack(ErrTx, err,
 		append([]any{"detailMsg", msg}, logCtx...)...)
 }
 
-func NewInputDataError(msg common.ErrMsg, err error, logCtx ...any) error {
+func NewInputDataError(msg string, err error, logCtx ...any) error {
 	return serrors.JoinNoStack(ErrInvalidInputData, err,
 		append([]any{"detailMsg", msg}, logCtx...)...)
 }
 
-func NewDataError(msg common.ErrMsg, err error, logCtx ...any) error {
+func NewDataError(msg string, err error, logCtx ...any) error {
 	return serrors.JoinNoStack(ErrDataInvalid, err,
 		append([]any{"detailMsg", msg}, logCtx...)...)
 }
 
-func NewReadError(msg common.ErrMsg, err error, logCtx ...any) error {
+func NewReadError(msg string, err error, logCtx ...any) error {
 	return serrors.JoinNoStack(ErrReadFailed, err,
 		append([]any{"detailMsg", msg}, logCtx...)...)
 }
 
-func NewWriteError(msg common.ErrMsg, err error, logCtx ...any) error {
+func NewWriteError(msg string, err error, logCtx ...any) error {
 	return serrors.JoinNoStack(ErrWriteFailed, err,
 		append([]any{"detailMsg", msg}, logCtx...)...)
 }
diff --git a/private/svc/resolver.go b/private/svc/resolver.go
index e4af3741f..e42ab0e07 100644
--- a/private/svc/resolver.go
+++ b/private/svc/resolver.go
@@ -16,6 +16,7 @@ package svc
 
 import (
 	"context"
+	"errors"
 	"net"
 	"net/netip"
 
@@ -34,14 +35,14 @@ import (
 
 // Internal resolver errors. These are implementation details and can change,
 // and calling code should not depend on them.
-const (
-	errNilPacket      common.ErrMsg = "packet is nil"
-	errNilUnderlay    common.ErrMsg = "underlay is nil"
-	errUnsupportedPld common.ErrMsg = "unsupported payload type"
-	errRegistration   common.ErrMsg = "unable to open conn"
-	errWrite          common.ErrMsg = "unable to write"
-	errRead           common.ErrMsg = "unable to read"
-	errDecode         common.ErrMsg = "decode failed"
+var (
+	errNilPacket      = errors.New("packet is nil")
+	errNilUnderlay    = errors.New("underlay is nil")
+	errUnsupportedPld = errors.New("unsupported payload type")
+	errRegistration   = errors.New("unable to open conn")
+	errWrite          = errors.New("unable to write")
+	errRead           = errors.New("unable to read")
+	errDecode         = errors.New("decode failed")
 )
 
 // For now, the request payload does not need to be dynamic. We initialize it
diff --git a/private/svc/svc.go b/private/svc/svc.go
index b6cc57810..6b536ed5b 100644
--- a/private/svc/svc.go
+++ b/private/svc/svc.go
@@ -16,6 +16,7 @@
 package svc
 
 import (
+	"errors"
 	"net"
 
 	"github.com/scionproto/scion/pkg/addr"
@@ -25,7 +26,7 @@ import (
 	"github.com/scionproto/scion/pkg/snet"
 )
 
-const ErrHandler common.ErrMsg = "Unable to handle SVC request"
+var ErrHandler = errors.New("unable to handle SVC request")
 
 // Result is used to inform Handler users on the outcome of handler execution.
 type Result int


Test patch:
diff --git a/pkg/private/common/errors_test.go b/pkg/private/common/errors_test.go
deleted file mode 100644
index 10e29f538..000000000
--- a/pkg/private/common/errors_test.go
+++ /dev/null
@@ -1,28 +0,0 @@
-// Copyright 2018 ETH Zurich
-// Copyright 2019 ETH Zurich, Anapaya Systems
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-//   http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package common
-
-import (
-	"testing"
-
-	"github.com/stretchr/testify/assert"
-)
-
-func TestErrMsg(t *testing.T) {
-	errText := "test error string"
-	err := ErrMsg(errText)
-	assert.Equal(t, errText, err.Error())
-}
diff --git a/private/storage/db/errors_test.go b/private/storage/db/errors_test.go
index ee9397b96..cdacb67d8 100644
--- a/private/storage/db/errors_test.go
+++ b/private/storage/db/errors_test.go
@@ -17,41 +17,20 @@ package db
 import (
 	"fmt"
 	"testing"
+
+	"github.com/stretchr/testify/require"
 )
 
 func TestErrFmt(t *testing.T) {
-	tests := []struct {
-		Err      error
-		Expected string
-	}{
-		{
-			Err:      NewTxError("test", nil),
-			Expected: fmt.Sprintf("%s {detailMsg=test}", ErrTx),
-		},
-		{
-			Err:      NewInputDataError("test", nil),
-			Expected: fmt.Sprintf("%s {detailMsg=test}", ErrInvalidInputData),
-		},
-		{
-			Err:      NewDataError("test", nil),
-			Expected: fmt.Sprintf("%s {detailMsg=test}", ErrDataInvalid),
-		},
-		{
-			Err:      NewReadError("test", nil),
-			Expected: fmt.Sprintf("%s {detailMsg=test}", ErrReadFailed),
-		},
-		{
-			Err:      NewWriteError("test", nil),
-			Expected: fmt.Sprintf("%s {detailMsg=test}", ErrWriteFailed),
-		},
-	}
-	for _, test := range tests {
-		checkStringEq(t, test.Err.Error(), test.Expected)
+	f := func(t *testing.T, expect error, err error) {
+		t.Helper()
+		expectedMsg := fmt.Sprintf("%s {detailMsg=test}", expect)
+		require.Equal(t, expectedMsg, err.Error())
 	}
-}
 
-func checkStringEq(t *testing.T, actual, expected string) {
-	if actual != expected {
-		t.Fatalf("Expected %s to be %s", actual, expected)
-	}
+	f(t, ErrTx, NewTxError("test", nil))
+	f(t, ErrInvalidInputData, NewInputDataError("test", nil))
+	f(t, ErrDataInvalid, NewDataError("test", nil))
+	f(t, ErrReadFailed, NewReadError("test", nil))
+	f(t, ErrWriteFailed, NewWriteError("test", nil))
 }