From 3ca57d92c64c16e58e78e4df0da04341b3fdc4fe Mon Sep 17 00:00:00 2001 From: Martin Hutchinson Date: Wed, 26 Apr 2023 16:15:54 +0100 Subject: [PATCH] Enable all lint checks in trillian repo Fixed all of the remaining lint errors. --- .golangci.yaml | 16 ------ storage/admin_helpers.go | 7 ++- storage/cache/subtree_cache.go | 6 ++- storage/cloudspanner/getdb_test.go | 6 ++- storage/cloudspanner/log_storage.go | 6 ++- storage/crdb/common_test.go | 4 +- storage/crdb/log_storage.go | 62 +++++++++++++++++++----- storage/crdb/log_storage_test.go | 6 ++- storage/crdb/queue.go | 6 ++- storage/crdb/sqladminstorage.go | 43 +++++++++++++--- storage/crdb/sqladminstorage_test.go | 2 +- storage/crdb/tree_storage.go | 18 +++++-- storage/memory/admin_storage.go | 6 ++- storage/memory/log_storage.go | 21 ++++++-- storage/mysql/admin_storage.go | 43 +++++++++++++--- storage/mysql/admin_storage_test.go | 2 +- storage/mysql/log_storage.go | 62 +++++++++++++++++++----- storage/mysql/log_storage_test.go | 7 ++- storage/mysql/queue.go | 6 ++- storage/mysql/storage_test.go | 2 +- storage/mysql/tree_storage.go | 18 +++++-- storage/provider_test.go | 18 ++++--- storage/testdb/testdb.go | 16 ++++-- storage/testonly/admin_storage_tester.go | 6 ++- storage/testonly/transaction.go | 13 ++++- 25 files changed, 315 insertions(+), 87 deletions(-) diff --git a/.golangci.yaml b/.golangci.yaml index 4784f8fde4..0c50dcbda2 100644 --- a/.golangci.yaml +++ b/.golangci.yaml @@ -15,22 +15,6 @@ linters-settings: - golang.org/x/net/context - github.com/gogo/protobuf/proto -linters: - disable-all: true - enable: - - depguard - - gocyclo - - gofmt - - goimports - - govet - - ineffassign - - megacheck - - misspell - - revive - - unused - # TODO(gbelvin): write license linter and commit to upstream. - # ./scripts/check_license.sh is run by ./scripts/presubmit.sh - issues: # Don't turn off any checks by default. We can do this explicitly if needed. exclude-use-default: false diff --git a/storage/admin_helpers.go b/storage/admin_helpers.go index d1cff4d132..fcc18686ab 100644 --- a/storage/admin_helpers.go +++ b/storage/admin_helpers.go @@ -20,6 +20,7 @@ import ( "github.com/google/trillian" "github.com/google/trillian/monitoring" + "k8s.io/klog/v2" ) const traceSpanRoot = "/trillian/storage" @@ -131,7 +132,11 @@ func RunInAdminSnapshot(ctx context.Context, admin AdminStorage, fn func(tx Read if err != nil { return err } - defer tx.Close() + defer func() { + if err := tx.Close(); err != nil { + klog.Errorf("tx.Close(): %v", err) + } + }() if err := fn(tx); err != nil { return err } diff --git a/storage/cache/subtree_cache.go b/storage/cache/subtree_cache.go index 24360dedc5..3e0cdf5638 100644 --- a/storage/cache/subtree_cache.go +++ b/storage/cache/subtree_cache.go @@ -25,6 +25,7 @@ import ( "github.com/transparency-dev/merkle" "github.com/transparency-dev/merkle/compact" "google.golang.org/protobuf/proto" + "k8s.io/klog/v2" ) // TODO(al): move this up the stack @@ -116,7 +117,10 @@ func (s *SubtreeCache) preload(ids []compact.NodeID, getSubtrees GetSubtreesFunc // return it when done defer func() { workTokens <- true }() - PopulateLogTile(t, s.hasher) + if err := PopulateLogTile(t, s.hasher); err != nil { + // TODO(mhutchinson): This error should be propagated. + klog.Errorf("PopulateLogTile(): %v", err) + } ch <- t // Note: This never blocks because len(ch) == len(subtrees). }() } diff --git a/storage/cloudspanner/getdb_test.go b/storage/cloudspanner/getdb_test.go index 5574eddcdc..a309b0af55 100644 --- a/storage/cloudspanner/getdb_test.go +++ b/storage/cloudspanner/getdb_test.go @@ -101,7 +101,11 @@ func inMemClient(ctx context.Context, t testing.TB, dbName string, statements [] } client, err := spanner.NewClient(ctx, dbName, option.WithGRPCConn(conn)) if err != nil { - conn.Close() + defer func() { + if err := conn.Close(); err != nil { + t.Errorf("conn.Close(): %v", err) + } + }() t.Fatalf("Connecting to in-memory fake: %v", err) } t.Cleanup(client.Close) diff --git a/storage/cloudspanner/log_storage.go b/storage/cloudspanner/log_storage.go index acf530f349..dd49b39523 100644 --- a/storage/cloudspanner/log_storage.go +++ b/storage/cloudspanner/log_storage.go @@ -190,7 +190,11 @@ func (ls *logStorage) begin(ctx context.Context, tree *trillian.Tree, readonly b if err := ltx.getLatestRoot(ctx); err == storage.ErrTreeNeedsInit { return ltx, err } else if err != nil { - tx.Close() + defer func() { + if err := tx.Close(); err != nil { + klog.Errorf("conn.Close(): %v", err) + } + }() return nil, err } return ltx, nil diff --git a/storage/crdb/common_test.go b/storage/crdb/common_test.go index 52d35a308b..45dafc679d 100644 --- a/storage/crdb/common_test.go +++ b/storage/crdb/common_test.go @@ -55,7 +55,9 @@ func TestMain(m *testing.M) { dburl.Path = "/" // Set the environment variable for the test server - os.Setenv(testdb.CockroachDBURIEnv, dburl.String()) + if err := os.Setenv(testdb.CockroachDBURIEnv, dburl.String()); err != nil { + klog.Exitf("Failed to SetEnv CockroachDBURIEnv: %v", err) + } if !testdb.CockroachDBAvailable() { klog.Errorf("CockroachDB not available, skipping all CockroachDB storage tests") diff --git a/storage/crdb/log_storage.go b/storage/crdb/log_storage.go index 499402c44e..f2057141d7 100644 --- a/storage/crdb/log_storage.go +++ b/storage/crdb/log_storage.go @@ -171,7 +171,11 @@ func (m *crdbLogStorage) GetActiveLogIDs(ctx context.Context) ([]int64, error) { if err != nil { return nil, err } - defer rows.Close() + defer func() { + if err := rows.Close(); err != nil { + klog.Errorf("rows.Close(): %v", err) + } + }() ids := []int64{} for rows.Next() { var treeID int64 @@ -204,12 +208,16 @@ func (m *crdbLogStorage) beginInternal(ctx context.Context, tree *trillian.Tree) ltx.treeTX.writeRevision = 0 return ltx, err } else if err != nil { - ttx.Close() + if err := ttx.Close(); err != nil { + klog.Errorf("ttx.Close(): %v", err) + } return nil, err } if err := ltx.root.UnmarshalBinary(ltx.slr.LogRoot); err != nil { - ttx.Close() + if err := ttx.Close(); err != nil { + klog.Errorf("ttx.Close(): %v", err) + } return nil, err } @@ -226,7 +234,11 @@ func (m *crdbLogStorage) ReadWriteTransaction(ctx context.Context, tree *trillia if err != nil && err != storage.ErrTreeNeedsInit { return err } - defer tx.Close() + defer func() { + if err := tx.Close(); err != nil { + klog.Errorf("tx.Close(): %v", err) + } + }() if err := f(ctx, tx); err != nil { return err } @@ -239,7 +251,11 @@ func (m *crdbLogStorage) AddSequencedLeaves(ctx context.Context, tree *trillian. // Ensure we don't leak the transaction. For example if we get an // ErrTreeNeedsInit from beginInternal() or if AddSequencedLeaves fails // below. - defer tx.Close() + defer func() { + if err := tx.Close(); err != nil { + klog.Errorf("tx.Close(): %v", err) + } + }() } if err != nil { return nil, err @@ -268,7 +284,11 @@ func (m *crdbLogStorage) QueueLeaves(ctx context.Context, tree *trillian.Tree, l // Ensure we don't leak the transaction. For example if we get an // ErrTreeNeedsInit from beginInternal() or if QueueLeaves fails // below. - defer tx.Close() + defer func() { + if err := tx.Close(); err != nil { + klog.Errorf("tx.Close(): %v", err) + } + }() } if err != nil { return nil, err @@ -328,7 +348,11 @@ func (t *logTreeTX) DequeueLeaves(ctx context.Context, limit int, cutoffTime tim klog.Warningf("Failed to prepare dequeue select: %s", err) return nil, err } - defer stx.Close() + defer func() { + if err := stx.Close(); err != nil { + klog.Errorf("stx.Close(): %v", err) + } + }() leaves := make([]*trillian.LogLeaf, 0, limit) rows, err := stx.QueryContext(ctx, t.treeID, cutoffTime.UnixNano(), limit) @@ -336,7 +360,11 @@ func (t *logTreeTX) DequeueLeaves(ctx context.Context, limit int, cutoffTime tim klog.Warningf("Failed to select rows for work: %s", err) return nil, err } - defer rows.Close() + defer func() { + if err := rows.Close(); err != nil { + klog.Errorf("rows.Close(): %v", err) + } + }() for rows.Next() { leaf, dqInfo, err := t.dequeueLeaf(rows) @@ -639,7 +667,11 @@ func (t *logTreeTX) getLeavesByRangeInternal(ctx context.Context, start, count i klog.Warningf("Failed to get leaves by range: %s", err) return nil, err } - defer rows.Close() + defer func() { + if err := rows.Close(); err != nil { + klog.Errorf("rows.Close(): %v", err) + } + }() ret := make([]*trillian.LogLeaf, 0, count) for wantIndex := start; rows.Next(); wantIndex++ { @@ -770,7 +802,11 @@ func (t *logTreeTX) StoreSignedLogRoot(ctx context.Context, root *trillian.Signe func (t *logTreeTX) getLeavesByHashInternal(ctx context.Context, leafHashes [][]byte, tmpl *sql.Stmt, desc string) ([]*trillian.LogLeaf, error) { stx := t.tx.StmtContext(ctx, tmpl) - defer stx.Close() + defer func() { + if err := stx.Close(); err != nil { + klog.Errorf("stx.Close(): %v", err) + } + }() var args []interface{} for _, hash := range leafHashes { @@ -782,7 +818,11 @@ func (t *logTreeTX) getLeavesByHashInternal(ctx context.Context, leafHashes [][] klog.Warningf("Query() %s hash = %v", desc, err) return nil, err } - defer rows.Close() + defer func() { + if err := rows.Close(); err != nil { + klog.Errorf("rows.Close(): %v", err) + } + }() // The tree could include duplicates so we don't know how many results will be returned var ret []*trillian.LogLeaf diff --git a/storage/crdb/log_storage_test.go b/storage/crdb/log_storage_test.go index e00ac458da..f95a434815 100644 --- a/storage/crdb/log_storage_test.go +++ b/storage/crdb/log_storage_test.go @@ -754,7 +754,11 @@ func TestGetActiveLogIDs(t *testing.T) { if err != nil { t.Fatalf("PrepareContext() returned err = %v", err) } - defer updateDeletedStmt.Close() + defer func() { + if err := updateDeletedStmt.Close(); err != nil { + t.Errorf("updateDeletedStmt.Close(): %v", err) + } + }() for _, treeID := range []int64{deletedLog.TreeId} { if _, err := updateDeletedStmt.ExecContext(ctx, true, treeID); err != nil { t.Fatalf("ExecContext(%v) returned err = %v", treeID, err) diff --git a/storage/crdb/queue.go b/storage/crdb/queue.go index c65de60154..937eb7a181 100644 --- a/storage/crdb/queue.go +++ b/storage/crdb/queue.go @@ -125,7 +125,11 @@ func (t *logTreeTX) removeSequencedLeaves(ctx context.Context, leaves []dequeued klog.Warningf("Failed to prep delete statement for sequenced work: %v", err) return err } - defer stx.Close() + defer func() { + if err := stx.Close(); err != nil { + klog.Errorf("stx.Close(): %v", err) + } + }() for _, dql := range leaves { result, err := stx.ExecContext(ctx, t.treeID, dql.queueTimestampNanos, dql.leafIdentityHash) err = checkResultOkAndRowCountIs(result, err, int64(1)) diff --git a/storage/crdb/sqladminstorage.go b/storage/crdb/sqladminstorage.go index 007de9add1..ec805163d3 100644 --- a/storage/crdb/sqladminstorage.go +++ b/storage/crdb/sqladminstorage.go @@ -27,6 +27,7 @@ import ( "google.golang.org/grpc/status" "google.golang.org/protobuf/proto" "google.golang.org/protobuf/types/known/timestamppb" + "k8s.io/klog/v2" ) const ( @@ -88,7 +89,11 @@ func (s *sqlAdminStorage) ReadWriteTransaction(ctx context.Context, f storage.Ad if err != nil { return err } - defer tx.Close() + defer func() { + if err := tx.Close(); err != nil { + klog.Errorf("tx.Close(): %v", err) + } + }() if err := f(ctx, tx); err != nil { return err } @@ -132,7 +137,11 @@ func (t *adminTX) GetTree(ctx context.Context, treeID int64) (*trillian.Tree, er if err != nil { return nil, err } - defer stmt.Close() + defer func() { + if err := stmt.Close(); err != nil { + klog.Errorf("stmt.Close(): %v", err) + } + }() // GetTree is an entry point for most RPCs, let's provide somewhat nicer error messages. tree, err := storage.ReadTree(stmt.QueryRowContext(ctx, treeID)) @@ -158,12 +167,20 @@ func (t *adminTX) ListTrees(ctx context.Context, includeDeleted bool) ([]*trilli if err != nil { return nil, err } - defer stmt.Close() + defer func() { + if err := stmt.Close(); err != nil { + klog.Errorf("stmt.Close(): %v", err) + } + }() rows, err := stmt.QueryContext(ctx) if err != nil { return nil, err } - defer rows.Close() + defer func() { + if err := rows.Close(); err != nil { + klog.Errorf("rows.Close(): %v", err) + } + }() trees := []*trillian.Tree{} for rows.Next() { tree, err := storage.ReadTree(rows) @@ -227,7 +244,11 @@ func (t *adminTX) CreateTree(ctx context.Context, tree *trillian.Tree) (*trillia if err != nil { return nil, err } - defer insertTreeStmt.Close() + defer func() { + if err := insertTreeStmt.Close(); err != nil { + klog.Errorf("insertTreeStmt.Close(): %v", err) + } + }() _, err = insertTreeStmt.ExecContext( ctx, @@ -269,7 +290,11 @@ func (t *adminTX) CreateTree(ctx context.Context, tree *trillian.Tree) (*trillia if err != nil { return nil, err } - defer insertControlStmt.Close() + defer func() { + if err := insertControlStmt.Close(); err != nil { + klog.Errorf("insertControlStmt.Close(): %v", err) + } + }() _, err = insertControlStmt.ExecContext( ctx, newTree.TreeId, @@ -318,7 +343,11 @@ func (t *adminTX) UpdateTree(ctx context.Context, treeID int64, updateFunc func( if err != nil { return nil, err } - defer stmt.Close() + defer func() { + if err := stmt.Close(); err != nil { + klog.Errorf("stmt.Close(): %v", err) + } + }() if _, err = stmt.ExecContext( ctx, diff --git a/storage/crdb/sqladminstorage_test.go b/storage/crdb/sqladminstorage_test.go index 1a93918c73..7665e38949 100644 --- a/storage/crdb/sqladminstorage_test.go +++ b/storage/crdb/sqladminstorage_test.go @@ -252,7 +252,7 @@ func setNulls(ctx context.Context, db *sql.DB, treeID int64) error { if err != nil { return err } - defer stmt.Close() + defer func() { _ = stmt.Close() }() _, err = stmt.ExecContext(ctx, treeID) return err } diff --git a/storage/crdb/tree_storage.go b/storage/crdb/tree_storage.go index 8eb547ba8c..8f1ef35697 100644 --- a/storage/crdb/tree_storage.go +++ b/storage/crdb/tree_storage.go @@ -189,7 +189,11 @@ func (t *treeTX) getSubtrees(ctx context.Context, treeRevision int64, ids [][]by return nil, err } stx := t.tx.StmtContext(ctx, tmpl) - defer stx.Close() + defer func() { + if err := stx.Close(); err != nil { + klog.Errorf("stx.Close(): %v", err) + } + }() args := make([]interface{}, 0, len(ids)+3) @@ -208,7 +212,11 @@ func (t *treeTX) getSubtrees(ctx context.Context, treeRevision int64, ids [][]by klog.Warningf("Failed to get merkle subtrees: %s", err) return nil, err } - defer rows.Close() + defer func() { + if err := rows.Close(); err != nil { + klog.Errorf("rows.Close(): %v", err) + } + }() if rows.Err() != nil { // Nothing from the DB @@ -297,7 +305,11 @@ func (t *treeTX) storeSubtrees(ctx context.Context, subtrees []*storagepb.Subtre return err } stx := t.tx.StmtContext(ctx, tmpl) - defer stx.Close() + defer func() { + if err := stx.Close(); err != nil { + klog.Errorf("stx.Close(): %v", err) + } + }() r, err := stx.ExecContext(ctx, args...) if err != nil { diff --git a/storage/memory/admin_storage.go b/storage/memory/admin_storage.go index 904f4e58ec..dd7839e5ed 100644 --- a/storage/memory/admin_storage.go +++ b/storage/memory/admin_storage.go @@ -44,7 +44,11 @@ func (s *memoryAdminStorage) Snapshot(ctx context.Context) (storage.ReadOnlyAdmi func (s *memoryAdminStorage) ReadWriteTransaction(ctx context.Context, f storage.AdminTXFunc) error { tx := &adminTX{ms: s.ms} - defer tx.Close() + defer func() { + if err := tx.Close(); err != nil { + klog.Errorf("tx.Close(): %v", err) + } + }() if err := f(ctx, tx); err != nil { return err } diff --git a/storage/memory/log_storage.go b/storage/memory/log_storage.go index 13c0daaa84..fcc742a214 100644 --- a/storage/memory/log_storage.go +++ b/storage/memory/log_storage.go @@ -33,6 +33,7 @@ import ( "github.com/transparency-dev/merkle/rfc6962" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" + "k8s.io/klog/v2" ) const logIDLabel = "logid" @@ -148,12 +149,16 @@ func (m *memoryLogStorage) beginInternal(ctx context.Context, tree *trillian.Tre if err == storage.ErrTreeNeedsInit { return ltx, err } else if err != nil { - ttx.Close() + if err := ttx.Close(); err != nil { + klog.Errorf("ttx.Close(): %v", err) + } return nil, err } if err := ltx.root.UnmarshalBinary(ltx.slr.LogRoot); err != nil { - ttx.Close() + if err := ttx.Close(); err != nil { + klog.Errorf("ttx.Close(): %v", err) + } return nil, err } @@ -167,7 +172,11 @@ func (m *memoryLogStorage) ReadWriteTransaction(ctx context.Context, tree *trill if err != nil && err != storage.ErrTreeNeedsInit { return err } - defer tx.Close() + defer func() { + if err := tx.Close(); err != nil { + klog.Errorf("tx.Close(): %v", err) + } + }() if err := f(ctx, tx); err != nil { return err } @@ -192,7 +201,11 @@ func (m *memoryLogStorage) QueueLeaves(ctx context.Context, tree *trillian.Tree, // Ensure we don't leak the transaction. For example if we get an // ErrTreeNeedsInit from beginInternal() or if QueueLeaves fails // below. - defer tx.Close() + defer func() { + if err := tx.Close(); err != nil { + klog.Errorf("tx.Close(): %v", err) + } + }() } if err != nil { return nil, err diff --git a/storage/mysql/admin_storage.go b/storage/mysql/admin_storage.go index ca73641e8e..16c62404b6 100644 --- a/storage/mysql/admin_storage.go +++ b/storage/mysql/admin_storage.go @@ -27,6 +27,7 @@ import ( "google.golang.org/grpc/status" "google.golang.org/protobuf/proto" "google.golang.org/protobuf/types/known/timestamppb" + "k8s.io/klog/v2" ) const ( @@ -87,7 +88,11 @@ func (s *mysqlAdminStorage) ReadWriteTransaction(ctx context.Context, f storage. if err != nil { return err } - defer tx.Close() + defer func() { + if err := tx.Close(); err != nil { + klog.Errorf("tx.Close(): %v", err) + } + }() if err := f(ctx, tx); err != nil { return err } @@ -131,7 +136,11 @@ func (t *adminTX) GetTree(ctx context.Context, treeID int64) (*trillian.Tree, er if err != nil { return nil, err } - defer stmt.Close() + defer func() { + if err := stmt.Close(); err != nil { + klog.Errorf("stmt.Close(): %v", err) + } + }() // GetTree is an entry point for most RPCs, let's provide somewhat nicer error messages. tree, err := storage.ReadTree(stmt.QueryRowContext(ctx, treeID)) @@ -157,12 +166,20 @@ func (t *adminTX) ListTrees(ctx context.Context, includeDeleted bool) ([]*trilli if err != nil { return nil, err } - defer stmt.Close() + defer func() { + if err := stmt.Close(); err != nil { + klog.Errorf("stmt.Close(): %v", err) + } + }() rows, err := stmt.QueryContext(ctx) if err != nil { return nil, err } - defer rows.Close() + defer func() { + if err := rows.Close(); err != nil { + klog.Errorf("rows.Close(): %v", err) + } + }() trees := []*trillian.Tree{} for rows.Next() { tree, err := storage.ReadTree(rows) @@ -226,7 +243,11 @@ func (t *adminTX) CreateTree(ctx context.Context, tree *trillian.Tree) (*trillia if err != nil { return nil, err } - defer insertTreeStmt.Close() + defer func() { + if err := insertTreeStmt.Close(); err != nil { + klog.Errorf("insertTreeStmt.Close(): %v", err) + } + }() _, err = insertTreeStmt.ExecContext( ctx, @@ -268,7 +289,11 @@ func (t *adminTX) CreateTree(ctx context.Context, tree *trillian.Tree) (*trillia if err != nil { return nil, err } - defer insertControlStmt.Close() + defer func() { + if err := insertControlStmt.Close(); err != nil { + klog.Errorf("insertControlStmt.Close(): %v", err) + } + }() _, err = insertControlStmt.ExecContext( ctx, newTree.TreeId, @@ -317,7 +342,11 @@ func (t *adminTX) UpdateTree(ctx context.Context, treeID int64, updateFunc func( if err != nil { return nil, err } - defer stmt.Close() + defer func() { + if err := stmt.Close(); err != nil { + klog.Errorf("stmt.Close(): %v", err) + } + }() if _, err = stmt.ExecContext( ctx, diff --git a/storage/mysql/admin_storage_test.go b/storage/mysql/admin_storage_test.go index 2907506c3f..54d636d551 100644 --- a/storage/mysql/admin_storage_test.go +++ b/storage/mysql/admin_storage_test.go @@ -228,7 +228,7 @@ func setNulls(ctx context.Context, db *sql.DB, treeID int64) error { if err != nil { return err } - defer stmt.Close() + defer func() { _ = stmt.Close() }() _, err = stmt.ExecContext(ctx, treeID) return err } diff --git a/storage/mysql/log_storage.go b/storage/mysql/log_storage.go index 2d57c7d4fd..1c3795d5c6 100644 --- a/storage/mysql/log_storage.go +++ b/storage/mysql/log_storage.go @@ -166,7 +166,11 @@ func (m *mySQLLogStorage) GetActiveLogIDs(ctx context.Context) ([]int64, error) if err != nil { return nil, err } - defer rows.Close() + defer func() { + if err := rows.Close(); err != nil { + klog.Errorf("rows.Close(): %v", err) + } + }() ids := []int64{} for rows.Next() { var treeID int64 @@ -199,12 +203,16 @@ func (m *mySQLLogStorage) beginInternal(ctx context.Context, tree *trillian.Tree ltx.treeTX.writeRevision = 0 return ltx, err } else if err != nil { - ttx.Close() + if err := ttx.Close(); err != nil { + klog.Errorf("ttx.Close(): %v", err) + } return nil, err } if err := ltx.root.UnmarshalBinary(ltx.slr.LogRoot); err != nil { - ttx.Close() + if err := ttx.Close(); err != nil { + klog.Errorf("ttx.Close(): %v", err) + } return nil, err } @@ -221,7 +229,11 @@ func (m *mySQLLogStorage) ReadWriteTransaction(ctx context.Context, tree *trilli if err != nil && err != storage.ErrTreeNeedsInit { return err } - defer tx.Close() + defer func() { + if err := tx.Close(); err != nil { + klog.Errorf("tx.Close(): %v", err) + } + }() if err := f(ctx, tx); err != nil { return err } @@ -234,7 +246,11 @@ func (m *mySQLLogStorage) AddSequencedLeaves(ctx context.Context, tree *trillian // Ensure we don't leak the transaction. For example if we get an // ErrTreeNeedsInit from beginInternal() or if AddSequencedLeaves fails // below. - defer tx.Close() + defer func() { + if err := tx.Close(); err != nil { + klog.Errorf("tx.Close(): %v", err) + } + }() } if err != nil { return nil, err @@ -263,7 +279,11 @@ func (m *mySQLLogStorage) QueueLeaves(ctx context.Context, tree *trillian.Tree, // Ensure we don't leak the transaction. For example if we get an // ErrTreeNeedsInit from beginInternal() or if QueueLeaves fails // below. - defer tx.Close() + defer func() { + if err := tx.Close(); err != nil { + klog.Errorf("tx.Close(): %v", err) + } + }() } if err != nil { return nil, err @@ -323,7 +343,11 @@ func (t *logTreeTX) DequeueLeaves(ctx context.Context, limit int, cutoffTime tim klog.Warningf("Failed to prepare dequeue select: %s", err) return nil, err } - defer stx.Close() + defer func() { + if err := stx.Close(); err != nil { + klog.Errorf("stx.Close(): %v", err) + } + }() leaves := make([]*trillian.LogLeaf, 0, limit) rows, err := stx.QueryContext(ctx, t.treeID, cutoffTime.UnixNano(), limit) @@ -331,7 +355,11 @@ func (t *logTreeTX) DequeueLeaves(ctx context.Context, limit int, cutoffTime tim klog.Warningf("Failed to select rows for work: %s", err) return nil, err } - defer rows.Close() + defer func() { + if err := rows.Close(); err != nil { + klog.Errorf("rows.Close(): %v", err) + } + }() for rows.Next() { leaf, dqInfo, err := t.dequeueLeaf(rows) @@ -601,7 +629,11 @@ func (t *logTreeTX) getLeavesByRangeInternal(ctx context.Context, start, count i klog.Warningf("Failed to get leaves by range: %s", err) return nil, err } - defer rows.Close() + defer func() { + if err := rows.Close(); err != nil { + klog.Errorf("rows.Close(): %v", err) + } + }() ret := make([]*trillian.LogLeaf, 0, count) for wantIndex := start; rows.Next(); wantIndex++ { @@ -732,7 +764,11 @@ func (t *logTreeTX) StoreSignedLogRoot(ctx context.Context, root *trillian.Signe func (t *logTreeTX) getLeavesByHashInternal(ctx context.Context, leafHashes [][]byte, tmpl *sql.Stmt, desc string) ([]*trillian.LogLeaf, error) { stx := t.tx.StmtContext(ctx, tmpl) - defer stx.Close() + defer func() { + if err := stx.Close(); err != nil { + klog.Errorf("stx.Close(): %v", err) + } + }() var args []interface{} for _, hash := range leafHashes { @@ -744,7 +780,11 @@ func (t *logTreeTX) getLeavesByHashInternal(ctx context.Context, leafHashes [][] klog.Warningf("Query() %s hash = %v", desc, err) return nil, err } - defer rows.Close() + defer func() { + if err := rows.Close(); err != nil { + klog.Errorf("rows.Close(): %v", err) + } + }() // The tree could include duplicates so we don't know how many results will be returned var ret []*trillian.LogLeaf diff --git a/storage/mysql/log_storage_test.go b/storage/mysql/log_storage_test.go index e228a68802..d0cfc50e41 100644 --- a/storage/mysql/log_storage_test.go +++ b/storage/mysql/log_storage_test.go @@ -32,6 +32,7 @@ import ( "github.com/google/trillian/types" "google.golang.org/protobuf/proto" "google.golang.org/protobuf/types/known/timestamppb" + "k8s.io/klog/v2" _ "github.com/go-sql-driver/mysql" ) @@ -721,7 +722,11 @@ func TestGetActiveLogIDs(t *testing.T) { if err != nil { t.Fatalf("PrepareContext() returned err = %v", err) } - defer updateDeletedStmt.Close() + defer func() { + if err := updateDeletedStmt.Close(); err != nil { + klog.Errorf("updateDeletedStmt.Close(): %v", err) + } + }() for _, treeID := range []int64{deletedLog.TreeId} { if _, err := updateDeletedStmt.ExecContext(ctx, true, treeID); err != nil { t.Fatalf("ExecContext(%v) returned err = %v", treeID, err) diff --git a/storage/mysql/queue.go b/storage/mysql/queue.go index fd936ec474..53ff1071a6 100644 --- a/storage/mysql/queue.go +++ b/storage/mysql/queue.go @@ -128,7 +128,11 @@ func (t *logTreeTX) removeSequencedLeaves(ctx context.Context, leaves []dequeued klog.Warningf("Failed to prep delete statement for sequenced work: %v", err) return err } - defer stx.Close() + defer func() { + if err := stx.Close(); err != nil { + klog.Errorf("stx.Close(): %v", err) + } + }() for _, dql := range leaves { result, err := stx.ExecContext(ctx, t.treeID, dql.queueTimestampNanos, dql.leafIdentityHash) err = checkResultOkAndRowCountIs(result, err, int64(1)) diff --git a/storage/mysql/storage_test.go b/storage/mysql/storage_test.go index 750dd1223f..13479c1445 100644 --- a/storage/mysql/storage_test.go +++ b/storage/mysql/storage_test.go @@ -243,7 +243,7 @@ func getVersion(db *sql.DB) (string, error) { if err != nil { return "", fmt.Errorf("getVersion: failed to perform query: %v", err) } - defer rows.Close() + defer func() { _ = rows.Close() }() if !rows.Next() { return "", errors.New("getVersion: cursor has no rows") } diff --git a/storage/mysql/tree_storage.go b/storage/mysql/tree_storage.go index ffb0159cd6..b9607f18c0 100644 --- a/storage/mysql/tree_storage.go +++ b/storage/mysql/tree_storage.go @@ -184,7 +184,11 @@ func (t *treeTX) getSubtrees(ctx context.Context, treeRevision int64, ids [][]by return nil, err } stx := t.tx.StmtContext(ctx, tmpl) - defer stx.Close() + defer func() { + if err := stx.Close(); err != nil { + klog.Errorf("stx.Close(): %v", err) + } + }() args := make([]interface{}, 0, len(ids)+3) @@ -203,7 +207,11 @@ func (t *treeTX) getSubtrees(ctx context.Context, treeRevision int64, ids [][]by klog.Warningf("Failed to get merkle subtrees: %s", err) return nil, err } - defer rows.Close() + defer func() { + if err := rows.Close(); err != nil { + klog.Errorf("rows.Close(): %v", err) + } + }() if rows.Err() != nil { // Nothing from the DB @@ -292,7 +300,11 @@ func (t *treeTX) storeSubtrees(ctx context.Context, subtrees []*storagepb.Subtre return err } stx := t.tx.StmtContext(ctx, tmpl) - defer stx.Close() + defer func() { + if err := stx.Close(); err != nil { + klog.Errorf("stx.Close(): %v", err) + } + }() r, err := stx.ExecContext(ctx, args...) if err != nil { diff --git a/storage/provider_test.go b/storage/provider_test.go index 1c9d848a47..d8a75aa6e3 100644 --- a/storage/provider_test.go +++ b/storage/provider_test.go @@ -48,10 +48,12 @@ func TestProviderRegistration(t *testing.T) { name := test.desc if test.reg { - RegisterProvider(name, func(_ monitoring.MetricFactory) (Provider, error) { + if err := RegisterProvider(name, func(_ monitoring.MetricFactory) (Provider, error) { called = true return &provider{}, nil - }) + }); err != nil { + t.Error(err) + } } _, err := NewProvider(name, nil) @@ -69,12 +71,16 @@ func TestProviderRegistration(t *testing.T) { } func TestProviders(t *testing.T) { - RegisterProvider("a", func(_ monitoring.MetricFactory) (Provider, error) { + if err := RegisterProvider("a", func(_ monitoring.MetricFactory) (Provider, error) { return &provider{}, nil - }) - RegisterProvider("b", func(_ monitoring.MetricFactory) (Provider, error) { + }); err != nil { + t.Error(err) + } + if err := RegisterProvider("b", func(_ monitoring.MetricFactory) (Provider, error) { return &provider{}, nil - }) + }); err != nil { + t.Error(err) + } sp := Providers() if got, want := len(sp), 2; got < want { diff --git a/storage/testdb/testdb.go b/storage/testdb/testdb.go index be25694baf..da3c0bc3df 100644 --- a/storage/testdb/testdb.go +++ b/storage/testdb/testdb.go @@ -163,7 +163,11 @@ func dbAvailable(driver DriverName) bool { log.Printf("sql.Open(): %v", err) return false } - defer db.Close() + defer func() { + if err := db.Close(); err != nil { + log.Printf("db.Close(): %v", err) + } + }() if err := db.Ping(); err != nil { log.Printf("db.Ping(): %v", err) return false @@ -214,7 +218,9 @@ func newEmptyDB(ctx context.Context, driver DriverName) (*sql.DB, func(context.C return nil, nil, fmt.Errorf("error running statement %q: %v", stmt, err) } - db.Close() + if err := db.Close(); err != nil { + return nil, nil, fmt.Errorf("failed to close DB: %v", err) + } uri := inf.uriFunc(name) db, err = sql.Open(inf.sqlDriverName, uri) if err != nil { @@ -222,7 +228,11 @@ func newEmptyDB(ctx context.Context, driver DriverName) (*sql.DB, func(context.C } done := func(ctx context.Context) { - defer db.Close() + defer func() { + if err := db.Close(); err != nil { + klog.Errorf("db.Close(): %v", err) + } + }() if _, err := db.ExecContext(ctx, fmt.Sprintf("DROP DATABASE %v", name)); err != nil { klog.Warningf("Failed to drop test database %q: %v", name, err) } diff --git a/storage/testonly/admin_storage_tester.go b/storage/testonly/admin_storage_tester.go index 9967a7af14..aa734fe5e0 100644 --- a/storage/testonly/admin_storage_tester.go +++ b/storage/testonly/admin_storage_tester.go @@ -532,7 +532,11 @@ func (tester *AdminStorageTester) TestAdminTXReadWriteTransaction(t *testing.T) if err != nil { t.Fatalf("%v: Snapshot() = (_, %v), want = (_, nil)", i, err) } - defer tx2.Close() + defer func() { + if err := tx2.Close(); err != nil { + t.Errorf("tx2.Close(): %v", err) + } + }() _, err = tx2.GetTree(ctx, tree.TreeId) if hasErr := err != nil; !test.wantCommit != hasErr { t.Errorf("%v: GetTree() = (_, %v), but wantCommit = %v", i, err, test.wantCommit) diff --git a/storage/testonly/transaction.go b/storage/testonly/transaction.go index ee3c0124f2..c30587b1f3 100644 --- a/storage/testonly/transaction.go +++ b/storage/testonly/transaction.go @@ -23,12 +23,17 @@ import ( "github.com/google/trillian/storage" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" + "k8s.io/klog/v2" ) // RunOnLogTX is a helper for mocking out the LogStorage.ReadWriteTransaction method. func RunOnLogTX(tx storage.LogTreeTX) func(ctx context.Context, treeID int64, f storage.LogTXFunc) error { return func(ctx context.Context, _ int64, f storage.LogTXFunc) error { - defer tx.Close() + defer func() { + if err := tx.Close(); err != nil { + klog.Errorf("tx.Close(): %v", err) + } + }() if err := f(ctx, tx); err != nil { return err } @@ -39,7 +44,11 @@ func RunOnLogTX(tx storage.LogTreeTX) func(ctx context.Context, treeID int64, f // RunOnAdminTX is a helper for mocking out the AdminStorage.ReadWriteTransaction method. func RunOnAdminTX(tx storage.AdminTX) func(ctx context.Context, f storage.AdminTXFunc) error { return func(ctx context.Context, f storage.AdminTXFunc) error { - defer tx.Close() + defer func() { + if err := tx.Close(); err != nil { + klog.Errorf("tx.Close(): %v", err) + } + }() if err := f(ctx, tx); err != nil { return err }