fuse_read: update based on code review

pkg/abi/linux/fuse.go

@@ -430,7 +430,7 @@ type FUSEReadIn struct {
 	// LockOwner is the id of the lock owner if there is one.
 	LockOwner uint64
 
-	// Flags for the underling file.
+	// Flags for the underlying file.
 	Flags uint32
 
 	_ uint32

pkg/sentry/fsimpl/fuse/dev.go

@@ -406,7 +406,7 @@ func (fd *DeviceFD) noReceiverAction(ctx context.Context, r *Response) error {
 		creds := auth.CredentialsFromContext(ctx)
 		rootUserNs := kernel.KernelFromContext(ctx).RootUserNamespace()
 		return fd.fs.conn.InitRecv(r, creds.HasCapabilityIn(linux.CAP_SYS_ADMIN, rootUserNs))
-		// TODO(gvisor/dev/issue/3247): support async read: correctly process the response using information from r.options.
+		// TODO(gvisor.dev/issue/3247): support async read: correctly process the response using information from r.options.
 	}
 
 	return nil

pkg/sentry/fsimpl/fuse/fusefs.go

@@ -59,7 +59,7 @@ type filesystemOptions struct {
 	// Call the server.
 	maxActiveRequests uint64
 
-	// maxRead is the max number of bytes to read.
+	// maxRead is the max number of bytes to read,
 	// specified as "max_read" in fs parameters.
 	// If not specified by user, use math.MaxUint32 as default value.
 	maxRead uint32
@@ -260,20 +260,13 @@ func (fs *filesystem) newInode(nodeID uint64, attr linux.FUSEAttr) *kernfs.Dentr
 	i := &inode{fs: fs, NodeID: nodeID}
 	creds := auth.Credentials{EffectiveKGID: auth.KGID(attr.UID), EffectiveKUID: auth.KUID(attr.UID)}
 	i.InodeAttrs.Init(&creds, linux.UNNAMED_MAJOR, fs.devMinor, fs.NextIno(), linux.FileMode(attr.Mode))
-	// TODO(gvisor.dev/issue/1193): This should be handled in Init,
-	// unfortunately we need it for this implementation now.
 	atomic.StoreUint64(&i.size, attr.Size)
 	i.OrderedChildren.Init(kernfs.OrderedChildrenOptions{})
 	i.dentry.Init(i)
 
 	return &i.dentry
 }
 
-// Ino overrides the Ino() from InodeAttrs.
-func (i *inode) Ino() uint64 {
-	return i.NodeID
-}
-
 // Open implements kernfs.Inode.Open.
 func (i *inode) Open(ctx context.Context, rp *vfs.ResolvingPath, vfsd *vfs.Dentry, opts vfs.OpenOptions) (*vfs.FileDescription, error) {
 	isDir := i.InodeAttrs.Mode().IsDir()
@@ -291,9 +284,9 @@ func (i *inode) Open(ctx context.Context, rp *vfs.ResolvingPath, vfsd *vfs.Dentr
 		fd = &(directoryFD.fileDescription)
 		fdImpl = directoryFD
 	} else {
-		regularFd := &regularFileFD{}
-		fd = &(regularFd.fileDescription)
-		fdImpl = regularFd
+		regularFD := &regularFileFD{}
+		fd = &(regularFD.fileDescription)
+		fdImpl = regularFD
 	}
 	// FOPEN_KEEP_CACHE is the defualt flag for noOpen.
 	fd.OpenFlag = linux.FOPEN_KEEP_CACHE
@@ -541,7 +534,7 @@ func (i *inode) Readlink(ctx context.Context, mnt *vfs.Mount) (string, error) {
 // TODO(gvisor.dev/issue/3679): Add support for other fields.
 func (i *inode) getFUSEAttr() linux.FUSEAttr {
 	return linux.FUSEAttr{
-		Ino:  i.NodeID,
+		Ino:  i.Ino(),
 		Size: atomic.LoadUint64(&i.size),
 		Mode: uint32(i.Mode()),
 	}
@@ -600,14 +593,14 @@ func statFromFUSEAttr(attr linux.FUSEAttr, mask, devMinor uint32) linux.Statx {
 	return stat
 }
 
-// getAttr get the attribute of this inode by issuing a FUSE_GETATTR request
+// getAttr gets the attribute of this inode by issuing a FUSE_GETATTR request
 // or read from local cache.
 // It updates the corresponding attributes if necessary.
 func (i *inode) getAttr(ctx context.Context, fs *vfs.Filesystem, opts vfs.StatOptions) (linux.FUSEAttr, error) {
 	attributeVersion := atomic.LoadUint64(&i.fs.conn.attributeVersion)
 
 	// TODO(gvisor.dev/issue/3679): send the request only if
-	// - local cache is invalid for the requested fields (as specified in the opts.Mask)
+	// - invalid local cache for fields specified in the opts.Mask
 	// - forced update
 	// - i.attributeTime expired
 	// If local cache is still valid, return local cache.
@@ -660,17 +653,15 @@ func (i *inode) getAttr(ctx context.Context, fs *vfs.Filesystem, opts vfs.StatOp
 	}
 
 	// Set the size if no error (after SetStat() check).
-	// TODO(gvisor.dev/issue/1193): This should be handled in SetStat,
-	// unfortunately we need it for this implementation now.
 	atomic.StoreUint64(&i.size, out.Attr.Size)
 
 	return out.Attr, nil
 }
 
-// renewAttr attempts to update the attributes for internal purposes
+// reviseAttr attempts to update the attributes for internal purposes
 // by calling getAttr with a pre-specified mask.
 // Used by read, write, lseek.
-func (i *inode) renewAttr(ctx context.Context) error {
+func (i *inode) reviseAttr(ctx context.Context) error {
 	// Never need atime for internal purposes.
 	_, err := i.getAttr(ctx, i.fs.VFSFilesystem(), vfs.StatOptions{
 		Mask: linux.STATX_BASIC_STATS &^ linux.STATX_ATIME,

pkg/sentry/fsimpl/fuse/read_write.go

@@ -29,12 +29,12 @@ import (
 )
 
 // ReadInPages sends FUSE_READ requests for the size after round it up to a multiple of page size,
-// block on it for reply, process the reply and return the payload (or joined payloads) as a byte slice.
+// blocks on it for reply, processes the reply and returns the payload (or joined payloads) as a byte slice.
 // This is used for the general purpose reading. We do not support direct IO (which read the exact number of bytes) at this moment.
 func (fs *filesystem) ReadInPages(ctx context.Context, fd *regularFileFD, off uint64, size uint32) ([]byte, uint32, error) {
 	attributeVersion := atomic.LoadUint64(&fs.conn.attributeVersion)
 
-	// Round up to a multiple of pages size.
+	// Round up to a multiple of page size.
 	readSize, _ := usermem.PageRoundUp(uint64(size))
 
 	// One request cannnot exceed either maxRead or maxPages.
@@ -80,7 +80,7 @@ func (fs *filesystem) ReadInPages(ctx context.Context, fd *regularFileFD, off ui
 			return nil, 0, err
 		}
 
-		// TODO(gvisor/dev/issue/3247): support async read.
+		// TODO(gvisor.dev/issue/3247): support async read.
 
 		res, err := fs.conn.Call(t, req)
 		if err != nil {
@@ -96,7 +96,7 @@ func (fs *filesystem) ReadInPages(ctx context.Context, fd *regularFileFD, off ui
 		// that cannot even fit the hdr.
 		if len(res.data) <= res.hdr.SizeBytes() {
 			if len(res.data) < res.hdr.SizeBytes() {
-				return nil, 0, fmt.Errorf("payload too small. Minimum data lenth required: %d,  but got data length %d", res.hdr.SizeBytes(), len(res.data))
+				return nil, 0, fmt.Errorf("FUSE_READ response too small. Minimum length required: %d,  but got length %d", res.hdr.SizeBytes(), len(res.data))
 			}
 			break
 		}
@@ -123,7 +123,7 @@ func (fs *filesystem) ReadInPages(ctx context.Context, fd *regularFileFD, off ui
 	}
 
 	// Join data from multiple fragmented replies.
-	// TODO(gvisor/dev/issue/3628): avoid this extra copy,
+	// TODO(gvisor.dev/issue/3628): avoid this extra copy,
 	// perhaps we can use iovec, which requires upperstream support.
 	buf := make([]byte, sizeRead)
 	bufPos := 0
@@ -136,7 +136,7 @@ func (fs *filesystem) ReadInPages(ctx context.Context, fd *regularFileFD, off ui
 
 // ReadCallback updates several information after receiving a read response.
 func (fs *filesystem) ReadCallback(ctx context.Context, fd *regularFileFD, off uint64, size uint32, sizeRead uint32, attributeVersion uint64) {
-	// TODO(gvisor/dev/issue/3247): support async read. If this is called by an async read, correctly process it.
+	// TODO(gvisor.dev/issue/3247): support async read. If this is called by an async read, correctly process it.
 	// May need to update the signature.
 
 	i := fd.inode()

pkg/sentry/fsimpl/fuse/regular_file.go

@@ -54,14 +54,14 @@ func (fd *regularFileFD) PRead(ctx context.Context, dst usermem.IOSequence, offs
 		return 0, nil
 	}
 
-	rw := getRegularFdReadWriter(ctx, fd, size, offset)
+	rw := getRegularFDReadWriter(ctx, fd, size, offset)
 
 	// TODO(gvisor.dev/issue/3678): Add direct IO support.
 
 	rw.read()
 	n, err := dst.CopyOutFrom(ctx, rw)
 
-	putRegularFdReadWriter(rw)
+	putRegularFDReadWriter(rw)
 
 	return n, err
 }
@@ -75,7 +75,7 @@ func (fd *regularFileFD) Read(ctx context.Context, dst usermem.IOSequence, opts
 	return n, err
 }
 
-type regularFdReadWriter struct {
+type regularFDReadWriter struct {
 	ctx context.Context
 	fd  *regularFileFD
 
@@ -97,26 +97,26 @@ type regularFdReadWriter struct {
 	buf []byte
 }
 
-func (rw *regularFdReadWriter) fs() *filesystem {
+func (rw *regularFDReadWriter) fs() *filesystem {
 	return rw.fd.inode().fs
 }
 
 var regularFdReadWriterPool = sync.Pool{
 	New: func() interface{} {
-		return &regularFdReadWriter{}
+		return &regularFDReadWriter{}
 	},
 }
 
-func getRegularFdReadWriter(ctx context.Context, fd *regularFileFD, size uint32, offset int64) *regularFdReadWriter {
-	rw := regularFdReadWriterPool.Get().(*regularFdReadWriter)
+func getRegularFDReadWriter(ctx context.Context, fd *regularFileFD, size uint32, offset int64) *regularFDReadWriter {
+	rw := regularFdReadWriterPool.Get().(*regularFDReadWriter)
 	rw.ctx = ctx
 	rw.fd = fd
 	rw.size = size
 	rw.off = uint64(offset)
 	return rw
 }
 
-func putRegularFdReadWriter(rw *regularFdReadWriter) {
+func putRegularFDReadWriter(rw *regularFDReadWriter) {
 	rw.ctx = nil
 	rw.fd = nil
 	rw.buf = nil
@@ -127,15 +127,15 @@ func putRegularFdReadWriter(rw *regularFdReadWriter) {
 
 // read handles and issues the actual FUSE read request.
 // See ReadToBlocks() regarding its purpose.
-func (rw *regularFdReadWriter) read() {
+func (rw *regularFDReadWriter) read() {
 	// TODO(gvisor.dev/issue/3237): support indirect IO (e.g. caching):
 	// use caching when possible.
 
 	inode := rw.fd.inode()
 
 	// Reading beyond EOF, update file size if outdated.
 	if rw.off+uint64(rw.size) >= atomic.LoadUint64(&inode.size) {
-		if err := inode.renewAttr(rw.ctx); err != nil {
+		if err := inode.reviseAttr(rw.ctx); err != nil {
 			rw.err = err
 			return
 		}
@@ -165,7 +165,7 @@ func (rw *regularFdReadWriter) read() {
 // will try to acquire the same lock), have to separate the rw.read() from the
 // ReadToBlocks() function. Therefore, ReadToBlocks() only handles copying
 // the result into user memory while read() handles the actual reading.
-func (rw *regularFdReadWriter) ReadToBlocks(dsts safemem.BlockSeq) (uint64, error) {
+func (rw *regularFDReadWriter) ReadToBlocks(dsts safemem.BlockSeq) (uint64, error) {
 	if rw.err != nil {
 		return 0, rw.err
 	}

test/fuse/linux/read_test.cc

@@ -24,20 +24,21 @@
 #include <string>
 #include <vector>
 
-#include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include "test/util/fuse_util.h"
 #include "test/util/test_util.h"
 
-#include "fuse_base.h"
+#include "test/fuse/linux/fuse_base.h"
 
 namespace gvisor {
 namespace testing {
 
 namespace {
 
 class ReadTest : public FuseTest {
-  void TearDown() { UnmountFuse(); }
+  // TearDown overrides the parent's function
+  // to skip checking the unconsumed release request at the end.
+  void TearDown() override { UnmountFuse(); }
 
  protected:
   const std::string test_file_ = "test_file";
@@ -62,19 +63,11 @@ TEST_F(ReadTest, ReadWhole) {
   FileDescriptor fd =
       ASSERT_NO_ERRNO_AND_VALUE(Open(test_file_path.c_str(), open_flag));
 
-  // Can replace with SkipServerActualRequest() after mergerd.
-  struct fuse_in_header in_header_open;
-  struct fuse_open_in in_payload_open;
-  auto iov_in_open = FuseGenerateIovecs(in_header_open, in_payload_open);
-  GetServerActualRequest(iov_in_open);
-
-  EXPECT_EQ(in_header_open.len,
-            sizeof(in_header_open) + sizeof(in_payload_open));
-  EXPECT_EQ(in_header_open.opcode, FUSE_OPEN);
+  SkipServerActualRequest();
 
   // Prepare for the read.
   const int n_read = 5;
-  std::string data = std::string(n_read, 'a');
+  std::vector<char> data(n_read, 'a');
   struct fuse_out_header read_out_header = {
       .len = static_cast<uint32_t>(sizeof(struct fuse_out_header) +
                                    data.size() + 1),
@@ -83,9 +76,8 @@ TEST_F(ReadTest, ReadWhole) {
   SetServerResponse(FUSE_READ, iov_out_read);
 
   // Read the whole "file".
-  char buf[n_read + 1];
-  EXPECT_THAT(read(fd.get(), buf, n_read), SyscallSucceedsWithValue(n_read));
-  buf[n_read] = '\0';
+  std::vector<char> buf(n_read);
+  EXPECT_THAT(read(fd.get(), buf.data(), n_read), SyscallSucceedsWithValue(n_read));
 
   // Check the read request.
   struct fuse_in_header in_header;
@@ -96,7 +88,7 @@ TEST_F(ReadTest, ReadWhole) {
   EXPECT_EQ(in_header.len, sizeof(in_header) + sizeof(in_payload));
   EXPECT_EQ(in_header.opcode, FUSE_READ);
   EXPECT_EQ(in_payload.offset, 0);
-  EXPECT_STREQ(buf, data.c_str());
+  EXPECT_EQ(buf, data);
 }
 
 TEST_F(ReadTest, ReadPartial) {
@@ -143,11 +135,11 @@ TEST_F(ReadTest, ReadPartial) {
   struct fuse_in_header in_header;
   struct fuse_read_in in_payload;
   auto iov_in = FuseGenerateIovecs(in_header, in_payload);
-  char buf[n_data + 1];
+  std::vector<char> buf(n_data);
 
   // Read 1 bytes.
   SetServerResponse(FUSE_READ, iov_out_read);
-  EXPECT_THAT(read(fd.get(), buf, 1), SyscallSucceedsWithValue(1));
+  EXPECT_THAT(read(fd.get(), buf.data(), 1), SyscallSucceedsWithValue(1));
 
   // Check the 1-byte read request.
   GetServerActualRequest(iov_in);
@@ -157,15 +149,15 @@ TEST_F(ReadTest, ReadPartial) {
 
   // Read 3 bytes.
   SetServerResponse(FUSE_READ, iov_out_read);
-  EXPECT_THAT(read(fd.get(), buf, 3), SyscallSucceedsWithValue(3));
+  EXPECT_THAT(read(fd.get(), buf.data(), 3), SyscallSucceedsWithValue(3));
 
   // Check the 3-byte read request.
   GetServerActualRequest(iov_in);
   EXPECT_EQ(in_payload.offset, 1);
 
   // Read 5 bytes.
   SetServerResponse(FUSE_READ, iov_out_read);
-  EXPECT_THAT(read(fd.get(), buf, 5), SyscallSucceedsWithValue(5));
+  EXPECT_THAT(read(fd.get(), buf.data(), 5), SyscallSucceedsWithValue(5));
 
   // Check the 5-byte read request.
   GetServerActualRequest(iov_in);
@@ -203,7 +195,7 @@ TEST_F(ReadTest, PRead) {
 
   // Prepare for the read.
   const int n_read = 5;
-  std::string data = std::string(n_read, 'a');
+  std::vector<char> data(n_read, 'a');
   struct fuse_out_header read_out_header = {
       .len = static_cast<uint32_t>(sizeof(struct fuse_out_header) +
                                    data.size() + 1),
@@ -212,11 +204,13 @@ TEST_F(ReadTest, PRead) {
   SetServerResponse(FUSE_READ, iov_out_read);
 
   // Read some bytes.
-  char buf[n_read + 1];
+  std::vector<char> buf(n_read);
+  // This 123 works since the hard-coded value for
+  // file size is 512.
+  // Need a way to change that value in the future.
   const int offset_read = 123;
-  EXPECT_THAT(pread(fd.get(), buf, n_read, offset_read),
+  EXPECT_THAT(pread(fd.get(), buf.data(), n_read, offset_read),
               SyscallSucceedsWithValue(n_read));
-  buf[n_read] = '\0';
 
   // Check the read request.
   struct fuse_in_header in_header;
@@ -227,7 +221,7 @@ TEST_F(ReadTest, PRead) {
   EXPECT_EQ(in_header.len, sizeof(in_header) + sizeof(in_payload));
   EXPECT_EQ(in_header.opcode, FUSE_READ);
   EXPECT_EQ(in_payload.offset, offset_read);
-  EXPECT_STREQ(buf, data.c_str());
+  EXPECT_EQ(buf, data);
 }
 
 }  // namespace