diff --git a/scripts/validate b/scripts/validate index ced8bc2..5c83da0 100755 --- a/scripts/validate +++ b/scripts/validate @@ -9,12 +9,9 @@ PACKAGES="$(find -name '*.go' | xargs -I{} dirname {} | cut -f2 -d/ | sort -u | echo Running: go vet go vet ${PACKAGES} -echo Running: golint -for i in ${PACKAGES}; do - if [ -n "$(golint $i | grep -v 'should have comment.*or be unexported' | grep -v 'sparse/fiemap.go.* use ALL_CAPS in Go names; use CamelCase' | tee /dev/stderr)" ]; then - failed=true - fi -done -test -z "$failed" + +echo "Running: golangci-lint" +golangci-lint run --timeout=5m + echo Running: go fmt test -z "$(go fmt ${PACKAGES} | tee /dev/stderr)" diff --git a/sparse/client.go b/sparse/client.go index de6a364..1d9acc3 100644 --- a/sparse/client.go +++ b/sparse/client.go @@ -198,6 +198,7 @@ func (client *syncClient) syncContent() error { select { case err = <-mergedErrc: break + default: } return err } diff --git a/sparse/file.go b/sparse/file.go index 5622aa6..187f13a 100644 --- a/sparse/file.go +++ b/sparse/file.go @@ -77,7 +77,9 @@ func (file *BufferedFileIoProcessor) Size() (int64, error) { } func (file *BufferedFileIoProcessor) Close() error { - file.File.Sync() + if err := file.File.Sync(); err != nil { + return err + } return file.File.Close() } diff --git a/sparse/rest/handlers.go b/sparse/rest/handlers.go index 87fc13f..c1e5daf 100644 --- a/sparse/rest/handlers.go +++ b/sparse/rest/handlers.go @@ -158,7 +158,7 @@ func (server *SyncServer) close(writer http.ResponseWriter, request *http.Reques } if err != nil { - log.WithError(err).Warnf("Failed ot set snapshot hash info to checksum file %v", server.filePath) + log.WithError(err).Warnf("Failed to set snapshot hash info to checksum file %v", server.filePath) } } @@ -207,6 +207,9 @@ func (server *SyncServer) doGetChecksum(writer http.ResponseWriter, request *htt // For the region to have valid data, it can only has one extent covering the whole region exts, err := sparse.GetFiemapRegionExts(server.fileIo, remoteDataInterval, 2) + if err != nil { + return errors.Wrapf(err, "failed to get fiemap region exts %+v", remoteDataInterval) + } if len(exts) == 1 && int64(exts[0].Logical) <= remoteDataInterval.Begin && int64(exts[0].Logical+exts[0].Length) >= remoteDataInterval.End { diff --git a/sparse/rest/router.go b/sparse/rest/router.go index 9d6fc90..3921f25 100644 --- a/sparse/rest/router.go +++ b/sparse/rest/router.go @@ -2,7 +2,7 @@ package rest import "github.com/gorilla/mux" -//NewRouter creates and configures a mux router +// NewRouter creates and configures a mux router func NewRouter(server *SyncServer) *mux.Router { // API framework routes diff --git a/sparse/rest/server.go b/sparse/rest/server.go index e161b27..ad9c239 100644 --- a/sparse/rest/server.go +++ b/sparse/rest/server.go @@ -117,6 +117,6 @@ func Server(ctx context.Context, port string, filePath string, syncFileOps SyncF } // TestServer daemon serves only one connection for each test then exits -func TestServer(ctx context.Context, port string, filePath string, timeout int) { - Server(ctx, port, filePath, &SyncFileStub{}) +func TestServer(ctx context.Context, port string, filePath string, timeout int) error { + return Server(ctx, port, filePath, &SyncFileStub{}) } diff --git a/sparse/sfold.go b/sparse/sfold.go index 85962d4..d1d39cb 100644 --- a/sparse/sfold.go +++ b/sparse/sfold.go @@ -98,7 +98,7 @@ func coalesce(parentFileIo, childFileIo FileIoProcessor, fileSize int64, ops Fil size = int(segment.End - offset) } // read a batch from child - n, err := childFileIo.ReadAt(buffer[:size], offset) + _, err := childFileIo.ReadAt(buffer[:size], offset) if err != nil { return errors.Wrapf(err, "failed to read childFile filename: %v, size: %v, at: %v", childFileIo.Name(), size, offset) @@ -130,6 +130,7 @@ func coalesce(parentFileIo, childFileIo FileIoProcessor, fileSize int64, ops Fil select { case err = <-mergedErrc: break + default: } log.Debugf("Finished fold for parent %v, child %v, size %v, elapsed %.2fs", diff --git a/sparse/sprune.go b/sparse/sprune.go index 73d4f17..091d65a 100644 --- a/sparse/sprune.go +++ b/sparse/sprune.go @@ -104,6 +104,7 @@ func prune(parentFileIo, childFileIo FileIoProcessor, fileSize int64, ops FileHa select { case err = <-mergedErrc: break + default: } log.Debugf("Finished pruning for parent %v, child %v, size %v, elapsed %.2fs", diff --git a/sparse/test/client_test.go b/sparse/test/client_test.go index de92147..aab7004 100644 --- a/sparse/test/client_test.go +++ b/sparse/test/client_test.go @@ -2,10 +2,12 @@ package test import ( "context" - "math/rand" + "crypto/rand" "os" "testing" + "github.com/stretchr/testify/assert" + . "github.com/longhorn/sparse-tools/sparse" "github.com/longhorn/sparse-tools/sparse/rest" ) @@ -249,7 +251,10 @@ func TestSyncAnyFile(t *testing.T) { func testSyncAnyFile(t *testing.T, src, dst string, directIO, fastSync bool) { // Sync - go rest.TestServer(context.Background(), port, dst, timeout) + go func() { + err := rest.TestServer(context.Background(), port, dst, timeout) + assert.Nil(t, err) + }() err := SyncFile(src, localhost+":"+port, timeout, directIO, fastSync) // Verify @@ -267,7 +272,10 @@ func testSyncAnyFile(t *testing.T, src, dst string, directIO, fastSync bool) { func testSyncAnyFileExpectFailure(t *testing.T, src, dst string, directIO, fastSync bool) { // Sync - go rest.TestServer(context.Background(), port, dst, timeout) + go func() { + err := rest.TestServer(context.Background(), port, dst, timeout) + assert.Nil(t, err) + }() err := SyncFile(src, localhost+":"+port, timeout, directIO, fastSync) // Verify @@ -726,7 +734,10 @@ func testSyncFile(t *testing.T, layoutLocal, layoutRemote []FileInterval, direct } // Sync - go rest.TestServer(context.Background(), port, remotePath, timeout) + go func() { + err := rest.TestServer(context.Background(), port, remotePath, timeout) + assert.Nil(t, err) + }() err := SyncFile(localPath, localhost+":"+port, timeout, true /* directIO */, false /* fastSync */) // Verify @@ -765,7 +776,10 @@ func Benchmark_1G_InitFiles(b *testing.B) { } func Benchmark_1G_SendFiles_Whole(b *testing.B) { - go rest.TestServer(context.Background(), port, remoteBigPath, timeout) + go func() { + err := rest.TestServer(context.Background(), port, remoteBigPath, timeout) + assert.Nil(b, err) + }() err := SyncFile(localBigPath, localhost+":"+port, timeout, true /* directIO */, false /* fastSync */) if err != nil { @@ -774,7 +788,10 @@ func Benchmark_1G_SendFiles_Whole(b *testing.B) { } func Benchmark_1G_SendFiles_Whole_No_DirectIO(b *testing.B) { - go rest.TestServer(context.Background(), port, remoteBigPath, timeout) + go func() { + err := rest.TestServer(context.Background(), port, remoteBigPath, timeout) + assert.Nil(b, err) + }() err := SyncFile(localBigPath, localhost+":"+port, timeout, false /* directIO */, false /* fastSync */) if err != nil { @@ -784,7 +801,10 @@ func Benchmark_1G_SendFiles_Whole_No_DirectIO(b *testing.B) { func Benchmark_1G_SendFiles_Diff(b *testing.B) { - go rest.TestServer(context.Background(), port, remoteBigPath, timeout) + go func() { + err := rest.TestServer(context.Background(), port, remoteBigPath, timeout) + assert.Nil(b, err) + }() err := SyncFile(localBigPath, localhost+":"+port, timeout, true /* directIO */, false /* fastSync */) if err != nil { @@ -810,7 +830,8 @@ func TestSyncSmallSnapshot4MB(t *testing.T) { size := 4 * 1024 * 1024 data := make([]byte, size) - rand.Read(data) + _, err := rand.Read(data) + assert.Nil(t, err) createTestSmallFile(localPath, len(data), data) @@ -847,7 +868,10 @@ func TestSyncSnapshotZeroByte(t *testing.T) { func testSyncAnyContent(t *testing.T, snapshotName string, dstFileName string, rw ReaderWriterAt, snapshotSize int64) { // Sync - go rest.TestServer(context.Background(), port, dstFileName, timeout) + go func() { + err := rest.TestServer(context.Background(), port, dstFileName, timeout) + assert.Nil(t, err) + }() err := SyncContent(snapshotName, rw, snapshotSize, localhost+":"+port, timeout, true, false) // Verify diff --git a/sparse/test/directfio_test.go b/sparse/test/directfio_test.go index 7b441ec..fc1e23d 100644 --- a/sparse/test/directfio_test.go +++ b/sparse/test/directfio_test.go @@ -7,6 +7,7 @@ import ( "time" log "github.com/sirupsen/logrus" + "github.com/stretchr/testify/assert" . "github.com/longhorn/sparse-tools/sparse" ) @@ -241,7 +242,8 @@ func BenchmarkIO8(b *testing.B) { } defer f.Close() - f.Truncate(fileSize) + err = f.Truncate(fileSize) + assert.Nil(b, err) log.Debug("") ioTest("pilot write", b, path, 8, 32, write) @@ -266,7 +268,8 @@ func BenchmarkIO8u(b *testing.B) { b.Fatal("Failed to OpenFile for write", err) } defer f.Close() - f.Truncate(fileSize) + err = f.Truncate(fileSize) + assert.Nil(b, err) log.Debug("") ioTest("pilot write", b, path, 8, 32, writeUnaligned) diff --git a/sparse/test/fiemap_test.go b/sparse/test/fiemap_test.go index d550c0f..ddb47ea 100644 --- a/sparse/test/fiemap_test.go +++ b/sparse/test/fiemap_test.go @@ -11,6 +11,7 @@ import ( "github.com/pkg/errors" log "github.com/sirupsen/logrus" + "github.com/stretchr/testify/assert" . "github.com/longhorn/sparse-tools/sparse" "github.com/longhorn/sparse-tools/sparse/rest" @@ -53,13 +54,16 @@ func TestFileSync(t *testing.T) { // defer fileCleanup(dstPath) log.Info("Syncing file...") startTime := time.Now() - go rest.TestServer(context.Background(), port, dstPath, timeout) + go func() { + err := rest.TestServer(context.Background(), port, dstPath, timeout) + assert.Nil(t, err) + }() time.Sleep(time.Second) err := SyncFile(srcPath, localhost+":"+port, timeout, true, false) if err != nil { t.Fatalf("sync error: %v", err) } - log.Infof("Syncing done, size: %v elapsed: %.2fs", testFileSize, time.Now().Sub(startTime).Seconds()) + log.Infof("Syncing done, size: %v elapsed: %.2fs", testFileSize, time.Since(startTime).Seconds()) startTime = time.Now() log.Info("Checking...") @@ -67,7 +71,7 @@ func TestFileSync(t *testing.T) { if err != nil { t.Fatal(err) } - log.Infof("Checking done, size: %v elapsed: %.2fs", testFileSize, time.Now().Sub(startTime).Seconds()) + log.Infof("Checking done, size: %v elapsed: %.2fs", testFileSize, time.Since(startTime).Seconds()) } func writeMultipleHolesData(filePath string, fileSize int64, dataSize int64, holeSize int64) (err error) { @@ -83,12 +87,32 @@ func writeMultipleHolesData(filePath string, fileSize int64, dataSize int64, hol return err } defer func() { - if err != nil { - _ = os.Remove(filePath) + if removeErr := os.Remove(filePath); removeErr != nil { + if err != nil { + err = errors.Wrapf(err, "failed to remove file %v: %v", filePath, removeErr) + } else { + err = removeErr + } + } + }() + defer func() { + if closeErr := f.Close(); closeErr != nil { + if err != nil { + err = errors.Wrapf(err, "failed to close file %v: %v", filePath, closeErr) + } else { + err = closeErr + } + } + }() + defer func() { + if syncErr := f.Sync(); syncErr != nil { + if err != nil { + err = errors.Wrapf(err, "failed to sync file %v: %v", filePath, syncErr) + } else { + err = syncErr + } } }() - defer f.Close() - defer f.Sync() if err := f.Truncate(fileSize); err != nil { return err } @@ -113,13 +137,13 @@ func writeMultipleHolesData(filePath string, fileSize int64, dataSize int64, hol writtenGB := offset / GB log.Infof("Wrote %vGB of %vGB time delta: %.2f time elapsed: %.2f", writtenGB, sizeInGB, - time.Now().Sub(deltaTime).Seconds(), - time.Now().Sub(startTime).Seconds()) + time.Since(deltaTime).Seconds(), + time.Since(startTime).Seconds()) deltaTime = time.Now() } } - log.Infof("Done creating a %vGB file with multiple hole, time elapsed: %.2f", sizeInGB, time.Now().Sub(startTime).Seconds()) + log.Infof("Done creating a %vGB file with multiple hole, time elapsed: %.2f", sizeInGB, time.Since(startTime).Seconds()) return nil } diff --git a/sparse/test/ssync_test.go b/sparse/test/ssync_test.go index 40a4328..c050b24 100644 --- a/sparse/test/ssync_test.go +++ b/sparse/test/ssync_test.go @@ -12,6 +12,7 @@ import ( "time" log "github.com/sirupsen/logrus" + "github.com/stretchr/testify/assert" . "github.com/longhorn/sparse-tools/sparse" "github.com/longhorn/sparse-tools/sparse/rest" @@ -197,20 +198,23 @@ func RandomSync(t *testing.T, size, seed int64, srcPath, dstPath string, dstCrea } log.Infof("Syncing with directIO: %v size: %v", directIO, size) - go rest.TestServer(context.Background(), port, dstPath, timeout) + go func() { + err := rest.TestServer(context.Background(), port, dstPath, timeout) + assert.Nil(t, err) + }() startTime := time.Now() err := SyncFile(srcPath, localhost+":"+port, timeout, directIO, fastSync) if err != nil { t.Fatal("sync error") } - log.Infof("Syncing done, size: %v elapsed: %.2fs", size, time.Now().Sub(startTime).Seconds()) + log.Infof("Syncing done, size: %v elapsed: %.2fs", size, time.Since(startTime).Seconds()) startTime = time.Now() err = checkSparseFiles(srcPath, dstPath) if err != nil { t.Fatal(err) } - log.Infof("Checking done, size: %v elapsed: %.2fs", size, time.Now().Sub(startTime).Seconds()) + log.Infof("Checking done, size: %v elapsed: %.2fs", size, time.Since(startTime).Seconds()) } // generate random hole and data interval, but just return data interval slcie @@ -260,7 +264,10 @@ func TestSyncCancellation(t *testing.T) { dstName := tempFilePath(dstPrefix) ctx, cancelFunc := context.WithCancel(context.Background()) - go rest.TestServer(ctx, port, dstName, timeout) + go func() { + err := rest.TestServer(ctx, port, dstName, timeout) + assert.Nil(t, err) + }() client := http.Client{} diff --git a/sparse/test/testutil_test.go b/sparse/test/testutil_test.go index badb2d1..4893a40 100644 --- a/sparse/test/testutil_test.go +++ b/sparse/test/testutil_test.go @@ -117,8 +117,12 @@ func doCreateSparseFile(name string, fileSize int64, layout []Interval) { offset += size } } - f.Sync() - f.Close() + if err != f.Sync() { + log.Fatal(err) + } + if err != f.Close() { + log.Fatal(err) + } } func mergeExts(exts []Extent) []Interval { @@ -210,7 +214,9 @@ func createTestSmallFile(name string, size int, pattern []byte) { } } - f.Sync() + if err = f.Sync(); err != nil { + log.Fatal(err) + } } func createNonOverlappingTestIntervals(fileSize int) (layoutFrom, layoutTo []FileInterval) { diff --git a/stats/stats.go b/stats/stats.go index 510fc09..6d59ad6 100644 --- a/stats/stats.go +++ b/stats/stats.go @@ -15,7 +15,7 @@ const ( defaultBufferSize = 4 * 1000 // sample buffer size (cyclic) ) -//SampleOp operation +// SampleOp operation type SampleOp int const ( @@ -202,7 +202,7 @@ func resetStats(size int) { initStats(size) } -//OpID pending operation id +// OpID pending operation id type OpID int var ( @@ -211,7 +211,7 @@ var ( mutexPendingOps sync.Mutex ) -//InsertPendingOp starts tracking of a pending operation +// InsertPendingOp starts tracking of a pending operation func InsertPendingOp(timestamp time.Time, targetID string, op SampleOp, size int) OpID { mutexPendingOps.Lock() defer mutexPendingOps.Unlock() @@ -229,7 +229,7 @@ func InsertPendingOp(timestamp time.Time, targetID string, op SampleOp, size int return OpID(id) } -//RemovePendingOp removes tracking of a completed operation +// RemovePendingOp removes tracking of a completed operation func RemovePendingOp(id OpID, status bool) error { log.Debug("RemovePendingOp id=", id) mutexPendingOps.Lock() @@ -248,7 +248,7 @@ func RemovePendingOp(id OpID, status bool) error { } // Update the duration and store in the recent stats - pendingOps[i].duration = time.Now().Sub(pendingOps[i].timestamp) + pendingOps[i].duration = time.Since(pendingOps[i].timestamp) pendingOps[i].status = status storeSample(pendingOps[i]) diff --git a/stats/stats_test.go b/stats/stats_test.go index 4683702..0d47bb3 100644 --- a/stats/stats_test.go +++ b/stats/stats_test.go @@ -1,7 +1,6 @@ package stats import ( - "os" "sync" "testing" "time" @@ -9,16 +8,6 @@ import ( log "github.com/sirupsen/logrus" ) -func tempFilePath() string { - // Make a temporary file path - f, err := os.CreateTemp("", "stat-test") - if err != nil { - log.Fatal("Failed to make temp file", err) - } - defer f.Close() - return f.Name() -} - var ( samples = make(chan dataPoint, 4) )