Re: [PATCH v4 07/12] virtiofsd: Let lo_inode_open() return a TempFd

[Hanna Reitz]

On 18.10.21 21:18, Vivek Goyal wrote:
On Thu, Sep 16, 2021 at 10:40:40AM +0200, Hanna Reitz wrote:
Strictly speaking, this is not necessary, because lo_inode_open() will
always return a new FD owned by the caller, so TempFd.owned will always
be true.
The auto-cleanup is nice, though.  Also, we get a more unified interface
where you always get a TempFd when you need an FD for an lo_inode
(regardless of whether it is an O_PATH FD or a non-O_PATH FD).

Signed-off-by: Hanna Reitz <hre...@redhat.com>
---
  tools/virtiofsd/passthrough_ll.c | 156 +++++++++++++++----------------
  1 file changed, 75 insertions(+), 81 deletions(-)

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 3bf20b8659..d257eda129 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -293,10 +293,8 @@ static void temp_fd_clear(TempFd *temp_fd)
  /**
   * Return an owned fd from *temp_fd that will not be closed when
   * *temp_fd goes out of scope.
- *
- * (TODO: Remove __attribute__ once this is used.)
   */
-static __attribute__((unused)) int temp_fd_steal(TempFd *temp_fd)
+static int temp_fd_steal(TempFd *temp_fd)
  {
      if (temp_fd->owned) {
          temp_fd->owned = false;
@@ -309,10 +307,8 @@ static __attribute__((unused)) int temp_fd_steal(TempFd 
*temp_fd)
  /**
   * Create a borrowing copy of an existing TempFd.  Note that *to is
   * only valid as long as *from is valid.
- *
- * (TODO: Remove __attribute__ once this is used.)
   */
-static __attribute__((unused)) void temp_fd_copy(const TempFd *from, TempFd 
*to)
+static void temp_fd_copy(const TempFd *from, TempFd *to)
  {
      *to = (TempFd) {
          .fd = from->fd,
@@ -689,9 +685,12 @@ static int lo_fd(fuse_req_t req, fuse_ino_t ino, TempFd 
*tfd)
   * when a malicious client opens special files such as block device nodes.
   * Symlink inodes are also rejected since symlinks must already have been
   * traversed on the client side.
+ *
+ * The fd is returned in tfd->fd.  The return value is 0 on success and -errno
+ * otherwise.
   */
  static int lo_inode_open(struct lo_data *lo, struct lo_inode *inode,
-                         int open_flags)
+                         int open_flags, TempFd *tfd)
  {
      g_autofree char *fd_str = g_strdup_printf("%d", inode->fd);
      int fd;
@@ -710,7 +709,13 @@ static int lo_inode_open(struct lo_data *lo, struct 
lo_inode *inode,
      if (fd < 0) {
          return -errno;
      }
-    return fd;
+
+    *tfd = (TempFd) {
+        .fd = fd,
+        .owned = true,
+    };
+
+    return 0;
  }
  
  static void lo_init(void *userdata, struct fuse_conn_info *conn)
@@ -854,7 +859,8 @@ static int lo_fi_fd(fuse_req_t req, struct fuse_file_info 
*fi)
  static void lo_setattr(fuse_req_t req, fuse_ino_t ino, struct stat *attr,
                         int valid, struct fuse_file_info *fi)
  {
-    g_auto(TempFd) path_fd = TEMP_FD_INIT;
+    g_auto(TempFd) path_fd = TEMP_FD_INIT; /* at least an O_PATH fd */
What does atleast O_PATH fd mean?

It means it might as well be an O_RDWR FD.  When we open an O_RDWR FD, 
it’s pointless to open an O_PATH FD, too, because we can use the O_RDWR 
FD everywhere where we’d need an O_PATH FD.  Hence in this case, we open 
rw_fd and copy it to path_fd.

Users still use rw_fd and path_fd according to what they need.


+    g_auto(TempFd) rw_fd = TEMP_FD_INIT; /* O_RDWR fd */
      int saverr;
      char procname[64];
      struct lo_data *lo = lo_data(req);
@@ -868,7 +874,15 @@ static void lo_setattr(fuse_req_t req, fuse_ino_t ino, 
struct stat *attr,
          return;
      }
  
-    res = lo_inode_fd(inode, &path_fd);
+    if (!fi && (valid & FUSE_SET_ATTR_SIZE)) {
+        /* We need an O_RDWR FD for ftruncate() */
+        res = lo_inode_open(lo, inode, O_RDWR, &rw_fd);
+        if (res >= 0) {
+            temp_fd_copy(&rw_fd, &path_fd);
I am lost here. If lo_inode_open() failed, why are we calling this
temp_fd_copy()? path_fd is not even a valid fd yet.

We’re calling it on success.

Still beats me that why open rw_fd now instead of down in
FUSE_SET_ATTR_SIZE block. I think we had this discussion and you
had some reasons to move it up.

Because it’s pointless overhead to open two FDs.

Before file handles, getting the O_PATH FD was a trivial operation. If 
we needed an O_RDWR FD later, we’d open it later.  No duplicate work 
involved.

With file handles, getting the O_PATH FD may mean opening a new FD. 
Opening another FD from the same file handle, just with different flags, 
later on would be strange, because we could have just opened the FD as 
O_RDWR right from the start.

Hanna