For readdirplus, execute lookups when adding name entry.

We can only do the lookup if we are sure that we will return the
result to the kernel, otherwise our lookup counts will go out of sync
with the kernel. As a bonus, this removes the allocation for the
EntryOut data structure, since we can write directly into the
readdirplus return data.

Add a port of the xfstest generic/257 that triggered this bug.

Fixes #57.

fuse/direntry.go

@@ -47,14 +47,14 @@ func NewDirEntryList(data []byte, off uint64) *DirEntryList {
 // AddDirEntry tries to add an entry, and reports whether it
 // succeeded.
 func (l *DirEntryList) AddDirEntry(e DirEntry) (bool, uint64) {
-	return l.Add(nil, e.Name, uint64(FUSE_UNKNOWN_INO), e.Mode)
+	return l.Add(0, e.Name, uint64(FUSE_UNKNOWN_INO), e.Mode)
 }
 
 // Add adds a direntry to the DirEntryList, returning whether it
 // succeeded.
-func (l *DirEntryList) Add(prefix []byte, name string, inode uint64, mode uint32) (bool, uint64) {
+func (l *DirEntryList) Add(prefix int, name string, inode uint64, mode uint32) (bool, uint64) {
 	padding := (8 - len(name)&7) & 7
-	delta := padding + direntSize + len(name) + len(prefix)
+	delta := padding + direntSize + len(name) + prefix
 	oldLen := len(l.buf)
 	newLen := delta + oldLen
 
@@ -62,8 +62,7 @@ func (l *DirEntryList) Add(prefix []byte, name string, inode uint64, mode uint32
 		return false, l.offset
 	}
 	l.buf = l.buf[:newLen]
-	copy(l.buf[oldLen:], prefix)
-	oldLen += len(prefix)
+	oldLen += prefix
 	dirent := (*_Dirent)(unsafe.Pointer(&l.buf[oldLen]))
 	dirent.Off = l.offset + 1
 	dirent.Ino = inode
@@ -82,17 +81,16 @@ func (l *DirEntryList) Add(prefix []byte, name string, inode uint64, mode uint32
 }
 
 // AddDirLookupEntry is used for ReadDirPlus. It serializes a DirEntry
-// and its corresponding lookup. Pass a zero entryOut if the lookup
-// data should be ignored.
-func (l *DirEntryList) AddDirLookupEntry(e DirEntry, entryOut *EntryOut) (bool, uint64) {
-	ino := uint64(FUSE_UNKNOWN_INO)
-	if entryOut.Ino > 0 {
-		ino = entryOut.Ino
+// and returns the space for entry. If no space is left, returns a nil
+// pointer.
+func (l *DirEntryList) AddDirLookupEntry(e DirEntry) (*EntryOut, uint64) {
+	lastStart := len(l.buf)
+	ok, off := l.Add(int(unsafe.Sizeof(EntryOut{})), e.Name,
+		uint64(FUSE_UNKNOWN_INO), e.Mode)
+	if !ok {
+		return nil, off
 	}
-	var lookup []byte
-	toSlice(&lookup, unsafe.Pointer(entryOut), unsafe.Sizeof(EntryOut{}))
-
-	return l.Add(lookup, e.Name, ino, e.Mode)
+	return (*EntryOut)(unsafe.Pointer(&l.buf[lastStart])), off
 }
 
 func (l *DirEntryList) bytes() []byte {

fuse/nodefs/dir.go

@@ -11,7 +11,6 @@ type connectorDir struct {
 	stream     []fuse.DirEntry
 	lastOffset uint64
 	rawFS      fuse.RawFileSystem
-	lookups    []fuse.EntryOut
 }
 
 func (d *connectorDir) ReadDir(input *fuse.ReadIn, out *fuse.DirEntryList) (code fuse.Status) {
@@ -58,21 +57,6 @@ func (d *connectorDir) ReadDirPlus(input *fuse.ReadIn, out *fuse.DirEntryList) (
 		if !code.Ok() {
 			return code
 		}
-		d.lookups = nil
-	}
-
-	if d.lookups == nil {
-		d.lookups = make([]fuse.EntryOut, len(d.stream))
-		for i, n := range d.stream {
-			if n.Name == "." || n.Name == ".." {
-				continue
-			}
-			// We ignore the return value
-			code := d.rawFS.Lookup(&input.InHeader, n.Name, &d.lookups[i])
-			if !code.Ok() {
-				d.lookups[i] = fuse.EntryOut{}
-			}
-		}
 	}
 
 	if input.Offset > uint64(len(d.stream)) {
@@ -80,16 +64,31 @@ func (d *connectorDir) ReadDirPlus(input *fuse.ReadIn, out *fuse.DirEntryList) (
 		return fuse.EINVAL
 	}
 	todo := d.stream[input.Offset:]
-	for i, e := range todo {
+	for _, e := range todo {
 		if e.Name == "" {
 			log.Printf("got empty directory entry, mode %o.", e.Mode)
 			continue
 		}
-		ok, off := out.AddDirLookupEntry(e, &d.lookups[input.Offset+uint64(i)])
-		d.lastOffset = off
-		if !ok {
+
+		if e.Name == "." || e.Name == ".." {
+			continue
+		}
+
+		// we have to be sure entry will fit if we try to add
+		// it, or we'll mess up the lookup counts.
+		entryDest, off := out.AddDirLookupEntry(e)
+		if entryDest == nil {
 			break
 		}
+		entryDest.Ino = uint64(fuse.FUSE_UNKNOWN_INO)
+
+		// We ignore the return value
+		code := d.rawFS.Lookup(&input.InHeader, e.Name, entryDest)
+		if !code.Ok() {
+			// if something went wrong, clear out the entry.
+			*entryDest = fuse.EntryOut{}
+		}
+		d.lastOffset = off
 	}
 	return fuse.OK
 

fuse/test/xfs_test.go

+package test
+
+import (
+	"fmt"
+	"os"
+	"path/filepath"
+	"syscall"
+	"testing"
+	"unsafe"
+)
+
+func clen(n []byte) int {
+	for i := 0; i < len(n); i++ {
+		if n[i] == 0 {
+			return i
+		}
+	}
+	return len(n)
+}
+
+type CDirent struct {
+	ino   uint64
+	ntype uint8
+	off   int64
+	name  string
+}
+
+func parseDirents(buf []byte) []CDirent {
+	var result []CDirent
+	for len(buf) > 0 {
+		de := *(*syscall.Dirent)(unsafe.Pointer(&buf[0]))
+		buf = buf[de.Reclen:]
+		bytes := (*[10000]byte)(unsafe.Pointer(&de.Name[0]))
+		var name = string(bytes[0:clen(bytes[:])])
+		result = append(result, CDirent{
+			ino:   de.Ino,
+			ntype: de.Type,
+			off:   de.Off,
+			name:  name,
+		})
+	}
+	return result
+}
+
+// This test is inspired on xfstest, t_dir_offset2.c
+func TestReaddirPlusSeek(t *testing.T) {
+	tc := NewTestCase(t)
+	defer tc.Cleanup()
+
+	var names []string
+	for i := 0; i < 20; i++ {
+		names = append(names, fmt.Sprintf("abcd%d", i))
+	}
+
+	for _, n := range names {
+		if err := os.MkdirAll(filepath.Join(tc.origSubdir, n), 0755); err != nil {
+			t.Fatalf("Mkdir failed: %v", err)
+		}
+	}
+
+	fd, err := syscall.Open(tc.mountSubdir, syscall.O_RDONLY, 0755)
+	if err != nil {
+		t.Fatalf("Open(%q): %v", tc.mountSubdir, err)
+	}
+	defer syscall.Close(int(fd))
+
+	// store offsets
+	type entryOff struct {
+		ino uint64
+		off int64
+	}
+	previous := map[int]entryOff{}
+	var bufdata [1024]byte
+	for {
+		buf := bufdata[:]
+		n, err := syscall.ReadDirent(fd, buf)
+		if err != nil {
+			t.Fatalf("ReadDirent: %v", err)
+		}
+		if n == 0 {
+			break
+		}
+
+		buf = buf[:n]
+		for _, d := range parseDirents(buf) {
+			if d.name == "." || d.name == ".." {
+				continue
+			}
+			i := len(previous)
+			previous[i] = entryOff{d.ino, d.off}
+		}
+	}
+
+	for i := len(previous) - 1; i >= 0; i-- {
+		var off int64
+		if i > 0 {
+			off = previous[i-1].off
+		}
+
+		if _, err := syscall.Seek(fd, off, 0); err != nil {
+			t.Fatalf("Seek %v", err)
+		}
+
+		buf := bufdata[:]
+		n, err := syscall.ReadDirent(fd, buf)
+		if err != nil {
+			t.Fatalf("readdir after seek %d: %v", i, err)
+		}
+		if n == 0 {
+			t.Fatalf("no dirent after seek to %d", i)
+		}
+
+		ds := parseDirents(buf[:n])
+		if ds[0].ino != previous[i].ino {
+			t.Errorf("got ino %d, want %d", ds[0].ino, previous[i].ino)
+		}
+	}
+
+	// Delete has a forget as side-effect: make sure we get the lookup counts correct.
+	for _, n := range names {
+		full := filepath.Join(tc.mountSubdir, n)
+		if err := syscall.Rmdir(full); err != nil {
+			t.Fatalf("Rmdir(%q): %v", n, err)
+		}
+	}
+}