Re: [PATCH 08/39] experimental: convert fs/overlayfs/file.c to CLASS(...)
From: Josef Bacik <josef@toxicpanda.com>
Date: 2024-07-30 19:10:27
Also in:
bpf, cgroups, kvm, linux-fsdevel
On Tue, Jul 30, 2024 at 01:15:54AM -0400, viro@kernel.org wrote:
quoted hunk ↗ jump to hunk
From: Al Viro <viro@zeniv.linux.org.uk> There are four places where we end up adding an extra scope covering just the range from constructor to destructor; not sure if that's the best way to handle that. The functions in question are ovl_write_iter(), ovl_splice_write(), ovl_fadvise() and ovl_copyfile(). This is very likely *NOT* the final form of that thing - it needs to be discussed. Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- fs/overlayfs/file.c | 72 ++++++++++++++++++--------------------------- 1 file changed, 29 insertions(+), 43 deletions(-)diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c index 4b9e145bc7b8..a2911c632137 100644 --- a/fs/overlayfs/file.c +++ b/fs/overlayfs/file.c@@ -132,6 +132,8 @@ static struct fderr ovl_real_fdget(const struct file *file) return ovl_real_fdget_meta(file, false); } +DEFINE_CLASS(fd_real, struct fderr, fdput(_T), ovl_real_fdget(file), struct file *file) + static int ovl_open(struct inode *inode, struct file *file) { struct dentry *dentry = file_dentry(file);@@ -174,7 +176,6 @@ static int ovl_release(struct inode *inode, struct file *file) static loff_t ovl_llseek(struct file *file, loff_t offset, int whence) { struct inode *inode = file_inode(file); - struct fderr real; const struct cred *old_cred; loff_t ret;@@ -190,7 +191,7 @@ static loff_t ovl_llseek(struct file *file, loff_t offset, int whence) return vfs_setpos(file, 0, 0); } - real = ovl_real_fdget(file); + CLASS(fd_real, real)(file); if (fd_empty(real)) return fd_error(real);@@ -211,8 +212,6 @@ static loff_t ovl_llseek(struct file *file, loff_t offset, int whence) file->f_pos = fd_file(real)->f_pos; ovl_inode_unlock(inode); - fdput(real); - return ret; }@@ -253,8 +252,6 @@ static void ovl_file_accessed(struct file *file) static ssize_t ovl_read_iter(struct kiocb *iocb, struct iov_iter *iter) { struct file *file = iocb->ki_filp; - struct fderr real; - ssize_t ret; struct backing_file_ctx ctx = { .cred = ovl_creds(file_inode(file)->i_sb), .user_file = file,@@ -264,22 +261,18 @@ static ssize_t ovl_read_iter(struct kiocb *iocb, struct iov_iter *iter) if (!iov_iter_count(iter)) return 0; - real = ovl_real_fdget(file); + CLASS(fd_real, real)(file); if (fd_empty(real)) return fd_error(real); - ret = backing_file_read_iter(fd_file(real), iter, iocb, iocb->ki_flags, - &ctx); - fdput(real); - - return ret; + return backing_file_read_iter(fd_file(real), iter, iocb, iocb->ki_flags, + &ctx); } static ssize_t ovl_write_iter(struct kiocb *iocb, struct iov_iter *iter) { struct file *file = iocb->ki_filp; struct inode *inode = file_inode(file); - struct fderr real; ssize_t ret; int ifl = iocb->ki_flags; struct backing_file_ctx ctx = {@@ -295,7 +288,9 @@ static ssize_t ovl_write_iter(struct kiocb *iocb, struct iov_iter *iter) /* Update mode */ ovl_copyattr(inode); - real = ovl_real_fdget(file); + {
Is this what we want to do from a code cleanliness standpoint? This feels
pretty ugly to me, I feal like it would be better to have something like
scoped_class(fd_real, real) {
// code
}
rather than the {} at the same indent level as the underlying block.
I don't feel super strongly about this, but I do feel like we need to either
explicitly say "this is the way/an acceptable way to do this" from a code
formatting standpoint, or we need to come up with a cleaner way of representing
the scoped area. Thanks,
Josef