From 4512006261adda77ddce8e106e60010335e60689 Mon Sep 17 00:00:00 2001 From: Jackson Owens Date: Mon, 12 Aug 2024 17:02:08 -0400 Subject: [PATCH] db: fix Open issue with single-item relative paths Previously Open with a relative path containing a single element would fail on real filesystems, because Open would attempt to open the empty path. Fix #3842. --- checkpoint.go | 11 ++--- open_test.go | 46 ++++++++++++++++++++ testdata/checkpoint | 12 +++--- testdata/checkpoint_shared | 12 +++--- testdata/cleaner | 36 ++++++++-------- testdata/event_listener | 18 ++++---- testdata/mkdir_all_and_sync_parents | 67 +++++++++++++++++++++++++++++ vfs/mem_fs.go | 3 ++ 8 files changed, 159 insertions(+), 46 deletions(-) create mode 100644 testdata/mkdir_all_and_sync_parents diff --git a/checkpoint.go b/checkpoint.go index f0c7f2d72a..62f4b6e060 100644 --- a/checkpoint.go +++ b/checkpoint.go @@ -91,23 +91,20 @@ func mkdirAllAndSyncParents(fs vfs.FS, destDir string) (vfs.File, error) { // Collect paths for all directories between destDir (excluded) and its // closest existing ancestor (included). var parentPaths []string - foundExistingAncestor := false - for parentPath := fs.PathDir(destDir); parentPath != "."; parentPath = fs.PathDir(parentPath) { + for parentPath := fs.PathDir(destDir); ; parentPath = fs.PathDir(parentPath) { parentPaths = append(parentPaths, parentPath) + if fs.PathDir(parentPath) == parentPath { + break + } _, err := fs.Stat(parentPath) if err == nil { // Exit loop at the closest existing ancestor. - foundExistingAncestor = true break } if !oserror.IsNotExist(err) { return nil, err } } - // Handle empty filesystem edge case. - if !foundExistingAncestor { - parentPaths = append(parentPaths, "") - } // Create destDir and any of its missing parents. if err := fs.MkdirAll(destDir, 0755); err != nil { return nil, err diff --git a/open_test.go b/open_test.go index 31f9a6a78f..7667d590ef 100644 --- a/open_test.go +++ b/open_test.go @@ -1478,3 +1478,49 @@ func TestOpenRatchetsNextFileNum(t *testing.T) { require.NoError(t, d.Compact([]byte("a"), []byte("z"), false)) } + +func TestMkdirAllAndSyncParents(t *testing.T) { + if filepath.Separator != '/' { + t.Skip("skipping due to path separator") + } + pwd, err := os.Getwd() + require.NoError(t, err) + defer func() { os.Chdir(pwd) }() + + filesystems := map[string]vfs.FS{} + rootPaths := map[string]string{} + var buf bytes.Buffer + datadriven.RunTest(t, "testdata/mkdir_all_and_sync_parents", func(t *testing.T, td *datadriven.TestData) string { + buf.Reset() + switch td.Cmd { + case "mkfs": + var fsName string + td.ScanArgs(t, "fs", &fsName) + if td.HasArg("memfs") { + filesystems[fsName] = vfs.NewMem() + return "new memfs" + } + filesystems[fsName] = vfs.Default + rootPaths[fsName] = t.TempDir() + return "new default fs" + case "mkdir-all-and-sync-parents": + var fsName, path string + td.ScanArgs(t, "fs", &fsName) + td.ScanArgs(t, "path", &path) + if p, ok := rootPaths[fsName]; ok { + require.NoError(t, os.Chdir(p)) + } + fs := vfs.WithLogging(filesystems[fsName], func(format string, args ...interface{}) { + fmt.Fprintf(&buf, format+"\n", args...) + }) + f, err := mkdirAllAndSyncParents(fs, path) + if err != nil { + return err.Error() + } + require.NoError(t, f.Close()) + return buf.String() + default: + return fmt.Sprintf("unrecognized command %q", td.Cmd) + } + }) +} diff --git a/testdata/checkpoint b/testdata/checkpoint index b9ae37c21e..c323f8c0cd 100644 --- a/testdata/checkpoint +++ b/testdata/checkpoint @@ -1,9 +1,9 @@ open db ---- mkdir-all: db 0755 -open-dir: -sync: -close: +open-dir: . +sync: . +close: . open-dir: db close: db open-dir: db @@ -92,9 +92,9 @@ mkdir-all: checkpoints/checkpoint1 0755 open-dir: checkpoints sync: checkpoints close: checkpoints -open-dir: -sync: -close: +open-dir: . +sync: . +close: . open-dir: checkpoints/checkpoint1 link: db/OPTIONS-000003 -> checkpoints/checkpoint1/OPTIONS-000003 open-dir: checkpoints/checkpoint1 diff --git a/testdata/checkpoint_shared b/testdata/checkpoint_shared index eda39dc9df..fd264a5e7f 100644 --- a/testdata/checkpoint_shared +++ b/testdata/checkpoint_shared @@ -1,9 +1,9 @@ open db ---- mkdir-all: db 0755 -open-dir: -sync: -close: +open-dir: . +sync: . +close: . open-dir: db close: db open-dir: db @@ -80,9 +80,9 @@ mkdir-all: checkpoints/checkpoint1 0755 open-dir: checkpoints sync: checkpoints close: checkpoints -open-dir: -sync: -close: +open-dir: . +sync: . +close: . open-dir: checkpoints/checkpoint1 link: db/OPTIONS-000003 -> checkpoints/checkpoint1/OPTIONS-000003 open-dir: checkpoints/checkpoint1 diff --git a/testdata/cleaner b/testdata/cleaner index 0bafa36954..b2219b053d 100644 --- a/testdata/cleaner +++ b/testdata/cleaner @@ -2,15 +2,15 @@ open db archive ---- mkdir-all: db 0755 -open-dir: -sync: -close: +open-dir: . +sync: . +close: . open-dir: db close: db mkdir-all: db_wal 0755 -open-dir: -sync: -close: +open-dir: . +sync: . +close: . open-dir: db_wal close: db_wal open-dir: db @@ -132,15 +132,15 @@ list db_wal/archive open db1 ---- mkdir-all: db1 0755 -open-dir: -sync: -close: +open-dir: . +sync: . +close: . open-dir: db1 close: db1 mkdir-all: db1_wal 0755 -open-dir: -sync: -close: +open-dir: . +sync: . +close: . open-dir: db1_wal close: db1_wal open-dir: db1 @@ -211,15 +211,15 @@ close: db1/000456.sst open db1 ---- mkdir-all: db1 0755 -open-dir: -sync: -close: +open-dir: . +sync: . +close: . open-dir: db1 close: db1 mkdir-all: db1_wal 0755 -open-dir: -sync: -close: +open-dir: . +sync: . +close: . open-dir: db1_wal close: db1_wal open-dir: db1 diff --git a/testdata/event_listener b/testdata/event_listener index 34b61403a0..b9e735c6b1 100644 --- a/testdata/event_listener +++ b/testdata/event_listener @@ -1,15 +1,15 @@ open ---- mkdir-all: db 0755 -open-dir: -sync: -close: +open-dir: . +sync: . +close: . open-dir: db close: db mkdir-all: wal 0755 -open-dir: -sync: -close: +open-dir: . +sync: . +close: . open-dir: wal close: wal open-dir: db @@ -346,9 +346,9 @@ sstables checkpoint ---- mkdir-all: checkpoint 0755 -open-dir: -sync: -close: +open-dir: . +sync: . +close: . open-dir: checkpoint link: db/OPTIONS-000003 -> checkpoint/OPTIONS-000003 open-dir: checkpoint diff --git a/testdata/mkdir_all_and_sync_parents b/testdata/mkdir_all_and_sync_parents new file mode 100644 index 0000000000..b78b974ae7 --- /dev/null +++ b/testdata/mkdir_all_and_sync_parents @@ -0,0 +1,67 @@ +mkfs memfs fs=mem1 +---- +new memfs + +mkdir-all-and-sync-parents fs=mem1 path=foo/bar/baz/bax +---- +mkdir-all: foo/bar/baz/bax 0755 +open-dir: foo/bar/baz +sync: foo/bar/baz +close: foo/bar/baz +open-dir: foo/bar +sync: foo/bar +close: foo/bar +open-dir: foo +sync: foo +close: foo +open-dir: . +sync: . +close: . +open-dir: foo/bar/baz/bax +close: foo/bar/baz/bax + +# Repeating the same command should only sync the parent, and then the new data +# directory itself. + +mkdir-all-and-sync-parents fs=mem1 path=foo/bar/baz/bax +---- +mkdir-all: foo/bar/baz/bax 0755 +open-dir: foo/bar/baz +sync: foo/bar/baz +close: foo/bar/baz +open-dir: foo/bar/baz/bax +close: foo/bar/baz/bax + +mkfs fs=default1 +---- +new default fs + +mkdir-all-and-sync-parents fs=default1 path=foo/bar/baz/bax +---- +mkdir-all: foo/bar/baz/bax 0755 +open-dir: foo/bar/baz +sync: foo/bar/baz +close: foo/bar/baz +open-dir: foo/bar +sync: foo/bar +close: foo/bar +open-dir: foo +sync: foo +close: foo +open-dir: . +sync: . +close: . +open-dir: foo/bar/baz/bax +close: foo/bar/baz/bax + +# Repeating the same command should only sync the parent, and then the new data +# directory itself. + +mkdir-all-and-sync-parents fs=default1 path=foo/bar/baz/bax +---- +mkdir-all: foo/bar/baz/bax 0755 +open-dir: foo/bar/baz +sync: foo/bar/baz +close: foo/bar/baz +open-dir: foo/bar/baz/bax +close: foo/bar/baz/bax diff --git a/vfs/mem_fs.go b/vfs/mem_fs.go index 0215afa062..f0448b8f1e 100644 --- a/vfs/mem_fs.go +++ b/vfs/mem_fs.go @@ -172,6 +172,9 @@ func (y *MemFS) walk(fullname string, f func(dir *memNode, frag string, final bo for len(fullname) > 0 && fullname[0] == sep[0] { fullname = fullname[1:] } + if fullname == "." { + fullname = "" + } dir := y.root for {