From: Vivek Goyal <vgoyal@redhat.com> To: Max Reitz <mreitz@redhat.com> Cc: virtio-fs@redhat.com, qemu-devel@nongnu.org, Stefan Hajnoczi <stefanha@redhat.com>, "Dr . David Alan Gilbert" <dgilbert@redhat.com> Subject: Re: [PATCH v3 02/10] virtiofsd: Add TempFd structure Date: Fri, 6 Aug 2021 10:41:15 -0400 [thread overview] Message-ID: <YQ1KCya+NHd96kvv@redhat.com> (raw) In-Reply-To: <20210730150134.216126-3-mreitz@redhat.com> On Fri, Jul 30, 2021 at 05:01:26PM +0200, Max Reitz wrote: > We are planning to add file handles to lo_inode objects as an > alternative to lo_inode.fd. That means that everywhere where we > currently reference lo_inode.fd, we will have to open a temporary file > descriptor that needs to be closed after use. > > So instead of directly accessing lo_inode.fd, there will be a helper > function (lo_inode_fd()) that either returns lo_inode.fd, or opens a new > file descriptor with open_by_handle_at(). It encapsulates this result > in a TempFd structure to let the caller know whether the FD needs to be > closed after use (opened from the handle) or not (copied from > lo_inode.fd). I am wondering why this notion of "owned". Why not have this requirement of always closing "fd". If we copied it from lo_inode.fd, then we will need to dup() it. Otherwise we opened it from file handle and we will need to close it anyway. I guess you are trying to avoid having to call dup() and that's why this notion of "owned" fd. > > By using g_auto(TempFd) to store this result, callers will not even have > to care about closing a temporary FD after use. It will be done > automatically once the object goes out of scope. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > Reviewed-by: Connor Kuehl <ckuehl@redhat.com> > --- > tools/virtiofsd/passthrough_ll.c | 49 ++++++++++++++++++++++++++++++++ > 1 file changed, 49 insertions(+) > > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c > index 1f27eeabc5..fb5e073e6a 100644 > --- a/tools/virtiofsd/passthrough_ll.c > +++ b/tools/virtiofsd/passthrough_ll.c > @@ -178,6 +178,28 @@ struct lo_data { > int user_posix_acl, posix_acl; > }; > > +/** > + * Represents a file descriptor that may either be owned by this > + * TempFd, or only referenced (i.e. the ownership belongs to some > + * other object, and the value has just been copied into this TempFd). > + * > + * The purpose of this encapsulation is to be used as g_auto(TempFd) > + * to automatically clean up owned file descriptors when this object > + * goes out of scope. > + * > + * Use temp_fd_steal() to get an owned file descriptor that will not > + * be closed when the TempFd goes out of scope. > + */ > +typedef struct { > + int fd; > + bool owned; /* fd owned by this object? */ > +} TempFd; > + > +#define TEMP_FD_INIT ((TempFd) { .fd = -1, .owned = false }) > + > +static void temp_fd_clear(TempFd *temp_fd); > +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(TempFd, temp_fd_clear); > + > static const struct fuse_opt lo_opts[] = { > { "sandbox=namespace", > offsetof(struct lo_data, sandbox), > @@ -255,6 +277,33 @@ static struct lo_data *lo_data(fuse_req_t req) > return (struct lo_data *)fuse_req_userdata(req); > } > > +/** > + * Clean-up function for TempFds > + */ > +static void temp_fd_clear(TempFd *temp_fd) > +{ > + if (temp_fd->owned) { > + close(temp_fd->fd); > + *temp_fd = TEMP_FD_INIT; > + } > +} > + > +/** > + * 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) > +{ > + if (temp_fd->owned) { > + temp_fd->owned = false; > + return temp_fd->fd; > + } else { > + return dup(temp_fd->fd); > + } > +} This also will be simpler if we always called dup() and every caller will close() fd. I think only downside is having to call dup()/close(). Not sure if this is an expensive operation or not. Vivek
WARNING: multiple messages have this Message-ID (diff)
From: Vivek Goyal <vgoyal@redhat.com> To: Max Reitz <mreitz@redhat.com> Cc: virtio-fs@redhat.com, qemu-devel@nongnu.org Subject: Re: [Virtio-fs] [PATCH v3 02/10] virtiofsd: Add TempFd structure Date: Fri, 6 Aug 2021 10:41:15 -0400 [thread overview] Message-ID: <YQ1KCya+NHd96kvv@redhat.com> (raw) In-Reply-To: <20210730150134.216126-3-mreitz@redhat.com> On Fri, Jul 30, 2021 at 05:01:26PM +0200, Max Reitz wrote: > We are planning to add file handles to lo_inode objects as an > alternative to lo_inode.fd. That means that everywhere where we > currently reference lo_inode.fd, we will have to open a temporary file > descriptor that needs to be closed after use. > > So instead of directly accessing lo_inode.fd, there will be a helper > function (lo_inode_fd()) that either returns lo_inode.fd, or opens a new > file descriptor with open_by_handle_at(). It encapsulates this result > in a TempFd structure to let the caller know whether the FD needs to be > closed after use (opened from the handle) or not (copied from > lo_inode.fd). I am wondering why this notion of "owned". Why not have this requirement of always closing "fd". If we copied it from lo_inode.fd, then we will need to dup() it. Otherwise we opened it from file handle and we will need to close it anyway. I guess you are trying to avoid having to call dup() and that's why this notion of "owned" fd. > > By using g_auto(TempFd) to store this result, callers will not even have > to care about closing a temporary FD after use. It will be done > automatically once the object goes out of scope. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > Reviewed-by: Connor Kuehl <ckuehl@redhat.com> > --- > tools/virtiofsd/passthrough_ll.c | 49 ++++++++++++++++++++++++++++++++ > 1 file changed, 49 insertions(+) > > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c > index 1f27eeabc5..fb5e073e6a 100644 > --- a/tools/virtiofsd/passthrough_ll.c > +++ b/tools/virtiofsd/passthrough_ll.c > @@ -178,6 +178,28 @@ struct lo_data { > int user_posix_acl, posix_acl; > }; > > +/** > + * Represents a file descriptor that may either be owned by this > + * TempFd, or only referenced (i.e. the ownership belongs to some > + * other object, and the value has just been copied into this TempFd). > + * > + * The purpose of this encapsulation is to be used as g_auto(TempFd) > + * to automatically clean up owned file descriptors when this object > + * goes out of scope. > + * > + * Use temp_fd_steal() to get an owned file descriptor that will not > + * be closed when the TempFd goes out of scope. > + */ > +typedef struct { > + int fd; > + bool owned; /* fd owned by this object? */ > +} TempFd; > + > +#define TEMP_FD_INIT ((TempFd) { .fd = -1, .owned = false }) > + > +static void temp_fd_clear(TempFd *temp_fd); > +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(TempFd, temp_fd_clear); > + > static const struct fuse_opt lo_opts[] = { > { "sandbox=namespace", > offsetof(struct lo_data, sandbox), > @@ -255,6 +277,33 @@ static struct lo_data *lo_data(fuse_req_t req) > return (struct lo_data *)fuse_req_userdata(req); > } > > +/** > + * Clean-up function for TempFds > + */ > +static void temp_fd_clear(TempFd *temp_fd) > +{ > + if (temp_fd->owned) { > + close(temp_fd->fd); > + *temp_fd = TEMP_FD_INIT; > + } > +} > + > +/** > + * 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) > +{ > + if (temp_fd->owned) { > + temp_fd->owned = false; > + return temp_fd->fd; > + } else { > + return dup(temp_fd->fd); > + } > +} This also will be simpler if we always called dup() and every caller will close() fd. I think only downside is having to call dup()/close(). Not sure if this is an expensive operation or not. Vivek
next prev parent reply other threads:[~2021-08-06 14:42 UTC|newest] Thread overview: 88+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-07-30 15:01 [PATCH v3 00/10] virtiofsd: Allow using file handles instead of O_PATH FDs Max Reitz 2021-07-30 15:01 ` [Virtio-fs] " Max Reitz 2021-07-30 15:01 ` [PATCH v3 01/10] virtiofsd: Limit setxattr()'s creds-dropped region Max Reitz 2021-07-30 15:01 ` [Virtio-fs] " Max Reitz 2021-08-06 14:16 ` Vivek Goyal 2021-08-06 14:16 ` [Virtio-fs] " Vivek Goyal 2021-08-09 10:30 ` Max Reitz 2021-08-09 10:30 ` [Virtio-fs] " Max Reitz 2021-07-30 15:01 ` [PATCH v3 02/10] virtiofsd: Add TempFd structure Max Reitz 2021-07-30 15:01 ` [Virtio-fs] " Max Reitz 2021-08-06 14:41 ` Vivek Goyal [this message] 2021-08-06 14:41 ` Vivek Goyal 2021-08-09 10:44 ` Max Reitz 2021-08-09 10:44 ` [Virtio-fs] " Max Reitz 2021-07-30 15:01 ` [PATCH v3 03/10] virtiofsd: Use lo_inode_open() instead of openat() Max Reitz 2021-07-30 15:01 ` [Virtio-fs] " Max Reitz 2021-08-06 15:42 ` Vivek Goyal 2021-08-06 15:42 ` [Virtio-fs] " Vivek Goyal 2021-07-30 15:01 ` [PATCH v3 04/10] virtiofsd: Add lo_inode_fd() helper Max Reitz 2021-07-30 15:01 ` [Virtio-fs] " Max Reitz 2021-08-06 18:25 ` Vivek Goyal 2021-08-06 18:25 ` [Virtio-fs] " Vivek Goyal 2021-08-09 10:48 ` Max Reitz 2021-08-09 10:48 ` [Virtio-fs] " Max Reitz 2021-07-30 15:01 ` [PATCH v3 05/10] virtiofsd: Let lo_fd() return a TempFd Max Reitz 2021-07-30 15:01 ` [Virtio-fs] " Max Reitz 2021-07-30 15:01 ` [PATCH v3 06/10] virtiofsd: Let lo_inode_open() " Max Reitz 2021-07-30 15:01 ` [Virtio-fs] " Max Reitz 2021-08-06 19:55 ` Vivek Goyal 2021-08-06 19:55 ` [Virtio-fs] " Vivek Goyal 2021-08-09 13:40 ` Max Reitz 2021-08-09 13:40 ` [Virtio-fs] " Max Reitz 2021-07-30 15:01 ` [PATCH v3 07/10] virtiofsd: Add lo_inode.fhandle Max Reitz 2021-07-30 15:01 ` [Virtio-fs] " Max Reitz 2021-08-09 15:21 ` Vivek Goyal 2021-08-09 15:21 ` [Virtio-fs] " Vivek Goyal 2021-08-09 16:41 ` Hanna Reitz 2021-08-09 16:41 ` [Virtio-fs] " Hanna Reitz 2021-07-30 15:01 ` [PATCH v3 08/10] virtiofsd: Add inodes_by_handle hash table Max Reitz 2021-07-30 15:01 ` [Virtio-fs] " Max Reitz 2021-08-09 16:10 ` Vivek Goyal 2021-08-09 16:10 ` [Virtio-fs] " Vivek Goyal 2021-08-09 16:47 ` Hanna Reitz 2021-08-09 16:47 ` [Virtio-fs] " Hanna Reitz 2021-08-10 14:07 ` Vivek Goyal 2021-08-10 14:07 ` [Virtio-fs] " Vivek Goyal 2021-08-10 14:13 ` Hanna Reitz 2021-08-10 14:13 ` [Virtio-fs] " Hanna Reitz 2021-08-10 17:51 ` Vivek Goyal 2021-08-10 17:51 ` [Virtio-fs] " Vivek Goyal 2021-07-30 15:01 ` [PATCH v3 09/10] virtiofsd: Optionally fill lo_inode.fhandle Max Reitz 2021-07-30 15:01 ` [Virtio-fs] " Max Reitz 2021-08-09 18:41 ` Vivek Goyal 2021-08-09 18:41 ` [Virtio-fs] " Vivek Goyal 2021-08-10 8:32 ` Hanna Reitz 2021-08-10 8:32 ` [Virtio-fs] " Hanna Reitz 2021-08-10 15:23 ` Vivek Goyal 2021-08-10 15:23 ` [Virtio-fs] " Vivek Goyal 2021-08-10 15:26 ` Hanna Reitz 2021-08-10 15:26 ` [Virtio-fs] " Hanna Reitz 2021-08-10 15:57 ` Vivek Goyal 2021-08-10 15:57 ` [Virtio-fs] " Vivek Goyal 2021-08-11 6:41 ` Hanna Reitz 2021-08-11 6:41 ` [Virtio-fs] " Hanna Reitz 2021-08-16 19:44 ` Vivek Goyal 2021-08-16 19:44 ` [Virtio-fs] " Vivek Goyal 2021-08-17 8:27 ` Hanna Reitz 2021-08-17 8:27 ` [Virtio-fs] " Hanna Reitz 2021-08-17 19:45 ` Vivek Goyal 2021-08-17 19:45 ` [Virtio-fs] " Vivek Goyal 2021-08-18 0:14 ` Vivek Goyal 2021-08-18 0:14 ` [Virtio-fs] " Vivek Goyal 2021-08-18 13:32 ` Vivek Goyal 2021-08-18 13:32 ` [Virtio-fs] " Vivek Goyal 2021-08-18 13:48 ` Hanna Reitz 2021-08-18 13:48 ` [Virtio-fs] " Hanna Reitz 2021-08-19 16:38 ` Dr. David Alan Gilbert 2021-08-19 16:38 ` [Virtio-fs] " Dr. David Alan Gilbert 2021-07-30 15:01 ` [PATCH v3 10/10] virtiofsd: Add lazy lo_do_find() Max Reitz 2021-07-30 15:01 ` [Virtio-fs] " Max Reitz 2021-08-09 19:08 ` Vivek Goyal 2021-08-09 19:08 ` [Virtio-fs] " Vivek Goyal 2021-08-10 8:38 ` Hanna Reitz 2021-08-10 8:38 ` [Virtio-fs] " Hanna Reitz 2021-08-10 14:12 ` Vivek Goyal 2021-08-10 14:12 ` [Virtio-fs] " Vivek Goyal 2021-08-10 14:17 ` Hanna Reitz 2021-08-10 14:17 ` [Virtio-fs] " Hanna Reitz
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=YQ1KCya+NHd96kvv@redhat.com \ --to=vgoyal@redhat.com \ --cc=dgilbert@redhat.com \ --cc=mreitz@redhat.com \ --cc=qemu-devel@nongnu.org \ --cc=stefanha@redhat.com \ --cc=virtio-fs@redhat.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.