On Tue, Jul 30, 2024 at 01:15:54AM -0400, viro@xxxxxxxxxx wrote: > From: Al Viro <viro@xxxxxxxxxxxxxxxxxx> > > 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@xxxxxxxxxxxxxxxxxx> > --- > 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