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
