Upstream/waveterm
1
0
Fork 0
mirror of https://github.com/wavetermdev/waveterm.git synced 2025-01-08 19:38:51 +01:00
Code Issues Packages Projects Releases Wiki Activity

Clean up cirfile and cirfile_tests, ensure we are using contexts with cancellation signals (#183)

Browse Source 
* Fix unvalidated path warning in cirfile.go

* Adding test

* move dir path vars into global scope, set wave home in test if not set already

* separate validation code into own testable func

* do removal of cir file in cleanup

* rename test file names

* flock should return err if waiting without done fn

* update comment

* use timeout context

* remove redundant validation

* add more error messages

* use filepath.join instead

* validate when creating cirfile too

* support .ptyout.cf, etc.

* remove validation as cirfile is "trusted", since it only accepts pre-validated input

* revert to passing nil context for flock in the Create file case
This commit is contained in:
Evan Simkowitz 2023-12-22 18:07:58 -08:00 committed by GitHub
parent 092a8de715
commit e7725e0e11
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 103 additions and 86 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

33
waveshell/pkg/cirfile/cirfile.go
View File

@ -52,7 +52,9 @@ func (f *File) flock(ctx context.Context, lockType int) error {
if err != syscall.EWOULDBLOCK { if err != syscall.EWOULDBLOCK {
return err return err
} }
if ctx == nil {
// Do not busy-wait unless we have a way to cancel the context
if ctx == nil || ctx.Done() == nil {
return syscall.EWOULDBLOCK return syscall.EWOULDBLOCK
} }
// busy-wait with context // busy-wait with context
@ -67,6 +69,7 @@ func (f *File) flock(ctx context.Context, lockType int) error {
} }
select { select {
case <-time.After(timeout): case <-time.After(timeout):
// TODO: Ineffective break statement
break break
case <-ctx.Done(): case <-ctx.Done():
return ctx.Err() return ctx.Err()
@ -80,7 +83,6 @@ func (f *File) flock(ctx context.Context, lockType int) error {
return err return err
} }
} }
return fmt.Errorf("could not acquire lock")
} }
func (f *File) unflock() { func (f *File) unflock() {
@ -88,7 +90,6 @@ func (f *File) unflock() {
syscall.Flock(int(f.OSFile.Fd()), syscall.LOCK_UN) // ignore error (nothing to do about it anyway) syscall.Flock(int(f.OSFile.Fd()), syscall.LOCK_UN) // ignore error (nothing to do about it anyway)
f.FlockStatus = 0 f.FlockStatus = 0
} }
return
} }
// does not read metadata because locking could block/fail. we want to be able // does not read metadata because locking could block/fail. we want to be able
@ -96,11 +97,11 @@ func (f *File) unflock() {
func OpenCirFile(fileName string) (*File, error) { func OpenCirFile(fileName string) (*File, error) {
fd, err := os.OpenFile(fileName, os.O_RDWR, 0777) fd, err := os.OpenFile(fileName, os.O_RDWR, 0777)
if err != nil { if err != nil {
return nil, err return nil, fmt.Errorf("cannot open file: %w", err)
} }
finfo, err := fd.Stat() finfo, err := fd.Stat()
if err != nil { if err != nil {
return nil, err return nil, fmt.Errorf("cannot get file info: %w", err)
} }
if finfo.Size() < HeaderLen { if finfo.Size() < HeaderLen {
return nil, fmt.Errorf("invalid cirfile, file length[%d] less than HeaderLen[%d]", finfo.Size(), HeaderLen) return nil, fmt.Errorf("invalid cirfile, file length[%d] less than HeaderLen[%d]", finfo.Size(), HeaderLen)
@ -130,6 +131,7 @@ func StatCirFile(ctx context.Context, fileName string) (*Stat, error) {
// if the file already exists, it is an error. // if the file already exists, it is an error.
// there is a race condition if two goroutines try to create the same file between Stat() and Create(), so // there is a race condition if two goroutines try to create the same file between Stat() and Create(), so
//
// they both might get no error, but only one file will be valid. if this is a concern, this call // they both might get no error, but only one file will be valid. if this is a concern, this call
// should be externally synchronized. // should be externally synchronized.
func CreateCirFile(fileName string, maxSize int64) (*File, error) { func CreateCirFile(fileName string, maxSize int64) (*File, error) {
@ -148,14 +150,14 @@ func CreateCirFile(fileName string, maxSize int64) (*File, error) {
return nil, err return nil, err
} }
rtn := &File{OSFile: fd, Version: CurrentVersion, MaxSize: maxSize, StartPos: FilePosEmpty} rtn := &File{OSFile: fd, Version: CurrentVersion, MaxSize: maxSize, StartPos: FilePosEmpty}
err = rtn.flock(nil, syscall.LOCK_EX) err = rtn.flock(nil, syscall.LOCK_EX) // pass nil context here for a fast fail if someone else is also creating the same file
if err != nil { if err != nil {
return nil, err return nil, fmt.Errorf("cannot lock file: %w", err)
} }
defer rtn.unflock() defer rtn.unflock()
err = rtn.writeMeta() err = rtn.writeMeta()
if err != nil { if err != nil {
return nil, err return nil, fmt.Errorf("cannot write metadata: %w", err)
} }
return rtn, nil return rtn, nil
} }
@ -214,10 +216,7 @@ func (f *File) readMeta() error {
return fmt.Errorf("invalid cbuf version[%d]", f.Version) return fmt.Errorf("invalid cbuf version[%d]", f.Version)
} }
// possible incomplete write, fix start/end pos to be within filesize // possible incomplete write, fix start/end pos to be within filesize
if f.FileDataSize == 0 { if f.FileDataSize == 0 || (f.StartPos >= f.FileDataSize && f.EndPos >= f.FileDataSize) {
f.StartPos = FilePosEmpty
f.EndPos = 0
} else if f.StartPos >= f.FileDataSize && f.EndPos >= f.FileDataSize {
f.StartPos = FilePosEmpty f.StartPos = FilePosEmpty
f.EndPos = 0 f.EndPos = 0
} else if f.StartPos >= f.FileDataSize { } else if f.StartPos >= f.FileDataSize {
@ -309,23 +308,23 @@ func (f *File) getFileChunks() []fileChunk {
return nil return nil
} }
if f.EndPos >= f.StartPos { if f.EndPos >= f.StartPos {
return []fileChunk{fileChunk{f.StartPos, f.EndPos - f.StartPos + 1}} return []fileChunk{{f.StartPos, f.EndPos - f.StartPos + 1}}
} }
return []fileChunk{ return []fileChunk{
fileChunk{f.StartPos, f.FileDataSize - f.StartPos}, {f.StartPos, f.FileDataSize - f.StartPos},
fileChunk{0, f.EndPos + 1}, {0, f.EndPos + 1},
} }
} }
func (f *File) getFreeChunks() []fileChunk { func (f *File) getFreeChunks() []fileChunk {
if f.StartPos == FilePosEmpty { if f.StartPos == FilePosEmpty {
return []fileChunk{fileChunk{0, f.MaxSize}} return []fileChunk{{0, f.MaxSize}}
} }
if (f.EndPos == f.StartPos-1) || (f.StartPos == 0 && f.EndPos == f.MaxSize-1) { if (f.EndPos == f.StartPos-1) || (f.StartPos == 0 && f.EndPos == f.MaxSize-1) {
return nil return nil
} }
if f.EndPos < f.StartPos { if f.EndPos < f.StartPos {
return []fileChunk{fileChunk{f.EndPos + 1, f.StartPos - f.EndPos - 1}} return []fileChunk{{f.EndPos + 1, f.StartPos - f.EndPos - 1}}
} }
var rtn []fileChunk var rtn []fileChunk
if f.EndPos < f.MaxSize-1 { if f.EndPos < f.MaxSize-1 {

152
waveshell/pkg/cirfile/cirfile_test.go
View File

@ -7,7 +7,7 @@ import (
"context" "context"
"fmt" "fmt"
"os" "os"
"path" "path/filepath"
"strings" "strings"
"syscall" "syscall"
"testing" "testing"
@ -53,25 +53,39 @@ func makeData(size int) string {
return rtn return rtn
} }
func TestCreate(t *testing.T) { func testFilePath(t *testing.T, name string) string {
tempDir := t.TempDir() tempDir := t.TempDir()
f1Name := path.Join(tempDir, "f1.cf") return filepath.Join(tempDir, name)
f, err := OpenCirFile(f1Name) }
if err == nil || f != nil {
t.Fatalf("OpenCirFile f1.cf should fail (no file)") func createTestFile(t *testing.T, name string) (*File, string, error) {
} fPath := testFilePath(t, name)
f, err = CreateCirFile(f1Name, 100) f, err := CreateCirFile(fPath, 100)
if err != nil { if err != nil {
t.Fatalf("CreateCirFile f1.cf failed: %v", err) return nil, fPath, err
}
return f, fPath, nil
}
func TestCreate(t *testing.T) {
const fName = "f1.cf"
fPath := testFilePath(t, fName)
f, err := OpenCirFile(fPath)
if err == nil || f != nil {
t.Fatalf("OpenCirFile %s should fail (no file)", fPath)
}
f, err = CreateCirFile(fPath, 100)
if err != nil {
t.Fatalf("CreateCirFile %s failed: %v", fPath, err)
} }
if f == nil { if f == nil {
t.Fatalf("CreateCirFile f1.cf returned nil") t.Fatalf("CreateCirFile %s returned nil", fPath)
} }
err = f.ReadMeta(context.Background()) err = f.ReadMeta(context.Background())
if err != nil { if err != nil {
t.Fatalf("cannot readmeta from f1.cf: %v", err) t.Fatalf("cannot readmeta from %s: %v", fPath, err)
} }
validateFileSize(t, f1Name, 256) validateFileSize(t, fPath, 256)
if f.Version != CurrentVersion || f.MaxSize != 100 || f.FileOffset != 0 || f.StartPos != FilePosEmpty || f.EndPos != 0 || f.FileDataSize != 0 || f.FlockStatus != 0 { if f.Version != CurrentVersion || f.MaxSize != 100 || f.FileOffset != 0 || f.StartPos != FilePosEmpty || f.EndPos != 0 || f.FileDataSize != 0 || f.FlockStatus != 0 {
t.Fatalf("error with initial metadata #%v", f) t.Fatalf("error with initial metadata #%v", f)
} }
@ -84,62 +98,65 @@ func TestCreate(t *testing.T) {
if realOffset != 0 || nr != 0 || err != nil { if realOffset != 0 || nr != 0 || err != nil {
t.Fatalf("error with empty read: real-offset[%d] nr[%d] err[%v]", realOffset, nr, err) t.Fatalf("error with empty read: real-offset[%d] nr[%d] err[%v]", realOffset, nr, err)
} }
f2, err := CreateCirFile(f1Name, 100) f2, err := CreateCirFile(fPath, 100)
if err == nil || f2 != nil { if err == nil || f2 != nil {
t.Fatalf("should be an error to create duplicate CirFile") t.Fatalf("should be an error to create duplicate CirFile")
} }
} }
const cannotAppendData = "cannot append data: %v"
const cannotReadNext = "cannot readnext: %v"
const cannotCreateCirFile = "cannot create cirfile [%s]: %v"
func TestFile(t *testing.T) { func TestFile(t *testing.T) {
tempDir := t.TempDir() const fName = "f1.cf"
f1Name := path.Join(tempDir, "f1.cf") f, fPath, err := createTestFile(t, fName)
f, err := CreateCirFile(f1Name, 100)
if err != nil { if err != nil {
t.Fatalf("cannot create cirfile: %v", err) t.Fatalf(cannotCreateCirFile, fPath, err)
} }
err = f.AppendData(context.Background(), nil) err = f.AppendData(context.Background(), nil)
if err != nil { if err != nil {
t.Fatalf("cannot append data: %v", err) t.Fatalf(cannotAppendData, err)
} }
validateFileSize(t, f1Name, HeaderLen) validateFileSize(t, fPath, HeaderLen)
validateMeta(t, "1", f, FilePosEmpty, 0, 0, 0) validateMeta(t, "1", f, FilePosEmpty, 0, 0, 0)
err = f.AppendData(context.Background(), []byte("hello")) err = f.AppendData(context.Background(), []byte("hello"))
if err != nil { if err != nil {
t.Fatalf("cannot append data: %v", err) t.Fatalf(cannotAppendData, err)
} }
validateFileSize(t, f1Name, HeaderLen+5) validateFileSize(t, fPath, HeaderLen+5)
validateMeta(t, "2", f, 0, 4, 5, 0) validateMeta(t, "2", f, 0, 4, 5, 0)
err = f.AppendData(context.Background(), []byte(" foo")) err = f.AppendData(context.Background(), []byte(" foo"))
if err != nil { if err != nil {
t.Fatalf("cannot append data: %v", err) t.Fatalf(cannotAppendData, err)
} }
validateFileSize(t, f1Name, HeaderLen+9) validateFileSize(t, fPath, HeaderLen+9)
validateMeta(t, "3", f, 0, 8, 9, 0) validateMeta(t, "3", f, 0, 8, 9, 0)
err = f.AppendData(context.Background(), []byte("\n"+makeData(20))) err = f.AppendData(context.Background(), []byte("\n"+makeData(20)))
if err != nil { if err != nil {
t.Fatalf("cannot append data: %v", err) t.Fatalf(cannotAppendData, err)
} }
validateFileSize(t, f1Name, HeaderLen+30) validateFileSize(t, fPath, HeaderLen+30)
validateMeta(t, "4", f, 0, 29, 30, 0) validateMeta(t, "4", f, 0, 29, 30, 0)
data120 := makeData(120) data120 := makeData(120)
err = f.AppendData(context.Background(), []byte(data120)) err = f.AppendData(context.Background(), []byte(data120))
if err != nil { if err != nil {
t.Fatalf("cannot append data: %v", err) t.Fatalf(cannotAppendData, err)
} }
validateFileSize(t, f1Name, HeaderLen+100) validateFileSize(t, fPath, HeaderLen+100)
validateMeta(t, "5", f, 0, 99, 100, 50) validateMeta(t, "5", f, 0, 99, 100, 50)
err = f.AppendData(context.Background(), []byte("foo ")) err = f.AppendData(context.Background(), []byte("foo "))
if err != nil { if err != nil {
t.Fatalf("cannot append data: %v", err) t.Fatalf(cannotAppendData, err)
} }
validateFileSize(t, f1Name, HeaderLen+100) validateFileSize(t, fPath, HeaderLen+100)
validateMeta(t, "6", f, 4, 3, 100, 54) validateMeta(t, "6", f, 4, 3, 100, 54)
buf := make([]byte, 5) buf := make([]byte, 5)
realOffset, nr, err := f.ReadNext(context.Background(), buf, 0) realOffset, nr, err := f.ReadNext(context.Background(), buf, 0)
if err != nil { if err != nil {
t.Fatalf("cannot ReadNext: %v", err) t.Fatalf(cannotReadNext, err)
} }
if realOffset != 54 { if realOffset != 54 {
t.Fatalf("wrong realoffset got[%d] expected[%d]", realOffset, 54) t.Fatalf("wrong realoffset got[%d] expected[%d]", realOffset, 54)
@ -152,7 +169,7 @@ func TestFile(t *testing.T) {
} }
realOffset, nr, err = f.ReadNext(context.Background(), buf, 60) realOffset, nr, err = f.ReadNext(context.Background(), buf, 60)
if err != nil { if err != nil {
t.Fatalf("cannot readnext: %v", err) t.Fatalf(cannotReadNext, err)
} }
if realOffset != 60 && nr != 5 { if realOffset != 60 && nr != 5 {
t.Fatalf("invalid rtn realoffset[%d] nr[%d]", realOffset, nr) t.Fatalf("invalid rtn realoffset[%d] nr[%d]", realOffset, nr)
@ -171,13 +188,12 @@ func TestFile(t *testing.T) {
} }
func TestFlock(t *testing.T) { func TestFlock(t *testing.T) {
tempDir := t.TempDir() const fName = "f1.cf"
f1Name := path.Join(tempDir, "f1.cf") f, fPath, err := createTestFile(t, fName)
f, err := CreateCirFile(f1Name, 100)
if err != nil { if err != nil {
t.Fatalf("cannot create cirfile: %v", err) t.Fatalf(cannotCreateCirFile, fPath, err)
} }
fd2, err := os.OpenFile(f1Name, os.O_RDWR, 0777) fd2, err := os.OpenFile(fPath, os.O_RDWR, 0777)
if err != nil { if err != nil {
t.Fatalf("cannot open file: %v", err) t.Fatalf("cannot open file: %v", err)
} }
@ -185,17 +201,18 @@ func TestFlock(t *testing.T) {
if err != nil { if err != nil {
t.Fatalf("cannot lock fd: %v", err) t.Fatalf("cannot lock fd: %v", err)
} }
err = f.AppendData(nil, []byte("hello")) err = f.AppendData(context.TODO(), []byte("hello"))
if err != syscall.EWOULDBLOCK { if err != syscall.EWOULDBLOCK {
t.Fatalf("append should fail with EWOULDBLOCK") t.Fatalf("append should fail with EWOULDBLOCK")
} }
timeoutCtx, _ := context.WithTimeout(context.Background(), 20*time.Millisecond) timeoutCtx, cancelFn := context.WithTimeout(context.Background(), 20*time.Millisecond)
defer cancelFn()
startTs := time.Now() startTs := time.Now()
err = f.ReadMeta(timeoutCtx) err = f.ReadMeta(timeoutCtx)
if err != context.DeadlineExceeded { if err != context.DeadlineExceeded {
t.Fatalf("readmeta should fail with context.DeadlineExceeded") t.Fatalf("readmeta should fail with context.DeadlineExceeded")
} }
dur := time.Now().Sub(startTs) dur := time.Since(startTs)
if dur < 20*time.Millisecond { if dur < 20*time.Millisecond {
t.Fatalf("readmeta should take at least 20ms") t.Fatalf("readmeta should take at least 20ms")
} }
@ -208,7 +225,7 @@ func TestFlock(t *testing.T) {
if err != nil { if err != nil {
t.Fatalf("cannot flock: %v", err) t.Fatalf("cannot flock: %v", err)
} }
err = f.AppendData(nil, []byte("hello")) err = f.AppendData(context.TODO(), []byte("hello"))
if err != syscall.EWOULDBLOCK { if err != syscall.EWOULDBLOCK {
t.Fatalf("append should fail with EWOULDBLOCK") t.Fatalf("append should fail with EWOULDBLOCK")
} }
@ -217,69 +234,70 @@ func TestFlock(t *testing.T) {
t.Fatalf("readmeta err (should work because LOCK_SH): %v", err) t.Fatalf("readmeta err (should work because LOCK_SH): %v", err)
} }
fd2.Close() fd2.Close()
err = f.AppendData(nil, []byte("hello")) err = f.AppendData(context.TODO(), []byte("hello"))
if err != nil { if err != nil {
t.Fatalf("append error (should work fd2 was closed): %v", err) t.Fatalf("append error (should work fd2 was closed): %v", err)
} }
} }
const writeAtError = "writeat error: %v"
func TestWriteAt(t *testing.T) { func TestWriteAt(t *testing.T) {
tempDir := t.TempDir() const fName = "f1.cf"
f1Name := path.Join(tempDir, "f1.cf") f, fPath, err := createTestFile(t, fName)
f, err := CreateCirFile(f1Name, 100)
if err != nil { if err != nil {
t.Fatalf("cannot create cirfile: %v", err) t.Fatalf("cannot create cirfile: %v", err)
} }
err = f.WriteAt(nil, []byte("hello\nmike"), 4) err = f.WriteAt(context.TODO(), []byte("hello\nmike"), 4)
if err != nil { if err != nil {
t.Fatalf("writeat error: %v", err) t.Fatalf(writeAtError, err)
} }
err = f.WriteAt(nil, []byte("t"), 2) err = f.WriteAt(context.TODO(), []byte("t"), 2)
if err != nil { if err != nil {
t.Fatalf("writeat error: %v", err) t.Fatalf(writeAtError, err)
} }
err = f.WriteAt(nil, []byte("more"), 30) err = f.WriteAt(context.TODO(), []byte("more"), 30)
if err != nil { if err != nil {
t.Fatalf("writeat error: %v", err) t.Fatalf(writeAtError, err)
} }
err = f.WriteAt(nil, []byte("\n"), 19) err = f.WriteAt(context.TODO(), []byte("\n"), 19)
if err != nil { if err != nil {
t.Fatalf("writeat error: %v", err) t.Fatalf(writeAtError, err)
} }
dumpFile(f1Name) dumpFile(fPath)
err = f.WriteAt(nil, []byte("hello"), 200) err = f.WriteAt(context.TODO(), []byte("hello"), 200)
if err != nil { if err != nil {
t.Fatalf("writeat error: %v", err) t.Fatalf(writeAtError, err)
} }
buf := make([]byte, 10) buf := make([]byte, 10)
realOffset, nr, err := f.ReadNext(context.Background(), buf, 200) realOffset, nr, err := f.ReadNext(context.Background(), buf, 200)
if err != nil || realOffset != 200 || nr != 5 || string(buf[0:nr]) != "hello" { if err != nil || realOffset != 200 || nr != 5 || string(buf[0:nr]) != "hello" {
t.Fatalf("invalid readnext: err[%v] realoffset[%d] nr[%d] buf[%s]", err, realOffset, nr, string(buf[0:nr])) t.Fatalf("invalid readnext: err[%v] realoffset[%d] nr[%d] buf[%s]", err, realOffset, nr, string(buf[0:nr]))
} }
err = f.WriteAt(nil, []byte("0123456789\n"), 100) err = f.WriteAt(context.TODO(), []byte("0123456789\n"), 100)
if err != nil { if err != nil {
t.Fatalf("writeat error: %v", err) t.Fatalf(writeAtError, err)
} }
dumpFile(f1Name) dumpFile(fPath)
dataStr := makeData(200) dataStr := makeData(200)
err = f.WriteAt(nil, []byte(dataStr), 50) err = f.WriteAt(context.TODO(), []byte(dataStr), 50)
if err != nil { if err != nil {
t.Fatalf("writeat error: %v", err) t.Fatalf(writeAtError, err)
} }
dumpFile(f1Name) dumpFile(fPath)
dataStr = makeData(1000) dataStr = makeData(1000)
err = f.WriteAt(nil, []byte(dataStr), 1002) err = f.WriteAt(context.TODO(), []byte(dataStr), 1002)
if err != nil { if err != nil {
t.Fatalf("writeat error: %v", err) t.Fatalf(writeAtError, err)
} }
err = f.WriteAt(nil, []byte("hello\n"), 2010) err = f.WriteAt(context.TODO(), []byte("hello\n"), 2010)
if err != nil { if err != nil {
t.Fatalf("writeat error: %v", err) t.Fatalf(writeAtError, err)
} }
err = f.AppendData(nil, []byte("foo\n")) err = f.AppendData(context.TODO(), []byte("foo\n"))
if err != nil { if err != nil {
t.Fatalf("appenddata error: %v", err) t.Fatalf("appenddata error: %v", err)
} }
dumpFile(f1Name) dumpFile(fPath)
} }