From 3282fd26af526d8490e84563a85fe09d26daf3d2 Mon Sep 17 00:00:00 2001 From: Nick Craig-Wood Date: Mon, 26 Feb 2018 16:59:14 +0000 Subject: [PATCH] vfs: clean path names before using them in the cache This avoids inconsistent cache behaviour on open("potato/") close("potato"). The tests were also adjusted to make them more comprehensive. --- vfs/cache.go | 15 +++ vfs/cache_test.go | 278 +++++++++++++++++++++++++++++++++------------- 2 files changed, 218 insertions(+), 75 deletions(-) diff --git a/vfs/cache.go b/vfs/cache.go index b8a3a103c..3af3fd448 100644 --- a/vfs/cache.go +++ b/vfs/cache.go @@ -124,6 +124,16 @@ func findParent(name string) string { return parent } +// clean returns the cleaned version of name for use in the index map +func clean(name string) string { + name = strings.Trim(name, "/") + name = path.Clean(name) + if name == "." || name == "/" { + name = "" + } + return name +} + // toOSPath turns a remote relative name into an OS path in the cache func (c *cache) toOSPath(name string) string { return filepath.Join(c.root, filepath.FromSlash(name)) @@ -179,6 +189,7 @@ func (c *cache) opens(name string) int { // // name should be a remote path not an osPath func (c *cache) get(name string) *cacheItem { + name = clean(name) c.itemMu.Lock() item, _ := c._get(true, name) c.itemMu.Unlock() @@ -190,6 +201,7 @@ func (c *cache) get(name string) *cacheItem { // // name should be a remote path not an osPath func (c *cache) updateTime(name string, when time.Time) { + name = clean(name) c.itemMu.Lock() item, found := c._get(true, name) if !found || when.Sub(item.atime) > 0 { @@ -219,6 +231,7 @@ func (c *cache) _open(isFile bool, name string) { // // name should be a remote path not an osPath func (c *cache) open(name string) { + name = clean(name) c.itemMu.Lock() c._open(true, name) c.itemMu.Unlock() @@ -228,6 +241,7 @@ func (c *cache) open(name string) { // // name should be a remote path not an osPath func (c *cache) cacheDir(name string) { + name = clean(name) c.itemMu.Lock() defer c.itemMu.Unlock() for { @@ -264,6 +278,7 @@ func (c *cache) _close(isFile bool, name string) { // // name should be a remote path not an osPath func (c *cache) close(name string) { + name = clean(name) c.itemMu.Lock() c._close(true, name) c.itemMu.Unlock() diff --git a/vfs/cache_test.go b/vfs/cache_test.go index 200a68aea..1ed31300d 100644 --- a/vfs/cache_test.go +++ b/vfs/cache_test.go @@ -6,7 +6,6 @@ import ( "os" "path/filepath" "sort" - "strings" "testing" "time" @@ -47,7 +46,7 @@ func TestCacheModeType(t *testing.T) { } // convert c.item to a string -func itemAsString(c *cache) string { +func itemAsString(c *cache) []string { c.itemMu.Lock() defer c.itemMu.Unlock() var out []string @@ -55,7 +54,7 @@ func itemAsString(c *cache) string { out = append(out, fmt.Sprintf("name=%q isFile=%v opens=%d", name, item.isFile, item.opens)) } sort.Strings(out) - return strings.Join(out, "\n") + return out } func TestCacheNew(t *testing.T) { @@ -73,13 +72,15 @@ func TestCacheNew(t *testing.T) { assert.Contains(t, c.root, "vfs") assert.Contains(t, c.f.Root(), filepath.Base(r.Fremote.Root())) - assert.Equal(t, "", itemAsString(c)) + assert.Equal(t, []string(nil), itemAsString(c)) // mkdir p, err := c.mkdir("potato") require.NoError(t, err) assert.Equal(t, "potato", filepath.Base(p)) - assert.Equal(t, `name="" isFile=false opens=0`, itemAsString(c)) + assert.Equal(t, []string{ + `name="" isFile=false opens=0`, + }, itemAsString(c)) fi, err := os.Stat(filepath.Dir(p)) require.NoError(t, err) @@ -106,11 +107,15 @@ func TestCacheNew(t *testing.T) { assert.Equal(t, 0, item.opens) // open - assert.Equal(t, `name="" isFile=false opens=0 -name="potato" isFile=true opens=0`, itemAsString(c)) - c.open("potato") - assert.Equal(t, `name="" isFile=false opens=1 -name="potato" isFile=true opens=1`, itemAsString(c)) + assert.Equal(t, []string{ + `name="" isFile=false opens=0`, + `name="potato" isFile=true opens=0`, + }, itemAsString(c)) + c.open("/potato") + assert.Equal(t, []string{ + `name="" isFile=false opens=1`, + `name="potato" isFile=true opens=1`, + }, itemAsString(c)) item = c.get("potato") assert.WithinDuration(t, time.Now(), item.atime, time.Second) assert.Equal(t, 1, item.opens) @@ -129,8 +134,10 @@ name="potato" isFile=true opens=1`, itemAsString(c)) item.atime = time.Now().Add(-24 * time.Hour) err = c.updateAtimes() require.NoError(t, err) - assert.Equal(t, `name="" isFile=false opens=1 -name="potato" isFile=true opens=1`, itemAsString(c)) + assert.Equal(t, []string{ + `name="" isFile=false opens=1`, + `name="potato" isFile=true opens=1`, + }, itemAsString(c)) item = c.get("potato") assert.Equal(t, atime, item.atime) @@ -141,8 +148,10 @@ name="potato" isFile=true opens=1`, itemAsString(c)) c.itemMu.Unlock() err = c.updateAtimes() require.NoError(t, err) - assert.Equal(t, `name="" isFile=false opens=1 -name="potato" isFile=true opens=0`, itemAsString(c)) + assert.Equal(t, []string{ + `name="" isFile=false opens=1`, + `name="potato" isFile=true opens=0`, + }, itemAsString(c)) item = c.get("potato") assert.Equal(t, atime, item.atime) c.itemMu.Lock() @@ -155,14 +164,20 @@ name="potato" isFile=true opens=0`, itemAsString(c)) assert.NoError(t, err) // close - assert.Equal(t, `name="" isFile=false opens=1 -name="potato" isFile=true opens=1`, itemAsString(c)) + assert.Equal(t, []string{ + `name="" isFile=false opens=1`, + `name="potato" isFile=true opens=1`, + }, itemAsString(c)) c.updateTime("potato", t2) - assert.Equal(t, `name="" isFile=false opens=1 -name="potato" isFile=true opens=1`, itemAsString(c)) - c.close("potato") - assert.Equal(t, `name="" isFile=false opens=0 -name="potato" isFile=true opens=0`, itemAsString(c)) + assert.Equal(t, []string{ + `name="" isFile=false opens=1`, + `name="potato" isFile=true opens=1`, + }, itemAsString(c)) + c.close("potato/") + assert.Equal(t, []string{ + `name="" isFile=false opens=0`, + `name="potato" isFile=true opens=0`, + }, itemAsString(c)) item = c.get("potato") assert.WithinDuration(t, time.Now(), item.atime, time.Second) assert.Equal(t, 0, item.opens) @@ -198,52 +213,127 @@ func TestCacheOpens(t *testing.T) { c, err := newCache(ctx, r.Fremote, &DefaultOpt) require.NoError(t, err) - assert.Equal(t, "", itemAsString(c)) + assert.Equal(t, []string(nil), itemAsString(c)) c.open("potato") - assert.Equal(t, `name="" isFile=false opens=1 -name="potato" isFile=true opens=1`, itemAsString(c)) + assert.Equal(t, []string{ + `name="" isFile=false opens=1`, + `name="potato" isFile=true opens=1`, + }, itemAsString(c)) c.open("potato") - assert.Equal(t, `name="" isFile=false opens=2 -name="potato" isFile=true opens=2`, itemAsString(c)) + assert.Equal(t, []string{ + `name="" isFile=false opens=2`, + `name="potato" isFile=true opens=2`, + }, itemAsString(c)) c.close("potato") - assert.Equal(t, `name="" isFile=false opens=1 -name="potato" isFile=true opens=1`, itemAsString(c)) + assert.Equal(t, []string{ + `name="" isFile=false opens=1`, + `name="potato" isFile=true opens=1`, + }, itemAsString(c)) c.close("potato") - assert.Equal(t, `name="" isFile=false opens=0 -name="potato" isFile=true opens=0`, itemAsString(c)) + assert.Equal(t, []string{ + `name="" isFile=false opens=0`, + `name="potato" isFile=true opens=0`, + }, itemAsString(c)) c.open("potato") - c.open("a/b/c/d/one") + c.open("a//b/c/d/one") c.open("a/b/c/d/e/two") c.open("a/b/c/d/e/f/three") - assert.Equal(t, `name="" isFile=false opens=4 -name="a" isFile=false opens=3 -name="a/b" isFile=false opens=3 -name="a/b/c" isFile=false opens=3 -name="a/b/c/d" isFile=false opens=3 -name="a/b/c/d/e" isFile=false opens=2 -name="a/b/c/d/e/f" isFile=false opens=1 -name="a/b/c/d/e/f/three" isFile=true opens=1 -name="a/b/c/d/e/two" isFile=true opens=1 -name="a/b/c/d/one" isFile=true opens=1 -name="potato" isFile=true opens=1`, itemAsString(c)) - + assert.Equal(t, []string{ + `name="" isFile=false opens=4`, + `name="a" isFile=false opens=3`, + `name="a/b" isFile=false opens=3`, + `name="a/b/c" isFile=false opens=3`, + `name="a/b/c/d" isFile=false opens=3`, + `name="a/b/c/d/e" isFile=false opens=2`, + `name="a/b/c/d/e/f" isFile=false opens=1`, + `name="a/b/c/d/e/f/three" isFile=true opens=1`, + `name="a/b/c/d/e/two" isFile=true opens=1`, + `name="a/b/c/d/one" isFile=true opens=1`, + `name="potato" isFile=true opens=1`, + }, itemAsString(c)) c.close("potato") c.close("a/b/c/d/one") c.close("a/b/c/d/e/two") - c.close("a/b/c/d/e/f/three") - assert.Equal(t, `name="" isFile=false opens=0 -name="a" isFile=false opens=0 -name="a/b" isFile=false opens=0 -name="a/b/c" isFile=false opens=0 -name="a/b/c/d" isFile=false opens=0 -name="a/b/c/d/e" isFile=false opens=0 -name="a/b/c/d/e/f" isFile=false opens=0 -name="a/b/c/d/e/f/three" isFile=true opens=0 -name="a/b/c/d/e/two" isFile=true opens=0 -name="a/b/c/d/one" isFile=true opens=0 -name="potato" isFile=true opens=0`, itemAsString(c)) + c.close("a/b/c//d/e/f/three") + assert.Equal(t, []string{ + `name="" isFile=false opens=0`, + `name="a" isFile=false opens=0`, + `name="a/b" isFile=false opens=0`, + `name="a/b/c" isFile=false opens=0`, + `name="a/b/c/d" isFile=false opens=0`, + `name="a/b/c/d/e" isFile=false opens=0`, + `name="a/b/c/d/e/f" isFile=false opens=0`, + `name="a/b/c/d/e/f/three" isFile=true opens=0`, + `name="a/b/c/d/e/two" isFile=true opens=0`, + `name="a/b/c/d/one" isFile=true opens=0`, + `name="potato" isFile=true opens=0`, + }, itemAsString(c)) +} +// test the open, mkdir, purge, close, purge sequence +func TestCacheOpenMkdir(t *testing.T) { + r := fstest.NewRun(t) + defer r.Finalise() + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + // Disable the cache cleaner as it interferes with these tests + opt := DefaultOpt + opt.CachePollInterval = 0 + c, err := newCache(ctx, r.Fremote, &opt) + require.NoError(t, err) + + // open + c.open("sub/potato") + + assert.Equal(t, []string{ + `name="" isFile=false opens=1`, + `name="sub" isFile=false opens=1`, + `name="sub/potato" isFile=true opens=1`, + }, itemAsString(c)) + + // mkdir + p, err := c.mkdir("sub/potato") + require.NoError(t, err) + assert.Equal(t, "potato", filepath.Base(p)) + assert.Equal(t, []string{ + `name="" isFile=false opens=1`, + `name="sub" isFile=false opens=1`, + `name="sub/potato" isFile=true opens=1`, + }, itemAsString(c)) + + // test directory exists + fi, err := os.Stat(filepath.Dir(p)) + require.NoError(t, err) + assert.True(t, fi.IsDir()) + + // clean the cache + c.purgeOld(-10 * time.Second) + + // test directory still exists + fi, err = os.Stat(filepath.Dir(p)) + require.NoError(t, err) + assert.True(t, fi.IsDir()) + + // close + c.close("sub/potato") + + assert.Equal(t, []string{ + `name="" isFile=false opens=0`, + `name="sub" isFile=false opens=0`, + `name="sub/potato" isFile=true opens=0`, + }, itemAsString(c)) + + // clean the cache + c.purgeOld(-10 * time.Second) + + assert.Equal(t, []string(nil), itemAsString(c)) + + // test directory does not exist + fi, err = os.Stat(filepath.Dir(p)) + require.True(t, os.IsNotExist(err)) } func TestCacheCacheDir(t *testing.T) { @@ -256,23 +346,29 @@ func TestCacheCacheDir(t *testing.T) { c, err := newCache(ctx, r.Fremote, &DefaultOpt) require.NoError(t, err) - assert.Equal(t, "", itemAsString(c)) + assert.Equal(t, []string(nil), itemAsString(c)) c.cacheDir("dir") - assert.Equal(t, `name="" isFile=false opens=0 -name="dir" isFile=false opens=0`, itemAsString(c)) + assert.Equal(t, []string{ + `name="" isFile=false opens=0`, + `name="dir" isFile=false opens=0`, + }, itemAsString(c)) c.cacheDir("dir/sub") - assert.Equal(t, `name="" isFile=false opens=0 -name="dir" isFile=false opens=0 -name="dir/sub" isFile=false opens=0`, itemAsString(c)) + assert.Equal(t, []string{ + `name="" isFile=false opens=0`, + `name="dir" isFile=false opens=0`, + `name="dir/sub" isFile=false opens=0`, + }, itemAsString(c)) c.cacheDir("dir/sub2/subsub2") - assert.Equal(t, `name="" isFile=false opens=0 -name="dir" isFile=false opens=0 -name="dir/sub" isFile=false opens=0 -name="dir/sub2" isFile=false opens=0 -name="dir/sub2/subsub2" isFile=false opens=0`, itemAsString(c)) + assert.Equal(t, []string{ + `name="" isFile=false opens=0`, + `name="dir" isFile=false opens=0`, + `name="dir/sub" isFile=false opens=0`, + `name="dir/sub2" isFile=false opens=0`, + `name="dir/sub2/subsub2" isFile=false opens=0`, + }, itemAsString(c)) } func TestCachePurgeOld(t *testing.T) { @@ -302,23 +398,55 @@ func TestCachePurgeOld(t *testing.T) { c._purgeOld(-10*time.Second, removeFile, removeDir) assert.Equal(t, []string(nil), removed) + c.open("sub/dir2/potato2") c.open("sub/dir/potato") + c.close("sub/dir2/potato2") + c.open("sub/dir/potato") + + assert.Equal(t, []string{ + `name="" isFile=false opens=2`, + `name="sub" isFile=false opens=2`, + `name="sub/dir" isFile=false opens=2`, + `name="sub/dir/potato" isFile=true opens=2`, + `name="sub/dir2" isFile=false opens=0`, + `name="sub/dir2/potato2" isFile=true opens=0`, + }, itemAsString(c)) + + removed = nil + removedDir = true + c._purgeOld(-10*time.Second, removeFile, removeDir) + assert.Equal(t, []string{ + "sub/dir2/potato2", + "sub/dir2/", + }, removed) + c.close("sub/dir/potato") - assert.Equal(t, `name="" isFile=false opens=0 -name="sub" isFile=false opens=0 -name="sub/dir" isFile=false opens=0 -name="sub/dir/potato" isFile=true opens=0`, itemAsString(c)) + removed = nil + removedDir = true + c._purgeOld(-10*time.Second, removeFile, removeDir) + assert.Equal(t, []string(nil), removed) + + c.close("sub/dir/potato") + + assert.Equal(t, []string{ + `name="" isFile=false opens=0`, + `name="sub" isFile=false opens=0`, + `name="sub/dir" isFile=false opens=0`, + `name="sub/dir/potato" isFile=true opens=0`, + }, itemAsString(c)) removed = nil removedDir = false c._purgeOld(10*time.Second, removeFile, removeDir) assert.Equal(t, []string(nil), removed) - assert.Equal(t, `name="" isFile=false opens=0 -name="sub" isFile=false opens=0 -name="sub/dir" isFile=false opens=0 -name="sub/dir/potato" isFile=true opens=0`, itemAsString(c)) + assert.Equal(t, []string{ + `name="" isFile=false opens=0`, + `name="sub" isFile=false opens=0`, + `name="sub/dir" isFile=false opens=0`, + `name="sub/dir/potato" isFile=true opens=0`, + }, itemAsString(c)) removed = nil removedDir = true @@ -330,5 +458,5 @@ name="sub/dir/potato" isFile=true opens=0`, itemAsString(c)) "/", }, removed) - assert.Equal(t, ``, itemAsString(c)) + assert.Equal(t, []string(nil), itemAsString(c)) }