Skip to content

Commit

Permalink
vfs: fix hard link names in String()
Browse files Browse the repository at this point in the history
This change fixes a bug in MemFS dump printer, as well as an incorrect
"logical" ownwership structure.

Files that are hard linked to the same node incorrectly print the same
file name (matching the name of the file which created the node).

This is due to the fact that we store the file name in memNode. Instead,
we should allow the same memNode to be used under different names. To
fix it, this commit moves the name field from memNode to memFile.
  • Loading branch information
pav-kv committed Jan 16, 2024
1 parent 7af753a commit 27d566e
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 34 deletions.
45 changes: 15 additions & 30 deletions vfs/mem_fs.go
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ func (y *MemFS) String() string {
defer y.mu.Unlock()

s := new(bytes.Buffer)
y.root.dump(s, 0)
y.root.dump(s, 0, sep)
return s.String()
}

Expand Down Expand Up @@ -209,9 +209,10 @@ func (y *MemFS) Create(fullname string) (File, error) {
if frag == "" {
return errors.New("pebble/vfs: empty file name")
}
n := &memNode{name: frag}
n := &memNode{}
dir.children[frag] = n
ret = &memFile{
name: frag,
n: n,
fs: y,
read: true,
Expand Down Expand Up @@ -275,13 +276,15 @@ func (y *MemFS) open(fullname string, openForWrite bool) (File, error) {
if final {
if frag == "" {
ret = &memFile{
n: dir,
fs: y,
name: sep, // this is the root directory
n: dir,
fs: y,
}
return nil
}
if n := dir.children[frag]; n != nil {
ret = &memFile{
name: frag,
n: n,
fs: y,
read: true,
Expand Down Expand Up @@ -406,7 +409,6 @@ func (y *MemFS) Rename(oldname, newname string) error {
return errors.New("pebble/vfs: empty file name")
}
dir.children[frag] = n
n.name = frag
}
return nil
})
Expand Down Expand Up @@ -442,7 +444,6 @@ func (y *MemFS) MkdirAll(dirname string, perm os.FileMode) error {
child := dir.children[frag]
if child == nil {
dir.children[frag] = &memNode{
name: frag,
children: make(map[string]*memNode),
isDir: true,
}
Expand Down Expand Up @@ -551,9 +552,6 @@ func (*MemFS) GetDiskUsage(string) (DiskUsage, error) {

// memNode holds a file's data or a directory's children.
type memNode struct {
// TODO(pav-kv): file name is a property of a file, not of a node. Multiple
// files with different names can hard link to the same node.
name string // protected by MemFS.mu
isDir bool
refs atomic.Int32

Expand All @@ -576,13 +574,12 @@ type memNode struct {

func newRootMemNode() *memNode {
return &memNode{
name: "/", // set the name to match what file systems do
children: make(map[string]*memNode),
isDir: true,
}
}

func (f *memNode) dump(w *bytes.Buffer, level int) {
func (f *memNode) dump(w *bytes.Buffer, level int, name string) {
if f.isDir {
w.WriteString(" ")
} else {
Expand All @@ -593,7 +590,7 @@ func (f *memNode) dump(w *bytes.Buffer, level int) {
for i := 0; i < level; i++ {
w.WriteString(" ")
}
w.WriteString(f.name)
w.WriteString(name)
if !f.isDir {
w.WriteByte('\n')
return
Expand All @@ -608,7 +605,7 @@ func (f *memNode) dump(w *bytes.Buffer, level int) {
}
sort.Strings(names)
for _, name := range names {
f.children[name].dump(w, level+1)
f.children[name].dump(w, level+1, name)
}
}

Expand All @@ -630,6 +627,7 @@ func (f *memNode) resetToSyncedState() {

// memFile is a reader or writer of a node's data. Implements File.
type memFile struct {
name string
n *memNode
fs *MemFS // nil for a standalone memFile
rpos int
Expand All @@ -643,9 +641,9 @@ func (f *memFile) Close() error {
if n := f.n.refs.Add(-1); n < 0 {
panic(fmt.Sprintf("pebble: close of unopened file: %d", n))
}
// TODO(pav-kv): set f.n to nil here, to detect double-close and
// use-after-close errors automatically. Currently this is not possible
// because the lifetime of Stat() can outlive the Close() of this file.
// Set node pointer to nil, to cause panic on any subsequent method call. This
// is a defence-in-depth to catch use-after-close or double-close bugs.
f.n = nil
return nil
}

Expand Down Expand Up @@ -741,24 +739,11 @@ func (f *memFile) WriteAt(p []byte, ofs int64) (int, error) {
func (f *memFile) Prefetch(offset int64, length int64) error { return nil }
func (f *memFile) Preallocate(offset, length int64) error { return nil }

// name returns the current file name.
func (f *memFile) name() string {
// NB: the file name must be accessed under MemFS.mu lock.
if fs := f.fs; fs != nil {
fs.mu.Lock()
defer fs.mu.Unlock()
}
return f.n.name
}

func (f *memFile) Stat() (os.FileInfo, error) {
// NB: can not inline f.name() below because it would violate the locking
// order: MemFS.mu can not be locked after the f.n.mu lock.
name := f.name()
f.n.mu.Lock()
defer f.n.mu.Unlock()
return &memFileInfo{
name: name,
name: f.name,
size: int64(len(f.n.mu.data)),
modTime: f.n.mu.modTime,
isDir: f.n.isDir,
Expand Down
6 changes: 2 additions & 4 deletions vfs/mem_fs_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -232,14 +232,12 @@ func TestList(t *testing.T) {

{
got := fs.String()
// TODO(pav-kv): fix the bar/link-to-3 and bar/another-link-to-3 link names.
// Currently String() erroneously prints the name of the linked file.
const want = ` /
0 a
bar/
0 3
0 another-link-to-3
0 baz
0 3
0 link-to-3
foo/
0 0
0 1
Expand Down

0 comments on commit 27d566e

Please sign in to comment.