netdev - Re: [PATCH 08/39] experimental: convert fs/overlayfs/file.c to CLASS(...)

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240731211153.GD3908975@perftesting>
Date: Wed, 31 Jul 2024 17:11:53 -0400
From: Josef Bacik <josef@...icpanda.com>
To: Al Viro <viro@...iv.linux.org.uk>
Cc: viro@...nel.org, linux-fsdevel@...r.kernel.org, amir73il@...il.com,
	bpf@...r.kernel.org, brauner@...nel.org, cgroups@...r.kernel.org,
	kvm@...r.kernel.org, netdev@...r.kernel.org,
	torvalds@...ux-foundation.org
Subject: Re: [PATCH 08/39] experimental: convert fs/overlayfs/file.c to
 CLASS(...)

On Tue, Jul 30, 2024 at 10:12:25PM +0100, Al Viro wrote:
> On Tue, Jul 30, 2024 at 03:10:25PM -0400, Josef Bacik wrote:
> > On Tue, Jul 30, 2024 at 01:15:54AM -0400, viro@...nel.org wrote:
> > > From: Al Viro <viro@...iv.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.
> 

Fair, I think I misunderstood what you were unhappy with in that code.

> > 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.
> 
> That's a bit painful in these cases - sure, we can do something like
> 	scoped_class(fd_real, real)(file) {
> 		if (fd_empty(fd_real)) {
> 			ret = fd_error(real);
> 			break;
> 		}
> 		old_cred = ovl_override_creds(file_inode(file)->i_sb);
> 		ret = vfs_fallocate(fd_file(real), mode, offset, len);
> 		revert_creds(old_cred);
> 
> 		/* Update size */
> 		ovl_file_modified(file);  
> 	}
> but that use of break would need to be documented.  And IMO anything like
>         scoped_cond_guard (mutex_intr, return -ERESTARTNOINTR,
> 			   &task->signal->cred_guard_mutex) {
> is just distasteful ;-/  Control flow should _not_ be hidden that way;
> it's hard on casual reader.
> 
> The variant I'd put in there is obviously not suitable for merge - we need
> something else, the question is what that something should be...

I went and looked at our c++ codebase to see what they do here, and it appears
that this is the accepted norm for this style of scoped variables

{
	CLASS(fd_real, real_out)(file_out);
	// blah blah
}

Looking at our code guidelines this appears to be the widely accepted norm, and
I don't hate it.  I feel like this is more readable than the scoped_class()
idea, and is honestly the cleanest solution.  Thanks,

Josef