Skip to content

Commit

Permalink
review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@ashmeenkaur
ashmeenkaur committed May 2, 2024
1 parent 0b8d7f1 commit 3bdb9ee
Show file tree
Hide file tree
Showing 2 changed files with 71 additions and 71 deletions.
44 changes: 22 additions & 22 deletions internal/fs/fs.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ import (
"github.com/googlecloudplatform/gcsfuse/v2/internal/cache/file"
"github.com/googlecloudplatform/gcsfuse/v2/internal/cache/file/downloader"
"github.com/googlecloudplatform/gcsfuse/v2/internal/cache/lru"
"github.com/googlecloudplatform/gcsfuse/v2/internal/cache/util"
cacheutil "github.com/googlecloudplatform/gcsfuse/v2/internal/cache/util"
"github.com/googlecloudplatform/gcsfuse/v2/internal/config"
"github.com/googlecloudplatform/gcsfuse/v2/internal/contentcache"
"github.com/googlecloudplatform/gcsfuse/v2/internal/fs/handle"
Expand All @@ -40,7 +40,7 @@ import (
"github.com/googlecloudplatform/gcsfuse/v2/internal/locker"
"github.com/googlecloudplatform/gcsfuse/v2/internal/logger"
"github.com/googlecloudplatform/gcsfuse/v2/internal/storage/gcs"
. "github.com/googlecloudplatform/gcsfuse/v2/internal/util"
"github.com/googlecloudplatform/gcsfuse/v2/internal/util"
"github.com/jacobsa/fuse"
"github.com/jacobsa/fuse/fuseops"
"github.com/jacobsa/fuse/fuseutil"
Expand Down Expand Up @@ -224,20 +224,20 @@ func createFileCacheHandler(cfg *ServerConfig) (fileCacheHandler *file.CacheHand
if cfg.MountConfig.FileCacheConfig.MaxSizeMB == -1 {
sizeInBytes = math.MaxUint64
} else {
sizeInBytes = uint64(cfg.MountConfig.FileCacheConfig.MaxSizeMB) * util.MiB
sizeInBytes = uint64(cfg.MountConfig.FileCacheConfig.MaxSizeMB) * cacheutil.MiB
}
fileInfoCache := lru.NewCache(sizeInBytes)

cacheDir := string(cfg.MountConfig.CacheDir)
// Adding a new directory inside cacheDir to keep file-cache separate from
// metadata cache if and when we support storing metadata cache on disk in
// the future.
cacheDir = path.Join(cacheDir, util.FileCache)
cacheDir = path.Join(cacheDir, cacheutil.FileCache)

filePerm := util.DefaultFilePerm
dirPerm := util.DefaultDirPerm
filePerm := cacheutil.DefaultFilePerm
dirPerm := cacheutil.DefaultDirPerm

cacheDirErr := util.CreateCacheDirectoryIfNotPresentAt(cacheDir, dirPerm)
cacheDirErr := cacheutil.CreateCacheDirectoryIfNotPresentAt(cacheDir, dirPerm)
if cacheDirErr != nil {
return nil, fmt.Errorf("createFileCacheHandler: while creating file cache directory: %w", cacheDirErr)
}
Expand Down Expand Up @@ -1318,7 +1318,7 @@ func (fs *fileSystem) LookUpInode(
// When ignore interrupts config is set, we are creating a new context not
// cancellable by parent context.
var cancel context.CancelFunc
ctx, cancel = IsolateContextFromParentContext(ctx)
ctx, cancel = util.IsolateContextFromParentContext(ctx)
defer cancel()
}
// Find the parent directory in question.
Expand Down Expand Up @@ -1354,7 +1354,7 @@ func (fs *fileSystem) GetInodeAttributes(
// When ignore interrupts config is set, we are creating a new context not
// cancellable by parent context.
var cancel context.CancelFunc
ctx, cancel = IsolateContextFromParentContext(ctx)
ctx, cancel = util.IsolateContextFromParentContext(ctx)
defer cancel()
}
// Find the inode.
Expand Down Expand Up @@ -1382,7 +1382,7 @@ func (fs *fileSystem) SetInodeAttributes(
// When ignore interrupts config is set, we are creating a new context not
// cancellable by parent context.
var cancel context.CancelFunc
ctx, cancel = IsolateContextFromParentContext(ctx)
ctx, cancel = util.IsolateContextFromParentContext(ctx)
defer cancel()
}
// Find the inode.
Expand Down Expand Up @@ -1448,7 +1448,7 @@ func (fs *fileSystem) MkDir(
// When ignore interrupts config is set, we are creating a new context not
// cancellable by parent context.
var cancel context.CancelFunc
ctx, cancel = IsolateContextFromParentContext(ctx)
ctx, cancel = util.IsolateContextFromParentContext(ctx)
defer cancel()
}
// Find the parent.
Expand Down Expand Up @@ -1507,7 +1507,7 @@ func (fs *fileSystem) MkNode(
// When ignore interrupts config is set, we are creating a new context not
// cancellable by parent context.
var cancel context.CancelFunc
ctx, cancel = IsolateContextFromParentContext(ctx)
ctx, cancel = util.IsolateContextFromParentContext(ctx)
defer cancel()
}
if (op.Mode & (iofs.ModeNamedPipe | iofs.ModeSocket)) != 0 {
Expand Down Expand Up @@ -1637,7 +1637,7 @@ func (fs *fileSystem) CreateFile(
// When ignore interrupts config is set, we are creating a new context not
// cancellable by parent context.
var cancel context.CancelFunc
ctx, cancel = IsolateContextFromParentContext(ctx)
ctx, cancel = util.IsolateContextFromParentContext(ctx)
defer cancel()
}
// Create the child.
Expand Down Expand Up @@ -1686,7 +1686,7 @@ func (fs *fileSystem) CreateSymlink(
// When ignore interrupts config is set, we are creating a new context not
// cancellable by parent context.
var cancel context.CancelFunc
ctx, cancel = IsolateContextFromParentContext(ctx)
ctx, cancel = util.IsolateContextFromParentContext(ctx)
defer cancel()
}
// Find the parent.
Expand Down Expand Up @@ -1756,7 +1756,7 @@ func (fs *fileSystem) RmDir(
// When ignore interrupts config is set, we are creating a new context not
// cancellable by parent context.
var cancel context.CancelFunc
ctx, cancel = IsolateContextFromParentContext(ctx)
ctx, cancel = util.IsolateContextFromParentContext(ctx)
defer cancel()
}
// Find the parent.
Expand Down Expand Up @@ -1858,7 +1858,7 @@ func (fs *fileSystem) Rename(
// When ignore interrupts config is set, we are creating a new context not
// cancellable by parent context.
var cancel context.CancelFunc
ctx, cancel = IsolateContextFromParentContext(ctx)
ctx, cancel = util.IsolateContextFromParentContext(ctx)
defer cancel()
}
// Find the old and new parents.
Expand Down Expand Up @@ -2077,7 +2077,7 @@ func (fs *fileSystem) Unlink(
// When ignore interrupts config is set, we are creating a new context not
// cancellable by parent context.
var cancel context.CancelFunc
ctx, cancel = IsolateContextFromParentContext(ctx)
ctx, cancel = util.IsolateContextFromParentContext(ctx)
defer cancel()
}
// Find the parent.
Expand Down Expand Up @@ -2152,7 +2152,7 @@ func (fs *fileSystem) ReadDir(
// When ignore interrupts config is set, we are creating a new context not
// cancellable by parent context.
var cancel context.CancelFunc
ctx, cancel = IsolateContextFromParentContext(ctx)
ctx, cancel = util.IsolateContextFromParentContext(ctx)
defer cancel()
}
// Find the handle.
Expand Down Expand Up @@ -2224,7 +2224,7 @@ func (fs *fileSystem) ReadFile(
// When ignore interrupts config is set, we are creating a new context not
// cancellable by parent context.
var cancel context.CancelFunc
ctx, cancel = IsolateContextFromParentContext(ctx)
ctx, cancel = util.IsolateContextFromParentContext(ctx)
defer cancel()
}
// Save readOp in context for access in logs.
Expand Down Expand Up @@ -2275,7 +2275,7 @@ func (fs *fileSystem) WriteFile(
// When ignore interrupts config is set, we are creating a new context not
// cancellable by parent context.
var cancel context.CancelFunc
ctx, cancel = IsolateContextFromParentContext(ctx)
ctx, cancel = util.IsolateContextFromParentContext(ctx)
defer cancel()
}
// Find the inode.
Expand All @@ -2302,7 +2302,7 @@ func (fs *fileSystem) SyncFile(
// When ignore interrupts config is set, we are creating a new context not
// cancellable by parent context.
var cancel context.CancelFunc
ctx, cancel = IsolateContextFromParentContext(ctx)
ctx, cancel = util.IsolateContextFromParentContext(ctx)
defer cancel()
}
// Find the inode.
Expand Down Expand Up @@ -2335,7 +2335,7 @@ func (fs *fileSystem) FlushFile(
// When ignore interrupts config is set, we are creating a new context not
// cancellable by parent context.
var cancel context.CancelFunc
ctx, cancel = IsolateContextFromParentContext(ctx)
ctx, cancel = util.IsolateContextFromParentContext(ctx)
defer cancel()
}
// Find the inode.
Expand Down
98 changes: 49 additions & 49 deletions internal/util/util_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,111 +44,111 @@ func TestUtilSuite(t *testing.T) {
// Tests
////////////////////////////////////////////////////////////////////////

func (suite *UtilTest) TestResolveWhenParentProcDirEnvNotSetAndFilePathStartsWithTilda() {
func (ts *UtilTest) TestResolveWhenParentProcDirEnvNotSetAndFilePathStartsWithTilda() {
resolvedPath, err := GetResolvedPath("~/test.txt")

assert.Equal(suite.T(), nil, err)
assert.Equal(ts.T(), nil, err)
homeDir, err := os.UserHomeDir()
assert.Equal(suite.T(), nil, err)
assert.Equal(suite.T(), filepath.Join(homeDir, "test.txt"), resolvedPath)
assert.Equal(ts.T(), nil, err)
assert.Equal(ts.T(), filepath.Join(homeDir, "test.txt"), resolvedPath)
}

func (suite *UtilTest) TestResolveWhenParentProcDirEnvNotSetAndFilePathStartsWithDot() {
func (ts *UtilTest) TestResolveWhenParentProcDirEnvNotSetAndFilePathStartsWithDot() {
resolvedPath, err := GetResolvedPath("./test.txt")

assert.Equal(suite.T(), nil, err)
assert.Equal(ts.T(), nil, err)
currentWorkingDir, err := os.Getwd()
assert.Equal(suite.T(), nil, err)
assert.Equal(suite.T(), filepath.Join(currentWorkingDir, "./test.txt"), resolvedPath)
assert.Equal(ts.T(), nil, err)
assert.Equal(ts.T(), filepath.Join(currentWorkingDir, "./test.txt"), resolvedPath)
}

func (suite *UtilTest) TestResolveWhenParentProcDirEnvNotSetAndFilePathStartsWithDoubleDot() {
func (ts *UtilTest) TestResolveWhenParentProcDirEnvNotSetAndFilePathStartsWithDoubleDot() {
resolvedPath, err := GetResolvedPath("../test.txt")

assert.Equal(suite.T(), nil, err)
assert.Equal(ts.T(), nil, err)
currentWorkingDir, err := os.Getwd()
assert.Equal(suite.T(), nil, err)
assert.Equal(suite.T(), filepath.Join(currentWorkingDir, "../test.txt"), resolvedPath)
assert.Equal(ts.T(), nil, err)
assert.Equal(ts.T(), filepath.Join(currentWorkingDir, "../test.txt"), resolvedPath)
}

func (suite *UtilTest) TestResolveWhenParentProcDirEnvNotSetAndRelativePath() {
func (ts *UtilTest) TestResolveWhenParentProcDirEnvNotSetAndRelativePath() {
resolvedPath, err := GetResolvedPath("test.txt")

assert.Equal(suite.T(), nil, err)
assert.Equal(ts.T(), nil, err)
currentWorkingDir, err := os.Getwd()
assert.Equal(suite.T(), nil, err)
assert.Equal(suite.T(), filepath.Join(currentWorkingDir, "test.txt"), resolvedPath)
assert.Equal(ts.T(), nil, err)
assert.Equal(ts.T(), filepath.Join(currentWorkingDir, "test.txt"), resolvedPath)
}

func (suite *UtilTest) TestResolveWhenParentProcDirEnvNotSetAndAbsoluteFilePath() {
func (ts *UtilTest) TestResolveWhenParentProcDirEnvNotSetAndAbsoluteFilePath() {
resolvedPath, err := GetResolvedPath("/var/dir/test.txt")

assert.Equal(suite.T(), nil, err)
assert.Equal(suite.T(), "/var/dir/test.txt", resolvedPath)
assert.Equal(ts.T(), nil, err)
assert.Equal(ts.T(), "/var/dir/test.txt", resolvedPath)
}

func (suite *UtilTest) ResolveEmptyFilePath() {
func (ts *UtilTest) ResolveEmptyFilePath() {
resolvedPath, err := GetResolvedPath("")

assert.Equal(suite.T(), nil, err)
assert.Equal(suite.T(), "", resolvedPath)
assert.Equal(ts.T(), nil, err)
assert.Equal(ts.T(), "", resolvedPath)
}

// Below all tests when GCSFUSE_PARENT_PROCESS_DIR env variable is set.
// By setting this environment variable, resolve will work for child process.
func (suite *UtilTest) ResolveWhenParentProcDirEnvSetAndFilePathStartsWithTilda() {
func (ts *UtilTest) ResolveWhenParentProcDirEnvSetAndFilePathStartsWithTilda() {
os.Setenv(GCSFUSE_PARENT_PROCESS_DIR, gcsFuseParentProcessDir)
defer os.Unsetenv(GCSFUSE_PARENT_PROCESS_DIR)

resolvedPath, err := GetResolvedPath("~/test.txt")

assert.Equal(suite.T(), nil, err)
assert.Equal(ts.T(), nil, err)
homeDir, err := os.UserHomeDir()
assert.Equal(suite.T(), nil, err)
assert.Equal(suite.T(), filepath.Join(homeDir, "test.txt"), resolvedPath)
assert.Equal(ts.T(), nil, err)
assert.Equal(ts.T(), filepath.Join(homeDir, "test.txt"), resolvedPath)
}

func (suite *UtilTest) ResolveWhenParentProcDirEnvSetAndFilePathStartsWithDot() {
func (ts *UtilTest) ResolveWhenParentProcDirEnvSetAndFilePathStartsWithDot() {
os.Setenv(GCSFUSE_PARENT_PROCESS_DIR, gcsFuseParentProcessDir)
defer os.Unsetenv(GCSFUSE_PARENT_PROCESS_DIR)

resolvedPath, err := GetResolvedPath("./test.txt")

assert.Equal(suite.T(), nil, err)
assert.Equal(suite.T(), filepath.Join(gcsFuseParentProcessDir, "./test.txt"), resolvedPath)
assert.Equal(ts.T(), nil, err)
assert.Equal(ts.T(), filepath.Join(gcsFuseParentProcessDir, "./test.txt"), resolvedPath)
}

func (suite *UtilTest) ResolveWhenParentProcDirEnvSetAndFilePathStartsWithDoubleDot() {
func (ts *UtilTest) ResolveWhenParentProcDirEnvSetAndFilePathStartsWithDoubleDot() {
os.Setenv(GCSFUSE_PARENT_PROCESS_DIR, gcsFuseParentProcessDir)
defer os.Unsetenv(GCSFUSE_PARENT_PROCESS_DIR)

resolvedPath, err := GetResolvedPath("../test.txt")

assert.Equal(suite.T(), nil, err)
assert.Equal(suite.T(), filepath.Join(gcsFuseParentProcessDir, "../test.txt"), resolvedPath)
assert.Equal(ts.T(), nil, err)
assert.Equal(ts.T(), filepath.Join(gcsFuseParentProcessDir, "../test.txt"), resolvedPath)
}

func (suite *UtilTest) ResolveWhenParentProcDirEnvSetAndRelativePath() {
func (ts *UtilTest) ResolveWhenParentProcDirEnvSetAndRelativePath() {
os.Setenv(GCSFUSE_PARENT_PROCESS_DIR, gcsFuseParentProcessDir)
defer os.Unsetenv(GCSFUSE_PARENT_PROCESS_DIR)

resolvedPath, err := GetResolvedPath("test.txt")

assert.Equal(suite.T(), nil, err)
assert.Equal(suite.T(), filepath.Join(gcsFuseParentProcessDir, "test.txt"), resolvedPath)
assert.Equal(ts.T(), nil, err)
assert.Equal(ts.T(), filepath.Join(gcsFuseParentProcessDir, "test.txt"), resolvedPath)
}

func (suite *UtilTest) ResolveWhenParentProcDirEnvSetAndAbsoluteFilePath() {
func (ts *UtilTest) ResolveWhenParentProcDirEnvSetAndAbsoluteFilePath() {
os.Setenv(GCSFUSE_PARENT_PROCESS_DIR, gcsFuseParentProcessDir)
defer os.Unsetenv(GCSFUSE_PARENT_PROCESS_DIR)

resolvedPath, err := GetResolvedPath("/var/dir/test.txt")

assert.Equal(suite.T(), nil, err)
assert.Equal(suite.T(), "/var/dir/test.txt", resolvedPath)
assert.Equal(ts.T(), nil, err)
assert.Equal(ts.T(), "/var/dir/test.txt", resolvedPath)
}

func (suite *UtilTest) TestStringifyShouldReturnAllFieldsPassedInCustomObjectAsMarshalledString() {
func (ts *UtilTest) TestStringifyShouldReturnAllFieldsPassedInCustomObjectAsMarshalledString() {
sampleMap := map[string]int{
"1": 1,
"2": 2,
Expand All @@ -166,18 +166,18 @@ func (suite *UtilTest) TestStringifyShouldReturnAllFieldsPassedInCustomObjectAsM
actual, _ := Stringify(customObject)

expected := "{\"Value\":\"test_value\",\"NestedValue\":{\"SomeField\":10,\"SomeOther\":{\"1\":1,\"2\":2,\"3\":3}}}"
assert.Equal(suite.T(), expected, actual)
assert.Equal(ts.T(), expected, actual)
}

func (suite *UtilTest) TestStringifyShouldReturnEmptyStringWhenMarshalErrorsOut() {
func (ts *UtilTest) TestStringifyShouldReturnEmptyStringWhenMarshalErrorsOut() {
customInstance := customTypeForError{
value: "example",
}

actual, _ := Stringify(customInstance)

expected := ""
assert.Equal(suite.T(), expected, actual)
assert.Equal(ts.T(), expected, actual)
}

type customTypeForSuccess struct {
Expand All @@ -197,7 +197,7 @@ func (c customTypeForError) MarshalJSON() ([]byte, error) {
return nil, errors.New("intentional error during JSON marshaling")
}

func (suite *UtilTest) TestMiBsToBytes() {
func (ts *UtilTest) TestMiBsToBytes() {
cases := []struct {
mib uint64
bytes uint64
Expand Down Expand Up @@ -225,11 +225,11 @@ func (suite *UtilTest) TestMiBsToBytes() {
}

for _, tc := range cases {
assert.Equal(suite.T(), tc.bytes, MiBsToBytes(tc.mib))
assert.Equal(ts.T(), tc.bytes, MiBsToBytes(tc.mib))
}
}

func (suite *UtilTest) TestBytesToHigherMiBs() {
func (ts *UtilTest) TestBytesToHigherMiBs() {
cases := []struct {
bytes uint64
mib uint64
Expand Down Expand Up @@ -261,20 +261,20 @@ func (suite *UtilTest) TestBytesToHigherMiBs() {
}

for _, tc := range cases {
assert.Equal(suite.T(), tc.mib, BytesToHigherMiBs(tc.bytes))
assert.Equal(ts.T(), tc.mib, BytesToHigherMiBs(tc.bytes))
}
}

func (suite *UtilTest) TestIsolateContextFromParentContext() {
func (ts *UtilTest) TestIsolateContextFromParentContext() {
parentCtx, parentCtxCancel := context.WithCancel(context.Background())

// Call the method and cancel the parent context.
newCtx, newCtxCancel := IsolateContextFromParentContext(parentCtx)
parentCtxCancel()

// Validate new context is not cancelled after parent's cancellation.
assert.NoError(suite.T(), newCtx.Err())
assert.NoError(ts.T(), newCtx.Err())
// Cancel the new context and validate.
newCtxCancel()
assert.ErrorIs(suite.T(), newCtx.Err(), context.Canceled)
assert.ErrorIs(ts.T(), newCtx.Err(), context.Canceled)
}

0 comments on commit 3bdb9ee

Please sign in to comment.